Skip to content

feat: JdocSpecReader.streamFiles file filter, @MustBeClosed annotation#135

Open
boolivar wants to merge 2 commits into
masterfrom
streamfiles
Open

feat: JdocSpecReader.streamFiles file filter, @MustBeClosed annotation#135
boolivar wants to merge 2 commits into
masterfrom
streamfiles

Conversation

@boolivar
Copy link
Copy Markdown
Owner

@boolivar boolivar commented Apr 8, 2026

No description provided.

@qodo-code-review
Copy link
Copy Markdown

Review Summary by Qodo

Add file filtering and @MustBeClosed annotation to streamFiles

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Add @MustBeClosed annotation to streamFiles method
• Improve file filtering with regular file check
• Use case-insensitive .java extension matching
• Add Apache Commons Lang3 dependency import
Diagram
flowchart LR
  A["streamFiles method"] --> B["Add @MustBeClosed annotation"]
  A --> C["Enhance file filter logic"]
  C --> D["Check Files.isRegularFile"]
  C --> E["Case-insensitive .java check"]
Loading

Grey Divider

File Changes

1. jdoc-junit-engine-commons/src/main/java/org/bool/jdoc/core/JdocSpecReader.java ✨ Enhancement +6/-3

Enhance file filtering and add resource management annotation

• Added @MustBeClosed annotation to streamFiles method to enforce resource management
• Enhanced file filtering in streamFiles to check for regular files using Files.isRegularFile()
• Changed .java extension matching to case-insensitive using StringUtils.endsWithIgnoreCase()
• Added imports for @MustBeClosed annotation and StringUtils from Apache Commons Lang3

jdoc-junit-engine-commons/src/main/java/org/bool/jdoc/core/JdocSpecReader.java


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented Apr 8, 2026

Code Review by Qodo

🐞 Bugs (2)   📘 Rule violations (0)   📎 Requirement gaps (0)   🎨 UX Issues (0)
🐞\ ≡ Correctness (1) ☼ Reliability (1)

Grey Divider


Action required

1. MustBeClosed stream not closed 🐞
Description
JdocSpecReader.streamFiles() is annotated @MustBeClosed but readSpecs() flatMaps it without
explicitly closing anything, which can violate ErrorProne’s MustBeClosed contract and may leak
Files.walk resources on some execution paths (e.g., exceptions). This can fail the build (ErrorProne
enabled) or exhaust file handles during discovery.
Code

jdoc-junit-engine-commons/src/main/java/org/bool/jdoc/core/JdocSpecReader.java[R41-45]

    public List<SpecSource> readSpecs(List<DiscoverySelector> selectors) {
        return selectors.stream()
-                .flatMap(this::streamFiles).distinct()
-                .map(parser::parse).collect(Collectors.toList());
+                .flatMap(this::streamFiles).distinct().map(parser::parse)
+                .collect(Collectors.toList());
    }
Evidence
readSpecs() composes a stream pipeline from selectors.stream().flatMap(this::streamFiles) and
immediately collects, but does not manage closing of the streams returned by streamFiles().
streamFiles() is explicitly marked @MustBeClosed and, for DirectorySelector, returns
Files.walk(...), which holds OS resources until closed. The repository enables ErrorProne in the
build, so this pattern is likely to be reported as a MustBeClosed violation and/or create runtime
resource pressure.

jdoc-junit-engine-commons/src/main/java/org/bool/jdoc/core/JdocSpecReader.java[41-58]
buildSrc/src/main/kotlin/jspecify-conventions.gradle.kts[4-29]
buildSrc/src/main/groovy/java-conventions.gradle[79-99]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`streamFiles()` returns a `Files.walk()` stream and is annotated `@MustBeClosed`, but the current `readSpecs()` pipeline does not explicitly manage closing. With ErrorProne enabled, this can be flagged as a `@MustBeClosed` contract violation, and it also risks leaking file descriptors if streams aren’t closed.

### Issue Context
- `Files.walk()` returns a resource-backed `Stream<Path>`.
- The project enables ErrorProne via build conventions.
- `@MustBeClosed` communicates (and can enforce) that the returned value must be closed by the caller.

### Fix Focus Areas
- jdoc-junit-engine-commons/src/main/java/org/bool/jdoc/core/JdocSpecReader.java[41-58]

### Suggested direction
Pick one of these approaches:
1) **Refactor to avoid returning a close-requiring stream**: for `DirectorySelector`, use `try (Stream<Path> walk = Files.walk(...)) { return walk.filter(...).toList().stream(); }` so the resource is closed before returning.
2) **Remove `@MustBeClosed`** if relying on downstream closure semantics is intentional and verified.
3) **Inline directory walking into `readSpecs()`** with explicit `try-with-resources` around `Files.walk()` per directory selector, and merge results into a single `Stream`/`List`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. FileSelector bypasses java filter 🐞
Description
streamFiles() filters DirectorySelector results to regular *.java files but returns FileSelector
paths unfiltered, so selecting a non-.java file can reach JavaFileParser.parse() and throw
JdocException. This makes behavior inconsistent and can break discovery unexpectedly.
Code

