FILTER-12: Setting "#####" placeholder for role names to avoid invalid SQL queries.

[Ruhanga]

Ticket: https://issues.openmrs.org/browse/FILTER-12

[mks-d]

Looks like you always set those placeholders now, so then what about lines 102-110, are they still needed then?

Or if you want to keep lines 102-110, should you enclose your change in an if that checks that the user is not authenticated?

Also, should the same logic not be added further up for the query involved here?

[Ruhanga]

Looks like you always set those placeholders now, so then what about lines 102-110, are they still needed then?

Yes the lines are needed. ##### is set as the default which is later overridden/filtered out if the user is authenticated.

Or if you want to keep lines 102-110, should you enclose your change in an if that checks that the user is not authenticated?

Let me arrange it then in a straight forward piece of code.

Also, should the same logic not be added further up for the query involved here?

No because that piece is not hit at-least by any api call trying to query the user table.

[mks-d]

Why not this as a one-liner with the original (better) comment?

// Avoid a 'select IN ()' which would be an invalid query, in theory we expect no role to match #####
Collection<String> userProgramRoleNames = new HashSet<>(Arrays.asList("#####"));

[Ruhanga]

Collection<String> userProgramRoleNames = new HashSet<>(Arrays.asList("#####"));

userProgramRoleNames should be final in nature since it will be used with the Stream filtering following.

[mks-d]

I'm suggesting a one-liner:

Collection<String> userProgramRoleNames = new HashSet<>(Arrays.asList("#####"));

instead of a two-liner:

Collection<String> userProgramRoleNames = new HashSet();
userProgramRoleNames.add("#####");

[Ruhanga]

Ahhh, yes for sure. My bad, I my mind I was referring to allProgramRoleNames . Let me apply the suggestion.

[mks-d]

As per @wluyima's comment on FILTER-12, and now that you have put your finger on the needed fix, what would it take to

1. revert back to the original code
2. introduce a falling test illustrating the issue
3. bring back your fix to make the test pass

?

[Ruhanga]

@mks-d, this literally happens at runtime and would require a runtime environment perhaps also involving oauth2login and a MariaDb to produce a failing test.

[mks-d]

@Ruhanga so there weren't any context-sensitive tests in which the malformed SQL exception would come out?

[wluyima]

I agree he should add a context sensitive test instead

[Ruhanga]

This will affect all the other existing tests that depend on the underlying h2 database. I'll have to make them run as separate forks and modify the base filter test class to run on a MySQL/Maria DB as a test container docker container. If green light are on, I can go ahead.

[wluyima]

I think this if clause and the one after it just need to be both moved out of their enclosing if clause to happen after and not before like you are doing

[wluyima]

And I think you need a context sensitive test to verify the fix, this should be a simple and straight forward test to put together.

[Ruhanga]

Yes but not when depending on the underlying H2 database env.

[wluyima]

It is just a test like any other context sensitive test, I don't see what the H2 database has to do with it, please explain

[mks-d]

@wluyima (I'm just being the messenger here), @Ruhanga is reporting that the faulty SQL syntax such as:

SELECT IN ()

• fails with MySQL/MariaDB;
• is in fact ok with H2.

Nathan is touching on that here already.

What do you recommend doing?

[wluyima]

I think this if clause and the one after it just need to be both moved out of their enclosing if clause to happen after and not before like you are doing

@Ruhanga did you also see this comment?

[Ruhanga]

did you also see this comment?

Yes @wluyima.

You don't have to set DATABASE_TO_LOWER

Ohh, ok sure. I was just demonstrating an example snippet of the database url. FWIW, simply setting the MODE without providing the corresponding databse url yields no difference.

[wluyima]

I'm not sure if I understand, why would you say the database url is not being provided

[Ruhanga]

I'm trying to imply that appending MODE=MySQL to the H2 overall URL requires an existing MySQL db sub url to work as expected. I.e. instead of

jdbc:h2:mem:openmrs;DB_CLOSE_DELAY=30;LOCK_TIMEOUT=10000;MODE=MySQL

I have to do some thing along the following line

jdbc:h2:localhost:<DATABASE_PORT>/openmrs?autoReconnect=true&sessionVariables=default_storage_engine%3DInnoDB&useUnicode=true&characterEncoding=UTF-8;MODE=MySQL

For the latter, I have to spin up a MySQL test container.

[Ruhanga]

I've produced a test demonstrating thrown error here #20. This uncovers bugs and breaks ~50% of the existing tests whose errors where silent with an H2 db env.

[wluyima]

I added some comments

[wluyima]

Context sensitive test needed

[Ruhanga]

Hi @wluyima, I've updated the PR. Could you review again? Thanks.

[Ruhanga]

Hi @wluyima. Happy new year!!

A kind reminder to have a look into this. Hopefully all is fine.

[wluyima]

Added comments

[Ruhanga]

I've addressed the comments @wluyima, could you review again? Thanks.

[wluyima]

Please remove the stray format change in InterceptorUtil before merging the PR

[Ruhanga]

Thanks @wluyima, I've addressed the final comments. I hope this can now be merged.... :-)

[wluyima]

There is one final comment, the PR is already approved and can be merged