adapting OpenmrsUtil.join handling of null

Hi there,

I just added some unit tests to OpenmrsUtil.join which is used in core only to join a list/set of modules to a string so it can be shown in a error message.

For example in MandatoryModuleException we use it to compose error message

"The following modules are marked as 'mandatory' but were unable to start: "
	        + OpenmrsUtil.join(moduleIds, ",")

I thought of replacing our logic in OpenmrsUtil.join with StringUtils.join which does almost exactly the same, except for two edge cases:

found two edge cases, were transform to (string on the right is the result):

  1. if an element in the list is null in a list like "module1",null,"module2" --> "module1,null,module2"
  2. if you choose a separator of null and have a list of "module1", "module2" --> "module1nullmodule2"

StringUtils.join replaces a null separator or null elements with an empty string, which would turn to

  1. “module1”,null,“module2” --> “module1,module2”
  2. if you choose a separator of null and have a list of “module1”, “module2” --> “module1module2”

Does that change make sense to you?

1 Like

Yeah OpenmrsUtil.join doesn’t make much sense. Better use Java 8’s in-built String.join(delimiter, elements) though.

2 Likes

thanks for the tip @gayanw! Java’s built-in would still behave the same as described in edge case 1

Note that if an individual element is null, then "null" is added.

not sure if we want that or not.

Am happy to hear more opinions.

If the edge case is not important (it doesn’t break compatibility) I’d vote to deprecate it and use String.join

Ohh I see. It would be even better to wrap it within OpenmrsUtil.join in case we want to make future changes without much penalty. And to provide single interface for all native OpenMRS code.

I probably wrote OpenmrsUtil.join…sorry!

I expect this is now used by various modules, so you’ll want to deprecate it and leave around an implementation that delegates to some other library.

I feel that it’s okay to change the null behavior. (Personally I would not expect to have a “convenience” removeNulls happen, but I don’t think it’s important.)

no need to be sorry, it does work, and now there are other solutions :slight_smile:

The issue is here

I deprecated it, but didnt change the implementation because I don’t think it is really necessary. I added tests for OpenmrsUtil.join before and it works the way it works for some time already. Now with the deprecation notice/hint developers can use String.join from java or StringUtils.join if needed instead.