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

Adds annotations to customize field names during Bean serialization #406

Merged
merged 2 commits into from
Mar 15, 2018

Conversation

johnjaylward
Copy link
Contributor

@johnjaylward johnjaylward commented Mar 7, 2018

What problem does this code solve?
Allows users to customize key names when performing Java Bean conversion into a JSONObject. This is in reference to #401. This PR also has support to force the serializer to ignore bean getters if a specific annotation is provided.

Risks
LOW. Should not affect existing programs. If an existing program does require a field to be ignored, then the new annotation can be used to force it out of the resulting JSONObject result.

Changes to the API?
Two (2) new annotation classes were created that allow customization of field names when used on Bean getters.

Example:

public class MyBean {
    @JSONPropertyName("myStringField")
    public String getSomeString() { return "someStringValue"; }
    @JSONPropertyIgnore
    public String getSomeString2() { return "someStringValue2"; }
}

new JSONObject(new MyBean());

the output of the above JSONObject should look like this:

{
    "myStringField" : "someStringValue"
}

Will this require a new release?
yes

Should the documentation be updated?
No, This PR updates both the Javadoc and the README where needed.

Does it break the unit tests?
No. New tests have been added to support the new annotations.

Was any code refactored in this commit?
Yes. Some private functions were created to supplement the populateMap method and support the new Annotations.

Review status
ACCEPTED

@johnjaylward johnjaylward changed the title Adds annotation to support custom field names during Bean serialization Adds annotation to customize field names during Bean serialization Mar 8, 2018
@johnjaylward johnjaylward changed the title Adds annotation to customize field names during Bean serialization Adds annotations to customize field names during Bean serialization Mar 8, 2018
@stleary
Copy link
Owner

stleary commented Mar 11, 2018

Nice work on the the project's first attempt with annotations!

  • Since this is only used for Bean getters, we might be able to come up with a better name than JSONPropertyName, which seems a little ambiguous, and overly similar to Jackson's JSONProperty. JSONBeanName? I don't know, that actually sounds worse...
  • There is no use case for JSONPropertyIgnore. Needs to be removed.
  • JSONObjectTest testGenericIntBean() failed because the new code considers 'able' to be a key. This is a regression caused I think by removing the uppercase check for key.charAt(0). Needs to be fixed.
  • Method doc for getAnnotationDepth() does not match actual return type. Although I think this method goes away if JSONPropertyIgnore is removed.

@johnjaylward
Copy link
Contributor Author

johnjaylward commented Mar 11, 2018

  • For the annotation name, I just used the one that was mentioned in Customize the default key naming logic/process in generated json from getters #401, but updated the case to match the rest of our class names. I was unaware of the property name from Jackson. I'm open to a better name though, I didn't think that hard about it and am not attached.
  • For JSONPropertyIgnore
    • I currently use a similar feature to make sure that my User beans don't emit the password field. Even though they are hashed, I still don't want it transmitted to the clients.
    • I also use them to skip of child lists that I require other REST actions to fetch separately: public List<Attachment> getAttachments() for example.
  • The regression was noted, and I updated the test case in New test cases for Bean Name customization JSON-Java-unit-test#84. I also feel that we probably shouldn't care about the casing there. In my private fork of the project, I encode everything that starts with "get", "has", or "is" regardless of casing.
  • Fixed the "return" javadoc for the depth method.

@johnjaylward johnjaylward force-pushed the FixBeanKeyNameing branch 2 times, most recently from 35d05b0 to df7c7e4 Compare March 11, 2018 19:39
@stleary
Copy link
Owner

stleary commented Mar 11, 2018

No preference on the name either, unless someone comes up with something better, and OK to proceed with JSONPropertyIgnore, since there are people who will use it.
No regressions please. Existing behavior has to be retained.

@johnjaylward
Copy link
Contributor Author

added in the skip over keys that start with a lowercase, but only in the event that the annotation is not present.

@stleary
Copy link
Owner

stleary commented Mar 11, 2018

Accepted, starting 3 day comment window.

…e tagged with the new JSONPropertyName annotation.

Also updates the javadoc to reflect the new name allowances
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants