The internal constructors for a model in DPG and mgmt/HLC are different

[ArcturusZhang]

The difference happens when the model has a discriminator. In mgmt/HLC, the discriminator property will show up as a parameter in the internal constructor, for example: https://github.com/Azure/autorest.csharp/blob/9151a2ec37f140ffbc53c8a5acda3e3280314afd/test/TestProjects/MgmtDiscriminator/Generated/Models/DeliveryRuleQueryStringCondition.cs#L31 In DPG, the discriminator property will not show up as parameter in the internal constructor, for example: https://github.com/Azure/autorest.csharp/blob/9151a2ec37f140ffbc53c8a5acda3e3280314afd/test/CadlRanchProjects/inheritance/Generated/Models/Salmon.cs#L30

We should align the behavior considering that we cannot quickly align the implementation.

[ArcturusZhang]

We are going to align the behavior in mgmt/HLC to the way in DPG by not exposing the discriminator property in their internal constructors.

[ArcturusZhang]

Another inconsistency: DPG models will put the discriminator property in their protected constructor, but mgmt does not. Mgmt (discriminator property is name): https://github.com/Azure/autorest.csharp/blob/8ee2ca051a2ac5f4f79fb2d82209bfa076f93cbd/test/TestProjects/MgmtDiscriminator/Generated/Models/DeliveryRuleAction.cs#L18 DPG (discriminator property is discriminatorProperty): https://github.com/Azure/autorest.csharp/blob/8ee2ca051a2ac5f4f79fb2d82209bfa076f93cbd/test/TestProjects/Models-Typespec/Generated/Models/BaseModelWithDiscriminator.cs#L20

[ArcturusZhang]

Differences between the discriminator in mgmt/HLC and DPG:

category	mgmt/HLC	DPG	alignment
assignment	property initializers in ctor	expose in public ctor of base	property initializers in ctor
accessibility	internal	public	configuration for opting, default: internal for mgmt, public for dpg
has setter?	no	no	no-op
type	generate type according to the swagger definition	always string	wrapped in extensible enum (add in emitter)
show up in internal ctor of base?	yes	yes	no
show up in public/protected ctor of base?	Always no	Always yes	no
show up in internal ctor of derived?	yes	no	no
show up in public ctor of derived?	Always no	Always no	no

FYI @m-nash Comments:

Assignments

Mgmt/HLC is doing the assignments of discriminator in this way:

public abstract class BaseModel 
{
protected BaseModel() {}
internal string DiscriminatorProperty { get; set; }
}
public class ModelA
{
public ModelA()
{
DiscriminatorProperty = "A";
}
}

DPG is doing it in this way:

public abstract class BaseModel
{
protected BaseModel(string discriminatorProperty)
{
DiscriminatorProperty = discriminatorProperty;
}
}
public class ModelA
{
public ModelA() : base("A")
{
}
}

Other disalignments kind of depend on the decision of this.

Accessibility

I believe it makes sense that the discriminator property is internal. The customers never need the discriminator to distinguish which derived class the current instance should be, if they could write code like this:

public void Distinguish(BaseModel model)
{
switch (model.DiscriminatorProperty)
case "A":
DoSomethingWithA((ModelA)model);
break;
case "B":
DoSomethingWithB((ModelB)model);
break;
default:
DoSomethingWithUnknown(model);
break;
}

They could always write this code instead:

public void Distinguish(BaseModel model)
{
switch (model)
{
case ModelA modelA:
DoSomethingWithA(modelA);
break;
case ModelB modelB:
DoSomethingWithB(modelB);
break;
default:
DoSomethingWithUnknown(model);
break;
}
}

And since we add descriptions about "what are my derived types in the description of the base model", the second version above is more intuitive for our customers to write. We never documented the possible values of the discriminator values. Therefore I believe it is better if we make the discriminator properties internal. This minimizes the public API, and actually provides more convenient and intuitive interface for our customers to use.

Setter

It really does not matter much if we agree that the discriminator could be internal. But it makes more sense if they are readonly. Because the nature of discriminator property in strong typed language is that they provide an one-one map from the value to the actual type: ModelA : BaseModel will always have a value of A for the discriminator and ModelB: BaseModel will always have a value of B. Therefore it really does not make sense if it has a setter.

