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

feat: add serde preset #1027

Closed

Conversation

jonaslagoni
Copy link
Member

Description
This PR introduces a few changes to the Rust generator:

  1. It removes default serde attributes alongside #[derive(Deserialize, Serialize) attributes.
  2. Adds a new preset for adding serde attributes and derive attributes alongside tests for the preset.
  3. Adapts rust docs to the general setup for serialization and deserialization that should show how to sterilize the data models.
  4. It adds two new examples, one for showing basic rust generation as we have for other outputs, and a new one for the serde preset.

Some things I could not figure out @leigh-johnson that I need your help with:

  1. Is there no other type we can use for AnyModel in rust than serde_json::Value? 🤔
  2. I could not figure out how to separate the package dependencies with the current preset hooks, might be missing one or more in order to allow presets to add their own dependencies and features 🤔 Not sure if this is required for this PR?

@sonarcloud
Copy link

sonarcloud bot commented Dec 1, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@leigh-johnson
Copy link
Collaborator

leigh-johnson commented Dec 1, 2022

👋 Before getting into a more detailed review, I'm curious about the motivation for this change.

Was there an ask to support a different serializer, like rustc_serialize? I'm asking because rustc_serialize is deprecated and the community has settled chiefly on serde as the standard serialize/deserialize library.

serde provides generic Deserialization/Serialization traits, which libraries like serde_json implement. You can inspect the data formats that implement serde's Deserialization/Serialization traits here: https://serde.rs/#data-formats

@jonaslagoni
Copy link
Member Author

jonaslagoni commented Dec 1, 2022

serde provides generic Deserialization/Serialization traits, which libraries like serde_json implement. You can inspect the data formats that implement serde's Deserialization/Serialization traits here: https://serde.rs/#data-formats

The main driver is that presets are used for adding extra functionality to the core models, cause not everyone will use the models for Deserialization/Serialization of them. This is the same setup we have for all other generators 🙂

And even so, it's completely up to the individual how they want to customize their models, which functionality they should support etc. This PR adds that customization.

So yea, it's nothing more then keeping it consistent across generators and keeping the core models (default) as minimalistic as possible🙂

@leigh-johnson
Copy link
Collaborator

serde provides generic Deserialization/Serialization traits, which libraries like serde_json implement. You can inspect the data formats that implement serde's Deserialization/Serialization traits here: https://serde.rs/#data-formats

The main driver is that presets are used for adding extra functionality to the core models, cause not everyone will use the models for Deserialization/Serialization of them. This is the same setup we have for all other generators slightly_smiling_face

And even so, it's completely up to the individual how they want to customize their models, which functionality they should support etc. This PR adds that customization.

So yea, it's nothing more then keeping it consistent across generators and keeping the core models (default) as minimalistic as possibleslightly_smiling_face

Gotcha, this revision is oriented towards encapsulating all "extra behavior" in Modelina into presets?

I'm struggling to understand the value added here, but I do see significant added complexity (e.g. 2x the number of test snapshots to maintain).

I could see the value if someone was looking to implement an alternate serialization library, but without that need, it strikes me as unnecessary. Can you help me understand the value added, and why it's worth the time/energy to maintain?

@leigh-johnson
Copy link
Collaborator

leigh-johnson commented Dec 1, 2022

Description This PR introduces a few changes to the Rust generator:

  1. It removes default serde attributes alongside #[derive(Deserialize, Serialize) attributes.
  2. Adds a new preset for adding serde attributes and derive attributes alongside tests for the preset.
  3. Adapts rust docs to the general setup for serialization and deserialization that should show how to sterilize the data models.
  4. It adds two new examples, one for showing basic rust generation as we have for other outputs, and a new one for the serde preset.

Some things I could not figure out @leigh-johnson that I need your help with:

  1. Is there no other type we can use for AnyModel in rust than serde_json::Value? thinking
  2. I could not figure out how to separate the package dependencies with the current preset hooks, might be missing one or more in order to allow presets to add their own dependencies and features thinking Not sure if this is required for this PR?

You can downcast a type that implements std::any::Any, but you need to use unsafe { } code blocks to do this and implement runtime error handling. As a guiding principle, you generally want to avoid unsafe {} blocks in Rust unless you perform fine-grain checks for undefined behavior at runtime.
See examples of std::any::Any here: https://doc.rust-lang.org/std/any/trait.Any.html

Here's an example of handling unsafe {} code blocks in the real world - in this case, I'm interacting with a raw pointer to memory allocated in nnstreamer's C libs. I am enforcing that the data has a known buffer length and shape. I'm just linking this as a concrete example of the info/context needed to ensure unsafe {} code does not produce undefined behavior.
https:/bitsy-ai/printnanny-gst-plugin-rs/blob/main/printnanny-gst-plugin/src/nnstreamer.rs

I wouldn't want to ship unsafe {} code in a generated client. It'd be an elephant-sized challenge to ensure you do not ship undefined behavior.

Also, implementing runtime checks for undefined behavior would make the code hella slow.


