-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Added support for JSON-B #1385
Added support for JSON-B #1385
Conversation
Excuse my impatience. Is there any chance that this pull request will be confirmed/merged soon? I can't wait bringing JSON-B POJO generation into action. |
Do we actually need to support JSONB1 at this point or is it already effectively obsolete? If we support the annotator it needs to be changed and supported over time. Makes me think that maybe we should just support |
I see your point and would wish I could say JSON-B 2 would be enough. Unfortunately, JSON-B 2 seems to be supported by Jakarta EE 9 on only. Jakarta EE 9 in turn is supported by Payara 6 on, which is not even available at this point. So, I don't think we come around JSON-B 1 at this point. |
Makes sense @TheTrueDentist, thanks! |
import java.util.Iterator; | ||
|
||
/** | ||
* Annotates generated Java types using the Jackson 2.x mapping annotations. |
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.
Looks like a copy/paste from Jackson annotator
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.
Yeah, I based my work on the Jackson annotator as the annotations and handling are quite similar, although not entirely the same. At the end, i copied JSON-B-annotator v1 for v2. This ways, I forgot to fix the comments and then copied the errors from one version to the other, sorry. I guess the iterator could also be improved by using for-each.
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.
Fixed the comments in both files. Iterator is fine in this case IMO.
import jakarta.json.bind.annotation.JsonbTransient; | ||
|
||
/** | ||
* Annotates generated Java types using the Jackson 2.x mapping annotations. |
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.
Looks like copy/paste from Jackson annotator
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.
See above.
I think there's a couple of integration tests missing here, to actually check whether the way these types are annotated is correct. Do the types we produce work correctly with JSON-B, to bind JSON data that is valid according to the schema? If you check out the GsonIT and MoshiIT, you can see a round trip test that shows you can parse a JSON file and bind the data to some types, and that you can produce the same JSON using the values. You can use the same two JSON snippets. So the tests would look like: @Test
public void annotationStyleJsonb1MakesTypesThatWorkWithMoshi1() throws ClassNotFoundException, SecurityException, IOException {
ClassLoader resultsClassLoader = schemaRule.generateAndCompile("/json/examples/", "com.example",
config("annotationStyle", "jsonb1",
"propertyWordDelimiters", "_",
"sourceType", "json",
"useLongIntegers", true));
assertJsonRoundTrip(resultsClassLoader, "com.example.Torrent", "/json/examples/torrent.json");
assertJsonRoundTrip(resultsClassLoader, "com.example.GetUserData", "/json/examples/GetUserData.json");
}
@Test
public void annotationStyleJsonb2MakesTypesThatWorkWithMoshi1() throws ClassNotFoundException, SecurityException, IOException {
ClassLoader resultsClassLoader = schemaRule.generateAndCompile("/json/examples/", "com.example",
config("annotationStyle", "jsonb2",
"propertyWordDelimiters", "_",
"sourceType", "json",
"useLongIntegers", true));
assertJsonRoundTrip(resultsClassLoader, "com.example.Torrent", "/json/examples/torrent.json");
assertJsonRoundTrip(resultsClassLoader, "com.example.GetUserData", "/json/examples/GetUserData.json");
}
@SuppressWarnings({"unchecked", "rawtypes"})
private void assertJsonRoundTrip(ClassLoader resultsClassLoader, String className, String jsonResource) throws ClassNotFoundException, IOException {
Class generatedType = resultsClassLoader.loadClass(className);
String expectedJson = IOUtils.toString(getClass().getResource(jsonResource));
JsonAdapter<Object> jsonAdapter = moshi.adapter(generatedType);
Object javaInstance = jsonAdapter.fromJson(expectedJson);
String actualJson = jsonAdapter.toJson(javaInstance);
assertEqualsJson(expectedJson, actualJson);
} |
I have provided ITs as suggested. Note that I have splitted the test class into two, one per version of JSON-B to decrease confusion. Please also note, that I had to use two different JSON-B implementations. This is because I would have Yasson 1 for JSON-B 1 and Yasson 2 for JSON-B 2 (Yasson 2 is seemingly not backwards compatible). When I tried to set that up, things did not work, wrong version of yasson was used. So I tricked it out and used Johnzon for JSON-B 1. |
So, everythings fine now? |
I have noticed that you have published version 1.1.2 of the codegenerator. I have merged master into the branch of this pull request, so everything should be up-to-date again. Please let me know if there is something I can do to speed up things. |
There seems to be a status report required about test result on Java 8 under Ubuntu 18.04 before this can be pulled. I have no Ubuntu 18.04 at hand, but under 20.04 with Java 8 tests are all green. Of course, if Ubuntu 18.04 is the reference here, tests should be run there as well. Also, I see that you want to check the results yourself. Still, the setup I have used makes me confident to state: I would not expect any problems under Ubuntu 18.04 with Java 8. |
Just had another look through this one and I think the last thing that needs to be done is an update to some docstrings:
|
You're right, of course. Documentation should be fixed now. |
* Added support for JSON-B * Fxied comments * Improved on ITs * Fixed documentation Co-authored-by: Daniel Kepes <[email protected]>
Is milestone 1.1.3 about to be released this year? |
My suggestion for JSON-B-support. I have provided two annotators for JSON-B 1 and 2. They differ only in the package of the annotations though. Alternatively, one could provide a baseclass, too, and let the two extending classes only provide the annotation classes. Thiw, however, would lead to a close coupleing of both versions. Another alternative would be to have the subversion be specified by the configuration of the plugin and provide one class that handles both versions by respecting the configuration. Thought I leave that decision up to you.