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

Should JSON be able to encode JSON.Encoder.t() keys? #14305

Closed
ruslandoga opened this issue Feb 28, 2025 · 7 comments
Closed

Should JSON be able to encode JSON.Encoder.t() keys? #14305

ruslandoga opened this issue Feb 28, 2025 · 7 comments

Comments

@ruslandoga
Copy link
Contributor

ruslandoga commented Feb 28, 2025

👋

Elixir and Erlang/OTP versions

Erlang/OTP 27 [erts-15.2] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] [jit]
Elixir 1.18.1 (compiled with Erlang/OTP 27)

Operating system

mac 15.3 (24D60)

Current behavior

The current mismatch between

iex> JSON.encode!(%{Date.utc_today => 1})
** (FunctionClauseError) no function clause matching in :elixir_json.key/2

    The following arguments were given to :elixir_json.key/2:

        # 1
        ~D[2025-02-28]

        # 2
        #Function<0.97253845/2 in JSON.protocol_encode>

    (elixir 1.18.1) src/elixir_json.erl:357: :elixir_json.key/2
    (elixir 1.18.1) src/elixir_json.erl:320: :elixir_json."-do_encode_map/2-lc$^0/1-0-"/2
    (elixir 1.18.1) src/elixir_json.erl:320: :elixir_json.do_encode_map/2
    (elixir 1.18.1) lib/json.ex:352: JSON.encode!/2
    iex:18: (file)

and

iex> JSON.encode!(%{1 => Date.utc_today})
#==> "{\"1\":\"2025-02-28\"}"

is a bit confusing.

Expected behavior

Maybe JSON could be permissive like Jason?

iex> Jason.encode!(%{Date.utc_today => 1})
#==> "{\"2025-02-28\":1}"
@ruslandoga
Copy link
Contributor Author

ruslandoga commented Feb 28, 2025

Ah! Seems to be by design:

%% Dispatching any value through `Encode` could allow incorrect
%% JSON to be emitted (with keys not being strings). To avoid this,
%% the default encoder only supports binaries, atoms, and numbers.
%% Customisation is possible by overriding how maps are encoded in general.

@ruslandoga ruslandoga closed this as not planned Won't fix, can't repro, duplicate, stale Feb 28, 2025
@ruslandoga ruslandoga changed the title Should Elixir.JSON be able to be able to encode keys? Should Elixir.JSON be able to be able to encode Date keys? Mar 1, 2025
@ruslandoga ruslandoga changed the title Should Elixir.JSON be able to be able to encode Date keys? Should JSON be able to be able to encode Date keys? Mar 1, 2025
@ruslandoga
Copy link
Contributor Author

ruslandoga commented Mar 2, 2025

I checked https://hexdocs.pm/elixir/1.18.2/JSON.html and https://hexdocs.pm/elixir/1.18.2/JSON.Encoder.html and didn't see any mention of this behavior. Should it be documented? I'd be happy to PR :) However, I still think it would be safer to attempt better compatibility with Jason. I don't fully understand how keys are different from values and why :elixir_json.key checks the raw value and not JSON.Encoder.encoded one. I assume it has something to do with iolists being slow to check? But there are already places like do_encode_checked where keys are converted from iolists to binaries anyway, so maybe if JSON.Encoder.encode for a key returns a list, it can be passed to something like do_encode_checked instead of raising an error.


Note that people are starting to switch to JSON assuming it's equivalent to Jason:

@ruslandoga ruslandoga reopened this Mar 2, 2025
@ruslandoga ruslandoga changed the title Should JSON be able to be able to encode Date keys? Should JSON be able to be able to encode JSON.Encoder.t() keys? Mar 2, 2025
@josevalim
Copy link
Member

We do document them here, only certain maps are allowed: https://hexdocs.pm/elixir/JSON.html#module-encoding

@michalmuskala do you think we should go ahead and allow any key to be encoded?

@ruslandoga
Copy link
Contributor Author

ruslandoga commented Mar 3, 2025

We do document them here, only certain maps are allowed: https://hexdocs.pm/elixir/JSON.html#module-encoding

🤦 I can't read :)

However date and co. don't show up in that table, and yet they are encodable. So it's possible to assume that maps with encodable keys are also encodable.

Could there be a warning highlighting maps with non-binary-atom-integer keys? And that a custom encoder is necessary for those maps?

@ruslandoga ruslandoga changed the title Should JSON be able to be able to encode JSON.Encoder.t() keys? Should JSON be able to encode JSON.Encoder.t() keys? Mar 3, 2025
@josevalim
Copy link
Member

We will either support additional keys or we will add a warning. But I don't see a reason to not support any type of key at the moment.

@michalmuskala
Copy link
Member

michalmuskala commented Mar 3, 2025

Jason supports any keys implementing the String.Chars protocol.

Fundamentally, it doesn't make sense to use the same protocol for keys & values - primarily because in JSON itself, per the spec, the keys can be only strings. So it wouldn't work well if we suddenly had a key encoder using the same encoder for values returning an array or anything else that isn't a string

@josevalim
Copy link
Member

Closing in favor of #14309.

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

No branches or pull requests

3 participants