Skip to content

Fixing the application of DoesNotIncludeDependency against files of the incorrect type.#5758

Merged
timtebeek merged 3 commits into
mainfrom
steve-fix-does-not-include-dependency-for-incorrect-types
Jul 17, 2025
Merged

Fixing the application of DoesNotIncludeDependency against files of the incorrect type.#5758
timtebeek merged 3 commits into
mainfrom
steve-fix-does-not-include-dependency-for-incorrect-types

Conversation

@steve-aom-elliott

@steve-aom-elliott steve-aom-elliott commented Jul 15, 2025

Copy link
Copy Markdown
Contributor

What's changed?

Reworked the visitor for DoesNotIncludeDependency (both versions) so that it will first check that the file would have been accepted by DependencyInsight's visitor before trying to visit it, and then only if an acceptable file was not marked, will it return a found result with that file in it.

What's your motivation?

Previously there was a bug wherein if you applied DoesNotIncludeDependency to a file of a conceptually inapplicable type (i.e. a Java or Gradle file for the Maven version or a Java or POM file for the Gradle version), it would end up marking the file because the visitor for DependencyInsight in each case was being run through the precondition not.

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

@steve-aom-elliott steve-aom-elliott self-assigned this Jul 15, 2025
@steve-aom-elliott steve-aom-elliott added the bug Something isn't working label Jul 15, 2025
@steve-aom-elliott steve-aom-elliott added test provided Already replicated with a unit test, using JUnit pioneer's ExpectedToFail maven gradle labels Jul 15, 2025
@github-project-automation github-project-automation Bot moved this to In Progress in OpenRewrite Jul 15, 2025
@steve-aom-elliott steve-aom-elliott force-pushed the steve-fix-does-not-include-dependency-for-incorrect-types branch from 956aabf to bece592 Compare July 15, 2025 15:57
@steve-aom-elliott steve-aom-elliott moved this from In Progress to Ready to Review in OpenRewrite Jul 15, 2025
@Laurens-W

Laurens-W commented Jul 16, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

I'm not sure I understand exactly what the issue is here.

rewrite/rewrite-core/src/main/java/org/openrewrite/Preconditions.java

Lines 65 to 67 in 41ce444

if (sourceFile != null && !v.isAcceptable(sourceFile, ctx)) {
return SearchResult.found(tree);
}

Preconditions.not marks every sourcefile that is not acceptable, but if the recipes(visitors) that come after that have the appropriate isAcceptable checks, that's fine right?

@steve-aom-elliott steve-aom-elliott force-pushed the steve-fix-does-not-include-dependency-for-incorrect-types branch from 6a96f31 to 33cd43e Compare July 16, 2025 12:48
@steve-aom-elliott

steve-aom-elliott commented Jul 16, 2025

Copy link
Copy Markdown
Contributor Author

I'm not sure I understand exactly what the issue is here.

rewrite/rewrite-core/src/main/java/org/openrewrite/Preconditions.java

Lines 65 to 67 in 41ce444

if (sourceFile != null && !v.isAcceptable(sourceFile, ctx)) {
return SearchResult.found(tree);
}

Preconditions.not marks every sourcefile that is not acceptable, but if the recipes(visitors) that come after that have the appropriate isAcceptable checks, that's fine right?

The problem was that when it would call the isAcceptable for DependencyInsight, it would say that all pom.xml files for example were fine to work on, but that line you referenced negates the isAcceptable and then adds the file to the acceptable list which meant that all non-pom.xml files were also fine to work on, which was the problem.

@timtebeek timtebeek self-requested a review July 16, 2025 14:08
This was referenced Jul 16, 2025
Merged
Merged
@steve-aom-elliott steve-aom-elliott force-pushed the steve-fix-does-not-include-dependency-for-incorrect-types branch from 33cd43e to 1e0aa4a Compare July 17, 2025 20:28
timtebeek
timtebeek approved these changes Jul 17, 2025
View reviewed changes

@timtebeek timtebeek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good; thanks for the fix here! We'll keep an eye on the for now only use downstream:
https://github.com/openrewrite/rewrite-testing-frameworks/blob/366deb6292a0e17daf021268e5acfc7550ce859f/src/main/resources/META-INF/rewrite/junit5.yml#L136-L146

@timtebeek timtebeek merged commit 36d68f7 into main Jul 17, 2025
2 checks passed
@timtebeek timtebeek deleted the steve-fix-does-not-include-dependency-for-incorrect-types branch July 17, 2025 22:04
@github-project-automation github-project-automation Bot moved this from Ready to Review to Done in OpenRewrite Jul 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@timtebeek timtebeek timtebeek approved these changes

+1 more reviewer

@jevanlingen jevanlingen jevanlingen left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@steve-aom-elliott steve-aom-elliott

Labels

bug Something isn't working gradle maven test provided Already replicated with a unit test, using JUnit pioneer's ExpectedToFail

Projects

OpenRewrite
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@steve-aom-elliott @Laurens-W @timtebeek @jevanlingen