Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spring Boot Starter Test includes dependencies with split packages which causes compilation failures when testing using the module path #15967

Closed
rajeshja opened this issue Feb 15, 2019 · 11 comments
Labels
status: declined A suggestion or change that we don't feel we should currently apply

Comments

@rajeshja
Copy link

I'm using Gradle 5.2 on Java 11.0.1 with a very simple project that adds a controller and an entity package to the base starter created with Spring 2.1.2. The code in src/main/java is setup as a Java module. He is my module definition (I only have one at this time)

module rja.price.main {
    requires java.sql;
    requires spring.beans;
    requires spring.core;
    requires spring.context;
    requires spring.web;
    requires spring.webflux;
    requires spring.boot;
    requires spring.boot.autoconfigure;
    requires reactor.core;
    opens rja.price to spring.core, spring.beans, spring.context;
    exports rja.price.application.controller;
    exports rja.price.domain.entity to com.fasterxml.jackson.databind;
}

Running gradlew build breaks at compileTestJava with 100 errors of the form:

error: the unnamed module reads package org.json from both jsonassert and android.json
error: the unnamed module reads package org.hamcrest from both hamcrest.core and hamcrest.library
error: module spring.boot reads package org.json from both jsonassert and android.json
error: module spring.boot reads package org.hamcrest from both hamcrest.core and hamcrest.library

Replacing the org.springframework.boot:spring-boot-starter-test dependencies with individual imports of the following works. Importing the failing code into Intellij IDEA and running the SpringBootApplication in the IDE also works.

A sample project to replicate the issue is at:
https://github.com/rajeshja/springboot2.1-java11-gradle-bug

@wilkinsona
Copy link
Member

Thanks for the sample.

The problem is that there are two split packages in the dependencies of spring-boot-starter-test:

  • org.json is defined in both org.skyscreamer:jsonassert and com.vaadin.external.google:android-json. The former depends on the latter.
  • org.hamcrest is defined in both org.hamcrest:hamcrest-library and org.hamcrest:hamcrest-core. The former depends on the latter.

In short, hamcrest-library and jsonassert are not compatible with the module path.

In Hamcrest 2.1, the hamcrest-core and hamcrest-library modules have been deprecated and replaced by a single hamcrest module. #15555 is tracking that upgrade. There doesn't appear to be anything on the horizon that will help with jsonassert.

Regardless of the above, please note that classpath scanning does not work for module path-based testing: spring-projects/spring-framework#21515. As such, it's hard to justify making changes to the test starter to accommodate module path-based testing and I would recommend using the class path instead.

@wilkinsona wilkinsona added the for: team-attention An issue we'd like other members of the team to review label Feb 15, 2019
@wilkinsona wilkinsona changed the title Spring Boot Starter Test causes module conflicts when building with gradle Spring Boot Starter Test includes dependencies with split packages which causes failures when testing using the module path Feb 15, 2019
@wilkinsona wilkinsona changed the title Spring Boot Starter Test includes dependencies with split packages which causes failures when testing using the module path Spring Boot Starter Test includes dependencies with split packages which causes compilation failures when testing using the module path Feb 15, 2019
@rajeshja
Copy link
Author

Thanks for the link to the module path-based testing issue.

Do you think that the Spring Initializr should generate a sample project with Java modules and maven/gradle scripts that work including tests (using classpath)? I could create an issue for this there.

@snicoll
Copy link
Member

snicoll commented Feb 18, 2019

@rajeshja Spring Initializr is not meant to generate sample projects, in the sense of showing how to do something. A guide is usually what we aim for though I am not sure one on the modules path would be accepted.

@wilkinsona
Copy link
Member

We've discussed this and agreed that we don't want to make any changes to the starter. @rajeshja You may want to raise an issue against jsonassert if its split package is causing you a problem.

@wilkinsona wilkinsona added status: declined A suggestion or change that we don't feel we should currently apply and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged labels Feb 22, 2019
@commonquail
Copy link

commonquail commented Mar 28, 2024

Five years on it appears unlikely that the practically dead-in-the-water org.skyscreamer:jsonassert will even attempt to address any of the several complaints about its split packages. I relied on it unknowingly through MockMvc but discovered that net.javacrumbs.json-unit:json-unit-spring is nearly a drop-in replacement:

+    <json-unit.version>3.2.7</json-unit.version>
@@ ... @@
+    <dependency>
+      <groupId>net.javacrumbs.json-unit</groupId>
+      <artifactId>json-unit-spring</artifactId>
+      <version>${json-unit.version}</version>
+      <scope>test</scope>
+    </dependency>
     <dependency>
       <groupId>org.springframework.boot</groupId>
       <artifactId>spring-boot-starter-test</artifactId>
       <scope>test</scope>
