[TCGC] [ARM] Expected client code on definition model UserAssignedIdentities is Record<UserAssignedIdentity>;

[haolingdong-msft]

typespec-azure-resource-manager changed the defnition for UserAssignedIdentities in version 0.42, from userAssignedIdentities?: Record<UserAssignedIdentity>;(code ) to model UserAssignedIdentities is Record<UserAssignedIdentity>;(code)

Link Bug #824

For model A is Record<> case, TCGC will return model A with additional property, this will be potential breaking changes in Mgmt plane, as current generated code based on swagger will use Map directly, instead of model with additional properties. I think we should align with the current generated code, otherwise there will be many breaking changes. See current code: https://github.com/Azure/azure-sdk-for-java/blob/main/sdk/maps/azure-resourcemanager-maps/src/main/java/com/azure/resourcemanager/maps/models/ManagedServiceIdentity.java#L43-L45

To solve this:

• One solution is to have TCGC do special handling for UserAssignedIdentities to return it as Map directly. common-types have similar definition, but previously M4 seems consider it as dictionary.
• Another solution is to let ARM change the definition back.

[tadelesh]

For swagger, since m4 will convert such model as just a record, I believe for TypeSpec it should be fixed in arm tsp definition. cc: @allenjzhang

[msyyc]

Python has similar issue, too.

[timotheeguerin]

This was changed to keep TypeSpec in sync with the common-types.json which we cannot break https://github.com/Azure/azure-rest-api-specs/blob/b627f6cb13b1d596e7329c6d7aae415519a373c2/specification/common-types/resource-management/v5/managedidentity.json#L9

[timotheeguerin]

can you explain what you would have been doing here in openapi and why this is different.

[allenjzhang]

Questions:
autorest generated SDK from swagger with $ref to v4/5, does it generate UserAssignedIdentities?

• If Yes, it would be right to change the behavior for MPG to generate that as well, so no breaking change switching from autorest to MPG.
• If No, autorest must have done some collapsing to skip UserAssignedIdentities. Therefor similar logic should be adopted in TCGC.

[ArcturusZhang]

I assume we should be achieving "to keep the generated SDK unchanged after switched to typespec". And it is too much a cost that we break the SDKs in order to keep swagger "not breaking", not to mention it semantically does not break because it actually represents the same JSON schema.

Swagger has a shortage that it cannot write different expressions for "a model with additional properties but no known properties", or "a pure dictionary", because these two things are the same from json's perspective, which is the thing swagger is representing.

And speaking of "keeping swagger common-types unchanged", I think it does not have to be as precisely as "it cannot change in a single character", I think it could be fine if the schema it represents is not changed.

It is an advantage that typespec now can express differently from the two different SDK APIs (models with additional properties and dictionaries), we should not give it up.

Therefore in order to keep our SDK unchanged - and the swagger is "unchanged" originally with some minor representation change, we have the following options:

1. change how typespec comprehends model Foo is Record<Foo>, because intuitively this means this model is a dictionary/record, and it should not allow other properties. We change its semantic meaning and stop using this to represent a model with additional properties, and let the compile give errors when other properties are added to this model.
2. let TCGC do the same thing as modeler v4 has done - when it sees a model with additional properties but no other known properties, it gives the downstream a record as if they write a Record<T> directly. But I think this is unacceptable with similar reason that we have to add constraints to unions as enums (union Foo { "a", "b" }). It changes from a type to a completely unrelated type when something not breaking is applied to the schema (in this case, a new property is added to the model).
3. the simplest solution, we just change how we write the resourcemanager typespec. We change the model XXX is Record<T> to alias XXX = Record<T>. This might not produce exactly the same swagger as before but I think maybe something could be done to the openapi emitter to have it? Or does it really matter the representation changes in swaggers? Because the purpose of existence of typespec is to let us get more information during sdk generation, and swagger should be as a generated public API, we will never use swaggers to generate SDK code any more when everything is ready.

[haolingdong-msft]

can you explain what you would have been doing here in openapi and why this is different.

@timotheeguerin
When generating mgmt SDK from TypeSpec directly, model UserAssignedIdentities is Record<UserAssignedIdentity>; will be interpreted as a Model named UserAssignedIdentities which has additionalProperties by TCGC, code

public final class ManagedServiceIdentity {
    /*
     * The identities assigned to this resource by the user.
     */
    @JsonProperty(value = "userAssignedIdentities")
    private UserAssignedIdentities userAssignedIdentities;
}

public final class UserAssignedIdentities {
    /*
     * The set of user assigned identities associated with the resource. The userAssignedIdentities dictionary keys will be ARM resource ids in the form: '/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.ManagedIdentity/userAssignedIdentities/{identityName}. The dictionary values can be empty objects ({}) in requests.",
     * 
     * Additional properties
     */
    @JsonIgnore
    private Map<String, UserAssignedIdentity> additionalProperties;
}

But previously generating from swagger, Modular4 will interprete the swagger as Map. code

@Fluent
public final class ManagedServiceIdentity {
    /*
     * The identities assigned to this resource by the user.
     */
    @JsonProperty(value = "userAssignedIdentities")
    @JsonInclude(value = JsonInclude.Include.NON_NULL, content = JsonInclude.Include.ALWAYS)
    private Map<String, UserAssignedIdentity> userAssignedIdentities;
}

To solve this:

• we either have TCGC to handle it similar as M4, i.e. to model model UserAssignedIdentities is Record<UserAssignedIdentity>; as dict type.
• or change the ARM typespec definition back.

[tadelesh]

add a playground to show the root cause.

[timotheeguerin]

1. Check with ARm Team what UserAssignedIdentities and DelegatedResources is meant for and if they might add known properties in the future
2. If not convert back to alias
3. To resolve inconsitency with the json file
   1. see if could remove it from managedIdentiy.json and replace with inline refs instead
   2. do a hack in the generation

[markcowl]

If this issue prevents correct / desired generation of greenfield APIs, then it should be high priority. If this is about not breaking BrownField APIs that convert to TypeSpec, I am not sure it has the same priority.

[haolingdong-msft]

1. Check with ARm Team what UserAssignedIdentities and DelegatedResources is meant for and if they might add known properties in the future

2. If not convert back to alias

3. To resolve inconsitency with the json file

   1. see if could remove it from managedIdentiy.json and replace with inline refs instead
   2. do a hack in the generation

Hi @timotheeguerin , woule like to double confirm my understanding: Tsp team tends to change typespec-azure-resource-manager TSP and find other way to make .json consistent?

[haolingdong-msft]

If this issue prevents correct / desired generation of greenfield APIs, then it should be high priority. If this is about not breaking BrownField APIs that convert to TypeSpec, I am not sure it has the same priority.

It affects greenfield service generation, and almost half of the mgmt-plane services have ManagedServiceIdentity.

[allenjzhang]

#868

[allenjzhang]

Fixed with hotfix version 0.42.1 for @azure-tools/typespec-azure-resource-manager