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

fix union types #1435

Merged
merged 13 commits into from
Oct 9, 2023
Merged

fix union types #1435

merged 13 commits into from
Oct 9, 2023

Conversation

cataggar
Copy link
Member

@cataggar cataggar commented Oct 4, 2023

This fixes a bug from #1414 which causes deserialization to fail.

  • create the base type without the discriminator
  • adds the description to the union type
  • fixes union types for vector aliases
  • fixes support for nested unions
  • removes empty base types

- create the base type without the discriminator
- adds the description to the union type
@cataggar cataggar requested a review from demoray October 4, 2023 13:44
@cataggar cataggar requested a review from demoray October 5, 2023 11:41
@demoray
Copy link
Contributor

demoray commented Oct 5, 2023

I'm digging into this and found at least one issue that I don't think is right.

In services/svc/agrifood/src/package_2023_07_01_preview/models.rs, AuthCredentialsKind is one of the enums defined in the spec. It's generated, but never used. As is AuthCredentialsUnion.

From looking at the spec, these should be used in ApiKeyAuthCredentials, but that doesn't include it.

Ref: https://github.com/Azure/azure-rest-api-specs/blob/a26794fe26cdc68f4d18bc782e0b4f2f29e24b40/specification/agrifood/data-plane/Microsoft.AgFoodPlatform/preview/2023-07-01-preview/agfood.json#L13738-L13758

@cataggar
Copy link
Member Author

cataggar commented Oct 5, 2023

Thanks for digging in. The generated code for that example looks fine to me. Here is the Union generated from #/definitions/AuthCredentials".

#[doc = "CredentialTypeEnum."]
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
#[serde(tag = "kind")]
pub enum AuthCredentialsUnion {
    ApiKeyAuthCredentials(ApiKeyAuthCredentials),
    OAuthClientCredentials(OAuthClientCredentials),
}


#[doc = "Api Key Auth Credentials class for API Key based Auth."]
#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
pub struct ApiKeyAuthCredentials {
    #[doc = "Properties of the key vault."]
    #[serde(rename = "apiKey", default, skip_serializing_if = "Option::is_none")]
    pub api_key: Option<KeyVaultProperties>,
}

#[doc = "OAuthClientCredentials for clientId clientSecret auth."]
#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
pub struct OAuthClientCredentials {
    #[doc = "ClientId associated with the provider."]
    #[serde(rename = "clientId", default, skip_serializing_if = "Option::is_none")]
    pub client_id: Option<String>,
    #[doc = "Properties of the key vault."]
    #[serde(rename = "clientSecret", default, skip_serializing_if = "Option::is_none")]
    pub client_secret: Option<KeyVaultProperties>,
}

It is strange that that definition is not referenced anywhere else in the spec. Most specs then define the string enumeration inline, but the spec as defined it externally

 "AuthCredentials": {
      "description": "AuthCredentials abstract base class for Auth Purpose.",
      "required": [
        "kind"
      ],
      "type": "object",
      "properties": {
        "kind": {
          "$ref": "#/definitions/AuthCredentialsKind"
        }
      },
      "discriminator": "kind"
    },
    "AuthCredentialsKind": {
      "description": "CredentialTypeEnum.",
      "enum": [
        "OAuthClientCredentials",
        "ApiKeyAuthCredentials"
      ],
      "type": "string",
      "x-ms-enum": {
        "name": "AuthCredentialsKind",
        "modelAsString": true
      }
    },

Which is why there is a enum generated:

#[doc = "CredentialTypeEnum."]
#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
#[serde(remote = "AuthCredentialsKind")]
pub enum AuthCredentialsKind {
    OAuthClientCredentials,
    ApiKeyAuthCredentials,
    #[serde(skip_deserializing)]
    UnknownValue(String),
}

It is not used anywhere else in the spec, but it is not wrong to also generate it. Usually, you are going to reference it elsewhere.

@demoray demoray merged commit 20c1d78 into Azure:main Oct 9, 2023
19 checks passed
@cataggar cataggar deleted the union-fix branch October 9, 2023 19:05
@cataggar cataggar mentioned this pull request Oct 9, 2023
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