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 Nullpointer for Gradle Parsing #6392

Merged
merged 2 commits into from
Jan 15, 2023

Conversation

sconvent
Copy link
Contributor

@sconvent sconvent commented Jan 8, 2023

This will fix issue #6384

The problem was that Dependabot initially recognized correctly that there is no build.gradle.kts in the root folder, but later tried to access it anyways (See top_level_build_file and the comment above here)

This problem is solved with an additional .compact() statement, which will remove any nil entries in the list of files to be looked into.

Tested it by executing it on (an old state) of one of my repos with:
bin/dry-run.rb gradle sconvent/dependabot-issue-demonstrator --commit addf67d1aa51dade24e47d1043abbd0baa54332a

Works as expected (i.e. no Nullpointers)

It will probably also fix #4793, as it is a very similar issue.

Happy to get some feedback as this is my first PR for Dependabot.

@sconvent sconvent requested a review from a team as a code owner January 8, 2023 12:18
@jeffwidman jeffwidman force-pushed the fix-nullpointer-gradle branch from 17b8b24 to cfc9dd9 Compare January 9, 2023 03:28
Copy link
Member

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

LGTM, however could you add a unit test for this scenario to prevent future regressions?

Odds of regressing on this exact problem are low, but the fact that we're hitting an error here shows we don't have much (any?) test coverage for this scenario of a missing top-level file... and odds of regressing in some other way for this scenario are high enough that we ought to get that test in place.

It should be straightforward to write by copying one of the existing tests (maybe one of the ones added in #4256?), but if you run into questions / need help please let me know.

@@ -86,7 +86,7 @@ def property_details(property_name:, callsite_buildfile:)
all_files = [callsite_buildfile, top_level_buildfile].concat(
FileParser.find_includes(callsite_buildfile, dependency_files),
FileParser.find_includes(top_level_buildfile, dependency_files)
Copy link
Member

Choose a reason for hiding this comment

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

I checked, and FileParser.find_includes() already protects against hitting NullPointer:

@sconvent
Copy link
Contributor Author

sconvent commented Jan 9, 2023

Linting issue should be fixed now.

Sure, I'll add a testcase. Thanks for the hint where I can find one.

@jeffwidman
Copy link
Member

Thanks. Feel free to squash your commits unless they're logically separate, or I'm also happy to do that at merge time. We aren't sticklers for single commit PR's esp if multiple discrete changes need to land in a single PR, but no need for a distinct commit just to fix a lint issue.

@sconvent sconvent force-pushed the fix-nullpointer-gradle branch from 3133069 to d34c98d Compare January 15, 2023 12:21
@sconvent
Copy link
Contributor Author

Added a testcase (for each Groovy and Kotln DSL) and squashed everything into one commit.
The testcases are a bit different from the one you mentioned. @jeffwidman . But they failed without the changes in the actual code and now succeed.

Copy link
Member

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

Thank you! Tests look reasonable to me.

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.

Gradle updates fail with "undefined method `name' for nil:NilClass'"
2 participants