Skip to content

GradleFiles project scan should adhere to global root settings#9263

Open
sid-srini wants to merge 1 commit into
apache:masterfrom
sid-srini:gradle-files-scan-root
Open

GradleFiles project scan should adhere to global root settings#9263
sid-srini wants to merge 1 commit into
apache:masterfrom
sid-srini:gradle-files-scan-root

Conversation

@sid-srini
Copy link
Copy Markdown
Contributor

@sid-srini sid-srini commented Mar 12, 2026 •
edited by matthiasblaesing
Loading

Fixed GradleFiles scan for project files to honour the global scan root settings, if any.

Fixed GradleFiles to find parentScript for kotlin script.

  • Added a unit test case for it in GradleFilesTest.

Fixed SimpleFileOwnerQueryImplementation to check for scan root dirs without allowing sibling dirs with the same name prefix.

  • Example: if scan root = "/tmp/app",
    • then "/tmp/application" should not be allowed for scan;
    • while "/tmp/app" and "/tmp/app/src" should remain allowed.

Fixed GradleFilesTest for File comparison on MacOS.

  • Needed due to the presence of "/private" prefix in the canonical path of unix temp folders.

^Add meaningful description above

Click to collapse/expand PR instructions

By opening a pull request you confirm that, unless explicitly stated otherwise, the changes -

  • are all your own work, and you have the right to contribute them.
  • are contributed solely under the terms and conditions of the Apache License 2.0 (see section 5 of the license for more information).

Please make sure (eg. git log) that all commits have a valid name and email address for you in the Author field.

If you're a first time contributor, see the Contributing guidelines for more information.

If you're a committer, please label the PR before pressing "Create pull request" so that the right test jobs can run.

PR approval and merge checklist:

  1. Was this PR correctly labeled, did the right tests run? When did they run?
  2. Is this PR squashed?
  3. Are author name / email address correct? Are co-authors correctly listed? Do the commit messages need updates?
  4. Does the PR title and description still fit after the Nth iteration? Is the description sufficient to appear in the release notes?

If this PR targets the delivery branch: don't merge. (full wiki article)

@mbien mbien added Gradle [ci] enable "build tools" tests ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) labels Mar 12, 2026
@apache apache locked and limited conversation to collaborators Mar 12, 2026
@apache apache unlocked this conversation Mar 12, 2026
@lkishalmi lkishalmi self-requested a review March 12, 2026 23:33
@sid-srini
Copy link
Copy Markdown
Contributor Author

sid-srini commented Apr 28, 2026

Hi @lkishalmi. Please review this when you get a chance. Thank you.

matthiasblaesing
matthiasblaesing reviewed May 5, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

In general makes sense to me. I left a few inline comments.

