zalando / logbook

An extensible Java library for HTTP request and response logging
MIT License
1.82k stars 259 forks source link

Logbook 3.7.0 pulls in Lombok as transitive dependency #1705

Closed bwaldvogel closed 9 months ago

bwaldvogel commented 9 months ago

When upgrading Logbook from 3.6.0 to 3.7.0 we get a new transitive dependency pulled in: org.projectlombok:lombok:1.18.30. For projects that are not using Lombok, that causes undesired effects. For example, IntelliJ IDEA auto-discovers that Lombok is used in the project and starts suggesting the use of Lombok annotations.

Steps to reproduce

  1. Create a new Gradle / Spring Boot project (use start.spring.io or IntelliJ’s native integration)
  2. Add the following dependency: org.zalando:logbook-spring-boot-starter:3.7.0

Actual behavior

./gradlew -q dependencyInsight --dependency lombok shows that Lombok is pulled in:

org.projectlombok:lombok:1.18.30
\--- org.zalando:logbook-spring-boot-autoconfigure:3.7.0
     \--- org.zalando:logbook-spring-boot-starter:3.7.0

When downgrading from 3.7.0 to 3.6.0, the Lombok dependency disappears.

Expected behavior

The Lombok is not pulled in as it was in version 3.6.0.

kasmarian commented 9 months ago

@msdousti it looks like this change started pulling lombok as a compile dependency, because it's declared as such in spring-boot-dependencies. Do you think we can revert to using spring-framework-bom? Alternatively, we'd need to override the scope for lombok (potentially other dependencies?) in logbook-spring-boot-autoconfigure module directly.

msdousti commented 9 months ago

@kasmarian There are several things going on that need a deeper look.

  1. The change you mentioned only touches the dependencyManagement section. By itself, dependencyManagement will not include dependencies in a project; it just says that if a dependency is ever requested, this specific version must be used.

  2. I tried creating a Gradle and a Maven project including the Steps to reproduce. The Gradle project is as mentioned in the ticket, but the Maven project behaves as expected: It does not include Lombok - mvn dependency:tree -Dincludes=org.projectlombok returns nothing. See for yourself here.

As a side note, depending on spring-framework-bom in a Spring Boot project is not right, since the BOM file for Spring Framework often includes newer material that might not be compatible with the current version of Spring Boot. When the project does not have Spring Boot as its parent (like here), it's customary to include spring-boot-dependencies in the dependencyManagement section.

I'll try to dig deeper why Gradle is not behaving as expected.

kasmarian commented 9 months ago

I also noticed that maven project doesn't pull lombok, still, with gradle this only happens for 3.0.7 version.

The change you mentioned only touches the dependencyManagement section. By itself, dependencyManagement will not include dependencies in a project; it just says that if a dependency is ever requested, this specific version must be used.

In the parent project, we do specify lombok without a specific scope. Can it be that the scope from spring-boot-dependencies is used in case of a gradle project instead of the scope from logbook-parent's dependencyManagement? What if we define the scope of lombok in parent's pom in <dependencies> directly and not under <dependencyManagement> ? I agree, knowing the root cause for this would be interesting. Just throwing the ideas for now.

msdousti commented 9 months ago

@kasmarian I believe the the above PR (#1706) fixes this.

msdousti commented 9 months ago

@kasmarian

PS: Th answers to this SO question nail it:

Anyway, save yourself the hassle, and leave scope out of your dependencyManagement.

dependencyManagement is just here to define dependencies version for all project submodules, the only pertinent scope in this section is import for BOMs. Scope must be defined in dependencies section.

I'd say we should review the parent project based on this observation.

PS: Did this in my PR.

kasmarian commented 9 months ago

We released 3.7.1 where this should be addressed. May take some time until the release is available in repositories

bwaldvogel commented 9 months ago

Thanks for fixing this so quickly. Version 3.7.1 does not pull in lombok anymore. However, it pulls in a couple of new dependencies to the runtimeClasspath that shouldn’t be pulled in, such as hamcrest.

./gradlew -q dependencyInsight --dependency hamcrest --configuration runtimeClasspath
org.hamcrest:hamcrest:2.2
\--- org.zalando:logbook-spring-boot-autoconfigure:3.7.1
     \--- org.zalando:logbook-spring-boot-starter:3.7.1
          \--- runtimeClasspath

Shall I create a new issue or reopen this one?

msdousti commented 9 months ago

@bwaldvogel Thanks a lot for your report!

It will be great if you create a new ticket mentioning all those "badly scoped" dependencies; I'll then fix them in one PR.

bwaldvogel commented 9 months ago

Please see https://github.com/zalando/logbook/issues/1711