search

spring-projects / spring-boot

Spring Boot
https://spring.io/projects/spring-boot
Apache License 2.0
74.13k stars 40.46k forks source link

Upgrade to QueryDSL 5.0.0.M1 #26956

Closed lpandzic closed 3 years ago

lpandzic commented 3 years ago

Instead of defining multiple dependencies, use querydsl bom instead - https://github.com/querydsl/querydsl/blob/master/querydsl-bom/pom.xml.

wilkinsona commented 3 years ago

Thanks for the suggestion. I think we should also consider removing the dependency management for QueryDSL entirely.

To limit its scope, we try to only provide dependency management for projects that Spring Boot interacts with directly. QueryDSL doesn't fall into that category so there's no real reason for Spring Boot to have an opinion about the version that should be used. We originally added it to benefit Spring Data users (https://github.com/spring-projects/spring-boot/issues/6399) (cc @odrotbohm) but, with hindsight, I'm not sure that we should have done so.

In 2.6, following discussions with the Spring Security team, we've already removed dependency management for some Nimbus dependencies that Boot also didn't interact with directly. The situation with QueryDSL is similar, although not identical, to the one that we were in with Nimbus. For example, in the case of Nimbus the rate of new major and minor releases is problematically quick.

If we decide to keep the dependency management for QueryDSL, we should try to automatically align its version with the version that Spring Data uses.

odrotbohm commented 3 years ago

If we decide to keep the dependency management for QueryDSL, we should try to automatically align its version with the version that Spring Data uses.

I'd vote for that.

Regarding the plan to remove the managed version: wouldn't that increase the friction a developer faces when they assemble dependencies for their projects? I guess it's immediately obvious that the version is not declared because of the build tool reporting it needed. But it might not be immediately obvious why some of the third-party dependencies very close to the product require a version and some don't. That "Boot integrates directly with it" (I assume that translates to "ships auto-configuration for") argument feels like a leaky abstraction and more thought from the inside rather than from how to deliver a pleasant development experience for our users.

wilkinsona commented 3 years ago

That "Boot integrates directly with it" (I assume that translates to "ships auto-configuration for") argument feels like a leaky abstraction and more thought from the inside rather than from how to deliver a pleasant development experience for our users

I don't think there's been a lack of thought here and I CCed you in the hope of involving you in thinking through the options.

Our experience has shown us that Spring Boot expressing an opinion about the version of a dependency can sometimes make things worse for our users so we need to be sure that we have a good reason to do so. In the case of Nimbus, there were users of it that were not using Spring Security. This led to Boot's opinion about its version, entirely driven by Spring Security, causing problems for them. Spring Security has made some changes to how and where it uses Nimbus which improves things for Spring Security's users whether or not they're using Boot and means that it makes sense for Boot to drop its dependency management for it.

IMO, in an ideal world, anyone using Spring Data would be able to use QueryDSL without having to specify the version and, by default, they'd get the version that Spring Data recommends. If this was possible with Spring Data in general then Spring Boot users would get the same benefit. IIRC, Spring Security achieved this by removing a dependency in one place and making the dependency transitive, i.e. no longer optional, in another.

odrotbohm commented 3 years ago

I don't think there's been a lack of thought here and I CCed you in the hope of involving you in thinking through the options.

Happy to chime in. I didn't want to imply "lack of thought" (I actually didn't state anything like that). The primary argument given was this:

To limit its scope, we try to only provide dependency management for projects that Spring Boot interacts with directly.

I merely outline that this means that – staying with the current way of working with parent POMs – the decision which dependency version in the user POM has to be declared and which not is not necessarily understandable unless you know what constitutes "Spring Boot interacts with it". Especially for users not intimately familiar with Boot this is not necessarily obvious. "Why do I need to declare the version for Querydsl but not for Guava?" Stuff like that.

I totally see that the current arrangement is a tradeoff, too, and causes difficulties, especially when the ecosystem project change their library arrangements. But I think that getting a compatible version of a third party library that is compatible with the Spring Boot (and Spring project) versions is a huge benefit to users. Of course, one different way of achieving that would be to rather move the dependency version declaration into the Spring Data BOM, but so far we followed the general agreement that our individual project BOMs only declare the project's artifacts. IIRC part of that decision was to reduce the risk of conflicts with Boot's version declarations. Also, it might cause additional complexity and problems if a 3rd party dependency's version is declared by multiple ecosystem projects and instead of all those paths being consolidate in Boot now have to be consolidated 1:1. Maybe it's time to revisit the entire arrangement and a topic for a project leads call?

philwebb commented 3 years ago

one different way of achieving that would be to rather move the dependency version declaration into the Spring Data BOM, but so far we followed the general agreement that our individual project BOMs only declare the project's artifacts.

I definitely think that we should keep project BOMs only containing the project's own artifacts. Anything else there is going to get very complicated very quickly.

We'll try discus the topic this week on our team call to see what the options could be. We can also discuss it on a leads call if needed. It's a bit of a tricky problem to deal with in general. We don't want to make things hard for our users, but we also don't want to end up trying to manage every optional dependency of every project.

wilkinsona commented 3 years ago

