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

Serde Behavior #629

Open
benhaney opened this issue Jul 7, 2024 · 3 comments
Open

Serde Behavior #629

benhaney opened this issue Jul 7, 2024 · 3 comments

Comments

@benhaney
Copy link
Contributor

benhaney commented Jul 7, 2024

This is a summary and continuation of benhaney/Jsonrs#26, by @filmor's request.

It was recently suggested to me that Jsonrs should switch to using Rustler's new Serde integration as its backend, which I'm very excited to do. But before I can, I need to make sure that Rustler's use of Serde can match what Jsonrs expects, so I don't break backwards compatibility. This issue represents the problems I've seen so far.

In approximate order of importance:

  1. Rustler explicitly serializes the atoms :ok and :error into the strings "Ok" and "Err", inconsistent with the serialization of all other atoms. This is by far the biggest issue and should also be the easiest to fix.
  2. I handle encoding Elixir's Decimal struct at the Rust layer, so I will either need that same support in Rustler (sorry!) or a way to pass some custom struct encoding functions to the deserializer. I think the former would be a lot easier if it isn't too objectionable (and I'm happy to contribute the implementation I already have), but I recognize that it would be a bit weird for a general purpose NIF library like Rustler to specially handle struct encoding of specific Elixir packages, even if they're very popular.
  3. I noticed some tests related to error tuples failed when testing Jsonrs using Rustler's Serde integration. An example:
     code:  Jsonrs.decode("invalid") === {:error, "expected value at line 1 column 1"}
     left:  {:error, "SerializationError(\"expected value at line 1 column 1\")"}
     right: {:error, "expected value at line 1 column 1"}
    
    This isn't necessarily something that Rustler should deal with, it just wasn't immediately obvious to me why some of these changed and I need to make a note to look into why it happened at all and what I might need to do in Jsonrs to hammer the errors back into the format I was seeing before. I'm not terribly against changing error strings, but it would technically be a breaking change for Jsonrs so I'd like to avoid it where reasonable. If anyone's immediately aware of what changed here, it would save me the investigation.

I think that's all I've found so far. Let me know your thoughts and what I can do to help with any of this!

@filmor
Copy link
Member

filmor commented Jul 7, 2024

On 1.: What this tries to achieve is to make {ok, Bla} and {error, Blubb} map to and from Rust Result types. I'll see whether there is a better way.

On 2.: I'd like to solve this via explicit registration of struct conversions.

@benhaney
Copy link
Contributor Author

@filmor When you have a chance, could you take a look at #639 and let me know if it seems like a good approach to fixing the first issue? It's mostly a cleanup of a change I made to my own serde_rustler fork, and I was originally just focusing on correcting the transcoding behavior, but I think this version should preserve the ok/error<->Result conversion for explicit encode/decode just fine.

Once we have that sorted out, I'd love to hear if you have any thoughts on how the interface for the second issue (registering struct conversions) should look

@filmor
Copy link
Member

filmor commented Jul 20, 2024

On the struct serde: I think registering using inventory, like what we do for resources and nifs already will do for the global "instance", but I'll need to give this a bit more thought.

As a first step, Deserializer and Serializer need to be extended to keep and use a map struct name -> dyn StructDe/encoder. This can be a simple dispatch mechanism specific to structs, I think.

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

2 participants