adapting OpenmrsUtil.join handling of null

Development
1

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
2

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

2 Likes
3

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.

4

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

5

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.

6

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.)

7

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

The issue is here

https://issues.openmrs.org/browse/TRUNK-5368

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.