WIP: flatten `allOf` models #1524

krishanjmistry · 2023-12-22T15:07:13Z

Primarily, this is an attempt to get further on #599.

What the solution has ended up looking like is [flattening/merging/composing] the allOf schema properties into the child models - there is no longer a use of #[serde(flatten)].
For starters, I'd say this provides a neater API to the end user - more resemblant of what we see in Bicep and easier to reason about.

It also removes some ambiguities in the models, where a property could be defined multiple times in the created models. For an example of this, see #1524 (comment)

krishanjmistry · 2023-12-22T16:40:40Z

Example where generated model is previously ambiguous

Using azure-rest-api-specs/specification/vmware/resource-manager/Microsoft.AVS/stable/2020-03-20/vmware.json - note this is an older tag than the latest, but sufficient as an example

ClusterProperties
- has a property of provisioningState
- takes an allOf over #/definitions/ManagementCluster
ManagementCluster
- has a property of provisioningState
- takes an allOf over #/definitions/ClusterUpdateProperties

Deserializing:
Whilst deserializing this response into a Rust model is successful, it's ambiguous where the provisioningState would end up:

#[cfg(feature = "package-2020-03-20")]
#[test]
fn test_cluster_get() -> anyhow::Result<()> {
    use anyhow::{bail, ensure};
    use azure_mgmt_vmware::package_2020_03_20::models::*;
    // copied from specification\vmware\resource-manager\Microsoft.AVS\stable\2023-03-01\examples\Clusters_Get.json
    let json = br#"
    {
        "id": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/group1/providers/Microsoft.AVS/privateClouds/cloud1/clusters/cluster1",
        "name": "cluster1",
        "sku": {
          "name": "AV20"
        },
        "properties": {
          "clusterSize": 4,
          "hosts": [
            "fakehost22.nyc1.kubernetes.center",
            "fakehost23.nyc1.kubernetes.center",
            "fakehost24.nyc1.kubernetes.center",
            "fakehost25.nyc1.kubernetes.center"
          ],
          "provisioningState": "Succeeded"
        },
        "type": "Microsoft.AVS/privateClouds/clusters"
      }
      "#;

    let cluster: Cluster = serde_json::from_slice(json)?;
    println!("{:#?}", cluster);
    Ok(())
}

Cluster {
    resource: Resource {
        id: Some(
            "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/group1/providers/Microsoft.AVS/privateClouds/cloud1/clusters/cluster1",
        ),
        name: Some(
            "cluster1",
        ),
        type_: Some(
            "Microsoft.AVS/privateClouds/clusters",
        ),
    },
    sku: Sku {
        name: "AV20",
    },
    properties: ClusterProperties {
        management_cluster: ManagementCluster {
            cluster_update_properties: ClusterUpdateProperties {
                cluster_size: Some(
                    4,
                ),
            },
            provisioning_state: None,
            cluster_id: None,
            hosts: [
                "fakehost22.nyc1.kubernetes.center",
                "fakehost23.nyc1.kubernetes.center",
                "fakehost24.nyc1.kubernetes.center",
                "fakehost25.nyc1.kubernetes.center",
            ],
        },
        provisioning_state: Some(
            Succeeded,
        ),
    },
}

Serializing:

#[cfg(feature = "package-2020-03-20")]
#[test]
fn serialize_cluster() -> anyhow::Result<()> {
    use anyhow::{bail, ensure};
    use azure_mgmt_vmware::package_2020_03_20::models::*;

    let cluster = Cluster {
        resource: Resource {
            id: Some(
                "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/group1/providers/Microsoft.AVS/privateClouds/cloud1/clusters/cluster1".to_string(),
            ),
            name: Some(
                "cluster1".to_string(),
            ),
            type_: Some(
                "Microsoft.AVS/privateClouds/clusters".to_string(),
            ),
        },
        sku: Sku {
            name: "AV20".to_string(),
        },
        properties: ClusterProperties {
            management_cluster: ManagementCluster {
                cluster_update_properties: ClusterUpdateProperties {
                    cluster_size: Some(
                        4,
                    ),
                },
                provisioning_state: Some(ClusterProvisioningState::Updating),
                cluster_id: None,
                hosts: vec![
                    "fakehost22.nyc1.kubernetes.center".to_string(),
                    "fakehost23.nyc1.kubernetes.center".to_string(),
                    "fakehost24.nyc1.kubernetes.center".to_string(),
                    "fakehost25.nyc1.kubernetes.center".to_string(),
                ],
            },
            provisioning_state: Some(
                ClusterProvisioningState::Succeeded,
            ),
        },
    };

    let json = serde_json::to_string_pretty(&cluster)?;
    println!("{}", json);
    Ok(())
}

