Skip to content
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

StringToObjectConstructor now can create object by its constructor. #183

Merged
merged 3 commits into from Mar 4, 2015

Conversation

@khaing211
Copy link
Contributor

@khaing211 khaing211 commented Jan 31, 2015

Signed-off-by: Khai Nguyen khaing211@gmail.com

Signed-off-by: Khai Nguyen <khaing211@gmail.com>
@rhuss
Copy link
Owner

@rhuss rhuss commented Feb 2, 2015

Thanks a lot for your pull request, I think something like that would be a quite addition for upstream serialization.

However there are some issues:

  • A minor one, Constructor.getParameterCount() is only available since Java 8 but Jolokia still supports Java 7. The fix is trivial, though.
  • More seriously, the change is not backwards compatible. E.g. for Date we a have a own extractor DateExtractor which converts strings to dates. However, Date has also a single string argument constructor, so your patch would pick that one first which results in a complete different behavior for converting string to date. Maybe we can move the constructor based conversion to the very end of the chain, so that it least its backwards compatible.

I can have a look at the last issue, too, but I'm currently quite time constrained. Maybe you can fix that ? (e.g. I believe the logic should go into convertFromString() at the end).

Also, could you please update the documentation (in protocol.xml, section serialization-request) ? That would be awesome ....

khaing211 added 2 commits Feb 2, 2015
+ Move convertByConstructor() to the end of convertFromString()

Signed-off-by: Khai Nguyen khaing211@gmail.com
@khaing211
Copy link
Contributor Author

@khaing211 khaing211 commented Feb 3, 2015

Thank for your review. I updated per your suggestion.

@rhuss rhuss merged commit 8b0a8d7 into rhuss:master Mar 4, 2015
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@rhuss
Copy link
Owner

@rhuss rhuss commented Mar 4, 2015

Thanks a lot !

It will be available in the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.