Show up in public/protected ctor?

I do not think it makes sense if we expose it in the public/protected ctor on the base.

1. If the base is abstract, the customers will never have a chance to construct it therefore there is no point that we expose it.
2. If the base is not abstract for some reasons, it makes a few sense that our customer could use it to construct an instance with a different value for future compatibility - for instance they could do this in an old version of the SDK (it does not have ModelC) but the service has been updated to support ModelC. The "Unknown" story of the discriminators only considered the covariant issue in the response side, the above is considering the covariant issue in the request issue.

The only reason we should do this is because in the public ctor of derived, we need to call it to assign my discriminator value to the property using base.

Show up in internal ctor?

This has been covered in my PR: https://github.com/Azure/autorest.csharp/pull/3200 It makes more sense that we do not expose this in the internal ctor.

[ArcturusZhang]

The generated code should look like:

public abstract class BaseModel
{
protected BaseModel(string requiredProperty)
{
    RequiredProperty = requiredProperty;
}

internal BaseModel(string requiredProperty, string otherProperty)
{
    RequiredProperty = requiredProperty;
    OtherProperty = otherProperty;
}

public string DiscriminatorProperty { get; protected internal set; }

// or if it is internal
// internal string DiscriminatorProperty { get; protected set; }

public string RequierdProperty {get;set;}

public string OtherProperty {get; set;}
}

public class ModelA
{
public ModelA(string requiredProperty, int requiredInt) : base(requiredProperty)
{
    DiscriminatorProperty = "A"; // this requires the `init` or `set` on the property
}

internal ModelA(string requiredProperty, string otherProperty, int requiredInt): base(requiredString, otherProperty)
{
    DiscriminatorProperty = "A"; // this requires the `init` or `set` on the property
    RequiredInt = requiredInt;
}

public int RequiredInt { get; set; }
}

[ArcturusZhang]

The other way around:

public abstract partial class BaseModel
{
// do not expose discriminator in public/protected ctor
protected BaseModel(string requiredProperty)
{
    RequiredProperty = requiredProperty;
}

// expose the discriminator in internal ctor for simpler internal usage
internal BaseModel(string discriminatorProperty, string requiredProperty, string otherProperty)
{
    DiscriminatorProperty = discriminatorProperty;
    RequiredProperty = requiredProperty;
    OtherProperty = otherProperty;
}

public string DiscriminatorProperty { get; protected internal set; }

// or if it is internal
// internal string DiscriminatorProperty { get; protected set; }

public string RequierdProperty {get;set;}

public string OtherProperty {get; set;}
}

public partial class ModelA : BaseModel
{
// never expose discriminator on public ctor on child
public ModelA(string requiredProperty, int requiredInt) : base(requiredProperty)
{
    // assign the value for discriminator with my own value
    DiscriminatorProperty = "A";
}

// use the internal ctor of base to assign discriminator value
internal ModelA(string requiredProperty, string otherProperty, int requiredInt): base("A", requiredString, otherProperty)
{
    RequiredInt = requiredInt;
}

public int RequiredInt { get; set; }
}

internal partial class UnknownBaseModel : BaseModel
{
// expose the discriminator property in internal ctor of the unknown to allow the model factory could enable the customer to construct an unknown instance with any property they want
internal UnknownBaseModel(string discriminatorProperty, string requiredProperty, string otherProperty): base(discriminatorProperty, requiredString, otherProperty)
{
}
}

[m-nash]

Last night we discussed removing the discriminator from the base serialization ctor since it is abstract we will alwyas construct a derived class even in the unknown case and that derived class will always set its value in both of its ctors.

The only place the discriminator should show up on the as a parameter is the model factory for the base type and the serialization ctor for the unknown derivation.

[ArcturusZhang]

Since our current code is following the pattern that I build the parameters from my own properties (ModelTypeProviderFields) and then putting them together to get the constructors (ModelTypeProvider), it is the most convenient that we keep the discriminator in the internal constructor. Because the unknown model is the only one that needs the discriminator as parameter in our previous design, therefore to get the parameter, we will have to get it from my base model and build it again in ModelTypeProvider, since it never passes along through the constructors. Based on above reasons, I prefer that we keep the discriminator in the base and unknown model's constructor so that we could easily get the parameter of the discriminator. Other version of derived constructors will not have this parameter.