+      <exclusions>
+        <exclusion>
+          <groupId>org.skyscreamer</groupId>
+          <artifactId>jsonassert</artifactId>
+        </exclusion>
+      </exclusions>
     </dependency>
+import static net.javacrumbs.jsonunit.spring.JsonUnitResultMatchers.json;
@@ ... @@
         final var requestJson = get("/foo").contentType(MediaType.APPLICATION_JSON);
-        final var matchingJsonBody = content().json(read("/foo.json"));
+        final var matchingJsonBody = json().when(Option.IGNORING_EXTRA_FIELDS).isEqualTo(read("/foo.json"));

It seems likely that o.s.t.w.s.r.ContentResultMatchers::json could transparently make that change, allowing anyone without a direct dependency on JSONassert to trivially exclude the dependency without Spring Boot inducing breakage. JPMS notwithstanding, it does look like reassessing JSONassert's continued viability would be prudent.

The Option.IGNORING_EXTRA_FIELDS was necessary for parity in my case but I was surprised to learn that was the original behavior, and removing it revealed an implementation error.

@philwebb philwebb added the for: team-meeting An issue we'd like to discuss as a team to make progress label Apr 1, 2024
@philwebb
Copy link
Member

@commonquail I think we'll need to take the lead from Spring Framework here. MockMVC also uses jsonassert. I've opened spring-projects/spring-framework#32791 for that team to consider.

@philwebb philwebb removed the for: team-meeting An issue we'd like to discuss as a team to make progress label May 10, 2024
@snicoll
Copy link
Member

snicoll commented May 23, 2024

Thanks to @philwebb, we have a JsonComparator abstraction in the core framework now, see spring-projects/spring-framework#32791 (comment) for more details.

@commonquail for the above a JsonComparator can be implemented for net.javacrumbs.json-unit:json-unit-spring and rather than calling json(String), you can call json(String, JsonComparator). This does not require JSONAssert.

@commonquail
Copy link

commonquail commented May 26, 2024

That's a superior core API, to be sure, but just for the record that's not very useful to somebody that wanders down this path. JSONassert is still broken and still bundled with Spring so we still have to manually exclude it and supply a work-around, and calling e.g. JsonUnit through a bespoke JsonComparator is not easier than through its MockMvc ResultMatcher. To make matters worse, I see now that JsonUnit does not export an API it considers appropriate for this extension mechanism so this silly sample implementation is the best I can come up with:

var matchingJsonBody = content().json(read("/foo.json"), (expectedJson, actualJson) -> {
    JsonAssertions.assertThatJson(actualJson).when(Option.IGNORING_EXTRA_FIELDS).isEqualTo(expectedJson);
    return JsonComparison.match();
});

In any case, I've filed lukas-krecan/JsonUnit#767, too.

It's unfortunate that JSONassert remains the only option available to the Spring project.

@snicoll
Copy link
Member

snicoll commented May 26, 2024

FWIW, that comment is a bit rude IMO. The API just got introduced so any follow-up that may happen in other projects still need to happen.

It's unfortunate that JSONassert remains the only option available to the Spring project.

That's inaccurate.

@commonquail
Copy link

FWIW, that comment is a bit rude IMO. The API just got introduced so any follow-up that may happen in other projects still need to happen.

I'm sorry to appear rude. It seems to me you have misunderstood the crux of the matter, however: there is no follow-up that needs to happen elsewhere because there is no follow-up anywhere else that matters; JSONassert does not compile on the module path, so spring-boot-starter-test does not compile on the module path absent an <exclude>. Fortunately userspace can work around it with not a lot of effort.

It's unfortunate that JSONassert remains the only option available to the Spring project.

That's inaccurate.

Not according to spring-projects/spring-framework#32791 (comment) nor to my own investigation.

@philwebb
Copy link
Member

I think it's going to take us some time to provide a complete solution and we can't even begin until we have the groundwork that spring-projects/spring-framework#32791 provides. We could consider removing JSONassert from spring-boot-starter-test, but that's going to break a lot of folks who perhaps don't care about the module system.

It might be helpful if JsonUnit separated their Spring module from the core release. That way we could depend on it without a cycle. It's quite a shame that https://github.com/skyscreamer/JSONassert isn't getting any updates these days.

@philwebb philwebb added for: team-meeting An issue we'd like to discuss as a team to make progress labels May 29, 2024
@philwebb philwebb removed the for: team-meeting An issue we'd like to discuss as a team to make progress label Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

6 participants