jdoc-junit-engine-commons/src/main/java/org/bool/jdoc/core/JdocSpecReader.java[R50-55]

        if (selector instanceof DirectorySelector) {
            return Files.walk(((DirectorySelector) selector).getPath())
-                    .filter(path -> path.getFileName().toString().endsWith(".java"));
+                    .filter(path -> Files.isRegularFile(path) && StringUtils.endsWithIgnoreCase(path.getFileName().toString(), ".java"));
        }
        if (selector instanceof FileSelector) {
            return Stream.of(((FileSelector) selector).getPath());
Evidence
Only the DirectorySelector branch applies the new isRegularFile && endsWithIgnoreCase('.java')
filter; the FileSelector branch returns the selected path as-is. JavaFileParser.parse() throws when
parsing is unsuccessful, so a non-Java file (or unreadable path) selected via FileSelector will
propagate an exception out of readSpecs().

jdoc-junit-engine-commons/src/main/java/org/bool/jdoc/core/JdocSpecReader.java[49-57]
jdoc-core/src/main/java/org/bool/jdoc/core/JavaFileParser.java[49-57]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`streamFiles()` applies a `.java` filter for directories but not for file selectors. This allows non-Java files to reach `JavaFileParser.parse()` and throw.

### Issue Context
The class-level intent is to read specs from selected Java files; directory traversal already enforces this after the PR change.

### Fix Focus Areas
- jdoc-junit-engine-commons/src/main/java/org/bool/jdoc/core/JdocSpecReader.java[49-57]

### Suggested fix
In the `FileSelector` branch, apply the same predicate used for directory results (regular file + `.java` case-insensitive). For example, return `Stream.of(path).filter(p -> Files.isRegularFile(p) && endsWithIgnoreCase(p.getFileName().toString(), ".java"))` (handling `getFileName()==null` if needed).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

Comment on lines 41 to 45
public List<SpecSource> readSpecs(List<DiscoverySelector> selectors) {
return selectors.stream()
.flatMap(this::streamFiles).distinct()
.map(parser::parse).collect(Collectors.toList());
.flatMap(this::streamFiles).distinct().map(parser::parse)
.collect(Collectors.toList());
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

1. Mustbeclosed stream not closed 🐞 Bug ☼ Reliability

JdocSpecReader.streamFiles() is annotated @MustBeClosed but readSpecs() flatMaps it without
explicitly closing anything, which can violate ErrorProne’s MustBeClosed contract and may leak
Files.walk resources on some execution paths (e.g., exceptions). This can fail the build (ErrorProne
enabled) or exhaust file handles during discovery.
Agent Prompt
### Issue description
`streamFiles()` returns a `Files.walk()` stream and is annotated `@MustBeClosed`, but the current `readSpecs()` pipeline does not explicitly manage closing. With ErrorProne enabled, this can be flagged as a `@MustBeClosed` contract violation, and it also risks leaking file descriptors if streams aren’t closed.

### Issue Context
- `Files.walk()` returns a resource-backed `Stream<Path>`.
- The project enables ErrorProne via build conventions.
- `@MustBeClosed` communicates (and can enforce) that the returned value must be closed by the caller.

### Fix Focus Areas
- jdoc-junit-engine-commons/src/main/java/org/bool/jdoc/core/JdocSpecReader.java[41-58]

### Suggested direction
Pick one of these approaches:
1) **Refactor to avoid returning a close-requiring stream**: for `DirectorySelector`, use `try (Stream<Path> walk = Files.walk(...)) { return walk.filter(...).toList().stream(); }` so the resource is closed before returning.
2) **Remove `@MustBeClosed`** if relying on downstream closure semantics is intentional and verified.
3) **Inline directory walking into `readSpecs()`** with explicit `try-with-resources` around `Files.walk()` per directory selector, and merge results into a single `Stream`/`List`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Stream.flatMap() closes mapped streams!

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 8, 2026

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.02%. Comparing base (1cc18bc) to head (0ac2399).
⚠️ Report is 21 commits behind head on master.

Files with missing lines Patch % Lines
...c/main/java/org/bool/jdoc/core/JdocSpecReader.java 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #135      +/-   ##
============================================
- Coverage     90.22%   90.02%   -0.21%     
- Complexity      172      173       +1     
============================================
  Files            36       36              
  Lines           481      481              
  Branches         21       21              
============================================
- Hits            434      433       -1     
  Misses           40       40              
- Partials          7        8       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

1 participant