Reject empty reference tokens in JsonPatch - #68613
Conversation
There was a problem hiding this comment.
Pull request overview
Updates ParsedPath parsing in both JsonPatch implementations to reject empty JSON Pointer reference tokens (e.g., //, trailing /), aligning behavior with the intended “no empty tokens” policy and preventing silent path collapsing.
Changes:
- Add explicit rejection of empty reference tokens during path parsing in
ParsedPath. - Treat empty string (
"") as a valid “root pointer” path (returns no segments). - Expand unit tests to cover leading
/paths, empty path, and empty-token rejection cases.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Features/JsonPatch/src/Internal/ParsedPath.cs | Rejects empty reference tokens and handles empty path early. |
| src/Features/JsonPatch/test/Internal/ParsedPathTests.cs | Adds coverage for leading /, empty string, and empty-token failures. |
| src/Features/JsonPatch.SystemTextJson/src/Internal/ParsedPath.cs | Mirrors the same empty-token rejection + empty-path handling for STJ. |
| src/Features/JsonPatch.SystemTextJson/test/Internal/ParsedPathTests.cs | Mirrors the same test coverage changes for STJ. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| if (path.Length == 0) | ||
| { | ||
| return Array.Empty<string>(); | ||
| } |
| if (path.Length == 0) | ||
| { | ||
| return Array.Empty<string>(); | ||
| } |
| [Theory] | ||
| [InlineData("//isSmth")] | ||
| [InlineData("//")] | ||
| [InlineData("/foo/")] | ||
| [InlineData("/foo//bar")] | ||
| [InlineData("foo//bar")] | ||
| [InlineData("foo/")] | ||
| public void PathWithEmptyReferenceTokenShouldFail(string path) |
| [Theory] | ||
| [InlineData("//isSmth")] | ||
| [InlineData("//")] | ||
| [InlineData("/foo/")] | ||
| [InlineData("/foo//bar")] | ||
| [InlineData("foo//bar")] | ||
| [InlineData("foo/")] | ||
| public void PathWithEmptyReferenceTokenShouldFail(string path) |
| } | ||
| else | ||
| { | ||
| throw new JsonPatchException(Resources.FormatInvalidValueForPath(path), null); |
There was a problem hiding this comment.
per RFC 6901 section 5 both "" and "/" are valid and we accept the first one and reject the latter. Should we acccept both instead?
There was a problem hiding this comment.
@DeagleGross From the same spec // is also valid but we are rejecting it.
Maybe we should try to be fully compliant then?
The reason I rejected / is that it essentially means a key which is empty string, which is very similar to // (just that an extra level of nesting).
So I would say we either handle empty keys everywhere, or we don't handle empty keys.
For "" it means whole document, not empty key, which is why I viewed it differently.
There was a problem hiding this comment.
Maybe we should try to be fully compliant then?
Ye, I imagined we started to treat none of these as errors.
There was a problem hiding this comment.
Okay. I'll look more into the implementation and the RFC to get it right.
| [InlineData("/foo//bar")] | ||
| [InlineData("foo//bar")] | ||
| [InlineData("foo/")] | ||
| public void PathWithEmptyReferenceTokenShouldFail(string path) |
There was a problem hiding this comment.
could we add an E2E tests for such scenario to not only asssert the exception, but validate what's the HTTP response?
|
|
||
| [Theory] | ||
| [InlineData("//isSmth")] | ||
| [InlineData("//")] |
Fixes #67819