-
Notifications
You must be signed in to change notification settings - Fork 96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: input escape mapping #311
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.
Files not reviewed (1)
- src/index.ts: Evaluated as low risk
Comments suppressed due to low confidence (1)
src/pathstringifier.test.ts:11
- The expected output for the input 'test\a.b' should be ['test\a', 'b'] based on the escaping logic.
['test\\a.b', ['test\a', 'b']]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I didn't realize you'd be willing to embed a data version—but since you are, 👍.
I do still think it would be valuable to require that every property in meta
always finds a json
node, though, because there are quite possibly current serializations of relatively simple objects that are silently broken (e.g., { "a.0": /regex that becomes a string/ }
, which you'll note does not have any backslashes at all).
if (!legacyPaths) { | ||
const isEscapedBackslash = char === '\\' && string.charAt(i + 1) === '\\'; | ||
if (isEscapedBackslash) { | ||
segment += '\\'; | ||
i++; | ||
continue; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This correctly handles escaped backslashes, but does not implement special behavior for backslashes that are in final position (e.g., "foo.bar.baz\\"
) or followed by something other than a dot or backslash (e.g., "foo.bar.b\\az"
or "foo.bar.b\\\n"
). I strongly recommend rejecting such input rather than treating it as synonymous with e.g. "foo.bar.baz\\\\"
/"foo.bar.b\\\\az"
/"foo.bar.b\\\\\n"
(respectively).
if (!legacyPaths) { | |
const isEscapedBackslash = char === '\\' && string.charAt(i + 1) === '\\'; | |
if (isEscapedBackslash) { | |
segment += '\\'; | |
i++; | |
continue; | |
} | |
} | |
if (!legacyPaths && char === '\\') { | |
const escaped = string.charAt(i + 1); | |
if (escaped === '\\') { | |
segment += '\\'; | |
i++; | |
continue; | |
} else if (escaped !== '.') { | |
throw Error('invalid path'); | |
} | |
} |
Closes #310
There's a bug in the path escaping logic where meta was applied to the wrong values. This PR fixes the escape bug. It also introduces a version tag to toggle the path escaping when deserializing. This ensures backwards compatibility for SuperJSON results that were created on a version with the bug.