JSON.Encoder shouldn't encode a decimal as a string

[azizk]

Hi!

I would like to raise a concern regarding the recent change that encodes Decimal.new("1.2") as "1.2" instead of 1.2.
This behaviour stems from the Jason library but in my opinion it is completely incorrect, and now this has been carried over to the Decimal library here.

This way of encoding decimals affects how data is sent to clients and other servers but also how data is dumped to the database (if ecto is configured as such).
It will break API protocols, database queries, data processing etc. when floats aren't encoded correctly preserving their natural type.

If for any reason it is desirable to encode a decimal as a string, that should be done manually by transforming the data before it is encoded.

[wojtekmach]

But decimals are not floats. I concede string is definitely not ideal and very inefficient but, for example, we can encode 1.00 (precision of two) and get that back after decoding, something we don’t have with floats. Additionally we can support +inf and -inf.

I’d personally much rather not implement JSON.Encoder at all than use floats. Curious however how popular languages and libraries do it, if you could do do some research and report back that’d be appreciated.

[azizk]

Ah, yes, there can be trailing zeros and special values. I didn't consider those cases and assumed no one would try to encode NaN and Inf to JSON.

But encoding 1.00 and getting back Decimal.new("1.00") actually does work (with a custom JSON.Encoder). Try: Decimal.new("1.00") |> JSON.encode!() |> JSON.decode(nil, float: &Decimal.new/1) |> elem(0)

As for NaN and Inf, maybe the right thing to do is not to allow it and to raise an exception. 🤔

Okay, I started doing the research. I'm looking at other languages and frameworks...

[azizk]

In the meantime it's possible to use a little hack to be able to encode Decimals as JSON floats.

Code.compiler_options(ignore_module_conflict: true)

defimpl JSON.Encoder, for: Decimal do
  def encode(%{coef: coef}, _encoder) when coef in [:NaN, :inf],
    do: raise(JSON.DecodeError, message: "cannot encode \"#{coef}\"")

  def encode(decimal, _encoder), do: Decimal.to_string(decimal)
end

Code.compiler_options(ignore_module_conflict: false)

I know setting compiler options like this is pretty horrible (it affects all VM processes), but what else are you gonna do?

Would you consider adding a compile-time configuration for disabling the library's defimpl, so we can define our own JSON.Encoder for Decimal?

[azizk]

But encoding 1.00 and getting back Decimal.new("1.00") actually does work (with a custom JSON.Encoder). Try: Decimal.new("1.00") |> JSON.encode!() |> JSON.decode(nil, float: &Decimal.new/1) |> elem(0)

Amendment: This works as shown, but when the data is saved to the database (Postgres) the trailing zeros are not preserved.

[azizk]

Okay, this is what I found out so far:

• Python:

  • Django provides the DjangoJSONEncoder which encodes Decimal objects as JSON strings.
  • The simplejson library supports the use_decimal=True option to serialize Decimal objects as JSON floats.
  • The builtin json module does not seem to support serializing Decimal objects as JSON floats (without precision loss).

• Ruby:

  • Ruby on Rails: encodes BigDecimals as JSON strings by default. See v8.0 ActiveSupport.
  • Ruby on Rails: this post by Phil Coggins suggests that you can override the default behavior without losing precision.

• Rust:

  • The serde_json library normally encodes BigDecimals as strings, but using the raw_value feature allows you to serialize BigDecimal as JSON floats.

• Java:

  • The Jackson library serializes a BigDecimal as a JSON float by default (but allows defining a custom encoder).

• Scala:

  • The circe library serializes a BigDecimal as a JSON float by default (but allows defining a custom encoder).

[azizk]

I guess the best thing to do is to provide a configuration setting. It should be documented well enough and we need to decide if an encoder should be included at all by default if nothing is configured.

[ericmj]

Using a string as the encoded type as default is preferred since that's the safer default with no precision lost.

Global config that affects protocols is usually not a good idea since another application in your runtime may depend and expect the string type and break when the type changes.

In this case I think it would be better for you to pass a custom JSON encoder function or wrap the decimal struct with a struct that has a custom JSON encoder implementation

[azizk]

Yes, it's the option that perfectly preserves the Decimal value. But it creates a lot of awkwardness in the code base and I'd argue it may be unsafe depending on the context. If it encodes to a JSON string, it will certainly break API protocols that require JSON floats. And if it's stored as a string in a JSON database column, queries doing calculations on that column will simply fail.

So fortunately, we don't need to wrap Decimals with another struct before encoding. I see that with Elixir v1.18 we can just use JSON.encode!/2 with a different encoder function.

Nevertheless, I think it should be the other way around. When you want to send JSON data to JavaScript clients, you should explicitly call JSON.encode!/2 with a custom encoder.

[ericmj]

For some code bases it will be inconvenient as a string, for others it will be inconvenient as a float, unfortunately we cannot make it convenient for everyone. As the purpose of this library is to preserve precision and all other float conversions are explicit an implicit lossy conversion for JSON does not fit so we will keep it as string for JSON.

