Summary

Excludes old asm lib from json-path

Fixes gh-24218

Details

Ideally this would be fixed in json-path -> json-smart -> smart-asserter but since those repos are inactive (AFAICT) I explored using a Gradle component metadata rule (see example 2 to simulate this sort of systemic fix. I have no experience w/ that portion of Gradle and it did not seem to work for me. Being out of my comfort zone I went w/ the less risky approach of brute-force exclusion from the only modules that outwardly publish json-path: spring-boot-starter-test and spring-boot-dependencies.

The other modules use optional or testImplementation for json-path. My initial fear was that by not excluding asm on these declarations as well that our tests would be running against the old asm (5.x) and thus hiding bugs that could possibly occur when pointing at the new asm (7.x). Due to Gradle version alignment this is not an issue - it picks the latest. For example, in spring-boot-actuator there is a testImplementation dependency on json-path. To see what it uses at runtime I executed the following:

IT-USA-B00159:spring-boot-actuator cbono$ ../../gradlew dependencyInsight --configuration testRuntimeClasspath --dependency asm

The results are the most recent asm:7.1 is chosen (you can see the 5.0.4 -> 7.1 version alignment happening:

org.ow2.asm:asm:5.0.4 -> 7.1
\--- net.minidev:accessors-smart:1.2
     \--- net.minidev:json-smart:2.3
          +--- project :spring-boot-project:spring-boot-dependencies
          |    \--- project :spring-boot-project:spring-boot-parent
          |         +--- testRuntimeClasspath
          |         \--- project :spring-boot-project:spring-boot-tools:spring-boot-test-support
          |              \--- testRuntimeClasspath
          \--- com.jayway.jsonpath:json-path:2.4.0
               +--- testRuntimeClasspath (requested com.jayway.jsonpath:json-path)
               \--- project :spring-boot-project:spring-boot-dependencies (*)

Comment From: philwebb

I wonder if we can get away with excluding json-smart entirely?

Comment From: onobc

@philwebb In order to do that I believe we would have to switch the JsonProvider - as json-smart is the default. I think we could do that for Spring Boot internal use of json-path. However, json-path is included in spring-boot-starter-test so I am not sure how/if we can set the default for downstream consumers. IOW - by not including json-smart we would probably break consumers of the test starter.

FWIW, here are the current references to json-smart within jsonpath:

grep: ./com/jayway/jsonpath/internal/filter/ValueNode.class: binary file matches
grep: ./com/jayway/jsonpath/internal/filter/ValueNode$JsonNode.class: binary file matches
grep: ./com/jayway/jsonpath/spi/mapper/JsonSmartMappingProvider$1.class: binary file matches
grep: ./com/jayway/jsonpath/spi/mapper/JsonSmartMappingProvider.class: binary file matches
grep: ./com/jayway/jsonpath/spi/mapper/JsonSmartMappingProvider$FloatReader.class: binary file matches
grep: ./com/jayway/jsonpath/spi/mapper/JsonSmartMappingProvider$BooleanReader.class: binary file matches
grep: ./com/jayway/jsonpath/spi/mapper/JsonSmartMappingProvider$IntegerReader.class: binary file matches
grep: ./com/jayway/jsonpath/spi/mapper/JsonSmartMappingProvider$BigIntegerReader.class: binary file matches
grep: ./com/jayway/jsonpath/spi/mapper/JsonSmartMappingProvider$LongReader.class: binary file matches
grep: ./com/jayway/jsonpath/spi/mapper/JsonSmartMappingProvider$DoubleReader.class: binary file matches
grep: ./com/jayway/jsonpath/spi/mapper/JsonSmartMappingProvider$StringReader.class: binary file matches
grep: ./com/jayway/jsonpath/spi/mapper/JsonSmartMappingProvider$BigDecimalReader.class: binary file matches
grep: ./com/jayway/jsonpath/spi/mapper/JsonSmartMappingProvider$DateReader.class: binary file matches
grep: ./com/jayway/jsonpath/spi/json/JsonSmartJsonProvider.class: binary file matches

Comment From: wilkinsona

I think I may be missing something here. If we exclude ASM as proposed, will spring-boot-starter-test not be left without a transitive dependency on ASM which will, presumably, result in code in accessors-smart failing as ASM classes that it requires cannot be found?

We also have a number of other managed dependencies that depend transitively upon ASM with a variety of versions, including Jetty and Cassandra's Driver. It would appear to be inconsistent to only apply the exclusion to json-path. Applying it more broadly would raise the same concern about runtime failures as I described above.

Comment From: onobc

Hi @wilkinsona, I am glad you replied. I was under the "assumption" that ASM was being brought in by spring-core. While it does use ASM it does what a good library should do and it shades it. My initial look (above) was at spring-boot-actuator which did surface the later version of ASM - but not from Spring (It came in elsewhere). Based on that, excluding it like I have proposed could in fact lead to breakage if the consuming application does not provide a version of ASM and an execution path triggers in accessors-smart that requires it.

So @philwebb suggestion of excluding json-smart entirely coupled with switching JsonPath's JsonProvider off of json-smart is starting to sound like a better alternative. It would require changes to JsonContent and JsonContentAssert where its calling com.jayway.jsonpath.Configuration.defaultConfiguration() which will trigger the JsonSmart class loading via imports etc..

Really, if json-path just shaded its internals, that would be great. Again, I believe that project is inactive and I am not sure if that is an option or not.

I also want to be mention that the driver of this issue is a Maven dependency convergence error whose simple workaround is to exclude the transitive dependency on the older ASM in the consuming application. Should we just leave this as is?

Comment From: wilkinsona

Should we just leave this as is?

I think so. I think all we can really do here is make sure that Spring projects are good citizens and make use of spring-core's shaded ASM. That was my feeling when I routed https://github.com/spring-projects/spring-boot/issues/24218 to Spring Cloud Contract and that hasn't changed.

Comment From: onobc

... is make sure that Spring projects are good citizens and make use of spring-core's shaded ASM.

Agreed. However, spring-boot-starter-test is violating that rule by relying on a lib that is not a good citizen and carries w/ it the older ASM.

Comment From: wilkinsona

The rule only applies to Spring projects that naturally have a dependency on spring-core anyway. Other projects should shade ASM themselves rather than tying themselves to Spring.

There's always likely to be third-party code with transitive dependencies on ASM. As long as the Spring portfolio doesn't contribute to the problem with its own direct dependencies on ASM, I think that's the best we can do.

Thanks anyway for the PR, but I think it's best if we leave this as-is.

Comment From: onobc

@wilkinsona - one more clarifying question - would you agree then based on the same reasoning as here that Spring Cloud Contract should do nothing for this case either? It brings in json-path just like spring-boot-starter-test does.

Comment From: wilkinsona

IMO, Spring Cloud Contract's dependency on json-path is fine and doesn't need any exclusions. What I think should change is that SCC should use CGLib from spring-core rather than as a dependency. This will have the knock-on effect of removing CGLib's transitive dependency on ASM from the equation.

Comment From: onobc

@wilkinsona I just got a notification that json-path finally has a new release. This gives us the ability to affect that project w/ pull requests if needed down the road. Just wanted to share that info as my original description in this pull request noted that there had not been a release in years.