A model unioned with null introduces inheritance

[pshao25]

This is what we found from the unmerged pipeline. We found this change. The problem comes from this ManagedServiceIdentity.userAssignedIdentities whose type is Record<UserAssignedIdentity | null>.

model ManagedServiceIdentity {
  /** The service principal ID of the system assigned identity. This property will only be provided for a system assigned identity. */
  @visibility("read")
  principalId?: uuid;

  /** The tenant ID of the system assigned identity. This property will only be provided for a system assigned identity. */
  @visibility("read")
  tenantId?: uuid;

  /** The type of managed identity assigned to this resource. */
  type: ManagedServiceIdentityType;

  /** The identities assigned to this resource by the user. */
  @typeChangedFrom(Versions.v5, Record<UserAssignedIdentity>)
  userAssignedIdentities?: Record<UserAssignedIdentity | null>;
}

See this playground, as long as a model is "unioned" with a null, its type is changed to an allOf. See the difference between property2 and property3 in B. This is not accurate,

[MaryGao]

A more generic question here is how to interpret nullable object in OpenAPI/Autorest emitter.

playground

model Foo {
  name: string;
}

model Bar {
  test: Foo | null;
}

A true value adds "null" to the allowed type specified by the type keyword, only if type is explicitly defined within the same Schema Object. ... nullable: true acts as a type modifier, allowing null in addition to the specified type.

According to nullable clarification and definition, the proper translation for Foo | null would be allowing null value in addition to Foo type and we could have following OpenAPI. This is also how above userAssignedIdentities is defined in swagger side. And some services define nullable object like this(here).The M4 could handle this nullable correctly but it would be problematic in OpenAPI and you could see discussions.

    Bar:
      type: object
      required:
        - test
      properties:
        test:
          $ref: '#/components/schemas/Foo'
          nullable: true

Another translation may introduce un-necessary inheritance, it would act as allowing null in addition to an anonymous object which extends Foo object, which may not what we want.

    properties:
        test:
          type: object
          allOf:
            - $ref: '#/components/schemas/Foo'
          nullable: true

[timotheeguerin]

I would note that allOf does not mean inheritance in openapi/json schema(it is just how we have interpreted it) and this syntax is the correct way you would add sibling properties in openapi 2 and 3.

Now deviate from teh actual spec and we do allow certain sibling properties next to $ref so we could see to try to flatten in this case.

[pshao25]

From SDK's perspective it is a breaking change especially you already pointed out that "we interpreted it as inheritance for swagger". We could correctly generate dictionary properties as IDictionary<string, A> property2 for TypeSpec, however, after customer emits the TypeSpec to swagger, the SDK changes.

Not sure what other generates, but .net generates IDictionary<string, Components1PvxqcSchemasBPropertiesProperty2Additionalproperties> property2 and the Components1PvxqcSchemasBPropertiesProperty2Additionalproperties extends A.

[pshao25]

@markcowl @timotheeguerin I understand there might be no better solution. I created this to explore if we could figure out a better one. If we finally don't have one, I think we should at least have a "known issue" section somewhere.

[timotheeguerin]

autorest flatten those cases, they result in the exact same codemodel as it is needed to represent openapi3 spec that are compliant.

[markcowl]

@timotheeguerin Is saying that AutoRest treats these two cases as the same in code model, so how would our autorest-based generators do anything different for these?

Similarly, emitters can choose how they treat Record<T | null> types, I assume using some kind of dictionary type

[MaryGao]

autorest flatten those cases, they result in the exact same codemodel as it is needed to represent openapi3 spec that are compliant.

I can understand from openapi3's side they are compliant. We do have a difference with generated name in codemodel,

• Option 1: the direct $ref would keep the object name e.g Foo
• Option 2: the allOf would generate a name according to the context e.g BarTest

The interesting part is the option 1 is not a standard OpenAPI spec because atributes next of a $ref must be ignored. However, autorest has allowed it. And the option 2 looks more correct spec but it has no way to indicate the nullable object name, so autorest generated one automatically.

Does that mean the autorest and openapi3 may have difference when handling nullable objects? But i think this is not what we want.

[timotheeguerin]

I don't understand why you say there is a difference in the codemodel, I tried autorest WILL flatten option2 to produce the same thing as option1. Can you show a repro where it is different.

And yeah option 1 is NOT compliant for openapi 2.0 and openapi 3.0. It is in openapi 3.1, this is something autorest decided to do to make things simpler. And yeah in general we try to make it emit option 1 when possible in the autorest emitter but always option 2 in openapi3.

[pshao25]

I tried these two examples with

"Bar1": {
  "type": "object",
  "properties": {
    "test": {
      "type": "object",
      "x-nullable": true,
      "$ref": "#/definitions/Foo"
    }
  },
  "required": [
    "test"
  ]
},
"Bar2": {
  "type": "object",
  "properties": {
    "test": {
      "type": "object",
      "x-nullable": true,
      "allOf": [
        {
          "$ref": "#/definitions/Foo"
        }
      ]
    }
  }

in autorest.csharp.
You could see the generated codemodel here.

Bar1 gives me expected behavior while Bar2.Test has type Bar2Test which extends Foo.

[timotheeguerin]

hhm ok I don't know then if there was an option or what that made this work but either way we usually flatten $ref when possible so just need to make sure this applies for this case as well.
Is this urgent?

[pshao25]

Is this urgent?

I think no. We could put aside till there is a real case.

[markcowl]

Closing until we have a real service example.

[timotheeguerin]

This triggers when using

  userAssignedIdentities?: Record<UserAssignedIdentity | null>;

https://github.com/Azure/azure-rest-api-specs/pull/31125/checks?check_run_id=32638837500