Jakarta JSON-P support. #56
Conversation
|
Thank you for the contribution. I will look it through as soon as I can. Could you tell me more about your use case in the meantime? It's interesting to hear what people are using this library for. |
|
A solution I am working on stores and serves JSON documents. I am playing with JMESPath for JSON transformation functionalities, both for document import / export as for creating alternate / partial document views. I think it can be composed with something like JSON Patch. |
There was a problem hiding this comment.
I made some inline comments about two methods that should be removed, but otherwise this looks good.
I have a small nitpick too, do with it as you want: the rest of the code in this repo does casts like this ((String) value) but this PR does ((String)value) (i.e. space between the type and variable).
Good job overall, it's very nice to see how it's possible to support another library with this little effort.
| return Json.createValue(n); | ||
| } | ||
|
|
||
| private JsonValue useJsonNull(JsonValue value) { return (value == null) ? JsonValue.NULL : value; } |
There was a problem hiding this comment.
What's the difference between this and nodeOrNullNode? They look identical to me.
There was a problem hiding this comment.
I can't believe I did this!
| } | ||
| } | ||
|
|
||
| private String textOnStringOrNull(JsonValue value) { |
There was a problem hiding this comment.
This name confuses me, specifically the "on" part. Could it be called stringOrNull or textOrNull?
There was a problem hiding this comment.
I've looked at the places where this method is used, and I think it's in fact not needed. The callers (createObject and getProperty) don't need to call this method.
There was a problem hiding this comment.
I replaced it by the toString(key) call.
There was a problem hiding this comment.
(Still being a bit paranoid here.)
| public JsonValue createObject(Map<JsonValue, JsonValue> obj) { | ||
| JsonObjectBuilder builder = Json.createObjectBuilder(); | ||
| for (Map.Entry<JsonValue, JsonValue> entry : obj.entrySet()) { | ||
| String key = textOnStringOrNull(entry.getKey()); |
There was a problem hiding this comment.
Can key really be null here? That's as far as I know not allowed in JSON, and the docs for the createObject method in Adapter says "The map keys must all be string values."
The Jackson adapter does not check for null in its implementation of this method so it's safe to skip the check.
| @Override | ||
| public JsonValue getProperty(JsonValue value, JsonValue name) { | ||
| if (value.getValueType() == OBJECT) { | ||
| return nodeOrNullNode(((JsonObject)value).get(textOnStringOrNull(name))); |
There was a problem hiding this comment.
How come the call to textOnStringOrNull here? The object can't have a null key. I think name.getString() should be sufficient here.
There was a problem hiding this comment.
it's very nice to see how it's possible to support another library with this little effort.
Indeed! It just took some hours (part of a day) and most of the effort was spent figuring out the right semantics of this API.
... and I do not benefit from that much experience with JSON, which kind of shows on the glitches you pointed out.
The test battery helps a lot!!!
There was a problem hiding this comment.
Congratulations. This library is a really easy both to use and to code for!
:)
|
Suggested changes applied. Please check if it is ok now. |
|
I went ahead and merged. I'll post here when I've done the release, it be a day or two, sorry. |
|
Thank you Theo, that is just fine. |
|
Released as v0.5.0. Sorry for the delay, and thanks again for the contribution! |
|
No problem. It was short enough for me. :) |
Add new JmesPath module to support Jakarta JSON-P 1.1.6.
It passes all tests.