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

application/json-merge+patch support #1649

Open
heaths opened this issue Apr 25, 2024 · 2 comments
Open

application/json-merge+patch support #1649

heaths opened this issue Apr 25, 2024 · 2 comments
Labels
Azure.Core The azure_core crate Client This issue points to a problem in the data-plane of the library. design-discussion An area of design currently under discussion and open to team and community feedback. Mgmt This issue is related to a management-plane library.

Comments

@heaths
Copy link
Member

heaths commented Apr 25, 2024

We need to figure out how to support application/json-merge+patch as described in our guidelines. We current plan to declare all fields as Option<T> to support an unfortunately all too common case of a service specification being incongruent with the service: one requires a field while the other does not, for which we do an in-place update as needed to "correct" the spec. But to support json-merge+patch, we need to support serializing an explicit null to delete data for that field.

Options discussed so far include the following, numbered only for easy reference and not by preference:

  1. Given our agreement for RequestContent<T> and how rarely json-merge+patch support is needed, it may be sufficient to ask callers to pass raw JSON content (as Bytes or str).

  2. For those models used by operations that use content-type: application/json-merge+patch, we could define in azure_core an Option<T>-like tristate enum e.g.,

    pub enum NullableOption<T> {
        Some(T),
        None,
        Null,
    }

    We'd then implement serde::Serialize and serde::Deserialize to handle the different variants much like serde implements support for Option<T>. Null would serialize null much like excluding #[serde(skip_serializing_if = "Option::is_none")] would do for an optional field if None, or elide the field much like including the attribute would if None.

We should look at what other implementations are doing in this scenario.

@heaths heaths added Client This issue points to a problem in the data-plane of the library. Mgmt This issue is related to a management-plane library. design-discussion An area of design currently under discussion and open to team and community feedback. Azure.Core The azure_core crate labels Apr 25, 2024
@heaths
Copy link
Member Author

heaths commented Apr 25, 2024

See https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=ec2c0deb734032010e4c789f5a585166 for an example. Deserialization doesn't quite work: there's no way to know during deserialization whether a null should map to None or Null. That said, mapping both to None by default during deserialization only makes sense since the absence of a field or if the field is set to null should use the default Option<T> value anyway, which would be None.

@heaths
Copy link
Member Author

heaths commented Apr 25, 2024

Looking for other solutions, about the only thing I'm finding is json-patch which has a merge function that takes a couple serde_json::Value parameters, making it dependent on serde_json and not generalized on serde: https://docs.rs/json-patch/1.2.0/src/json_patch/lib.rs.html#642-644

I don't think it's a non-starter, but less than ideal since it relies on compile-time (via the serde_json::json!() macro) or runtime via something like:

let value: Value = r#"{"foo":"bar","baz":1,"qux":null}"#.parse()?;

We really only need something like NullableOption<T> from my playground link for serialization, though it does raise question about server-side generated code down the line and whether it should support deserialization for such instances. /cc @allenjzhang @markcowl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Core The azure_core crate Client This issue points to a problem in the data-plane of the library. design-discussion An area of design currently under discussion and open to team and community feedback. Mgmt This issue is related to a management-plane library.
Projects
Status: No status
Development

No branches or pull requests

1 participant