Skip to content
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

Pointer::push_back: Handle empty keys #21

Merged
merged 1 commit into from
Feb 23, 2024
Merged

Pointer::push_back: Handle empty keys #21

merged 1 commit into from
Feb 23, 2024

Conversation

wngr
Copy link

@wngr wngr commented Feb 23, 2024

Hey!

When using Pointer::push_back with empty strings as keys the count was incremented, but the actual inner pointer value was not updated.

This PR fixes that.

I'm not sure why the check !self.inner.ends_with('/') was present in the first place.

@asmello
Copy link
Collaborator

asmello commented Feb 23, 2024

Adding some context.

Per RFC 6901§5:

   For example, given the JSON document

   {
      "foo": ["bar", "baz"],
      "": 0,
      "a/b": 1,
      "c%d": 2,
      "e^f": 3,
      "g|h": 4,
      "i\\j": 5,
      "k\"l": 6,
      " ": 7,
      "m~n": 8
   }

   The following JSON strings evaluate to the accompanying values:

    ""           // the whole document
    "/foo"       ["bar", "baz"]
    "/foo/0"     "bar"
    "/"          0
    "/a~1b"      1
    "/c%d"       2
    "/e^f"       3
    "/g|h"       4
    "/i\\j"      5
    "/k\"l"      6
    "/ "         7
    "/m~0n"      8

Note how / is a valid JSON pointer that evaluates to a root field keyed by an empty string (""), and not the whole document (which is represented as "", confusingly enough). Unlike with UNIX-style paths, every forward slash character in JSON pointers is significant and cannot be ignored when constructing paths with Pointer::push_back.

@chanced
Copy link
Owner

chanced commented Feb 23, 2024

Yea, the "/" being a valid, non-root path confused me when I first wrote it.

The check for "/" must have been an artifact I forgot to remove when I realized "" was actually the root.

Sorry about that and thank you for the fix!

@chanced chanced merged commit dfbbd87 into chanced:main Feb 23, 2024
1 check passed
@chanced
Copy link
Owner

chanced commented Feb 23, 2024

This should be available under v0.4.5. Thanks y'all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants