Jackson-3 compatibility / upgrade

[timothyjward]

There doesn't seem to have been any activity for the last two years, even merging already accepted pull requests. Would there be any value in me attempting to provide improvements here, or do I need to create a fork to make improvements (e.g. providing support for using Jackson 3)?

[randallwhitman]

Regarding Jackson dependency version: If made compatible with Jackson-3, would it remain compatible with Jackson-2? vs, would it cease to be compatible with Jackson-2.

[timothyjward]

The move to Jackson 3 is breaking and uses different package names. The only way to maintain compatibility with both would be to extract the function behind an SPI. This would open up the possibility of using other JSON implementations as well as Jackson 2 or 3.

A less good option would be to keep the adapted functions inside the main module and use optional dependencies, dynamically selecting an implementation at runtime, however this doesn’t usually work brilliantly as you have to manage dependencies very carefully to remain truly decoupled and “optional”.

A third (and possibly good) option would be to break the JSON dependent function into a separate module, leaving a core module with no json dependencies and a json extension which could be made pluggable or not. It seems as though the JSON parts of the geometry API would lift out pretty easily as they don’t relate to any of the actual geometry calculations, just to parsing and serialising certain representations.

I’d be happy to look into one of these options if there is actually an appetite for the update.

[stolstov]

@timothyjward You can implement JSONReader and JSONWriter around any parser.

Here is example of wrapping org.json: https://github.com/Esri/samples-geometry-api-java/blob/master/JsonObjectReader/src/main/java/com/esri/core/geometry/examples/JsonObjectReader.java

[timothyjward]

You can implement JSONReader and JSONWriter around any parser.

This would make it very easy to extract the existing dependency into a separate module and add others to support Jackson 3 and any other json parsers that people want to use. My questions are:

1. Would this be an acceptable change?
2. If so, would you prefer to extract all of the JSON dependent code into a separate module as well so that the core function can be used without JSON support, or is it so integral that it should remain part of the core?

[randallwhitman]

From my perspective that should be fine, so long as it continues to work with Jackson-2, which is used by most if not all of: Spatial Framework for Hadoop, Apache Spark, Apache Sedona, GeoAnalytics Engine ...

[stolstov]

@timothyjward The library has convenience methods that read JSON from string, so it needs to depend on some parser.
There are two places that consume JsonParser directly apart from the JsonParserReader. Both of those places provide an overload that accepts JsonReader. I suspect that the overloads that accepts JsonParser can be safely removed, for example

	public static SpatialReference fromJson(JsonParser parser) throws Exception { <<<< can it be removed?
		return fromJson(new JsonParserReader(parser));
	}
	
	public static SpatialReference fromJson(String string) throws Exception {
		return fromJson(JsonParserReader.createFromString(string));
	}

However, the library does historically provide the JsonParserReader implementation of the JsonReader as a public class and it is probably being used.

[stolstov]

The driver for the changes here is Spatial Framework for Hadoop. So if Randall does not need the new Jackson, I would not want the changes too.

The library is maintained: if a geometry bug is found, it would be fixed, if Jackson upgrade is required due to critical security issue we'll upgrade it. New features - not so much.

[timothyjward]

However, the library does historically provide the JsonParserReader implementation of the JsonReader as a public class and it is probably being used.

There may be a difficulty here as the JsonParserReader is in the main geometry package. Splitting packages across multiple jars is a bad practice, and forbidden by Java 9 modules. It would also cause issues in frameworks such as OSGi. That would mean that the JsonParserReader implementation would have to change package as well as removing the two method signatures taking a JsonParser.

Making this change would be a good long-term benefit, but I don’t see a way to do it as a non-breaking change.

The library is maintained: if a geometry bug is found, it would be fixed, if Jackson upgrade is required due to critical security issue we'll upgrade it. New features - not so much.

This is useful to know, thank you. As I have mentioned above I’m happy to put some effort into making the upgrade (which I need for my deployment use case) as long as feature contributions are being accepted. If the project is truly critical bug/security fix only, and wouldn’t accent a feature contribution, then I may need to look at alternatives.

[randallwhitman]

Looking at Spatial-Framework-for-Hadoop, it does indeed call SpatialReference.fromJson(JsonParser) --
https://github.com/Esri/spatial-framework-for-hadoop/blob/7f37ef425502be12eecc22b62f54525d744cd06a/json/src/main/java/com/esri/json/deserializer/SpatialReferenceJsonDeserializer.java#L24
If that is identified as a hurdle to eventual upgrade to Jackson-3, we would do well to assess our options for the path forward.

As for this Geometry library itself: if jackson-3 compatibility requires a breaking change, and a breaking change requires major version, then it would require a v3 major version. It might be awkward to have a major version motivated only by a dependency upgrade and no major new features, but i don't see it as a showstopper.

[randallwhitman]

for convenient reference -- https://github.com/FasterXML/jackson/blob/main/jackson3/MIGRATING_TO_JACKSON_3.md

[randallwhitman]

Is it feasible to introduce shim modules for Jackson? One would compile under Jackson-2 only, the other under Jackson-3 only. Exactly/only one would be built/used at a time, per a maven profile. The primary geometry module would call shim methods, rather than directly calling Jackson constructors/methods that change between v2 to v3. Let me know your thoughts on that.

(Note: at above-linked issue, de-coupling Spatial-Framework-for-Hadoop from Jackson-2 is being evaluated.)

[randallwhitman]

I'm not sure whether it would be considered a sub-category of critical/security bug fix, versus a separate category, but in any case, the Spatial Framework for Hadoop has kept compatibility with dependency upgrades. Changes for Hadoop and especially Hive upgrades can be seen in its commit history. I'd think of Jackson version compatibility in the same way as versions of Hive, Hadoop, etc.