JAVA-2982: Follow-up to recent PR making ESRI an optional dependency #1580

absurdfarce · 2022-01-07T21:29:54Z

Follow up to recent PR (#1575) making ESRI an optional dependency.

I've empirically validated that as of this change the following statements are true:

The core JAR contains no classes from the ESRI jar (expected... this was true before and shouldn't have changed in this effort)
The shaded JAR contains no classes from the ESRI jar (new)
- After 953c72f no classes from ESRI's transitive dependencies are included in the shaded JAR
Both the core and shaded JAR now declare an optional dependency on ESRI (new)
- ESRI's transitive dependencies are also optional as well
All unit and integration tests still pass

absurdfarce · 2022-01-07T21:54:02Z

core-shaded/pom.xml

+                  <groupId>com.esri.geometry</groupId>
+                  <artifactId>esri-geometry-api</artifactId>
+                  <version>1.2.1</version>
+                </additionalDependency>


javadoc generation was throwing warnings about missing classes without this declaration in place

absurdfarce · 2022-01-07T21:56:06Z

manual/core/integration/README.md

@@ -479,8 +479,9 @@ don't use any of the above features, you can safely exclude the dependency:
 Our [geospatial types](../dse/geotypes/) implementation is based on the [Esri Geometry
 API](https://github.com/Esri/geometry-api-java).

-Esri is declared as a required dependency, but the driver can operate normally without it. If you
-don't use geospatial types anywhere in your application, you can exclude the dependency:
+For driver versions >= 4.4.0 and < 4.14.0 Esri is declared as a required dependency,


The TinkerPop example explicitly referred to an inclusive range, but in this case I wanted the range to be inclusive on the low end but exclusive on the high end (in order to explicitly convey the idea that all drivers before the named version used a different behaviour). Since this changed the semantics slightly I thought that perhaps identifying versions via the >= and < syntax might be more explicit.

I'm open to other ideas of presenting this information if this isn't clear.

absurdfarce · 2022-01-07T21:57:57Z

upgrade_guide/README.md

+```
+
+See the [integration](../manual/core/integration/#esri) section in the manual for more details.
+


Very much based on the example used when TinkerPop was made optional.

I've explicitly added this in the 4.14.0 section under the assumption that changing a dependency from required to optional breaks backwards compat and thus requires at least a minor release.

adutra · 2022-01-17T19:15:10Z

core-shaded/pom.xml

@@ -146,7 +146,6 @@
                  -->
                  <include>com.datastax.oss:java-driver-core</include>
                  <include>io.netty:*</include>
-                  <include>com.esri.geometry:*</include>


I'm a bit hesitant about this. Making a dependency optional and shading it are different concerns.

Initially, the decision to shade a dependency was taken by looking at the likelihood of a version conflict for a given dependency. For Esri, we deemed it high because the driver requires a very old Esri version.

I think we should keep shading Esri even if it is optional, wdyt? This means that in the shaded jar Esri would be present and shaded, just like it is now. I can't see a situation where this could be a problem for the end user.

I see your point here... I'm just not sure I agree.

To be clear, the case you're describing is fairly precise. It would have to be something like the following:

The user's app uses ESRI 2.x for some other functionality in their app

The user's app also uses the (shaded) Java driver

The user runs this app against DSE

The user wants to leverage the geometric type support in DSE

This user will (possibly, even likely) experience a class conflict. But the cost for supporting this (quite limited case) is that every user of the shaded JAR has a larger JAR on their classpath. Initially it seemed like optimizing for the broad case made sense rather than doing so for the extremely precise case but I'm certainly open to argument.

every user of the shaded JAR has a larger JAR on their classpath

Well the shaded jar already has all of Netty in it, so adding Esri on top of it won't really make a noticeable size difference :-)

But OK, I don't mind if we stop shading Esri.

manual/core/integration/README.md

upgrade_guide/README.md

manual/core/integration/README.md

adutra · 2022-01-24T14:39:27Z

@absurdfarce we are almost good to merge this but I wanted first your input on this:

spring-projects/spring-boot#29387

What should we do with org.json:json-api? I guess it should be made optional, or even removed completely from our pom (if we can keep the OSGi tests passing).

absurdfarce · 2022-01-24T18:48:15Z

@adutra I looked at the Spring Boot issue briefly but I haven't really set out to address it specifically yet. We're still getting the transitive dependencies from the Esri lib (the JSON JAR and the older version of Jackson) included in the shaded JAR so I wanted to get that sorted out before considering the Spring issue (since they looked to be related).

More on that shortly (I hope).

absurdfarce · 2022-01-24T20:05:16Z

Okay, I believe all the relevant issues are sorted out now. Would you mind taking another look @adutra to confirm that we're good?

adutra

I manually verified that the OSGi tests pass against DSE with geotypes, so we are good wrt to the org.json dependency there. I also manually confirmed that org.json is not present neither in driver-core nor driver-core-shaded, so we are good there too.

However I just spotted two bnd directives in driver-core-shaded that should be removed since they reference packages that are not shaded anymore. Please have a look.

adutra · 2022-01-25T18:46:34Z

core-shaded/pom.xml

@@ -340,7 +312,7 @@
                  <!--
                  1) Don't import packages shaded in the driver bundle. Note that shaded-guava lives
                  in its own bundle, so we must explicitly *not* mention it here.
-                  -->!com.datastax.oss.driver.shaded.netty.*, !com.datastax.oss.driver.shaded.esri.*, !com.datastax.oss.driver.shaded.json.*, !com.datastax.oss.driver.shaded.codehaus.jackson.*, !com.datastax.oss.driver.shaded.fasterxml.jackson.*,
+                  -->!com.datastax.oss.driver.shaded.netty.*, !com.datastax.oss.driver.shaded.json.*, !com.datastax.oss.driver.shaded.codehaus.jackson.*, !com.datastax.oss.driver.shaded.fasterxml.jackson.*,


I just noticed: we are still mentioning here two shaded packages that do not exist anymore:

com.datastax.oss.driver.shaded.json

com.datastax.oss.driver.shaded.codehaus.jackson

Ah, good catch, you're absolutely right! I'll clean this up (and the one below) shortly.

adutra · 2022-01-25T18:46:51Z

core-shaded/pom.xml

@@ -366,7 +338,7 @@
                1) The driver's packages (API and internal);
                2) All shaded packages, except Guava which resides in a separate bundle.
                -->
-                <Export-Package>com.datastax.oss.driver.api.core.*, com.datastax.oss.driver.internal.core.*, com.datastax.dse.driver.api.core.*, com.datastax.dse.driver.internal.core.*, com.datastax.oss.driver.shaded.netty.*, com.datastax.oss.driver.shaded.esri.*, com.datastax.oss.driver.shaded.json.*, com.datastax.oss.driver.shaded.codehaus.jackson.*, com.datastax.oss.driver.shaded.fasterxml.jackson.*,</Export-Package>
+                <Export-Package>com.datastax.oss.driver.api.core.*, com.datastax.oss.driver.internal.core.*, com.datastax.dse.driver.api.core.*, com.datastax.dse.driver.internal.core.*, com.datastax.oss.driver.shaded.netty.*, com.datastax.oss.driver.shaded.json.*, com.datastax.oss.driver.shaded.codehaus.jackson.*, com.datastax.oss.driver.shaded.fasterxml.jackson.*,</Export-Package>


adutra

And we are done \o/ :shipit:

Follow-up to recent PR making ESRI an optional dependency

9e43d52

absurdfarce mentioned this pull request Jan 7, 2022

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

Merged

Documentation updates

130489c

absurdfarce commented Jan 7, 2022

View reviewed changes

absurdfarce requested a review from adutra January 7, 2022 22:02

adutra requested changes Jan 17, 2022

View reviewed changes

adutra mentioned this pull request Jan 18, 2022

Warning in build about json dependencies spring-projects/spring-boot#29387

Closed

Added note re: use of older version of ESRI

4c7e838

adutra reviewed Jan 21, 2022

View reviewed changes

manual/core/integration/README.md Outdated Show resolved Hide resolved

Revising language of the Esri include version

5e9ce0d

Remove transitive dependencies of Esri lib from shaded JAR as well

953c72f

absurdfarce changed the title ~~Follow-up to recent PR making ESRI an optional dependency~~ JAVA-2982: Follow-up to recent PR making ESRI an optional dependency Jan 24, 2022

absurdfarce requested a review from adutra January 24, 2022 21:15

adutra requested changes Jan 25, 2022

View reviewed changes

Removing OSGi spec for shaded packages which no longer exist

07293da

adutra approved these changes Jan 25, 2022

View reviewed changes

absurdfarce merged commit 415f789 into 4.x Jan 25, 2022

absurdfarce deleted the optional_esri_dependency_followup branch January 25, 2022 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JAVA-2982: Follow-up to recent PR making ESRI an optional dependency #1580

JAVA-2982: Follow-up to recent PR making ESRI an optional dependency #1580

absurdfarce commented Jan 7, 2022 •

edited

Loading

absurdfarce Jan 7, 2022

absurdfarce Jan 7, 2022

absurdfarce Jan 7, 2022

adutra Jan 17, 2022

absurdfarce Jan 21, 2022 •

edited

Loading

adutra Jan 21, 2022

adutra commented Jan 24, 2022

absurdfarce commented Jan 24, 2022

absurdfarce commented Jan 24, 2022

adutra left a comment

adutra Jan 25, 2022

absurdfarce Jan 25, 2022

adutra Jan 25, 2022

adutra left a comment

		```

		See the [integration](../manual/core/integration/#esri) section in the manual for more details.

JAVA-2982: Follow-up to recent PR making ESRI an optional dependency #1580

JAVA-2982: Follow-up to recent PR making ESRI an optional dependency #1580

Conversation

absurdfarce commented Jan 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

absurdfarce Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adutra commented Jan 24, 2022

absurdfarce commented Jan 24, 2022

absurdfarce commented Jan 24, 2022

adutra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adutra left a comment

Choose a reason for hiding this comment

absurdfarce commented Jan 7, 2022 •

edited

Loading

absurdfarce Jan 21, 2022 •

edited

Loading