feat(fs,fs-lite): add dataSuffix option to prevent file/directory key collisions#767
Open
claygeo wants to merge 5 commits into
Open
feat(fs,fs-lite): add dataSuffix option to prevent file/directory key collisions#767claygeo wants to merge 5 commits into
claygeo wants to merge 5 commits into
Conversation
… collisions When both `foo` and `foo:bar` exist as storage keys, the fs drivers try to create a file at `<base>/foo` and a directory at `<base>/foo/`, causing ENOTDIR errors. Add a `dataSuffix` option (e.g. `".data"`) that appends a suffix to all stored file paths on disk. With `dataSuffix: ".data"`, key `foo` is stored as `foo.data` and key `foo:bar` as `foo/bar.data`, eliminating the collision. The suffix is transparently stripped from keys returned by `getKeys()` and watch callbacks. The option defaults to `undefined` (no suffix) for full backward compatibility -- consumers like Nitro can opt in by setting it.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughFilesystem drivers fs and fs-lite gain an optional ChangesCollision Avoidance via File Suffix
Sequence Diagram(s)sequenceDiagram
participant Client
participant Driver as "FS Driver<br/>(fs / fs-lite)"
participant rFile as "rFile(key)<br/>Path Resolver"
participant Disk as "Filesystem"
participant Watcher as "Chokidar Watcher<br/>(fs only)"
Client->>Driver: setItem("foo", value)
Driver->>rFile: rFile("foo")
rFile->>rFile: r("foo") + ".data"
rFile-->>Driver: "/base/foo.data"
Driver->>Disk: write "/base/foo.data"
Disk-->>Driver: ✓
Driver-->>Client: ✓
Client->>Driver: getKeys()
Driver->>Disk: readdir recursive
Disk-->>Driver: ["foo.data", "bar.data", ...]
Driver->>Driver: filter *.data, strip suffix
Driver-->>Client: ["foo", "bar", ...]
Disk-->>Watcher: fs event "/base/foo.data"
Watcher->>Driver: event(relPath: "foo.data")
Driver->>Driver: strip ".data"
Driver->>Client: callback("update", "foo")
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/drivers/fs.test.ts (1)
115-121: Consider reusing the storage instance from the test.The
afterEachhook creates a new storage instance for cleanup. Since each test already creates and disposes its own storage instance, you could track the storage instance at the describe level and reuse it for cleanup.♻️ Suggested refactor
describe("dataSuffix option", () => { const suffixDir = resolve(__dirname, "tmp/fs-suffix"); + let storage: ReturnType<typeof createStorage>; afterEach(async () => { - const s = createStorage({ - driver: driver({ base: suffixDir, dataSuffix: ".data" }), - }); - await s.clear(); - await s.dispose(); + if (storage) { + await storage.clear(); + await storage.dispose(); + } }); it("prevents file/directory collision with dataSuffix", async () => { const d = driver({ base: suffixDir, dataSuffix: ".data" }); - const storage = createStorage({ driver: d }); + storage = createStorage({ driver: d }); // ... rest of test without the dispose call at the end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/drivers/fs.test.ts` around lines 115 - 121, The afterEach currently creates a new storage with createStorage/driver to clean up; instead declare a describe-scoped variable (e.g., let storage = null) and have each test assign its storage instance to that variable when it creates one, then update afterEach to check that variable and call storage.clear() and storage.dispose() if present (and reset it to null); update references in afterEach from the temporary s to this describe-scoped storage and ensure tests still dispose or let afterEach handle disposal consistently (methods: createStorage, driver, afterEach, clear, dispose).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/drivers/fs.test.ts`:
- Around line 115-121: The afterEach currently creates a new storage with
createStorage/driver to clean up; instead declare a describe-scoped variable
(e.g., let storage = null) and have each test assign its storage instance to
that variable when it creates one, then update afterEach to check that variable
and call storage.clear() and storage.dispose() if present (and reset it to
null); update references in afterEach from the temporary s to this
describe-scoped storage and ensure tests still dispose or let afterEach handle
disposal consistently (methods: createStorage, driver, afterEach, clear,
dispose).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40e828ed-5314-4a62-93d0-75b0b27886e2
📒 Files selected for processing (4)
src/drivers/fs-lite.tssrc/drivers/fs.tstest/drivers/fs-lite.test.tstest/drivers/fs.test.ts
…ffix is set getKeys was using map (passing through non-suffixed files as phantom keys) instead of filter+map. Watcher was emitting events for non-suffixed files. Both now ignore files that don't end with the configured dataSuffix.
|
@pi0 (sorry for the direct ping), but would you mind taking a look at this PR? The issue it resolves directly impacts Nitro and, as a result, real Nuxt applications. Thanks in advance! |
Harden the dataSuffix option from this PR: - Validate dataSuffix at construction: reject empty, "." , ".." and any value containing "/", "\" or ":". These would create unexpected nesting, escape the base directory, or break the getKeys/watch suffix round-trip. - Fix a base escape: a root/empty key resolves to `base` itself, so `base + dataSuffix` wrote a sibling file *outside* the configured base directory (e.g. `<base>.data`). Keep the suffixed file inside base instead. - Document `dataSuffix` for both drivers, including the on-disk migration caveat (enabling it hides pre-existing un-suffixed files). - Add tests: suffix validation, strip-exactly-once for a key already ending in the suffix, getKeys ignoring non-suffixed files, watcher suffix stripping, and a root-key no-escape regression.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/drivers/fs.test.ts`:
- Around line 204-205: Replace the hardcoded 700ms setTimeout delay in the
watcher test with a condition-based polling mechanism that repeatedly checks for
the expected update (the watcher observing the file change) within a reasonable
bounded timeout (such as 5 seconds with small intervals like 50-100ms). This
ensures the test passes quickly when the condition is met while still providing
enough time on slow CI runners, eliminating flakiness from fixed sleep
durations.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3965eb4-ae28-413e-89d4-2e94ac5ab2d8
📒 Files selected for processing (5)
docs/2.drivers/fs.mdsrc/drivers/fs-lite.tssrc/drivers/fs.tstest/drivers/fs-lite.test.tstest/drivers/fs.test.ts
✅ Files skipped from review due to trivial changes (1)
- docs/2.drivers/fs.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/drivers/fs.ts
- src/drivers/fs-lite.ts
Comment on lines
+204
to
+205
| await new Promise((resolve) => setTimeout(resolve, 700)); | ||
| // separator-agnostic (key may use ":", "/" or "\\" depending on OS/normalization) |
There was a problem hiding this comment.
Replace fixed sleep with condition-based waiting in watcher test.
The hardcoded 700ms delay can cause flaky CI (slow runners) and unnecessary wait time (fast runners). Wait until the expected update is observed with a bounded timeout instead of sleeping a fixed duration.
Suggested change
- await new Promise((resolve) => setTimeout(resolve, 700));
+ const deadline = Date.now() + 3000;
+ while (Date.now() < deadline) {
+ const updates = watcher.mock.calls
+ .filter(([event]) => event === "update")
+ .map(([, key]) => String(key));
+ if (updates.some((k) => /(^|[:/\\])random$/.test(k))) {
+ break;
+ }
+ await new Promise((resolve) => setTimeout(resolve, 25));
+ }🤖 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 `@test/drivers/fs.test.ts` around lines 204 - 205, Replace the hardcoded 700ms
setTimeout delay in the watcher test with a condition-based polling mechanism
that repeatedly checks for the expected update (the watcher observing the file
change) within a reasonable bounded timeout (such as 5 seconds with small
intervals like 50-100ms). This ensures the test passes quickly when the
condition is met while still providing enough time on slow CI runners,
eliminating flakiness from fixed sleep durations.
Author
|
Polished this and re-confirmed it's still needed: the released and current This push adds:
@pi0 this is the unstorage |
The migration caveat used ::alert{type="warning"}, which is not a
container component in this docs setup (every other doc uses ::warning).
It would have rendered as literal text. Switch to ::warning.
Adversarial review surfaced two honest caveats worth documenting (no code change needed): - A key that itself ends with the suffix can still collide with a same-named namespace (e.g. "foo" vs "foo.data:bar" with suffix ".data"). Tell users to pick a suffix their keys do not end with. - clear() removes the whole base dir, including the non-suffixed files that getItem/getKeys otherwise ignore.
Author
|
Friendly nudge on this one, and a small update. I re-confirmed the file/directory key collision still reproduces on the latest Since the original PR I've hardened the
Still mergeable with no conflicts, default @pi0 you pointed nitrojs/nitro#4156 here as the right home for this fix. Would you (or another maintainer) be able to take a look when you get a chance? Happy to adjust naming or scope. Thanks! |
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.
Summary
When both
fooandfoo:barexist as storage keys, thefsandfs-litedrivers map them to<base>/foo(file) and<base>/foo/bar(inside directoryfoo/). This creates an ENOTDIR collision becausefoocannot be both a file and a directory on disk.This PR adds a
dataSuffixoption to both drivers. When set (e.g.dataSuffix: ".data"):foois stored asfoo.dataon diskfoo:baris stored asfoo/bar.dataon diskfoo.datais a file,foo/is a directoryThe suffix is transparently stripped from keys returned by
getKeys()and watch callbacks. The option defaults toundefinedfor full backward compatibility.Context
This addresses the root cause of file/directory collisions reported in:
fsorfs-litefails when a route is both a path and a prefix of another path nitrojs/nitro#4142Per pi0's feedback, the fix belongs in the unstorage fs driver rather than in Nitro's cache key sanitization.
Changes
src/drivers/fs.ts-- AddeddataSuffixoption, splitr()intor()(directory resolution) andrFile()(file resolution with suffix), updated all file operations to userFile(), strip suffix ingetKeys()and watch callbackssrc/drivers/fs-lite.ts-- Same changes asfs.tstest/drivers/fs.test.ts-- Added test verifyingfoo+foo:barcoexistence withdataSuffix, and basic CRUD with suffixtest/drivers/fs-lite.test.ts-- Same tests asfs.test.tsUsage
Summary by CodeRabbit
dataSuffixto filesystem storage drivers (fs/fs-lite) to prevent on-disk collisions (e.g.,foovsfoo:bar) via suffixed leaf files.getKeys()and change notifications now return logical keys with the suffix stripped.dataSuffixvalues and ensured the empty/root key can’t escape the storage base.dataSuffix, collision scenarios, constraints, and related behaviors (includingclear()).