Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jul 17, 2025
Merged

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

merged 3 commits into from
Jul 17, 2025

Conversation

steve-aom-elliott
Copy link
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
Copy link
Contributor

Laurens-W commented Jul 16, 2025 •
edited
Loading

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
Copy link
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
Copy link
Member

@timtebeek timtebeek left a comment

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

@jevanlingen jevanlingen jevanlingen left review comments

@timtebeek timtebeek timtebeek approved these changes

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