Unfortunately, we cannot use the bom as it contains dependency management for several dependencies that aren't part of QueryDSL. Here's the complete <dependencyManagement> from its effective pom:

  <dependencyManagement>
    <dependencies>
      <dependency>
        <groupId>com.querydsl</groupId>
        <artifactId>querydsl-core</artifactId>
        <version>5.0.0.M1</version>
      </dependency>
      <dependency>
        <groupId>com.querydsl</groupId>
        <artifactId>querydsl-codegen</artifactId>
        <version>5.0.0.M1</version>
      </dependency>
      <dependency>
        <groupId>com.querydsl</groupId>
        <artifactId>querydsl-codegen-utils</artifactId>
        <version>5.0.0.M1</version>
      </dependency>
      <dependency>
        <groupId>com.querydsl</groupId>
        <artifactId>querydsl-spatial</artifactId>
        <version>5.0.0.M1</version>
      </dependency>
      <dependency>
        <groupId>com.querydsl</groupId>
        <artifactId>querydsl-apt</artifactId>
        <version>5.0.0.M1</version>
      </dependency>
      <dependency>
        <groupId>com.querydsl</groupId>
        <artifactId>querydsl-collections</artifactId>
        <version>5.0.0.M1</version>
      </dependency>
      <dependency>
        <groupId>com.querydsl</groupId>
        <artifactId>querydsl-sql</artifactId>
        <version>5.0.0.M1</version>
      </dependency>
      <dependency>
        <groupId>com.querydsl</groupId>
        <artifactId>querydsl-sql-spatial</artifactId>
        <version>5.0.0.M1</version>
      </dependency>
      <dependency>
        <groupId>com.querydsl</groupId>
        <artifactId>querydsl-sql-codegen</artifactId>
        <version>5.0.0.M1</version>
      </dependency>
      <dependency>
        <groupId>com.querydsl</groupId>
        <artifactId>querydsl-sql-spring</artifactId>
        <version>5.0.0.M1</version>
      </dependency>
      <dependency>
        <groupId>com.querydsl</groupId>
        <artifactId>querydsl-jpa</artifactId>
        <version>5.0.0.M1</version>
      </dependency>
      <dependency>
        <groupId>com.querydsl</groupId>
        <artifactId>querydsl-jpa-codegen</artifactId>
        <version>5.0.0.M1</version>
      </dependency>
      <dependency>
        <groupId>com.querydsl</groupId>
        <artifactId>querydsl-jdo</artifactId>
        <version>5.0.0.M1</version>
      </dependency>
      <dependency>
        <groupId>com.querydsl</groupId>
        <artifactId>querydsl-lucene3</artifactId>
        <version>5.0.0.M1</version>
      </dependency>
      <dependency>
        <groupId>com.querydsl</groupId>
        <artifactId>querydsl-lucene4</artifactId>
        <version>5.0.0.M1</version>
      </dependency>
      <dependency>
        <groupId>com.querydsl</groupId>
        <artifactId>querydsl-lucene5</artifactId>
        <version>5.0.0.M1</version>
      </dependency>
      <dependency>
        <groupId>com.querydsl</groupId>
        <artifactId>querydsl-hibernate-search</artifactId>
        <version>5.0.0.M1</version>
      </dependency>
      <dependency>
        <groupId>com.querydsl</groupId>
        <artifactId>querydsl-mongodb</artifactId>
        <version>5.0.0.M1</version>
      </dependency>
      <dependency>
        <groupId>com.querydsl</groupId>
        <artifactId>querydsl-scala</artifactId>
        <version>5.0.0.M1</version>
      </dependency>
      <dependency>
        <groupId>com.querydsl</groupId>
        <artifactId>querydsl-kotlin</artifactId>
        <version>5.0.0.M1</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains</groupId>
        <artifactId>annotations</artifactId>
        <version>21.0.1</version>
        <scope>provided</scope>
      </dependency>
      <dependency>
        <groupId>jakarta.persistence</groupId>
        <artifactId>jakarta.persistence-api</artifactId>
        <version>2.2.3</version>
        <scope>provided</scope>
      </dependency>
      <dependency>
        <groupId>org.datanucleus</groupId>
        <artifactId>javax.jdo</artifactId>
        <version>3.2.0-m13</version>
      </dependency>
      <dependency>
        <groupId>com.vividsolutions</groupId>
        <artifactId>jts</artifactId>
        <version>1.13</version>
      </dependency>
      <dependency>
        <groupId>com.h2database</groupId>
        <artifactId>h2</artifactId>
        <version>1.4.197</version>
      </dependency>
      <dependency>
        <groupId>javax.validation</groupId>
        <artifactId>validation-api</artifactId>
        <version>2.0.1.Final</version>
      </dependency>
      <dependency>
        <groupId>junit</groupId>
        <artifactId>junit</artifactId>
        <version>4.13.2</version>
      </dependency>
      <dependency>
        <groupId>org.hamcrest</groupId>
        <artifactId>hamcrest</artifactId>
        <version>2.2</version>
        <scope>test</scope>
      </dependency>
      <dependency>
        <groupId>org.hamcrest</groupId>
        <artifactId>hamcrest-core</artifactId>
        <version>2.2</version>
        <scope>test</scope>
      </dependency>
      <dependency>
        <groupId>org.hamcrest</groupId>
        <artifactId>hamcrest-all</artifactId>
        <version>2.2</version>
        <scope>test</scope>
      </dependency>
      <dependency>
        <groupId>org.eclipse.jdt.core.compiler</groupId>
        <artifactId>ecj</artifactId>
        <version>4.6.1</version>
      </dependency>
      <dependency>
        <groupId>io.github.classgraph</groupId>
        <artifactId>classgraph</artifactId>
        <version>4.8.108</version>
        <exclusions>
          <exclusion>
            <artifactId>javassist</artifactId>
            <groupId>org.javassist</groupId>
          </exclusion>
        </exclusions>
      </dependency>
      <dependency>
        <groupId>org.javassist</groupId>
        <artifactId>javassist</artifactId>
        <version>3.28.0-GA</version>
      </dependency>
      <dependency>
        <groupId>com.google.guava</groupId>
        <artifactId>guava</artifactId>
        <version>30.1.1-jre</version>
      </dependency>
      <dependency>
        <groupId>org.geolatte</groupId>
        <artifactId>geolatte-geom</artifactId>
        <version>1.8.2</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains.kotlin</groupId>
        <artifactId>kotlin-scripting-jsr223</artifactId>
        <version>1.5.10</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains.kotlin</groupId>
        <artifactId>kotlin-stdlib</artifactId>
        <version>1.5.10</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains.kotlin</groupId>
        <artifactId>kotlin-stdlib-jdk7</artifactId>
        <version>1.5.10</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains.kotlin</groupId>
        <artifactId>kotlin-stdlib-jdk8</artifactId>
        <version>1.5.10</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains.kotlin</groupId>
        <artifactId>kotlin-stdlib-js</artifactId>
        <version>1.5.10</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains.kotlin</groupId>
        <artifactId>kotlin-stdlib-common</artifactId>
        <version>1.5.10</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains.kotlin</groupId>
        <artifactId>kotlin-reflect</artifactId>
        <version>1.5.10</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains.kotlin</groupId>
        <artifactId>kotlin-osgi-bundle</artifactId>
        <version>1.5.10</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains.kotlin</groupId>
        <artifactId>kotlin-test</artifactId>
        <version>1.5.10</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains.kotlin</groupId>
        <artifactId>kotlin-test-junit</artifactId>
        <version>1.5.10</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains.kotlin</groupId>
        <artifactId>kotlin-test-junit5</artifactId>
        <version>1.5.10</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains.kotlin</groupId>
        <artifactId>kotlin-test-testng</artifactId>
        <version>1.5.10</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains.kotlin</groupId>
        <artifactId>kotlin-test-js</artifactId>
        <version>1.5.10</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains.kotlin</groupId>
        <artifactId>kotlin-test-common</artifactId>
        <version>1.5.10</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains.kotlin</groupId>
        <artifactId>kotlin-test-annotations-common</artifactId>
        <version>1.5.10</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains.kotlin</groupId>
        <artifactId>kotlin-main-kts</artifactId>
        <version>1.5.10</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains.kotlin</groupId>
        <artifactId>kotlin-script-runtime</artifactId>
        <version>1.5.10</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains.kotlin</groupId>
        <artifactId>kotlin-script-util</artifactId>
        <version>1.5.10</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains.kotlin</groupId>
        <artifactId>kotlin-scripting-common</artifactId>
        <version>1.5.10</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains.kotlin</groupId>
        <artifactId>kotlin-scripting-jvm</artifactId>
        <version>1.5.10</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains.kotlin</groupId>
        <artifactId>kotlin-scripting-jvm-host</artifactId>
        <version>1.5.10</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains.kotlin</groupId>
        <artifactId>kotlin-scripting-ide-services</artifactId>
        <version>1.5.10</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains.kotlin</groupId>
        <artifactId>kotlin-compiler</artifactId>
        <version>1.5.10</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains.kotlin</groupId>
        <artifactId>kotlin-compiler-embeddable</artifactId>
        <version>1.5.10</version>
      </dependency>
      <dependency>
        <groupId>org.jetbrains.kotlin</groupId>
        <artifactId>kotlin-daemon-client</artifactId>
        <version>1.5.10</version>
      </dependency>
    </dependencies>
  </dependencyManagement>

I've opened https://github.com/querydsl/querydsl/issues/2962. We can just avoid using the bom for now.

lpandzic commented 3 years ago

Unfortunately, we cannot use the bom as it contains dependency management for several dependencies that aren't part of QueryDSL.

@wilkinsona this was accidental when I introduced it in https://github.com/querydsl/querydsl/pull/2796.

@tstavinoha inside my company identified that problem and I've fixed it with help from @mp911de who suggested I use flatten plugin similar to how Spring Data does it resulting in https://github.com/querydsl/querydsl/pull/2943. So this problem should be addressed already, I'm just waiting for @jwgmeligmeyling to release 5.0.0 which I believe should happen soon enough.