Should Polymorphism base class be able to use as a type directly ?

[qiaozha]

In recoveryservicesiterecovery service, there're several base classes that with a discriminator, but in the generated code, it seems only allow the subclass to be used as a type. For example: in this definition of ReplicationProviderSpecificContainerCreationInput https://github.com/Azure/azure-rest-api-specs/blob/main/specification/recoveryservicessiterecovery/resource-manager/Microsoft.RecoveryServices/stable/2021-12-01/service.json#L21167-L21180, instanceType is the discriminator, it has three subclasses A2ACrossClusterMigrationContainerCreationInput, VMwareCbtContainerCreationInput, A2AContainerCreationInput And they all have their x-ms-discriminator-value set, If we allow customer to use base class itself as a type, then we should have four choosable values for the instantType defined here https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/recoveryservicessiterecovery/arm-recoveryservices-siterecovery/src/models/index.ts#L853-L856, but it only has three values for their subclasses.

And the swagger example that service team provide is also a base class example here https://github.com/Azure/azure-rest-api-specs/blob/main/specification/recoveryservicessiterecovery/resource-manager/Microsoft.RecoveryServices/stable/2021-12-01/examples/ReplicationProtectionContainers_Create.json#L13

[lirenhe]

@sarangan12, previously, we checked with ARM team that the base class is also valid to use in APIs and I believe service team also support that in service side. So could you help to double check the implementation in CodeGen?

[sarangan12]

@qiaozha @lirenhe I have checked the implementation in TS and also other languages. All of them are consistent. Actually, Could you explain the ask here? The current code looks as:

/** Provider specific input for container creation operation. */
export interface ReplicationProviderSpecificContainerCreationInput {
  /** Polymorphic discriminator, which specifies the different types this object can be */
  instanceType: "A2A" | "A2ACrossClusterMigration" | "VMwareCbt";
}

What is your expected output?Do you want it to look like this?

/** Provider specific input for container creation operation. */
export interface ReplicationProviderSpecificContainerCreationInput {
  /** Polymorphic discriminator, which specifies the different types this object can be */
  instanceType: "instanceType" | "A2A" | "A2ACrossClusterMigration" | "VMwareCbt";
}

The above code is not correct. Now, what is the requirement that you are trying to satisfy? Could you provide the scenario?

[qiaozha]

@sarangan12 As you can see from the swagger example link above, ReplicationProviderSpecificContainerCreationInput is also a valid value for instanceType.

[qiaozha]

@sarangan12 By the way, if x-ms-discriminator-value is provided, then the class name should be the value of x-ms-discriminator-value, if it's not provided, then the class name should be the type name itself.

[sarangan12]

@qiazhoa I do not see how the base class could be used by the service. Also, the logic seems to be consistent with other language SDKs. Let me discuss with autorest crew and update the issue

[qiaozha]

@lirenhe Do ARM have any kind of document to explicitly tell us whether base class could be used by the services ?

[lirenhe]

@sarangan12, @qiaozha , I don't think we have the any doc for that. But this issue was discussed to decide the behaviour for Live validation which is used for validating wheather payload is consistent with the Swagger definition. For 'discriminator', the suggestion from ARM is 'In SDKs if the discriminator value is not found in spec then base class is instantiated, so this would not cause problem to SDK.' So In live validation, it also use base class for validation if not found the derived class. Also, some service teams also use the base class as a valid model in their API.

[sarangan12]

@lirenhe But, how could we represent this in the generated code? Unless, there is a valid discriminatorType, we cannot use the one from base class

[qiaozha]

I think if you found a property is discriminator you probably can check if there's a discriminatorValue, if there's no x-ms-discriminator-value in the extension, you can just put the SchemaName itself into the discriminator value definition.

[sarangan12]

@qiaozha If I follow that logic, the code will look similar to:

/** Provider specific input for container creation operation. */
export interface ReplicationProviderSpecificContainerCreationInput {
  /** Polymorphic discriminator, which specifies the different types this object can be */
  instanceType: "instanceType" | "A2A" | "A2ACrossClusterMigration" | "VMwareCbt";
}

How will be considered correct? @joheredi Do you have any thoughts in this issue?

[joheredi]

This feels like a swagger issue to me, I'm interested to understand what the other languages are doing? @qiaozha would you mind talking to the other languages and add some information regarding what is being done across the board, please?

@sarangan12 I'm not sure the representation above would be helpful. If I understand correctly, the requirement is to be able to assign instanceType any string value for that one operation that doesn't provide a discriminator.

One possible representation in TS would be to generate an additional type IndiscriminateReplicationProviderSpecificContainerCreationInput

/** Provider specific input for container creation operation. */
type IndiscriminateReplicationProviderSpecificContainerCreationInput = ReplicationProviderSpecificContainerCreationInput & {
  instanceType: string
}

And use it only for the operation that takes the indiscriminate shape

[qiaozha]

@sarangan12 The code will look like this

/** Provider specific input for container creation operation. */
export interface ReplicationProviderSpecificContainerCreationInput {
  /** Polymorphic discriminator, which specifies the different types this object can be */
  instanceType: "ReplicationProviderSpecificContainerCreationInput" | "A2A" | "A2ACrossClusterMigration" | "VMwareCbt";
}

[qiaozha]

But I agree with @joheredi said above, this instanceType should be able to accept any string besides the above certain values.