A better approach is to rely on the Value enum in serde libraries.

Each serde implementation usually provides a Value container for heterogeneous types. I imagine there would be a Preset for each of these. However, these would all implement the Deserialize and Serialize traits.

JSON:
https://docs.rs/serde_json/latest/serde_json/value/enum.Value.html

YAML:
https://docs.rs/serde_json/latest/serde_json/value/enum.Value.html

Avro:
https://docs.rs/avro-rs/latest/avro_rs/types/enum.Value.html

Notably, Rust's binary data serde implementations cannot implement a Value enum. So BSON, MsgPack, Flat/FlexBuffer, Arrow, etc.

@jonaslagoni
Copy link
Member Author

jonaslagoni commented Dec 2, 2022

Gotcha, this revision is oriented towards encapsulating all "extra behavior" in Modelina into presets?

Yes!

I could see the value if someone was looking to implement an alternate serialization library, but without that need, it strikes me as unnecessary.

While one strong use-case is adding serializing and deserializing functionality, Modelina's primary use-case is in its diversity, in order to become part of your toolbox and be useful as soon as you want to generate models in any use-case. The models come in the most basic form and you as a user will have to add the layers of functionality you need through the presets. Making you in control of what fits into your use-case.

It does not assume that you need to serialize and deserialize the models, or even that you are using a particular library (even though it sounds like for Rust, there is JUST one 😆) Might have misunderstood you in our discussion in your first PR then as I read it as there were multiple libraries for serializing models in Rust 😄

In an AsyncAPI world, would you ever generate the models without serialize and deserialize capabilities? In general probably not. But we have gotten feature requests such as #982, where there would be no need for it, and there are others in a similar fashion.

Modelina is not meant as a "client" generator for MQTT, NATS, REST, or any other protocol, it's only a tool for generating models and if it's in the context of protocols, of course, serialization and deserialization are a huge part. Hence why I suggested we have At least three ways to serialize models. But that's just a single feature within Modelina 🙂

Maybe these goals of Modelina need to be well described somewhere instead of indirectly 🤔

Can you help me understand the value added, and why it's worth the time/energy to maintain?

I want to point something out here, just because there exists a preset within Rust, does mean YOU have to provide the time to maintain it. Always keep that in mind, that presets can have dedicated maintainers as well as you are for the core Rust generator 🙂 Hence why I suggested we pursue 2 code owners per area of responsibility.

There are many parts to Modelina that we need to figure out how are maintained, and what their compatibility is. But that is a thing for the future I think 🙂


Hope that clarified my intentions with this PR, otherwise do let me know 😄 I just wanted to make this change before we released version 1, as this change would have to wait for another major version if it was to be fixed later.

Do you think we should create a mission statement somewhere so this is clear? 🤔

@jonaslagoni
Copy link
Member Author

jonaslagoni commented Dec 2, 2022

Also, implementing runtime checks for undefined behavior would make the code hella slow.

Hmmmmm, yea that does pose a problem 🤔 What to do 🤔

Would it make sense we render our own any structure in a similar fashion as serde_json::Value 🤔? Otherwise yea, we might have to just rely on serde functionality for the any structure 😄 And we can always assess in the future whether there is a better option for it's value.

@leigh-johnson
Copy link
Collaborator

leigh-johnson commented Dec 2, 2022

@jonaslagoni Is there a funding commitment to staff 2 maintainers per Modelina language?

Apologies if this sounds harsh, but I depend on this code in production and thus have a pragmatic approach to maintenance costs. The Modelina implementation is already 2-3 orders of magnitude more time-intensive than maintaining my OpenAPI schemas + generator, so from a business PoV, I'm already having a hard time justifying time investment.

I want to point something out here, just because there exists a preset within Rust, does mean YOU have to provide the time to maintain it. Always keep that in mind, that presets can have dedicated maintainers as well as you are for the core Rust generator slightly_smiling_face Hence why #848.

@leigh-johnson
Copy link
Collaborator

Also, implementing runtime checks for undefined behavior would make the code hella slow.

Hmmmmm, yea that does pose a problem thinking What to do thinking

Would it make sense we render our own any structure in a similar fashion as serde_json::Value thinking? Otherwise yea, we might have to just rely on serde functionality for the any structure smile And we can always assess in the future whether there is a better option for it's value.

Can you help me understand the problem with using serde_json::Value here? If you implement some special AsyncApi::Any type enum, then that type will leak into the rest of my code. serde_json::Value is used to indicate the contained value is 1) heterogeneous and 2) JSON-serializable.

If you need a different serialization library (YAML, Avro, etc), then the equivalent would be serde_yaml::Value or avro_rs::types::Value

@jonaslagoni
Copy link
Member Author

Closing as I have misunderstood what Serde enables 😄

@jonaslagoni jonaslagoni closed this Dec 2, 2022
@jonaslagoni jonaslagoni deleted the separate_serde_annotation branch December 2, 2022 19:59
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

Successfully merging this pull request may close these issues.

2 participants