JAVA-2982: Switch Esri Geometry API to an optional dependency

[toretre]

Switch Esri Geometry API to an optional dependency

When using the OSS driver it looks like dependency 'esri-geometry-api' is not needed and could be made optional much like Tinkerpop in #1522 ?

The reason for wanting this change is that 'esri-geometry-api' pulls an old version of Jackson library. I tried update the Esri dependency, but it will fail the serialization tests.

[absurdfarce]

Thanks for the contribution @toretre! Unfortunately I've been swamped with other work recently but I'm trying to make my way back to PRs for the Java driver (including this one!) soon.

Have you signed the Contributor License Agreement for contributions to DataStax open source projects? If not you can find it at https://cla.datastax.com. Thanks!

[absurdfarce]

Apologies for the delay @toretre; it's been really busy lately. But I have spent some time looking into your PR and have some thoughts to share... let's see where this takes us. :)

Updating the ESRI dependency does indeed break the serialization tests, but the differences there are pretty minor. The tests in question are for LineString, Point and Polygon but the changes are pretty small:

Expected :"{"type":"LineString","coordinates":[[30.0,10.0],[10.0,30.0],[40.0,40.0]]}"
Actual   :"{"type":"LineString","coordinates":[[30,10],[10,30],[40,40]],"crs":{"type":"name","properties":{"name":"EPSG:4326"}}}"

Expected :"{"type":"Point","coordinates":[1.1,2.2]}"
Actual   :"{"type":"Point","coordinates":[1.1,2.2],"crs":{"type":"name","properties":{"name":"EPSG:4326"}}}"

Expected :"{"type":"Polygon","coordinates":[[[30.0,10.0],[10.0,20.0],[20.0,40.0],[40.0,40.0],[30.0,10.0]]]}"
Actual   :"{"type":"Polygon","coordinates":[[[30,10],[40,40],[20,40],[10,20],[30,10]]],"crs":{"type":"name","properties":{"name":"EPSG:4326"}}}"

Really there are only two differences that I can see here:

1. Integer values are always displayed with no decimal components
2. The "crs" field is included in each of the generated maps

The arguments in favor of upgrading are significant:

1. Newer versions of the ESRI lib have brought JSON parsing in-house, completely removing the Jackson dependency
2. In general it's nice to keep up with new versions of dependencies

That first point is quite significant. We currently expose a "legacy" Jackson version as a dependency, apparently only to make ESRI happy. Getting rid of that all together may make any such effort worth it by itself.

Anyway, upshot is I'm working on seeing how bad it would be to simply do the upgrade. It's my understanding @toretre that this would address your situation just as well... do I have that right?

More to come on this.

[toretre]

Hi, thanks for looking into this issue @absurdfarce !

The serialization test issues is the same I saw if I recall correctly. I see now from the git log that this change in serialization would be expected as there was a rewrite of this in Esri back in 2016 :

Esri/geometry-api-java#115

I think your proposed changes looks sound and would remove the references legacy Jackson 1.x library and leave us with Jackson 2.x library which we use in our project and can control the version of.

Back to the 'esri-geometry-api' dependency, it also looks from the git log that it was brought into the project at the same time as TinkerPop when DSE and OSS drivers was merged into the same module. It still looks like the dependency is still only used in the DSE driver of the driver and not of any use for the OSS driver, but I have not followed all code path and there could be usages that I did not spot.

Overall your proposed changes is all we need to get rid of Jackson 1.x without doing exclusions. I also agree it is good idea to update Esri to get rid of Jackson 1.x since it is no longer maintained.

Thank you !

[absurdfarce]

@toretre Unfortunately we hit a bit of a problem with the idea of upgrading to ESRI 2.x. Details are on the PR (#1577) if you're interested, but the overall implication is that we'll be moving forward with the idea of making this dependency optional instead.

I'll take another look at this PR with that additional context and see what we need to do to get this guy resolved!

Thanks for your patience!

[absurdfarce]

@toretre Would you mind pulling in the latest changes from upstream (the 4.x branch) so that we can easily merge this guy? Thanks!

[toretre]

@toretre Would you mind pulling in the latest changes from upstream (the 4.x branch) so that we can easily merge this guy? Thanks!

@absurdfarce

I have rebased against 4.x and hope it is correct.
I also found references for Esri in the code base around packaging of the code. Some comments suggest there could be done some changes since the dependency is optional, but I have not done any changes in that regard.
Example : https://github.com/datastax/java-driver/blob/d954af9d59c6493c78cbbcdfbada89c1e341087a/core-shaded/pom.xml#L353

Thanks!

[absurdfarce]

Thanks for the merge @toretre. All looks good there!

Don't worry about any changes surrounding packaging of the lib. I have a follow-up PR that addresses those issues as well as some other changes to get all tests passing again. Removing these dependencies from the lib and/or the dependency list caused a few tests to fail due to a ClassNotFoundException around the ESRI classes. My changes should clean that up.

I'll be merging this PR soon and creating the follow-up PR shortly thereafter. Thanks again for your efforts (and your patience) while we worked through these issues!

[absurdfarce]

Follow-up PR is #1580