bug due to localized messages in setup wizard

Development
1

A bug was introduced by using the localized APIExceptions

this shows when you setup OpenMRS and for example enter a too short password

see bug

caused by

how can we prevent this going forward?

2

By improving our test coverage? :smile:

3

seems like this has already caused issues else where https://issues.openmrs.org/browse/TRUNK-5171

guess we are missing integration tests, otherwise this should have been seen.

can we come up with a list of classes that should themselves not use the localized APIException and also not use anything that does?

seems like a bad idea that we introduced this and use the Context in APIException

what alternatives do we have?

move responsibility further up the stack. for ex. into the service layer or even further in webapp’s themselves https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/context/MessageSourceAware.html

4

FWIW i have just back ported the fix for https://issues.openmrs.org/browse/TRUNK-5016 to 2.0.x

5

thanks for that, but I am 100% sure this bug is not fixed week password is just one example which I could easily reproduce.

another place in the code where this happened is here

and I’m pretty sure there is more. For example anything in Listener which calls multiple methods on OpenmrsUtil; and maybe some ServletFilters which also throw exceptions, …

please keep the bug open

6

Fair enough. I have reopened it. :slight_smile:

7

@darius @pushpa446 inviting you since reverting what causes the bug will affect your past request:

We need to revert a lot of the internationalized APIException creations since they break the OpenMRS setup where the MessageSourceService cannot be found.

This means UI’s expecting internationalized messages will not get them.

Solution ideas for the long run: I think the responsibility of internationalized messages should move away from the openmrs-core into the client apps (UI) which have json files of key/value pairs matching the exception codes; another solution might be to do this in web mvc controllers which can MessageSourceAware (so they are injected the MessageSource) + read the locale from the request (so we prevent the constant use of the static Context) and translate exceptions it passes upwards.

8

How about simply ensuring that spring is loaded even during the setup wizard? :slight_smile:

9

@teleivo this is my proposed fix: https://github.com/openmrs/openmrs-core/pull/2208/commits/2e19b522bb5614dbfeb7b4d51c77573600454975

10

I agree in theory with @teleivo’s point; I don’t care very much in practice which way we choose to go. (And I’m only skimming this thread.)

Note that if we were going to ask API consumer to translate exception messages they need to have access to raw message values (including the {0}, etc), and they’ll need both the specific message code and any variables to be substituted.

11

In platform 2.x, we moved messages from the web to the api layer. The pull request i have opened ensures that OpenMRS setup is not broken. How? By translating the messages in English. As a result, we do not have to revert any existing functionality that took advantage of the automatically localized exception messages, which happen after spring is started.

12

we can still keep these method signatures

public APIException(String messageKey, Object[] parameters)
public APIException(String messageKey, Object[] parameters, Throwable cause)

just not call the static Context to fetch the message. This in my opinion is best served closer to the request of the user (i.e. Controller, RestController or maybe even UI entirely)

13

thanks for your work! I havent had time to try it out, will do so on the weekend.

The problem I see is one in general of the extreme octopus like reach of the static Context. It makes testing a nightmare. I again stumbled on this when wanting to test the untested installation wizard and the servlet filters/Listener. We really need to make an effort of reducing these static methods and embrace spring and DI more.

Therefore I would hope we take more time to figure this out, so we can reduce the Context calls.

14

I fully agree with the need to avoid the static Context calls! :slight_smile: The pull request was just to fix the bug introduced for localized exceptions thrown before spring starts up.

15

While updating apache beanutils TRUNK-5192 Update apache beanutils by teleivo · Pull Request #2222 · openmrs/openmrs-core · GitHub

I am running into this again. in the ObsTest in tests which expect an APIExceptions to be thrown

Screen Shot 2017-07-30 at 11.49.01.png1526×1360 324 KB

the InputStream is null which causes an NPE

private DefaultMessageSourceServiceImpl() {
	InputStream stream = OpenmrsClassLoader.getInstance().getResourceAsStream("messages.properties");
	OpenmrsUtil.loadProperties(props, stream);
}

might be a combination of things: that its since the ObsTest is not a ContextSensitiveTest thus the DefaultMessageSourceServiceImpl is used + classes are loaded differently in tests and “message.properties” is not found because its not in the test resources or so.

It just confirms my gut feeling that we should really rethink translation of exception messages.

16

I fully agree with you in regards to thinking of a better way. :slight_smile: My commit was just a place holder such that at least an english translation is used instead of the application failing to start, for those cases when spring is not yet loaded.