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

Improve JSON and Jason support #4565

Open
warmwaffles opened this issue Jan 9, 2025 · 5 comments
Open

Improve JSON and Jason support #4565

warmwaffles opened this issue Jan 9, 2025 · 5 comments
Labels

Comments

@warmwaffles
Copy link
Member

Elixir version

1.18

Database and Version

all

Ecto Versions

any

Database Adapter and Versions (postgrex, myxql, etc)

postgrex, myxql, exqlite

Current behavior

When running Elixir 1.18 and setting the json adapter to JSON, we encounter issues where encode/1 is not defined, but encode!/1 is. This issue was started here elixir-sqlite/ecto_sqlite3#159

Expected behavior

I should be able to flip between Jason and JSON to support Elixir 1.17 and 1.18.

A "quick-ish" fix is to wrap the encode!/1 from both Jason and JSON with a try rescue to keep the {:ok, term} | {:error, term} typespec behaviour.

@wojtekmach
Copy link
Member

wojtekmach commented Jan 9, 2025

Database Adapter and Versions (postgrex, myxql, etc)

postgrex, myxql, exqlite

you mentioned all adapters, does the problem appear in all of them or just exqlite?

@josevalim
Copy link
Member

We can just do {:ok, JSON.encode!(...)} I believe. Even Jason.encode fails in a bunch of different situations today and the situations where it doesn't, I think the users are unlikely to run into

@warmwaffles
Copy link
Member Author

Right.

I also see this unit https://github.com/elixir-ecto/ecto/blob/master/lib/ecto/json.ex, may need to implement the JSON.Encoder and such here as well.

@TylerWitt
Copy link
Contributor

I made a PR that handles some of the needed changes, but am I correct in thinking that the encode vs encode! changes also discussed here need to happen in the adapters themselves?

@josevalim
Copy link
Member

I made a PR that handles some of the needed changes, but am I correct in thinking that the encode vs encode! changes also discussed here need to happen in the adapters themselves?

Yes, they must!

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

No branches or pull requests

4 participants