Update the MC fork of BasicRowProcessor to maintain column order

[jonbartels]

There was a lively discussion in Slack to help a user identify why his DB rows in XML were not coming back in the same order defined by his query.

We discovered that the the MC fork of org.apache.commons.dbutils.BasicRowProcessor is out of date or not consistent with the upstream Apache project.

If we look at the implementation of CaseInsensitiveMap in MC: https://github.com/nextgenhealthcare/connect/blob/development/server/src/org/apache/commons/dbutils/BasicRowProcessor.java#L176

And compare to the one in the upstream project: https://github.com/apache/commons-dbutils/blob/master/src/main/java/org/apache/commons/dbutils/BasicRowProcessor.java#L195

We see that the latter is based off of a LinkedHashMap which will preserve order while the MC fork is a plain HashMap. This means that the column order in the XML or JSON row representation is dependent on the vagaries of HashMap key ordering. This issue shouldn't be a problem at runtime, but seeing columns in order in the XML or JSON representations is helpful to interface engineers.

The apparent solutions are ONE OF:

• update the MC fork of the BasicRowProcessor against the latest stable upstream
  OR
• include the latest stable upstream DBUtils dependency and eliminate the MC fork

[jonbartels]

Further digging shows that commons-dbutils-1.4.jar is included as a dependency. Diffing the two classes shows this comment:
/* * Currently, duplicate keys/aliases would get overwritten in the resultMap, so we * throw an exception to keep the message from processing since it would contain * incomplete data and so that the user can have a chance to modify the query and * select those records again. In the future, we plan to allow duplicate field names * (MIRTH-3138). */

This points back to:

• DatabaseReceiver: Allow duplicate keys/fields in the result XML #3051
• Database Reader generates errors selecting multiple columns from a single table in a SQL 2005 database when one of those columns is of type TEXT. #1075

That explains why the fork is necessary.

Even if the fork is necessary can it or should it merge in the updates and features from a newer release of DBUtils?

[narupley]

This was a very old override, and you're right we need to merge in updates from the newer version of the library. We make sure to do this for other overridden classes like ones from Rhino, but this one has obviously slipped by.

[jonbartels]

I think I found an alternative place to implement the desired behavior. The BasicRowProcessor is only ever used once at com.mirth.connect.connectors.jdbc.DatabaseReceiver#processResultSet

Instead of checking the map keys at put time in BasicRowProcessor, could the ResultSetMetadata be used in processResultSet to check the resultSet column names in a case-insensitive way as a precondition?

The problem this prevents is that a user has a columnA and ColumnA and CoLuMnA, which would be represented in the map as the same key "columna" and thus lose data.

I think that would maintain the necessary error handling and eliminate a forked class.

I would be keen to do a PR for this if that would help.

[jonbartels]

This also impacts com.mirth.connect.connectors.jdbc.DatabaseReceiver#processResultList which uses the BasicRowProcesssor implementation of CaseInsensitiveMap which is private in DBUtils but public in the NextGen fork.

[cturczynskyj]

We ended up removing our custom, overridden, org.apache.commons.dbutils.BasicRowProcessor class and updating the DatabaseReceiver to handle our special needs ensuring the columns are returned in the expected order.

[tonygermano]

We ended up removing our custom, overridden, org.apache.commons.dbutils.BasicRowProcessor class and updating the DatabaseReceiver to handle our special needs ensuring the columns are returned in the expected order.

@cturczynskyj thanks for sharing the details!