[ArcturusZhang]

It turns out that we have to take a way that currently in mgmt/HLC. The approach shown in this comment has issues when we have multiple layers of derivced types, which we are planning to support (tracking in this issue)

For instance, we have this multiple layers of models like this:

@discriminator("kind")
model Fish
{}

model Shark : Fish
{
kind: "shark"
}

model CookieCutterShark : Shark
{
kind: "cookieCutterShark "
}

Please note the above typespec is not valid until this issue is resolved. But we have this pattern in swagger already, see this test project: https://github.com/Azure/autorest.csharp/blob/cfdc1a97f4f22817afc18a5ac9027136c3a6c96e/test/TestServerProjects/body-complex/Generated/Models/Cookiecuttershark.cs#L14

We do not have issues in the base model and the first layer of derived models,

public partial abstract class Fish
{
internal Fish(string kind) {}
}

public partial class Shark : Fish
{
internal Shark() : base("shark") {} // this is fine
}

But we no longer can set the value of discriminator in CookieCutterShark in the same way. Therefore to keep things consistency, I would like to apply the pattern of ctors in mgmt into DPG.

Proposal is shown in the next comment

[ArcturusZhang]

New proposal:

public abstract partial class BaseModel
{
// do not expose discriminator in public/protected ctor
protected BaseModel(string requiredProperty)
{
    RequiredProperty = requiredProperty;
}

// expose the discriminator in internal ctor for simpler internal usage
internal BaseModel(string discriminatorProperty, string requiredProperty, string otherProperty)
{
    DiscriminatorProperty = discriminatorProperty;
    RequiredProperty = requiredProperty;
    OtherProperty = otherProperty;
}

public string DiscriminatorProperty { get; protected internal set; }

// or if it is internal
// internal string DiscriminatorProperty { get; protected set; }

public string RequierdProperty {get;set;}

public string OtherProperty {get; set;}
}

// first layer derived model
public partial class ModelA : BaseModel
{
// never expose discriminator on public ctor on child
public ModelA(string requiredProperty, int requiredInt) : base(requiredProperty)
{
    // assign the value for discriminator with my own value
    DiscriminatorProperty = "A";
}

// use the internal ctor of base to assign discriminator value
internal ModelA(string discriminatorProperty, string requiredProperty, string otherProperty, int requiredInt): base(discriminatorProperty, requiredString, otherProperty)
{
    RequiredInt = requiredInt;
    DiscriminatorProperty = discriminatorProperty ?? "A";
}

public int RequiredInt { get; set; }
}

// second layer derived model
public partial class ModelAA : ModelA
{
// never expose discriminator on public ctor on child
public ModelAA(string requiredProperty, int requiredInt) : base(requiredProperty, requiredInt)
{
    // assign the value for discriminator with my own value
    DiscriminatorProperty = "AA";
}

// use the internal ctor of base to assign discriminator value
internal ModelAA(string discriminatorProperty, string requiredProperty, string otherProperty, int requiredInt, int optionalIntInAA): base(discriminatorProperty, requiredString, otherProperty)
{
    DiscriminatorProperty = discriminatorProperty ?? "AA";
    OptionalIntInAA = optionalIntInAA;
}

public int OptionalIntInAA { get; set; }
}

internal partial class UnknownBaseModel : BaseModel
{
// expose the discriminator property in internal ctor of the unknown to allow the model factory could enable the customer to construct an unknown instance with any property they want
internal UnknownBaseModel(string discriminatorProperty, string requiredProperty, string otherProperty): base(discriminatorProperty, requiredString, otherProperty)
{
}
}

[ArcturusZhang]

This issue has been fixed by https://github.com/Azure/autorest.csharp/pull/3207

Because during the development of this alignment, we realized that the way in mgmt/HLC of implementing discriminator related model hierarchies is always correct, therefore the thing we need to do for this issue is only to change the way DPG is doing, and this has been done by https://github.com/Azure/autorest.csharp/pull/3207

Closing this issue