So, John.doe and john.doe are same person?

@david-labs-ca, there's discussion on the original ticket.

I think the world at large usually assumes usernames to be case-insensitive. It's also just plum broken on several case-insensitive database engines in main, so if the decision were to make them case sensitive, a change would still be required.

@david-labs-ca, there's discussion on the original ticket.

I think the world at large usually assumes usernames to be case-insensitive. It's also just plum broken on several case-insensitive database engines in main, so if the decision were to make them case sensitive, a change would still be required.

I get angry any time I encounter a system with case-sensitive usernames. I expect to always be able to type my username in all lowercase, regardless of how the admin added it to the system.

suggestion: don't assume anything about which charset, collation, or locale are in use

I think it would be a good idea to use LOWER(USERNAME) = LOWER(#{username}) across all databases, and if that returns multiple results because a case-sensitive collation is in use and there are similar names, then fall back to USERNAME = #{username}.

@tonygermano
Re: collation, at a certain level every string compare is done in the context of a collation, implicit or explicit. UPPER and LOWER also produce sargability challenges. In my experience, you will make more DBA friends with COLLATE than with LOWER(...). The only reason I did not use COLLATE uniformly is to avoid DDL changes.

Re: existing users, I've added a fallback to match case-sensitive if multiple users match. Adding a new user with a different casing fails with com.mirth.connect.client.core.ControllerException: Error adding user: username must be unique

@tonygermano Re: collation, at a certain level every string compare is done in the context of a collation, implicit or explicit. UPPER and LOWER also produce sargability challenges. In my experience, you will make more DBA friends with COLLATE than with LOWER(...). The only reason I did not use COLLATE uniformly is to avoid DDL changes.

@mgaffigan I don't think the PERSON table is expected to get very large where a scan would be a problem, and this seems evident because there isn't an index on USERNAME. In this case, I think it's better to be safe and use the standard SQL function without making assumptions about the environment rather than going for performance. LOWER should work across all databases respecting their current collation, character encoding, and locale.

This is completely up to you, but if you want to address this issue, too, it's related nextgenhealthcare/connect#3386

@tonygermano, I still think COLLATE is the correct way to do this - but I've updated it to use LOWER(...). Let me know if you see any blockers.

@mgaffigan for completeness, what are your thoughts on updating mysql-user.xml and sqlserver-user.xml, too? Though it's not needed for a default configuration, both of these servers can be configured to be case sensitive.

@mgaffigan for completeness, what are your thoughts on updating mysql-user.xml and sqlserver-user.xml, too?

If it will get the PR merged, I'm unopposed.

I do not think it is called for, but I do not like lower to start with. If those default collations are changed, the system is likely running with locale specific collation that is more correct than what lower would provide. Running with a non-default collation is also something I would call an unsupported configuration.

From a maintenance perspective I think it is counterproductive to keep the different database server scripts in sync: I've never found SQL to be an actually portable language at scale (DDL aside, the query engines are too different). The appropriate interface for changing persistence is at the DAL - not at SQL. This also allows in memory stores for unit tests, no-SQL, etc.

I don't know how this will affect the user roles plugin from NG - maybe we don't care?

We don't care. This will release in a version number unsupported by any open source NG release. With their plugin architecture change - bwc won't work either.

I don't know how this will affect the user roles plugin from NG

@pacmano1, I can't see how it would be affected. No API changes in this PR, and the case sensitive username is still used after login.

@NicoPiel

Do we have any call paths where getUser(userId, userName) can be invoked with both args null?

I don't follow. There's already a check to throw an exception when both are null. Perhaps I'm not understanding which getUser you are referring to.

@kayyagari

The use of LOWER function made me pause for a while, but that is not a big deal, considering the number of rows person table contains in production deployments.

I agree. See prior discussion and edits away from COLLATE.

The use of LOWER function made me pause for a while, but that is not a big deal, considering the number of rows person table contains in production deployments.

I agree. See prior discussion and edits away from COLLATE.

@kayyagari @mgaffigan Yes, it was edited for Oracle db at my request. I concede that it's not the most efficient method, but I think it is the most forgiving of whatever settings the users happen to have configured on their db (especially for non-English databases or alternate character encodings.) I think that's ok considering the likely size of this table, the fact that it isn't indexed in the first place, and the low frequency of this call.