[Dpg][Bug] `An item with the same key has already been added` reported from HealthInsights

[ArcturusZhang]

Minimal tsp to reproduce this issue:

model Base
{
    kind: string;
}
@discriminator("kind")
model DiscriminatedBase extends Base {}

model Foo extends DiscriminatedBase
{
    kind: "foo";
}

op Bar(@body body: Foo): Foo;

[ArcturusZhang]

The root cause of this issue is that The discriminator is not defined on the model with the discriminator decorator, instead, it is inherited from a base model of this model. Swagger cannot handle this properly as well.

[ArcturusZhang]

Similar issue should also happen when we try to override a property in the derived type, such as:

model Foo
{
p: int64;
}

model Bar extends Foo {
p: int32;
}

because tsp supports we could narrow down the type of a property in derived types.

[ArcturusZhang]

This issue also involves with the issue of usage. For instance, with this typespec:

model Base {
    name: string;
}

model Derived extends Base {
    age?: int32;
}

op Foo(@body body: Derived): Derived;

Our current behavior:

• usage of Derived is roundtrip. This is expected.
• usage of Base is none. This is unexpected.

Reason: Usage is used to control our public API, including:

1. if the usage does not have input, the model will not have public ctors (both its ctors will be internal)
2. if the usage does not have input, all its properties will be generated as if they are all readonly.

Therefore the above tsp will generate these models:

public partial class Base
{
    private protected IDictionary<string, BinaryData> _serializedAddtionalRawData;
    internal Base(string name) {} // this is internal which is fine in this scenario
    internal Base(string name, IDictionary<string, BinaryData> serializedAdditionalRawData) {} // this is for deserialization, irrelevant in this case.
    public Name {get;} // this does not have setter because its usage is None.
}

public partial class Derived : Base
{
    public Derived(string name) : base(name} {} // this is expected
    internal Derived(string name, IDictionary<string, BinaryData> serializedAddtionalRawData, int? age) : base(name, serializedAddtionalRawData) {} // this is expected

    public int? Age {get;set;}
}

The major problem in this case is that the Name property on Derived class is not settable while the spec says otherwise.

This might seem fine here in this case, but it falls apart when there is a discriminator inherited from the base (the derived class needs to set the value of the discriminator property).

We could not just simply let all the classes in the inheritance tree to have the same usage, which introduces a lot of useless APIs to others that do not need them. For instance, for this spec:

model Base {}
model Foo extends Base {}
model Bar extends Base {}
model Qux extends Base {}
op A(@body body: Foo) : Foo;
op B(@body body: Bar): void;
op C(): Qux;

Clearly, the usage of Foo is roundtrip, the usage of Bar is input, and the usage of Qux is output.

What should be the usage of Base? Foo is roundtrip, therefore all its properties should follow the way of a roundtrip model, including those inherited from Base. Therefore the usage of Base here should be roundtrip. This creates minor issues to others, for instance, Qux is output, but the inherited properties of Qux will be settable (or muttable if they are lists). I believe it is a small price to take.

How we could achieve the result? We should:

1. calculate the usage of every model as we do right now.
2. propagate the usage down the inheritance tree from base to derived as we do right now. If we have the result in step 1 of Base: roundtrip, Derived: none, now we will have Base: roundtrip, Derived: roundtrip.
3. propagate the usage up the inheritance tree only once. For instance, in the above example, we have:

   Base: none
   Foo: roundtrip
   Bar: input
   Qux: output

   we bubble up the usage only to its base, and we will finally have:

   Base: roundtrip
   Foo: roundtrip
   Bar: input
   Qux: output

   we will not have another round of propagate down of the usage therefore the usage of Bar and Qux will remain and are not polluted to roundtrip.

[catalinaperalta]

Minimal tsp to reproduce this issue:

model Base
{
  kind: string;
}
@discriminator("kind")
model DiscriminatedBase extends Base {}

model Foo extends DiscriminatedBase
{
  kind: "foo";
}

op Bar(@body body: Foo): Foo;

As a workaround would the issue be resolved by spreading Base into DiscriminatedBase? Or is there a reason to define that relationship explicitly? @asaflevi-ms please let me know if this is intended

[catalinaperalta]

Separately, @ArcturusZhang @chunyu3 do we have an ETA for resolving this issue? The team would need to regenerate in the next couple of days to make sure they meet their arch board deadline.

[ArcturusZhang]

Minimal tsp to reproduce this issue:

model Base
{
    kind: string;
}
@discriminator("kind")
model DiscriminatedBase extends Base {}

model Foo extends DiscriminatedBase
{
    kind: "foo";
}

op Bar(@body body: Foo): Foo;

As a workaround would the issue be resolved by spreading Base into DiscriminatedBase? Or is there a reason to define that relationship explicitly? @asaflevi-ms please let me know if this is intended

yes spread the base into discriminatedBase should workaround this issue.

ETA is end of this week

[asaflevi-ms]

@catalinaperalta , @ArcturusZhang I am not sure it can be resolved by spreading Base into DiscriminatorBase I have input property: propertyInfo : Array

or in your example: op Bar(@body body: Base): Base

Base is a base FHIR resource, there are many FHIR resources, and since I cannot define all of them, I created Base with additionalProperties

model Base is Record {}

In Typespec I just defined 2-3, common resources.

[ArcturusZhang]

@catalinaperalta , @ArcturusZhang I am not sure it can be resolved by spreading Base into DiscriminatorBase I have input property: propertyInfo : Array

or in your example: op Bar(@Body body: Base): Base

Base is a base FHIR resource, there are many FHIR resources, and since I cannot define all of them, I created Base with additionalProperties

model Base is Record {}

In Typespec I just defined 2-3, common resources.

Nevertheless now there is a linter rule warning:

model.common.fhir.resources.tsp:32:30 - warning @azure-tools/typespec-azure-core/composition-over-inheritance: Model 'DomainResource' is extending
   'FHIR.R4.Resource' that doesn't define a discriminator. If 'FHIR.R4.Resource' is meant to be used:
   - For composition consider using spread `...` or `model is` instead.
   - As a polymorphic relation, add the `@discriminator` decorator on the base model.
  > 32 | model DomainResource extends Resource {

which means this is not a recommended usage.