Skip to content

Test uses negative array indexes #47

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

Open
pbrisbin opened this issue Apr 11, 2025 · 1 comment · May be fixed by #48
Open

Test uses negative array indexes #47

pbrisbin opened this issue Apr 11, 2025 · 1 comment · May be fixed by #48

Comments

@pbrisbin
Copy link

pbrisbin commented Apr 11, 2025

Correct me if I'm wrong, but in the JSON Pointer spec, I don't think it is possible to even parse necessary to represent a negative array index:

   o  If the currently referenced value is a JSON array, the reference
      token MUST contain either:

      *  characters comprised of digits (see ABNF below; note that
         leading zeros are not allowed) that represent an unsigned
         base-10 integer value, making the new referenced value the
         array element with the zero-based index identified by the
         token, or

      *  exactly the single character "-", making the new referenced
         value the (nonexistent) member after the last array element.

   The ABNF syntax for array indices is:

   array-index = %x30 / ( %x31-39 *(%x30-39) )
                 ; "0", or digits without a leading "0"

Therefore, I think this is a misleading test:

  {
    "doc": {
      "bar": [
        1,
        2
      ]
    },
    "patch": [
      {
        "op": "add",
        "path": "/bar/-1",
        "value": "5"
      }
    ],
    "error": "Out of bounds (lower)"
  }

I think it would be reasonable for an implementation to a) not parse negative numbers in reference tokens and b) never check for lower-bounds errors (since negative indexes are simply not possible). Doing that seems to result in path being reported as invalid because "-1" is treated as an object key that is inappropriate to reference an array value, which I think is more in the spirit of the spec.

The path is reported invalid either way, so the test is still valuable in that it tests for some error condition, but it currently tests for an error due to bounds specifically, so I bumped on it when trying to make my test suite a little more strict.

mnot added a commit that referenced this issue Apr 20, 2025
@mnot mnot linked a pull request Apr 20, 2025 that will close this issue
@mnot
Copy link
Member

mnot commented Apr 20, 2025

Seems reasonable; see #48.

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 a pull request may close this issue.

2 participants