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

Fix the scope for "provided" dependencies #1706

Merged
merged 3 commits into from
Dec 8, 2023

Conversation

msdousti
Copy link
Collaborator

@msdousti msdousti commented Dec 6, 2023

Fixes an issue where Lombok is transitively imported as a compile-time dependency into projects that use Logbook. For all other dependencies that have scope "provided" in the dependenyManagement section (gag, jsr305, Lombok), mentions this scope explicitly in the dependencies section.

Description

In the Logbook parent project, Lombok is mentioned as a dependency with default scope (i.e., compile time).
The dependenyManagement section uses the scope "provided", which seems to be working with Maven projects. However, Gradle projects do not honor this when there's an overriding dependenyManagement section in the child project that lacks the scope "provided".

Better safe than sorry: Setting the scope to provided in the dependencies section of the parent should fix the issue.

How to check:

  1. Check out this branch and run mvn install -DskipTests -Ddependency-check.skip=true -Djacoco.skip=true.

  2. Follow the steps in issue Logbook 3.7.0 pulls in Lombok as transitive dependency #1705 to create a Gradle project, with the following two modifications:

    • Use version 3.8.0-SNAPSHOT for Logbook (i.e., org.zalando:logbook-spring-boot-starter:3.8.0-SNAPSHOT)
    • Add mavenLocal() to the list of repositories (in addition to mavenCentral())
  3. Running ./gradlew -q dependencyInsight --dependency lombok should not show anything:

    image

Motivation and Context

Addresses #1705

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@msdousti msdousti changed the title Fix the scope for Lombok Fix the scope for "provided" dependencies Dec 6, 2023
@@ -317,14 +317,17 @@
<dependency>
<groupId>com.google.code.findbugs</groupId>
<artifactId>jsr305</artifactId>
<scope>provided</scope>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also move the scopes of other dependencies, not only those that are directly declared in this pom, to the respective dependencies blocks? E.g. jakarta.servlet-api or ktor-client-cio-jvm, scopes for them are not explicitly defined in their modules. I wonder if gradle projects make them compile time dependencies.

Copy link
Collaborator Author

@msdousti msdousti Dec 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kasmarian I have done a detailed analysis, would love to get feedback!

image

The issue will happen if all three conditions are satisifed (and only in Gradle-based projects):

  1. Logbook Parent module has specified a scope like provided or test for a dependency X in the dependencyManagement section
  2. Logbook Parent module has imported dependency X in the dependencies section, without specifying the scope.
  3. A child module has a dependencyManagement section, that imports a BOM dependency (scope = import). This BOM must have dependency X in its own dependencyManagement section, without specifying the scope.

In our example, Lombok was (1) included in the parent both in dependency management section (scope = provided) and (2) in the dependency section (no scope). Then (3) I imported spring-boot-dependencies as a BOM, which had no scope.

For ktor-client-cio-jvm and jakarta.servlet-api, this does not happen as at least condition 2 does not hold: They are not included in the dependencies section of the parent. However, if a child module uses them in the dependencies section, and provides no scope, and condition 3 holds, it can also create a compile-time transitive dependency.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an interesting finding and a nice article! (maybe you could also add a link to the GH repo with your playground project in the article)

However, if a child module uses them in the dependencies section, and provides no scope, and condition 3 holds, it can also create a compile-time transitive dependency.

If someone defines a dependency and doesn't specify a scope, I'd expect them to want a compile-time dependency. Then we probably should be fine here. Would the dependency scope from dependencies take priority in case someone specifies another scope?

Copy link
Collaborator Author

@msdousti msdousti Dec 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe you could also add a link to the GH repo with your playground project in the article

Thanks! Good suggestion, will do!

I'd expect them to want a compile-time dependency. Then we probably should be fine here.

I expect the same.

Would the dependency scope from dependencies take priority in case someone specifies another scope?

I guess so, because that's what I'm doing in this PR: In the parent project, I specified the scope provided for Lombok. Even though spring-boot-dependencies does not specify the scope (effectively making it compile), the explicit scope wins (both for Maven and Gradle).

I also tested this guess:

image

which, as expected, results in the scope explicitly mentioned in the dependencies section (i.e., test):

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking. Looks good then. One last question: how can we make sure not to have a regression here? Maybe a comment explaining why the scope is needed here would help?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 18bea53

@kasmarian
Copy link
Member

👍

@msdousti msdousti merged commit 13f2911 into main Dec 8, 2023
2 checks passed
@msdousti msdousti deleted the fix-lombok-dependency-handling branch December 8, 2023 13:17
@msdousti
Copy link
Collaborator Author

msdousti commented Dec 8, 2023

@kasmarian
Thanks for approving.

Is the underlying issue severe enough to warrant a new release?

@kasmarian
Copy link
Member

I can do a patch release, wdyt?

@msdousti
Copy link
Collaborator Author

msdousti commented Dec 8, 2023

I think it's a good idea 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants