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

allOf with overlapping schemas fails to deserialize #370

Closed
bjorn3 opened this issue Aug 16, 2023 · 5 comments
Closed

allOf with overlapping schemas fails to deserialize #370

bjorn3 opened this issue Aug 16, 2023 · 5 comments

Comments

@bjorn3
Copy link

bjorn3 commented Aug 16, 2023

For example the following schemas derived from Zulip's api:

JsonResponseBase:
  type: object
  properties:
    result:
      type: string

JsonSuccessBase:
  description: |
    **Changes**: As of Zulip 7.0 (feature level 167), if any
    parameters sent in the request are not supported by this
    endpoint, a successful JSON response will include an
    [`ignored_parameters_unsupported`][ignored_params] array.

    A typical successful JSON response may look like:

    [ignored_params]: /api/rest-error-handling#ignored-parameters
  allOf:
    - $ref: "#/components/schemas/JsonResponseBase"
    - required:
        - result
        - msg
      properties:
        result:
          enum:
            - success
        msg:
          type: string
        ignored_parameters_unsupported:
          $ref: "#/components/schemas/IgnoredParametersUnsupported"
      example: {"msg": "", "result": "success"}

would result in the following code:

#[derive(Clone, Debug, Deserialize, Serialize)]
pub struct JsonResponseBase {
    #[serde(default, skip_serializing_if = "Option::is_none")]
    pub result: Option<String>,
}

#[derive(Clone, Debug, Deserialize, Serialize)]
pub struct JsonSuccessBase {
    #[serde(flatten)]
    pub subtype_0: JsonResponseBase,
    #[serde(flatten)]
    pub subtype_1: JsonSuccessBaseSubtype1,
}
#[derive(Clone, Debug, Deserialize, Serialize)]
pub struct JsonSuccessBaseSubtype1 {
    #[serde(default, skip_serializing_if = "Option::is_none")]
    pub ignored_parameters_unsupported: Option<IgnoredParametersUnsupported>,
    pub msg: String,
    pub result: JsonSuccessBaseSubtype1Result,
}

#[derive(Clone, Copy, Debug, Deserialize, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)]
pub enum JsonSuccessBaseSubtype1Result {
    #[serde(rename = "success")]
    Success,
}

(some parts omitted for brevity)

This fails to deserialize { "result": "success", "msg": "" } as JsonResponseBase already consumed the result that JsonSuccessBaseSubtype1 wants to read too.


Another pattern used by Zulip is:

JsonSuccessBase:
  allOf:
    - $ref: "#/components/schemas/JsonResponseBase"
    - required:
        - result
        - msg
      properties:
        result:
          enum:
            - success
        msg:
          type: string
        ignored_parameters_unsupported:
          $ref: "#/components/schemas/IgnoredParametersUnsupported"
      example: {"msg": "", "result": "success"}

JsonSuccess:
  allOf:
    - $ref: "#/components/schemas/JsonSuccessBase"
    - additionalProperties: false
      properties:
        result: {}
        msg: {}
        ignored_parameters_unsupported: {}

results in

#[derive(Clone, Debug, Deserialize, Serialize)]
pub struct JsonSuccessBase {
    #[serde(flatten)]
    pub subtype_0: JsonResponseBase,
    #[serde(flatten)]
    pub subtype_1: JsonSuccessBaseSubtype1,
}

#[derive(Clone, Debug, Deserialize, Serialize)]
pub struct JsonSuccessBaseSubtype1 {
    #[serde(default, skip_serializing_if = "Option::is_none")]
    pub ignored_parameters_unsupported: Option<IgnoredParametersUnsupported>,
    pub msg: String,
    pub result: JsonSuccessBaseSubtype1Result,
}

#[derive(Clone, Debug, Deserialize, Serialize)]
pub struct JsonSuccess {
    #[serde(flatten)]
    pub subtype_0: JsonSuccessBase,
    #[serde(flatten)]
    pub subtype_1: JsonSuccessSubtype1,
}

