search

FasterXML / jackson-datatype-jsr353

(DEPRECATED) -- moved under `jackson-datatypes-misc` https://github.com/FasterXML/jackson-datatypes-misc/
19 stars 14 forks source link

Support for `JsonPatch` and `JsonMergePatch` defined in JSON-P 1.1 #13

Closed regulskimichal closed 4 years ago

regulskimichal commented 5 years ago

JSR353 defines also JsonPatch and JsonMergePatch type. These types should not be instantiated via reflection, but using Json.createPatch() and Json.createMergePatch(), which are methods that use SPI. IMO library should provide support for these types.

cowtowncoder commented 5 years ago

First of all, thank you for suggesting this!

I assume this refers to a newer reversion of JSON-P, 1.1? 1.0 does not have these types, but:

https://static.javadoc.io/javax.json/javax.json-api/1.1/overview-summary.html

So this would require an update to javax.json-api dependency; probably reasonable, but needs to go in 2.11 as it is a change that should go in minor release.

Assuming that factory method (and serialization counterpart) is easy, which sounds like it is, I think this makes sense. And it might be quite easy to do, so tagging as "good first issue" to hopefully get contributors interested.

regulskimichal commented 5 years ago

You're right. It's actually defined in JSON-P 1.1 specification - JSR374 (BTW Apache Johnzon Project contains wrong statement on their project webpage). Maybe a better solution would be to create another module, which would extend this one instead of modifying this one. Also there is one more type which someone would like to use: JsonPointer

cowtowncoder commented 5 years ago

I think it's probably ok to add it here, even if it becomes bit of misnomer. Although we could also consider renaming of this repo and/or artifacts as jackson-datatype-json-p... but then again, due to bad naming decision by Oracle (IMO), that can be more, not less confusing (there is already older concept named "jsonp" that commonly gets mixed up) :)

regulskimichal commented 5 years ago

I've just created PR for this issue: https://github.com/FasterXML/jackson-datatype-jsr353/pull/14. I guess it resolves this problem. Please share your thoughts. Thanks.

regulskimichal commented 4 years ago

@cowtowncoder can we do something with this issue, according to RC of jackson-core?

Also, I updated the code and changed base in PR

cowtowncoder commented 4 years ago

@regulskimichal Apologies for totally missing follow up on this.

First thoughts: looks good, although I wish indentation etc was not reformatted as that makes diff more verbose. But that aside, the only real concern I have is that there are no unit tests to cover support for types. It would be great to have just sanity checks, and to show expected usage.

Another question I have is about having 2 separate modules. I can see why this might be kind of useful but curious whether it might just be simpler to extend existing module. One practical problem with 2 modules is that SPI for modules gets tricky: while both modules could be added, there's overlap. And if only one is to be included, need to decide which one. Then again, I am not big fan of SPI approaches anyway due to ambiguity and too much hidden magic, so maybe this is not a big deal (could just leave old module as the only thing registered via mapper.findAndRegisterModules()).

But beyond that, I am happy to get this merged. But before that, one practical thing I need (if I didn't ask for it yet -- apologies if I did): CLA. It can be found here:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

and is easiest usually to just print, fill & sign, scan/take photo, email to info at fasterxml dot com.

With that (only needs to be done once for any contributions to Jackson projects), I could merge the PR.

Thank you again!

regulskimichal commented 4 years ago

Thank you for your commitment. I made some changes according to your remarks. Please check if it satisfies you. Meanwhile, I sent a CA to the company email box.

cowtowncoder commented 4 years ago

Excellent, thank you for providing these very quickly! I'll have a look and I think this will now make it into 2.11.0 (I am about finalize the release) which should be good.

cowtowncoder commented 4 years ago

Looks good: merged as-is.

One other thing: it would be great to add mention in README.md about functionality: I can add something mentioning extended functionality, but if you have a good idea of changes/additions (for this change, or generally), let me know (via PR or just comment here).

Once again: thank you for the contribution and apologies for slow follow up.

regulskimichal commented 4 years ago

JsonPatch and JsonMergePatch are defined as interfaces so the implementation class depends on library implementing it. Thanks to these deserializers, it is possible to use these types in much easier way without boilerplate.

I needed this feature, because thanks to this module, I will be able to use JsonPatch and JsonMergePatch directly in Spring HTTP method handlers.

I don't feel comfortable to write documentation in English, so you can use this information as you want.

cowtowncoder commented 4 years ago

@regulskimichal Thank you; that is fine, I can add something based on this!

One other note: I will likely move JSR-353 datatype module under new multi-project repo:

https://github.com/FasterXML/jackson-datatypes-misc/

for 2.11. Will update READMEs here, but thought I will mention it now so you won't be surprised (or if there are changes to be made).

regulskimichal commented 4 years ago

I got two more thoughts about this:

  1. Since in dependencies we are using javax.json-api 1.1, we actually implementing interoperability with JSR 374
  2. What if we could provide patching functionality I was looking for directly in Jackson core?
cowtowncoder commented 4 years ago

yes, it could be considered jsr-374 possibly now.

As to including support in core: jackson-core and jackson-databind intentionally keep their dependencies minimal, and I do not want to add new dependency there. If there was a way to abstract things out (i.e. not to depend on jsr implementations), it might be possible to support patch/merge-patch (I think specified in an RFC or two?), however. If so there could be jackson-native implementation bundled.

regulskimichal commented 4 years ago

https://tools.ietf.org/html/rfc6902 and https://tools.ietf.org/html/rfc7386. Yes, I am thinking about jackson-native implementation of patching process.

cowtowncoder commented 4 years ago

@regulskimichal Ah! Yes, that could make sense, although not sure if to ObjectMapper, ObjectReader or ObjectWriter. Theoretically could even have something besides ObjectReader/-Writer, of course, created by mapper (to pass configuration settings).

regulskimichal commented 4 years ago

I guess this feature is quite useful (makes PATCH method in REST-style app easy to implement) and quite not well-known, but should be.

When I will have more time I can provide some commits to make it possible, also with Kotlin extensions.