FasterXML / jackson-datatypes-misc

Collection of common Jackson datatype modules not part of other multi-project repos
Apache License 2.0
22 stars 20 forks source link

Add Jakarta JSON Processing module #10

Closed Thihup closed 3 years ago

Thihup commented 3 years ago

Fixes #6

Basically, a copy of the jsr-353 module with the package rename from javax to jakarta.

cowtowncoder commented 3 years ago

Hmmh. How did I miss this one? Apologies... I just merged PR #11 which I think works around the same issue bit differently (by creating jar with different classifier).

I'll have to think about what to do beyond 2.12, however: Jackson 3.0 (from master branch) especially would likely benefit from simpler approach like this one. Classifiers work but add more maintenance hassle for downstream, I think.

Thihup commented 3 years ago

Sure. I can leave this PR open if you will or I can close it. Either way, having it solved is a good thing :)

cowtowncoder commented 3 years ago

@Thihup yup, thank you for contributing this and apologies for missing it (I don't think I got a notification, maybe I wasn't following my own repo). I will keep it open for now since it might be useful, just need to figure out longer term plan.

Thihup commented 3 years ago

Sure ;)

cowtowncoder commented 3 years ago

LOL.

So due to really nasty surprise of 2.12.2 release failing due to magic of jakarta-classifier (not sure if it's due to maven plug-in, release plugin, OSS Nexus or Nexus->MC sync), I think I'll at least remove the magic from jsr-353 and instead use this PR, try to re-release 2.12.2.

cowtowncoder commented 3 years ago

Published 2.12.2:

https://repo.maven.apache.org/maven2/com/fasterxml/jackson/datatype/jackson-datatype-jakarta-jsonp/2.12.2/

cowtowncoder commented 3 years ago

Hmmmh. Ok, so 2.12.2 is successfully published, and I was trying to add jackson-integration-tests unit tests for old and new modules. And looks like I found an unfortunate bug in Jakarta implementation: unless I made some rookie mistake, it looks like there's a bug in that if BOTH old and new providers, like:

    <dependency> <!-- old jsr-353 impl -->
        <groupId>org.glassfish</groupId>
        <artifactId>javax.json</artifactId>
        <version>1.1.4</version>
    </dependency>
    <dependency>  <!--  "new" jakarta jsonp impl -->
        <groupId>org.glassfish</groupId>
        <artifactId>jakarta.json</artifactId>
        <version>2.0.0</version>
    </dependency>

are in classpath, trying to create new instance fails with ClassCastException:

jakarta.json.JsonException: Provider org.glassfish.json.JsonProviderImpl could not be instantiated: java.lang.ClassCastException: org.glassfish.json.JsonProviderImpl cannot be cast to jakarta.json.spi.JsonProvider
    at jakarta.json.spi.JsonProvider.provider(JsonProvider.java:78)

This is not good, but I am not sure this module can do anything about it. :-/

I'll try to see if this has been reported somewhere.

cowtowncoder commented 3 years ago

Possible related (but likely not exact cause):

https://bugs.eclipse.org/bugs/show_bug.cgi?id=539939

Also verified that I can easily break the new module by simply including dependency to "javax.json" implementation. And although users can technically use excludes to (try to) elimintate that dependency, this is likely very confusing to users and will probably end up with this project getting a stream of bug reports for the problem.

I'd like to think people who conceived the "javax.json" -> "jakarta.json" move had the best intentions but it seems to result in a big heap of trouble for everyone involved. :-(

Thihup commented 3 years ago

I'll take a look

Thihup commented 3 years ago

So, looks like the following is happening: Only the API changed the namespace, not the implementation. So it leads to having the same implementation classes in two jars. Also, looks like both jars don't have the META-INF/services/jakarta.json.spi.JsonProvider. But it has a fallback to load the default implementation org.glassfish.json.JsonProviderImpl. In this case, as the Javax dependency is defined before the Jakarta, it tries to load the default implementation via ServiceLoader and doesn't find, it fallbacks to the default implementation, but it uses the old namespace, and then the ClassCastException happens.

For running in the module path, it is required to change the dependencies: use the API and the implementation with classifier, as here: https://github.com/eclipse-ee4j/jsonp/blob/master/bundles/ri/src/main/resources/README.txt

Probably it should be fixed if the jakarta.json included the META-INF/services, so it would found by the service loader and won't try the fallback implementation.

cowtowncoder commented 3 years ago

@Thihup right, it definitely looks like something that would need to be resolved by provider implementation or its use of metadata. Default implementation is a bad idea, to begin with, but if one is provided at least that should match api flavor.

But one thing that might be useful -- and something that I thought should have been there (not sure how I overlooked this) -- would be to add a constructor in JSONPModule (and also JSR353Module) -- one that takes JsonProvider as argument.

This way provider would not require use of icky SPI system, which (IMO) does not add much value but does add overhead and new failure modes.

I'd be happy to merge PR for such addition in 2.12 branch (or 2.13 whatever).

I'll file a new issue to match that.

Thihup commented 3 years ago

Sure ;)

cowtowncoder commented 3 years ago

So, added #12. Also realized that not only would this be good for users (main goal), but it would also allow fixing "jackson-integration-tests" problem; can pass provider there as well.

Thihup commented 3 years ago

I guess they released a bugged version. In the source code, they have the service provider file, but in the released jar it doesn't include it: https://github.com/eclipse-ee4j/jsonp/blob/master/impl/src/main/resources/META-INF/services/jakarta.json.spi.JsonProvider