Comment thread ide/projectapi/src/org/netbeans/modules/projectapi/SimpleFileOwnerQueryImplementation.java
for (String scanRoot : projectScanRoots) {
if (path.startsWith(scanRoot)
&& (path.length() == scanRoot.length()
|| path.charAt(scanRoot.length()) == '/'))
Copy link
Copy Markdown
Contributor

@matthiasblaesing matthiasblaesing May 5, 2026

Choose a reason for hiding this comment

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

I assume this change intents to check that we don't get a substring match for the last component. So far so sensible, but will it work on windows paths?

PS: Also does not match logic in GradleFiles.Searcher#notWithinProjectScanRoots

Copy link
Copy Markdown
Contributor Author

@sid-srini sid-srini May 14, 2026

Choose a reason for hiding this comment

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

Thanks @matthiasblaesing for your review and the close attention on these path comparison differences.

As noted in this class, in the separatePaths method, projectScanRoots store FileObject.getPath() values since getOwner supplies and compares with FileObject.getPath() strings. The FileObject instances canonicalize and always return / as the file-separator character, even on Windows.

This explicitly does not match the logic in GradleFiles.Searcher#notWithinProjectScanRoots which compares with File.separatorChar, because there GradleFiles uses File instance path comparisons instead. Thus, GradleFiles.Searcher#separatePaths notes that it needs to store both absolute and canonical paths in projectScanRoots.

I hope this clarifies the difference in approach.

Comment thread extide/gradle/test/unit/src/org/netbeans/modules/gradle/spi/GradleFilesTest.java
Comment thread extide/gradle/src/org/netbeans/modules/gradle/NbGradleProjectFactory.java Outdated
Comment thread extide/gradle/src/org/netbeans/modules/gradle/spi/GradleFiles.java Outdated
@sid-srini
Copy link
Copy Markdown
Contributor Author

sid-srini commented May 14, 2026

Thank you @matthiasblaesing for your review and feedback.
I've added a commit incorporating the feedback, fix and comments. Please review them. Thanks a lot.

@mbien mbien added this to the NB31 milestone May 14, 2026
@sid-srini sid-srini force-pushed the gradle-files-scan-root branch from af01fc0 to 53c3535 Compare May 19, 2026 12:34
@sid-srini
Copy link
Copy Markdown
Contributor Author

sid-srini commented May 19, 2026

I've rebased this to the current master which includes post NB30 changes.
@matthiasblaesing - Kindly re-trigger the CI checks and review the changes. Thanks a lot.

matthiasblaesing
matthiasblaesing approved these changes May 29, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

Looks sane to me. Thank you.

@matthiasblaesing
Copy link
Copy Markdown
Contributor

matthiasblaesing commented May 29, 2026

@sid-srini would you please squash the commits?

@sid-srini
GradleFiles project scan should adhere to global root settings
754446c 
Fixed GradleFiles scan for project files to honour the global scan root
settings, if any.
- Similar to that in projectapi SimpleFileOwnerQueryImpl.
- GradleFiles needs to necessarily traverse the parent directory
  hierarchy to find the root project settings.
- Further, the project detection needs to give precedence to maven
  projects also defined for the same directory, which has a similar
  hierarchy traversal requirement introduced in apache#1280.
- Note that future changes in logic should be done in both the classes.

Fixed GradleFiles to find parentScript for kotlin script.
- Added a unit test case for it in GradleFilesTest.

Fixed SimpleFileOwnerQueryImplementation to check for scan root dirs
without allowing sibling dirs with the same name prefix.
- Example: if scan root = "/tmp/app",
    - then "/tmp/application" should not be allowed for scan;
    - while "/tmp/app" and "/tmp/app/src" should remain allowed.

Fixed the SimpleFileOwnerQueryImplementation.separatePaths() method
to not trim() the split paths and only ignore completely blank paths.

Fixed GradleFilesTest for File comparison on MacOS.
- Needed due to the presence of "/private" prefix in the canonical path
  of unix temp folders.

Unit tests are split into 2 classes:
- GradleFilesWithScanRootTest only runs the GradleFilesTest
  with the project limitScanRoot and forbiddenFolders
  properties set.
- GradleFilesScanRootTest contains only the functionality of the
  scan-root and forbidden folders specifically.
    - Only a single JUnit @test can be used here since the static
      initialization in @before must be done once only.
    - This means the test root folder must not change between tests.
    - The actual gradle project is now a grandchild in the tree of the
      test root dir.

Signed-off-by: Siddharth Srinivasan <siddharth.srinivasan@oracle.com>
@sid-srini sid-srini force-pushed the gradle-files-scan-root branch from 53c3535 to 754446c Compare May 31, 2026 11:48
@sid-srini
Copy link
Copy Markdown
Contributor Author

sid-srini commented May 31, 2026

Thank you @matthiasblaesing for your review.

@sid-srini would you please squash the commits?

I've pushed the squashed commit now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matthiasblaesing matthiasblaesing matthiasblaesing approved these changes

@lkishalmi lkishalmi Awaiting requested review from lkishalmi

Assignees

No one assigned

Labels

ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) Gradle [ci] enable "build tools" tests Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form)

Projects

None yet

Milestone

NB31

Development

Successfully merging this pull request may close these issues.

3 participants

@sid-srini @matthiasblaesing @mbien