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

Some map keys not encodable #93

Open
lizziepaquette opened this issue Oct 9, 2019 · 9 comments
Open

Some map keys not encodable #93

lizziepaquette opened this issue Oct 9, 2019 · 9 comments

Comments

@lizziepaquette
Copy link

We recently had a situation where our map keys were mixed tuples eg. {:hi, 1}. Jason encoding was failing because it expected our map's keys to be of a type that implements the String.Chars protocol.

What is the rational for not recursively calling encode on the map's keys?

I think it would require a small change in this part of the code:

string = String.Chars.to_string(other)

I'm happy to open a pr if you think this is a good idea!

@OvermindDL1
Copy link

What is the rational for not recursively calling encode on the map's keys?

How would that work? JSON keys can only be strings, you can't encode another structure there.

@mskv
Copy link

mskv commented Oct 10, 2019

On a similar note. With Jason 1.1.2 When I do the following:

Jason.encode(%{{"some", "tuple"} => "val"})

I'd expect to receive {:error, Exception.t()} with Exception informing about {"some", "tuple"} not implementing the String.Chars protocol. Especially since the specification says {:error, Jason.EncodeError.t() | Exception.t()}. However, what I get is a raised exception. Is that a bug or is it intentional?

@OvermindDL1
Copy link

A thrown exception is pretty common in both erlang and elixir for invalid inputs. For valid inputs that just aren't able to be encoded I'd expect an EncodeError or so.

@mskv
Copy link

mskv commented Oct 11, 2019

Right. You could argue though that trying to pass a key that does not implement String.Chars protocol is just as much an invalid input as trying to pass a value that does not implement Jason.Encoder protocol. The latter case results in

{:error, %Protocol.UndefinedError{}}

while the former throws an exception.

@OvermindDL1
Copy link

You could argue though that trying to pass a key that does not implement String.Chars protocol is just as much an invalid input as trying to pass a value that does not implement Jason.Encoder protocol.

Eh, I'm not sure I would. Something that does not implement the Jason.Encoder protocol is a potential valid input, it just needs a slight change (an implementation) to fix it. Passing something that can't be a string as a key is just outright invalid in all cases and cannot be as fixed as it shows an inherent issue in the structure of the passed in data to begin with. JSON is exceptionally strict in that keys must be strings. Where you can encode something in a value position in a variety of ways.

@rockneurotiko
Copy link

Something that does not implement the Jason.Encoder protocol is a potential valid input, it just needs a slight change (an implementation) to fix it.

In that case, something that does not implement String.Chars is a potential valid input too, it just need a slight change to fix it:

defimpl String.Chars, for: Tuple do
  def to_string(tuple), do: tuple |> Tuple.to_list |> Enum.join(":")
end

@chulkilee
Copy link
Contributor

So the main issue here is: whether Jason should raise error, or return error tuple, for such invalid value.

Jason.encode(%{{:hello, 1} => 1})

** (Protocol.UndefinedError) protocol String.Chars not implemented for {:hello, 1} of type Tuple...

Is there performance hit if check the protocol is implemented for value, or catch the protocol error, for valid input? If there is a way to not raise exception with minimum performance impact.. I think that could be reasonable "nice interface" of Jason.

For example, if exception is raised, and not handled properly, it may lead to nasty issues, such as exposing secret information to logging. Of course, that should be different topic, but as a user, when there are encode/1 andencode!, I expect encode/1 not to raise exception for known cases.

@voughtdq
Copy link

voughtdq commented Aug 15, 2023

If there is an issue with performance, safe_decode or unsafe_decode could be introduced so there is always an escape hatch to the more performant decode function. Do you think that would work? The only problem is that this would cause breaking changes in the case of adding this functionality to the regular decode call.

@michalmuskala
Copy link
Owner

The behavior of not encoding certain values I consider correct - JSON spec defines keys as strings and in Elixir the best concept of what a "string" is (with coercions), is the to_string function and the String.Chars protocol.

The main issue, as I see it, is backwards compatibility. Arguably Jason should be returning the error in the tuple here, rather than raising - after all it does the exact same thing, if the value is not encodable and key shouldn't be any different.

This could change potentially in the 2.0 release, though I currently have no plans for bigger changes like that. If you need to handle in now, I'd recommend a simple wrapper function intercepting the exception.

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

No branches or pull requests

7 participants