Virtual member call in constructor warning

[ArthurMa1978]

UPDATE https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/resourcemanager/Azure.ResourceManager/src/ArmResource.cs#L67

Be Careful with Virtual Method Calls from the Constructor (and Destructor) of your Classes in C++/C#/Java! - CodeProject.

While the following is probably ok, it would be best and follow convention to call into base()'s constructor.

public abstract class TrackedResource<TModel> : TrackedResource
    where TModel : class
{
    /// <summary>
    /// Initializes a new instance of the <see cref="TrackedResource{TModel}"/> class.
    /// </summary>
    /// <param name="id"> The identifier of the resource that is the target of operations. </param>
    /// <param name="location"> The location of the resource. </param>
    /// <param name="data"> The model to copy from. </param>
    protected TrackedResource(ResourceIdentifier id, LocationData location, TModel data)
    {
        Id = id;
        Location = location;
        Model = data;
    }

Copied from Repro Steps when changed from Bug to User Story

Copied from Description when changed from Feature to Bug
Be Careful with Virtual Method Calls from the Constructor (and Destructor) of your Classes in C++/C#/Java! - CodeProject.

While the following is probably ok, it would be best and follow convention to call into base()'s constructor.

public abstract class TrackedResource<TModel> : TrackedResource
    where TModel : class
{
    /// <summary>
    /// Initializes a new instance of the <see cref="TrackedResource{TModel}"/> class.
    /// </summary>
    /// <param name="id"> The identifier of the resource that is the target of operations. </param>
    /// <param name="location"> The location of the resource. </param>
    /// <param name="data"> The model to copy from. </param>
    protected TrackedResource(ResourceIdentifier id, LocationData location, TModel data)
    {
        Id = id;
        Location = location;
        Model = data;
    }

[archerzz]

About TrackedResource

TrackedResource doesn't have virtual calls in its constructor now.

About ArmResource

Its constructor has 2 kinds of virtual calls:

• Virtual properties like Id, ClientOptions. We cannot remove the virtual keywords since they are required for testing. Those virtual properties will be intercepted by test framework.
• ValidateResourceType

About ValidateResourceType

I think we don't have a good solution here. What we want is to:

• enforce validation during object construction
• validation logic can be overwritten

The problem is that virtual calls in constructor is normally considered not good. If we choose to eliminate this virtual call, we can:

1. make those ValidateResourceType methods private, and invoke them in their own class constructors. But that actually changes the behavior that the ValidateResourceType is not overwritten. Instead, each level of class will have its own check.
   • [update]: This solution is not feasible, because the base class ValidateResourceType logic is not suitable for child class. An example is GenericResourceCollection, it won't pass the validation in ArmResource.
2. make those ValidateResourceType methods private, but constructors of each level of class will not invoke the non-default base constructor. Instead, they repeat the same logic of base constructors, and invoke the its own ValidateResourceType. This is kind of rigid, create duplicated codes, and is not easy to maintain this pattern in the future.

---

On the other hand, ValidateResourceType is OK here. See the reference doc about how csharp does in constructors.

constructor()
{
    auto generated: initialize the member variables (fields) of this class
    auto generated: call the base class constructor (if we have a base class)
    [your constructor code here]
}

The properties of each class is initialized, before the base class constructor is invoked. As long as we keep it simple, virtual calls in this case should be fine.

[archerzz]

The experimental PR is Azure/azure-sdk-for-net#25103

[archerzz]

After discussion, we decided to keep existing virtual calls, but only for debug version.