Thanks for creating the issue so we could clarify the intentions of the library.

[azizk]

What's the rush to close this issue? I find this a bit alienating, especially since I spent some effort doing the research.

This decision certainly has a high impact on every project. Yes, unfortunately we can't make it convenient for everyone. But it doesn't seem like the due diligence is being done to find out which default is the best idea. It's not smart to force the now builtin encoder on everyone either. Even if everyone in the core team agrees that encoding as strings is preferred, why not let us be able to decide if we want it that way for our projects? You mentioned external dependencies may break. That may happen, and if it ever does, I'm sure we can find a workaround or ask the library author to accommodate both use cases.

At the very least document the why in the encoder and give us a configuration option to leave it out.

[wojtekmach]

Good call about lack of documentation, I've opened #220.

Regarding closing the issue, the intention was not to shut down the conversation (for that, it would have been locked which would obviously be an overkill). Please feel free to continue discussing this. I personally believe the current behavior is the most pragmatic one and validated in prior art you cited.

As you mentioned in the initial post this is the same behavior as in the Jason library. A lot of effort has been put into making Elixir's JSON be basically a drop-in replacement for Jason, by far the most popular JSON library at the moment, so compatibility with it is very important.

[azizk]

Thanks for opening a PR on improving the documentation. 👍

I suppose being backward compatible has a high priority. However, even if other frameworks/languages opt for decimals as strings, I think Elixir should try to do better and not blindly cater to the world of JavaScript. But I admit the issue is difficult and none of the solutions are ideal. For that JSON and JS would have to be amended. I just wish that going the other way (Decimals as JSON floats) were made as easy as possible and that developers were informed about the pros and cons.

Elixir's JSON module isn't only a drop-in replacement for Jason now, but also a big improvement in terms of the new decoding and encoding options (thanks to Erlang). 🙌

You haven't commented on the idea of a config switch to be able to leave out the protocol encoder entirely. What do you think?

[wojtekmach]

How would that look like? Something like:

config :decimal, …

If something along those lines I’m curious what exactly you have in mind.

But yeah if so, I’d be wary of that. I think Elixir seldom uses global settings and instead promotes explicitly passing things around to avoid surprises. Great example is second argument to JSON.encode!/2. Of course once you start using libraries that use JSON under the hood you can’t easily pass that argument into them but that’s the thing, I would argue the libraries should allow you to do that.

[ericmj]

What's the rush to close this issue? I find this a bit alienating, especially since I spent some effort doing the research.

We usually try to close issues that we believe are not actionable. We can always reopen later if that changes and a closed issue does not mean the discussion stops. Thank you @wojtekmach for opening the PR that adds the documentation.

You haven't commented on the idea of a config switch to be able to leave out the protocol encoder entirely. What do you think?

Using global configuration for dependencies, specially config that changes protocol behavior is an anti-pattern elixir. Since we have escape hatches by passing a custom encoder function or by wrapping the decimals adding the global config is not necessary.

[azizk]

How would that look like? Something like:

config :decimal, …

Yes, I was thinking about something like this:

config :decimal, json_encoder?: false

But yeah if so, I’d be wary of that. I think Elixir seldom uses global settings and instead promotes explicitly passing things around to avoid surprises.

Okay, but isn't this what :decimal is arguably doing now? It's including a global "setting" (the JSON.Encoder) which you cannot avoid without effort? I was surprised and I'm sure there are others too, but remain silent and just do what's necessary to make it work™.

..., I would argue the libraries should allow you to do that

I agree, that would be good library design.

[ericmj]

Okay, but isn't this what :decimal is arguably doing now? It's including a global "setting" (the JSON.Encoder) which you cannot avoid without effort? I was surprised and I'm sure there are others too, but remain silent and just do what's necessary to make it work™.

Sure, if you want to call that global config, but so is Elixir standard library for all of the types it provides. You cannot easily change the JSON encoder implementations of those types either.

For example DateTime are encoded as a ISO8601 string in JSON, if you want another encoding you would have to do the one of the same things we have suggested for Decimal.

[azizk]

We usually try to close issues that we believe are not actionable. We can always reopen later if that changes and a closed issue does not mean the discussion stops.

Thanks for clarifying. I understand that creating and maintaining issues involves significant time and effort for everyone involved, so I appreciate knowing the conversation can still continue.

Using global configuration for dependencies, specially config that changes protocol behavior is an anti-pattern elixir. Since we have escape hatches by passing a custom encoder function or by wrapping the decimals adding the global config is not necessary.

Hm, maybe it can be considered an anti-pattern. Doesn't seem to be documented though. I'm not sure if I agree completely, because I have a feeling that we won't think of the scenarios where that actually makes sense.

Sure, if you want to call that global config, but so is Elixir standard library for all of the types it provides. You cannot easily change the JSON encoder implementations of those types either.
For example DateTime are encoded as a ISO8601 string in JSON, if you want another encoding you would have to do the one of the same things we have suggested for Decimal.

That's a very good point. Can't argue against that. 😅