feat: adding Storage, FileService, and FileServiceResolver - BED-8314#2801
Open
mykeelium wants to merge 5 commits into
Open
feat: adding Storage, FileService, and FileServiceResolver - BED-8314#2801mykeelium wants to merge 5 commits into
mykeelium wants to merge 5 commits into
Conversation
mykeelium
added
enhancement
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds backend-agnostic storage interfaces and a sandboxed LocalStore filesystem implementation with atomic operations, path-safety, service wrappers/resolver, generated mocks, comprehensive tests, dependency bumps, and a nil-check in upload validation. ChangesStorage Abstraction and LocalStore Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
mykeelium
force-pushed
the
mcuomo/BED-8313
branch
from
082af65 to
1969000
Compare
mykeelium
force-pushed
the
mcuomo/BED-8314
branch
from
14fac98 to
bc5f6fb
Compare
wes-mil
approved these changes
May 26, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/api/src/services/upload/upload.go (1)
96-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn an error when temp file close fails.
Line 96 only logs close failures, but Line 117 can still return success. That can surface partially written files as valid.
Proposed fix
- if closeErr := tempFile.Close(); closeErr != nil { - slog.Error( - "Error closing temp file", - slog.String("file", tempFileName), - attr.Error(closeErr), - ) - } + closeErr := tempFile.Close() + if closeErr != nil { + slog.Error( + "Error closing temp file", + slog.String("file", tempFileName), + attr.Error(closeErr), + ) + } @@ if validationErr != nil { if removeErr := os.Remove(tempFileName); removeErr != nil { @@ } - return "", validationErr + if closeErr != nil { + return "", errors.Join(validationErr, fmt.Errorf("error closing temp file: %w", closeErr)) + } + return "", validationErr } + + if closeErr != nil { + if removeErr := os.Remove(tempFileName); removeErr != nil { + slog.Error( + "Error deleting temp file after close failure", + slog.String("file", tempFileName), + attr.Error(removeErr), + ) + } + return "", fmt.Errorf("error closing temp file: %w", closeErr) + }Also applies to: 116-117
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/api/src/services/upload/upload.go` around lines 96 - 103, The temp file close error is only logged but the function can still return success; update the upload flow so that after calling tempFile.Close() (check the closeErr from tempFile.Close) you return or propagate that error instead of continuing; locate the tempFile.Close() call and the subsequent success return around tempFileName (and where the code later returns success near lines 116-117) and make the function return an error (or wrap and return closeErr) when closeErr != nil so partially written temp files are not treated as successful uploads.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/api/src/services/graphify/tasks.go`:
- Around line 51-52: The current early return in tasks.go drops any successful
per-file results from ExtractIngestFiles and leaves temp files behind; change
the logic around the call to ExtractIngestFiles so that when it returns
(fileData, err) you do not discard fileData: either return fileData along with
the error (i.e., return fileData, err) so the caller can handle successes, or,
if you must fail-fast, iterate the returned IngestFileData slice and perform
explicit cleanup of their temp paths before returning the error; update the
calling code that expects []IngestFileData so it handles non-empty slices with
non-nil errors.
In `@cmd/api/src/services/storage/storage.go`:
- Around line 151-159: In StorageFileService.ReadFile, preserve and return any
rc.Close() error instead of dropping it: after calling s.Storage.Get and
deferring/avoiding an unhandled defer, read bytes with io.ReadAll(rc) into
variables (data, readErr), then call closeErr := rc.Close(); if both readErr and
closeErr are non-nil return a wrapped/joined error (e.g., wrap readErr and
include closeErr), otherwise return whichever non-nil error (or data,nil).
Update the function around s.Storage.Get, io.ReadAll and rc.Close to implement
this error-joining logic.
- Around line 277-299: In NewDefaultFileServices, if any NewLocalStore call
fails you must close previously opened stores (workStore, ingestStore,
retainStore, collectorsStore) to avoid descriptor leaks; update the error paths
so that when, for example, NewLocalStore(cfg.TempDirectory()) returns an error
you call Close() on workStore before returning, and similarly close
workStore+ingestStore if retainStore creation fails, etc.; implement this by
checking err after each NewLocalStore and calling Close() on all non-nil
previously created stores (or use a small helper/cleanup slice to iterate Close
on failure) before returning the error.
In `@cmd/api/src/services/upload/upload.go`:
- Around line 75-93: The function WriteAndValidateFileWithPrefix currently calls
validationFunc without checking for nil; add a nil guard at the top of the
function (before invoking validationFunc) that returns a clear error if
validationFunc == nil, and ensure any opened tempFile is properly closed and
removed when returning due to this error (use tempFile.Close() and
os.Remove(tempFileName) if the temp file was created). Update references to
validationFunc, tempFile and tempFileName to ensure no nil dereference or
resource leak occurs when returning early.
---
Outside diff comments:
In `@cmd/api/src/services/upload/upload.go`:
- Around line 96-103: The temp file close error is only logged but the function
can still return success; update the upload flow so that after calling
tempFile.Close() (check the closeErr from tempFile.Close) you return or
propagate that error instead of continuing; locate the tempFile.Close() call and
the subsequent success return around tempFileName (and where the code later
returns success near lines 116-117) and make the function return an error (or
wrap and return closeErr) when closeErr != nil so partially written temp files
are not treated as successful uploads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 2531ec7b-1941-4c05-8647-aa849b70a559
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
cmd/api/src/services/graphify/ingest_storage.gocmd/api/src/services/graphify/ingest_storage_test.gocmd/api/src/services/graphify/tasks.gocmd/api/src/services/storage/localstore.gocmd/api/src/services/storage/localstore_test.gocmd/api/src/services/storage/mocks/fs.gocmd/api/src/services/storage/storage.gocmd/api/src/services/storage/storage_test.gocmd/api/src/services/upload/upload.gocmd/api/src/services/upload/upload_test.gogo.mod
Comment on lines
+51
to
52
| if fileData, err := ExtractIngestFiles(s.ctx, s.cfg, task.StoredFileName, task.OriginalFileName, task.FileType, "bh"); err != nil { | ||
| return []IngestFileData{}, err |
Contributor
There was a problem hiding this comment.
Preserve partial extraction results on helper errors.
ExtractIngestFiles can return successful fileData entries alongside a non-nil aggregated error. Returning an empty slice here drops those per-file statuses and leaves any already-extracted temp files unprocessed and unremoved. Keep the partial results visible to the caller, and either continue ingesting the successful entries or explicitly clean up their paths before returning.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/api/src/services/graphify/tasks.go` around lines 51 - 52, The current
early return in tasks.go drops any successful per-file results from
ExtractIngestFiles and leaves temp files behind; change the logic around the
call to ExtractIngestFiles so that when it returns (fileData, err) you do not
discard fileData: either return fileData along with the error (i.e., return
fileData, err) so the caller can handle successes, or, if you must fail-fast,
iterate the returned IngestFileData slice and perform explicit cleanup of their
temp paths before returning the error; update the calling code that expects
[]IngestFileData so it handles non-empty slices with non-nil errors.
Contributor
Author
There was a problem hiding this comment.
not in this changeset. plus was existing functionality on a refactor PR. If desired I can add a change for this.
Contributor
There was a problem hiding this comment.
@mykeelium, that's a fair point — this behavior predates the refactor and the PR is intentionally not changing semantics. Would you like me to open a GitHub issue to track the follow-up improvement (returning partial fileData alongside errors and cleaning up temp files on failure)?
🧠 Learnings used
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 2382
File: cmd/api/src/api/v2/ad_related_entity.go:44-57
Timestamp: 2026-02-23T17:06:23.991Z
Learning: In BloodHound, AD nodes are represented by a base kind (ad.Entity) plus specific kinds (ad.User, ad.Computer, ad.Group, etc.), and Azure nodes by az.Entity. The query.KindIn function matches if a node has any of the specified kinds, so using the base kind (ad.Entity) will correctly include all AD nodes regardless of their specific subtype. During reviews, ensure code that filters or queries graph nodes via KindIn uses the base kind to capture all inherited subkinds for AD nodes, and similarly recognizes base kinds for Azure nodes. This design pattern should be consistently respected across files that model or query graph schema kinds.
f54015d to
108fc40
Compare
mykeelium
force-pushed
the
mcuomo/BED-8314
branch
from
92891ed to
5f21b2d
Compare
Contributor
|
Actionable comments posted: 0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This change sets up and builds out the implementation of a centralized file service and storage mechanism that can be used in future updated.
Motivation and Context
Resolves BED-8314
How Has This Been Tested?
This change does not add any functionality to current workflows. Tests were added and previous tests pass.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores