Fix NPE error during directory file listing in file sync #377
Conversation
WalkthroughRefactors FileSyncTask to support multiple directories via a new constructor, makes key fields final, improves logging, adds null/existence guards for directory listing, reuses listed subFiles in size extraction, and enhances error handling by rethrowing exceptions as TaskException while updating log phrasing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Task as FileSyncTask
participant FS as Filesystem
participant Repo as FileStatRepository
Caller->>Task: start()
Note over Task: Log: starting with N directories
loop For each directory
Task->>FS: listFiles(dir)
alt Directory missing or listFiles == null
Task-->>Task: skip directory
else Valid listing
Task->>Task: extractFolderSize(subFiles)
Task->>Repo: save/update directoryStat
end
end
Task-->>Caller: success
Note over Task: Log: Completed file stat sync
rect rgba(255,224,224,0.35)
Note over Task: Error path
Caller->>Task: start()
Task->>Task: catches Exception
Task->>Caller: throws TaskException
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
owlplug-client/src/main/java/com/owlplug/plugin/tasks/FileSyncTask.java (1)
86-94: Order of operations issue: check directory existence before deleting stats.The current implementation deletes existing file stats from the database (line 86) and flushes (line 88) before checking if the directory exists or if
listFiles()returns null (line 92). If the directory doesn't exist or an I/O error occurs, the stats are deleted but not recreated, leading to data loss.Apply this diff to check existence before deletion:
this.updateMessage("Collecting file metrics on directory: " + directory.getAbsolutePath()); - fileStatRepository.deleteByPath(FileUtils.convertPath(directory.getAbsolutePath())); - // Flushing context to the database as next queries will recreate entities - fileStatRepository.flush(); - // files can be null in case of I/O exceptions File[] subFiles = directory.listFiles(); if (!directory.exists() || subFiles == null) { return length; } + + fileStatRepository.deleteByPath(FileUtils.convertPath(directory.getAbsolutePath())); + // Flushing context to the database as next queries will recreate entities + fileStatRepository.flush();
🧹 Nitpick comments (1)
owlplug-client/src/main/java/com/owlplug/plugin/tasks/FileSyncTask.java (1)
67-70: Consider logging when a directory doesn't exist.The code silently skips non-existent directories without logging. This could make troubleshooting difficult when a configured directory path is invalid.
Consider adding a log statement:
File directory = new File(directoryPath); if (directory.exists() && directory.isDirectory()) { length = extractFolderSize(directory, null); log.info("Completed file stat sync on directory {}, computed length: {}", directoryPath, length); + } else { + log.warn("Directory does not exist or is not a directory: {}", directoryPath); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
owlplug-client/src/main/java/com/owlplug/plugin/tasks/FileSyncTask.java(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (6)
owlplug-client/src/main/java/com/owlplug/plugin/tasks/FileSyncTask.java (6)
38-38: LGTM! Fields properly marked as final.Making
fileStatRepositoryanddirectoriesfinal improves immutability and clearly communicates that these fields won't be reassigned after construction.Also applies to: 40-40
44-44: Good optimization with Collections.singletonList.Using
Collections.singletonListinstead ofArrays.asListis more memory-efficient for single-element lists and better expresses the intent.
48-52: LGTM! Multi-directory support added.The new constructor properly enables processing multiple directories while maintaining the existing single-directory API.
60-60: Good logging addition.Logging the number of directories being processed improves observability.
90-94: LGTM! Null check prevents NPE.The null check for
subFilescorrectly addresses the NPE issue from #376. ThelistFiles()method returns null when the path doesn't denote a directory or an I/O error occurs.
107-107: Good optimization reusing subFiles.Reusing the
subFilesvariable avoids callingdirectory.listFiles()twice, which improves performance.
Relates to #376