#[derive(Clone, Debug, Deserialize, Serialize)]
#[serde(deny_unknown_fields)]
pub struct JsonSuccessSubtype1 {
    #[serde(default, skip_serializing_if = "Option::is_none")]
    pub ignored_parameters_unsupported: Option<serde_json::Value>,
    #[serde(default, skip_serializing_if = "Option::is_none")]
    pub msg: Option<serde_json::Value>,
    #[serde(default, skip_serializing_if = "Option::is_none")]
    pub result: Option<serde_json::Value>,
}

which duplicates all fields in JsonSuccessSubtype1 despite them only being present there to avoid disallowing the JsonSuccessBase fields from existing.

For this pattern separate from the deserialization issue it would be nice if JsonSuccessSubtype1 could be flattened into JsonSuccess.

@ahl
Copy link
Collaborator

ahl commented Aug 19, 2023

I think I'd ideally like typify to compile the schema for JsonSuccessBase into Rust code like this:

#[derive(Clone, Debug, Deserialize, Serialize)]
pub struct JsonResponseBase {
    #[serde(default, skip_serializing_if = "Option::is_none")]
    pub result: Option<String>,
}

#[derive(Clone, Debug, Deserialize, Serialize)]
pub struct JsonSuccessBase {
    #[serde(default, skip_serializing_if = "Option::is_none")]
    pub ignored_parameters_unsupported: Option<IgnoredParametersUnsupported>,
    pub msg: String,
    pub result: JsonSuccessBaseResult,
}

#[derive(Clone, Copy, Debug, Deserialize, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)]
pub enum JsonSuccessBaseResult {
    #[serde(rename = "success")]
    Success,
}

impl From<JsonSuccessBase> for JsonResponseBase {
    fn from(_: JsonSuccessBase) -> Self {
        Self {
            result: Some("success".to_string()),
        }
    }
}

With regard to JsonSuccess, #[serde(flatten)] is incompatible with #[serde(deny_unknown_fields)]; for typify it would seem perfectly reasonable to "flatten" by hand:

#[derive(Clone, Debug, Deserialize, Serialize)]
#[serde(deny_unknown_fields)]
pub struct JsonSuccess {
    #[serde(default, skip_serializing_if = "Option::is_none")]
    pub ignored_parameters_unsupported: Option<IgnoredParametersUnsupported>,
    pub msg: String,
    pub result: JsonSuccessBaseResult,
}

impl From<JsonSuccessBase> for JsonSuccess {
    fn from(value: JsonSuccessBase) {
        let JsonSuccessBase { ignored_parameters_unsupported, msg, result };
        Self { ignored_parameters_unsupported, msg, result }
    }
}

Any thoughts on the proposed generation above?

I think this is another case relevant to #176.

@bjorn3
Copy link
Author

bjorn3 commented Aug 24, 2023

Makes sense to me.

ahl added a commit that referenced this issue Sep 23, 2023
Before this PR, the handling of allOf was basically wrong. It worked in a small number of cases, but on the whole it was flawed in concept. Most of the time, it would result in a struct with all subschemas embedded with a #[serde(flatten)] directive. This was broken in many ways, but in particular because allOf constructs are often used to augment, narrow, and expand other types.

This PR instead "merges" the subschemas of an allOf construction. This results in many more types, but also (per our testing), types that compile and are usable which was often not the case previously.

This implements much of what is discussed in #176, but doesn't include the impl From that issue imagines to go from the "merged" type to the component type (i.e. the type that appeared in the allOf array).

Fixes #315 and #370
@ahl ahl closed this as completed Sep 23, 2023
@bjorn3
Copy link
Author

bjorn3 commented Sep 23, 2023

Thanks!

@ahl
Copy link
Collaborator

ahl commented Sep 23, 2023

If you give it another shot please let me know if the new code improves things.

@bjorn3
Copy link
Author

bjorn3 commented Sep 23, 2023

Tried it and haven't hit any of the deserialization failures I hit previously.

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