Skip to content

Json.Decode.index checks for too large index but not too small #18

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
lydell opened this issue Sep 26, 2019 · 0 comments
Open

Json.Decode.index checks for too large index but not too small #18

lydell opened this issue Sep 26, 2019 · 0 comments

Comments

@lydell
Copy link

lydell commented Sep 26, 2019

If the index given to Json.Decode.index is too large we get this message:

> JD.decodeString (JD.index 1 JD.int) """[1]""" |> Result.mapError JD.errorToString
Err ("Problem with the given value:\n\n[\n        1\n    ]\n\nExpecting a LONGER array. Need index 1 but only see 1 entries")
    : Result String Int

In other words, there is an out-of-bounds check that produces that nice message. But the index can also be out-of-bounds by being negative. In that case, there is currently no check for this, so array[-1] (which is undefined) is attempted to be decoded:

> JD.decodeString (JD.index -1 JD.int) """[1]""" |> Result.mapError JD.errorToString
Err ("Problem with the value at json[-1]:\n\n    undefined\n\nExpecting an INT")
    : Result String Int

That’s still an error message, but not quite as nice. But using -1 doesn’t always fail:

> JD.decodeString (JD.index 1 (JD.succeed ())) """[1]""" |> Result.mapError JD.errorToString
Err ("Problem with the given value:\n\n[\n        1\n    ]\n\nExpecting a LONGER array. Need index 1 but only see 1 entries")
    : Result String ()
> JD.decodeString (JD.index -1 (JD.succeed ())) """[1]""" |> Result.mapError JD.errorToString
Ok () : Result String ()

I think the error message could be nicer for a negative index. Similar to how oneOf has a nice error message for an empty list:

> JD.decodeString (JD.oneOf []) """true""" |> Result.mapError JD.errorToString
Err ("Ran into a Json.Decode.oneOf with no possibilities!")

Maybe something like this: "Ran into a Json.Decode.index with a negative index: -1".

In summary:

  • If there’s an upper bounds check, why is there no lower?
  • The error message for negative index could be better.
  • An out-of-bounds index should always fail (not just when too large) for consistency.

Note: I only noticed this because I’m re-implementing this package in Elm as a learning exercise. I’ve never accidentally passed a negative index in real code.

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

No branches or pull requests

1 participant