This means we end up with two provisioningState properties within the serialized result.

{
  "id": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/group1/providers/Microsoft.AVS/privateClouds/cloud1/clusters/cluster1",
  "name": "cluster1",
  "type": "Microsoft.AVS/privateClouds/clusters",
  "sku": {
    "name": "AV20"
  },
  "properties": {
    "clusterSize": 4,
    "provisioningState": "Updating",
    "hosts": [
      "fakehost22.nyc1.kubernetes.center",
      "fakehost23.nyc1.kubernetes.center",
      "fakehost24.nyc1.kubernetes.center",
      "fakehost25.nyc1.kubernetes.center"
    ],
    "provisioningState": "Succeeded"
  }
}

krishanjmistry · 2023-12-22T16:59:24Z

Now, the example I've used in #1524 (comment)

serializes as:

#[cfg(feature = "package-2020-03-20")]
#[test]
fn serialize_cluster() -> anyhow::Result<()> {
    use azure_mgmt_vmware::package_2020_03_20::models::*;

    let cluster = Cluster {
        id: Some(
            "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/group1/providers/Microsoft.AVS/privateClouds/cloud1/clusters/cluster1".to_string(),
        ),
        name: Some(
            "cluster1".to_string(),
        ),
        type_: Some(
            "Microsoft.AVS/privateClouds/clusters".to_string(),
        ),
        sku: Sku {
            name: "AV20".to_string(),
        },
        properties: ClusterProperties {
            cluster_size: 4,
            provisioning_state: Some(ClusterProvisioningState::Updating),
            cluster_id: None,
            hosts: vec![
                "fakehost22.nyc1.kubernetes.center".to_string(),
                "fakehost23.nyc1.kubernetes.center".to_string(),
                "fakehost24.nyc1.kubernetes.center".to_string(),
                "fakehost25.nyc1.kubernetes.center".to_string(),
            ],
        },
    };

    let json = serde_json::to_string_pretty(&cluster)?;
    println!("{}", json);
    Ok(())
}

into

{
  "sku": {
    "name": "AV20"
  },
  "properties": {
    "provisioningState": "Updating",
    "hosts": [
      "fakehost22.nyc1.kubernetes.center",
      "fakehost23.nyc1.kubernetes.center",
      "fakehost24.nyc1.kubernetes.center",
      "fakehost25.nyc1.kubernetes.center"
    ],
    "clusterSize": 4
  },
  "id": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/group1/providers/Microsoft.AVS/privateClouds/cloud1/clusters/cluster1",
  "name": "cluster1",
  "type": "Microsoft.AVS/privateClouds/clusters"
}

deserializes as:

#[cfg(feature = "package-2020-03-20")]
#[test]
fn test_cluster_get() -> anyhow::Result<()> {
    use anyhow::ensure;
    use azure_mgmt_vmware::package_2020_03_20::models::*;
    // copied from specification\vmware\resource-manager\Microsoft.AVS\stable\2023-03-01\examples\Clusters_Get.json
    let json = br#"
    {
        "id": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/group1/providers/Microsoft.AVS/privateClouds/cloud1/clusters/cluster1",
        "name": "cluster1",
        "sku": {
          "name": "AV20"
        },
        "properties": {
          "clusterSize": 4,
          "hosts": [
            "fakehost22.nyc1.kubernetes.center",
            "fakehost23.nyc1.kubernetes.center",
            "fakehost24.nyc1.kubernetes.center",
            "fakehost25.nyc1.kubernetes.center"
          ],
          "provisioningState": "Succeeded"
        },
        "type": "Microsoft.AVS/privateClouds/clusters"
      }
      "#;

    let cluster: Cluster = serde_json::from_slice(json)?;
    println!("{:#?}", cluster);
    ensure!(cluster.name == Some("cluster1".to_string()));
    ensure!(cluster.sku.name == "AV20".to_string());
    ensure!(cluster.properties.cluster_size == 4);
    ensure!(cluster.properties.hosts.len() == 4);
    ensure!(cluster.properties.provisioning_state == Some(ClusterProvisioningState::Succeeded));
    Ok(())
}

cataggar · 2023-12-22T20:52:39Z

Example where generated model is previously ambiguous

Using azure-rest-api-specs/specification/vmware/resource-manager/Microsoft.AVS/stable/2020-03-20/vmware.json - note this is an older tag than the latest, but sufficient as an example

Hi @krishanjmistry, I appreciate using the vmware spec as the example, since that I the one I manage. Having duplicate entries in the spec like this is an error. I am not sure it is something that we should support. In this particular case, I fixed it in the next version 2021-06-01.

cataggar · 2023-12-22T21:12:51Z

For starters, I'd say this provides a neater API to the end user - more resemblant of what we see in Bicep and easier to reason about.

