-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#288 - Fix Global Properties loading for OpenMRS 2.7+ #289
Conversation
try { | ||
Method allowTypeHierarchy = XStream.class.getMethod("allowTypeHierarchy", Class.class); | ||
allowTypeHierarchy.invoke(xs, GlobalPropertiesConfig.class); | ||
allowTypeHierarchy.invoke(xs, GlobalProperty.class); | ||
log.debug("Successfully configured global properties config serializer with allowed types"); | ||
} | ||
catch (Exception e) { | ||
log.debug("Error configuring global properties config serializer with allowed types", e); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic seems to imply that it fails on older versions of core. We might not necessarily need a separate InitializerSerializer_2_7
, but it could be useful to have some unit test(s) enforcing the logic .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean about this logic seems to imply that it fails on older versions of core
. I don't think it implies that. But your point about having some unit tests that actually show this failure and prove that this fixes it is totally valid. I copied the test from the api
project (which tests using OpenMRS 2.1) into the api-2.7
project (which tests using OpenMRS 2.7). This test fails without my fix above, which is easily testable if you comment out lines 62 and 63 above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean about
this logic seems to imply that it fails on older versions of core
.
@mseaton, sorry for the confusion. I was just wondering why the error is logged instead of actually re-throwing a runtime exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see @Ruhanga . It is logged as it is assumed that any Exception thrown by this block is due to the fact that this method is not present in the Xstream library, which is an expected condition for any platform < 2.7.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, makes sense to me. Thanks @mseaton.
No description provided.