I think there is room for improvement here. I think there are some advantages of keeping the structs to match the definitions. There are ways to improve the API to the end user without changing the structs. For #601, I brought up using the builder pattern. @demoray, then mentioned:

I think having a middle ground of providing something akin to the setters! macro would be sufficient for many (most?) uses.

krishanjmistry · 2024-01-02T10:47:21Z

@cataggar, thanks for taking a look.

Hi @krishanjmistry, I appreciate using the vmware spec as the example, since that I the one I manage. Having duplicate entries in the spec like this is an error. I am not sure it is something that we should support. In this particular case, I fixed it in the next version 2021-06-01.

I did notice that I was picking on an old tag! More generally, the main thing that I believe would influence whether we support this case is how prevalent this is in the existing specs. I think it's worth noting that I don't think such a thing could be described in TypeSpec (which I believe is where the specs are heading).

Likewise, I also don't think the scenario in #599 could be described in TypeSpec. In TypeSpec, it seems a property can either be defined required or optional in its base model. In other words, it should be possible to inherit the required properties, but not overwrite them.

I think there is room for improvement here. I think there are some advantages of keeping the structs to match the definitions. There are ways to improve the API to the end user without changing the structs.

My main concern here would be, in the case of if we do want to support a scenario like #599 - the models won't reflect what gets [de]serialized. I do like the suggestion of a builder pattern/setters! macro.

It seems there's two questions, on both I'll try and get some stats:

how prevalent are duplicated properties in models that include allOf?
how prevalent is the issue in required parameters in allOf model #599, where a required property is not set in the base class?

krishanjmistry · 2024-01-17T19:52:23Z

Answering the questions I left at the end of the last comment.

how prevalent are duplicated properties in models that include allOf?
- 39 resource manager crates have schemas where the property is defined in both the schema, and the allOf it takes.
- Among those 39 resource manager crates, there's 435 cases in total.
- Here's a CSV containing all the places: all_of_issues.csv
how prevalent is it that a property is mentioned to be required outside of the base class?
- 46 resource manager crates
- 4 data plane crates
- 1193 cases across both sets of crates
- Here's a CSV containing all the places: required_issues.csv

heaths · 2024-01-18T00:41:37Z

FWIW - and I don't know if/how this factors in here - but for context, all the other language code generators have traditionally treated the first allOf as derivation (if that even applies in this case) and any subsequent allOfs as flattened members. Also worth keeping in mind when we start supporting a TypeSpec-compiled object model.

krishanjmistry changed the title ~~WIP - comments welcome: flatten allOf models~~ WIP: flatten allOf models Dec 22, 2023

krishanjmistry added 14 commits January 5, 2024 19:35

flatten all of

5cd0ec4

remove adding allOf properties via serde flatten

44d8b80

add debug code to show how the properties overlap

f2e9076

beginnings of union stuff

14dc0c1

wip - use resolved unions

2f9c158

remove discriminator property from child class

b87b9fd

deriving default

a68a4d9

orderings + beginnings of tidying

e3e912e

some changes

42f4c57

add reference to allOf root enum rather than multiple copies

26132b3

hide printlns

d91706a

a few bits of cleanup

ae52a56

attempt to fix clippy new without default lint

0003fc5

use resolved discriminator parent instead

63b866d

krishanjmistry force-pushed the flatten-all-of branch from 92a9a20 to 63b866d Compare January 5, 2024 19:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: flatten `allOf` models #1524

WIP: flatten `allOf` models #1524

krishanjmistry commented Dec 22, 2023 •

edited

krishanjmistry commented Dec 22, 2023 •

edited

krishanjmistry commented Dec 22, 2023

cataggar commented Dec 22, 2023

cataggar commented Dec 22, 2023 •

edited

krishanjmistry commented Jan 2, 2024

krishanjmistry commented Jan 17, 2024

heaths commented Jan 18, 2024 •

edited

WIP: flatten allOf models #1524

Are you sure you want to change the base?

WIP: flatten allOf models #1524

Conversation

krishanjmistry commented Dec 22, 2023 • edited

krishanjmistry commented Dec 22, 2023 • edited

krishanjmistry commented Dec 22, 2023

cataggar commented Dec 22, 2023

cataggar commented Dec 22, 2023 • edited

krishanjmistry commented Jan 2, 2024

krishanjmistry commented Jan 17, 2024

heaths commented Jan 18, 2024 • edited

WIP: flatten `allOf` models #1524

WIP: flatten `allOf` models #1524

krishanjmistry commented Dec 22, 2023 •

edited

krishanjmistry commented Dec 22, 2023 •

edited

cataggar commented Dec 22, 2023 •

edited

heaths commented Jan 18, 2024 •

edited