Review designs to minimize breaking changes and implement the solution - overload for parameter change

[ArthurMa1978]

Parameter ordering such as moving CancellationToken from the end to the same spot it was.
does require us to know what position it was in for the last iteration

Implementation and design

[ArthurMa1978]

#1638, #1641

[ArthurMa1978]

Azure/autorest#4356 m4 PR
https://gist.github.com/timotheeguerin/28fb5844bcffbd4d798bd9efedb8d8ec#file-codemodelv2-yaml-L117 example generation

[archerzz]

Parameters Overload Design

Prerequisites

1. waitForCompletion and cancellationToken is excluded in the discussion below, for simplicity purpose.
2. Parameter list cannot shrink. (Guaranteed by M4)
   • Valid cases: A,B => A,B,C* ; A,B=>A,B* (* means optional)
   • Invalid cases: A,B* => A ; A,B,C* => A,C*
   • [Conclusion]: The latest parameter list should be the longest
3. Required parameters cannot grow. (Guaranteed by M4)
   • Valid cases: A,B => A,B*; A,B => A,B,C*
   • Invalid cases: A,B => A
4. Parameter list is sorted from required to optional (e.g. required parameters are at the beginning, and optional are at the bottom). (CSharp generator will do this)
   • Valid cases: A,B*; A,C,B*
   • Invalid cases: A,B*,C
5. We do not allow insert parameter before any existing parameter. Otherwise, it is a breaking change. See swagger lint rule: https://github.com/Azure/azure-rest-api-specs/blob/main/documentation/openapi-authoring-automated-guidelines.md#r4039

Axioms

1. If a parameter list P1 is a subset of another list P2, then P1 will have no conflict with P2 if all parameters in P1 are required.
   • Conflict: A,B,C* v.s. A,B,C*,D*
     • F(A,B) will trigger compilation error
   • No conflict: A,B,C v.s. A,B,C,D*
     • F(A,B,C) => A,B,C
     • F(A,B,C,D) => A,B,C,D*
2. If P1 and P2 have the same parameter sequence, then they have conflict.
   • e.g. A,B,C* v.s. A,B*,C*
     • F(A,B,C) will trigger compilation error
3. If P1 and P2 is not subsequence of each other, then they do not have conflict.
   • e.g. A,B,C v.s. A,C,B*

---

Use Case

Overall

• The evolution of parameter lists can create diverse branches, see case 5 and case 6 below.
• For each branch, we only need one overload for the same size of parameter lists, e.g. A,B,C; A,B,C*; A,B*,C*; we only need A,B,C as the overload.

Case 1

version	parameters
v1	<empty>
v2	A*
v3	A*,B*
Expected Overloads
	<empty>
	A
	A*,B*

Case 2

version	parameters
v1	A
v2	A,B*
v3	A*,B*
v4	A*,B*,C*
Expected Overloads
	A
	A,B
	A*,B*,C*

Case 3

version	parameters
v1	A
v2	A*
v3	A*,B*
Expected Overloads
	A
	A*,B*

Case 4

version	parameters
v1	A,B
v2	A,B,C*
v3	A,B*,C*
v4	A*,B*,C*
Expected Overloads
	A,B
	A*,B*,C*

Case 5

version	parameters
v1	A,B,C
v2	A,C,B*
v3	A,C,B*,D*
v4	A,B*,C*,D*
v5	A*,B*,C*,D*
Expected Overloads
	A,B,C
	A,C,B
	A,C,B*,D*
	A,B*,C*,D*

Case 6

version	parameters
v1	A,B
v2	B,A*
v3	B,A*,C*
v4	B,A*,C*,D*
v5	A*,B*,C*,D*
Expected Overloads
	A,B
	B,A
	B,A,C
	B,A*,C*,D*
	A*,B*,C*,D*

Algorithm

• Sort overloaded parameter lists (e.g. signatureParametersOverloads), move required parameters at the beginning.
• Sort all parameter lists (overloaded + latest) by length, but keep the original sequence (e.g. in order) if length is equal.
• Split parameter lists into branches, each branch starts with different sequence. See case 5 and case 6 above.
  • For case 6, the divisions will be {A,B}, {A*,B*,C*,D*} and {B,A*}, {B,A*,C*}, {B,A*,C*,D*}.
• For each branch, for parameter lists of the same size, keep the first one, and remove all the others.
• For each branch, make parameters in each parameter lists required, except the last parameter list.

// sort parameter lists including parameter overloads (change existing codes)
foreach (var overload in operation.SignatureParametersOverloads)
{
    // move required parameters to the beginning, in order
}
// implement in maybe ParameterOverloadsChooser.cs?
var parameterLists = GetOverloadParameterLists(...).Append(parameters).Sort(//by length, in order);

var parameterListBranches = new HashSet<IList<IList<Parameter>>>(); // divide parameter lists by sequence, see rule #6 above
while (parameterLists.Count > 0)
{
    var branch = new List<List<Parameer>>();
    branch.Add(parameterLists.Pop()); // push the shortest to new branch
    for (var i = 0; i < parameterLists.Count; i ++)
    {
        if (IsSubsequence(branch.Last(), parameterLists[i])
        {
            // remove from Parameter List
            // add to branch
        }
        else
        {
            //add to newParameterLists
         }
    }
    parameterListBranches.Add(branch);
    parameterLists = newParameterLists;
}

foreach (var branch in parameterListBranches)
{
    // remove sequence having the same length in each branch
    for (var i  = 0; i <branch.Count - 1; i ++)
    {
        if (branch[i].Count == branch[i+1].Count)
        {
             //remove branch[i]
        }
    }

    // make all parameters except those in last list required
    for (var i  = 0; i <branch.Count - 1; i ++)
    {
        // make parameters required
    }
}

[archerzz]

LLC has the requirement: #1659

Both mgmt plan and LLC should share some common codes: #1658

[archerzz]

About Property Bag

Criteria to provide property bag

Provide a property bag for optional parameters (excluding body parameters) when optional parameter count >= 3.

• no property bag for required parameters
• body parameter is independent, see Sort parameters ordering #1843 if you're interested at the background.

Reasons behind

In track2 mgmt SDK, there won't be many required parameters in each method. Providing property bag for optional parameters should be sufficient to control the total number of parameters in each method, because:

• In track 2, we adopt an object hierarchical API. As a result, for each method, there won't be many path parameters. Normally there is only one path parameter. That will reduce the number of required parameters, since path parameters are required.

Before (storage track1)

FileShare shareSnapshot = storageMgmtClient.FileShares.Get(rgName, accountName, shareName, "stats", shareSnapshot1.SnapshotTime.Value.ToUniversalTime().ToString("o"));

After (storage track2)

var resourceGroup = await ArmClient.GetDefaultSubscription().GetResourceGroups().GetAsync(rgName);
var storageAccount = await resourceGroup.GetStorageAccounts().GetAsync(accountName);
var fileService = await _storageAccount.GetFileService().GetAsync();
FileShare shareSnapshot = await _fileService.GetFileShares().GetAsync(shareName, "stats", shareSnapshot1.Data.SnapshotTime.Value.UtcDateTime.ToString("o"));

---

• We've decided to not unfold body parameter, which will also reduce the number of required parameters.

Before (resource SDK, Deployment class)

public async virtual Task<DeploymentWhatIfOperation> WhatIfAsync(bool waitForCompletion, string location, DeploymentWhatIfProperties properties, CancellationToken cancellationToken = default)

After

public async virtual Task<DeploymentWhatIfOperation> WhatIfAsync(bool waitForCompletion, ScopedDeploymentWhatIf parameters, CancellationToken cancellationToken = default)

How to

Before we split parameter lists into branches and do the filtering, for parameter lists which triggers property bag wrapping, create a new parameter list for it. That is

For example:

Version	Parameters
v1	A,B,C,D*
v2	A,B,C,D*,E*
v3	A,B,C,D*,E*,F*
v4	A,B,C,D*,E*,F*,G*
After Property Bag Wrapping
v1	A,B,C,D*
v2	A,B,C,D*,E*
v3	A,B,C,PropertyBag*
v4	A,B,C,PropertyBag*
Expected Overloads
	A,B,C,D
	A,B,C,D*,E*
	A,B,C,PropertyBag

[Note]:

• We only need one property bag for different branches, because you can only add optional parameters. The property bag for the last version should be the superset of all the previous ones.
• Body parameter doesn't impact the logic here, because:
  • If the body parameter is required, then it's just part of the required parameters.
  • If the body parameter is optional, then it's part of the optional parameters with the exception that it won't be included in the property bag. So there could be a case that:
    • A,B,C,body*,PropertyBag*
• In parameter overloading, the PropertyBag parameter must be required, otherwise there will be cases the compiler cannot distinguish which method to invoke.
  • For example, in above case, if the PropertyBag is optional, then the codes Foo(A,B,C) will have ambiguous invocation between Foo(A,B,C,D*,E*) and Foo(A,B,C,PropertyBag*).

Options class

It's named after {ResourceClass}{Operation}Options. For example, if StorageAccountCollection class has a CreateOrUpdate method (also CreateOrUpdateAsync) which has 3 optional parameters, then the option class will be StorageAccountCollectionCreateOrUpdateOptions.

Examples

In storage, the FileShareCollection has a method which originally look like below:

public virtual Pageable<FileShare> GetAll(int? maxpagesize = null, string filter = null, string expand = null, CancellationToken cancellationToken = default)

It will be transformed into the following signature:

public virtual Pageable<FileShare> GetAll(FileShareCollectionGetAllOptions? options, CancellationToken cancellationToken = default)

where the FileShareCollectionGetAllOptions is like below:

public partial struct FileShareCollectionGetAllOptions
{
    public int? maxpagesize = null;
    public string filter = null;
    public string expand = null;
}

[blueww]

The above design from @archerzz is generally good to me.
The parameter name "StorageAccountCollectionCreateOrUpdateOptions" might be too long, and createorUpdate is actually on account , so "StorageAccountCreateOrUpdateOptions" might be better

[annelo-msft]

Hi @archerzz, this is a great discussion -- thank you for organizing this! I have a few comments from the data plane side:

• In data plane generated code, we do not use property bags. These can be added to generated clients via manually-written convenience APIs.
• In cases 5 and 6, you show that we change the ordering of parameters that might be of the same type. This could have the following problem:

In v1, the client is like this:

class Client {
    Response DoThing(string a, string b, RequestContext context = default);
}

If a becomes optional in v2, the generator would create the client as:

class Client {
    Response DoThing(string b, string a=default, RequestContext context = default);
}

Anyone caller whose application was doing DoThing(myAValue, myBValue); would pass the wrong values for a and b in v2, with no compiler warnings - this is a behavioral breaking change.

I have suggested the following guidance for data plane generated code: Version-Aware Generated Methods: Optional Parameters. I am interested to hear your thoughts on this slightly-modified design!

[archerzz]

@annelo-msft Agree with your comments. For the mgmt plane, the only extra work item is the property bag which can be implemented separately. Let's move parameter overload discussion to the gist you've pasted.

[Yao725]

How to keep the code compatibility when implementing property bag

(Updated based on offline discussion on April 29, 2022)

Currently We are trying to implement the property bag feature in mgmt. plane. One of the issues here is how we can ensure the compatibility of our code. Considering that we are in the process of continually GA more RPs, I think this issue can be devided into two parts, that is, the non-GAed RP and GAed RP.

• 1 non-GAed RP

As the RP is still in preview, so the breaking changes are allowed, so we can just change the method signature, below is an example in Azure.ResourceManager.Cdn.

Before

public virtual async Task<Response<MetricsResponse>> GetLogAnalyticsMetricsAsync(IEnumerable<LogMetric> metrics, DateTimeOffset dateTimeBegin, DateTimeOffset dateTimeEnd, LogMetricsGranularity granularity, IEnumerable<string> customDomains, IEnumerable<string> protocols, IEnumerable<LogMetricsGroupBy> groupBy = null, IEnumerable<string> continents = null, IEnumerable<string> countryOrRegions = null, CancellationToken cancellationToken = default){}

After

public virtual async Task<Response<MetricsResponse>> GetLogAnalyticsMetricsAsync(IEnumerable<LogMetric> metrics, DateTimeOffset dateTimeBegin, DateTimeOffset dateTimeEnd, LogMetricsGranularity granularity, IEnumerable<string> customDomains, IEnumerable<string> protocols, LogAnalyticsGetLogAnalyticsMetricsOptions options, CancellationToken cancellationToken = default){}

• 2 GAed RP

As this RP is already GAed, we can't simply delete the old method and add the new method, and because we haven't implemented parameter overloading, so the customization is needed. For now, I think we might have two scenarios here and in the following I will use the following method in Azure.ResourceManager as an example.

• Scenario 1

Scenario 1 is that We started with the method with one optional parameter, which then became two, three and up to multiple. In this scenario we need to consider the history of this method and provide the fully parameter overloads when we want to turn the optional parameters that are greater than two into property bag.

Before

// v1
public virtual async Task<Response<ManagementGroupResource>> GetAsync(string groupId, ManagementGroupExpandType? expand = null, CancellationToken cancellationToken = default){}
// v2
public virtual async Task<Response<ManagementGroupResource>> GetAsync(string groupId, ManagementGroupExpandType? expand = null, bool? recurse = null, CancellationToken cancellationToken = default){}
// v3
public virtual async Task<Response<ManagementGroupResource>> GetAsync(string groupId, ManagementGroupExpandType? expand = null, bool? recurse = null, string filter = null, CancellationToken cancellationToken = default){}
// v4
public virtual async Task<Response<ManagementGroupResource>> GetAsync(string groupId, ManagementGroupExpandType? expand = null, bool? recurse = null, string filter = null, string cacheControl = null, CancellationToken cancellationToken = default){}

After

public virtual async Task<Response<ManagementGroupResource>> GetAsync(string groupId, ManagementGroupGetOptions options, CancellationToken cancellationToken = default){}
// Customization
public virtual async Task<Response<ManagementGroupResource>> GetAsync(string groupId, ManagementGroupExpandType? expand = null, bool? recurse = null, string filter = null, string cacheControl = null, CancellationToken cancellationToken = default){}
public virtual async Task<Response<ManagementGroupResource>> GetAsync(string groupId, ManagementGroupExpandType? expand, bool? recurse, string filter, CancellationToken cancellationToken){}
public virtual async Task<Response<ManagementGroupResource>> GetAsync(string groupId, ManagementGroupExpandType? expand, bool? recurse, CancellationToken cancellationToken){}
public virtual async Task<Response<ManagementGroupResource>> GetAsync(string groupId, ManagementGroupExpandType? expand, CancellationToken cancellationToken){}

• Scenario 2

Scenario 2 is that we only start with one method in the history and it meets the criteria of the property bag. In this case, we keep the new generated method same as the scenario 1 and only need to add one customized method.

Before

public virtual async Task<Response<ManagementGroupResource>> GetAsync(string groupId, ManagementGroupExpandType? expand = null, bool? recurse = null, string filter = null, string cacheControl = null, CancellationToken cancellationToken = default){}

After

public virtual async Task<Response<ManagementGroupResource>> GetAsync(string groupId, ManagementGroupGetOptions options, CancellationToken cancellationToken = default){}
// Customization
public virtual async Task<Response<ManagementGroupResource>> GetAsync(string groupId, ManagementGroupExpandType? expand = null, bool? recurse = null, string filter = null, string cacheControl = null, CancellationToken cancellationToken = default){}

[archerzz]

@Yao725 Thanks for the update. So the conclusion is that:

• Option class is always required.
• When parameter overloading is involved:
  • we keep the latest overload version as-is
  • for all the prior versions, the optional parameters will be changed to required

And I think we cover the following use cases without ambiguity:

• Users only invoke with required parameters, e.g. GetAsync(groupId)
• Users skip some optional parameters using named parameters, e.g. GetAsync(groupId, filter:abc)
• Users skip some optional parameters using null, e.g. GetAsync(gropuId, null, cancellationToken)
• Users use option class, e.g. GetAsync(groupId, new ManagementGroupGetOptions{})

[ArthurMa1978]

Need align with DPG

[ArthurMa1978]

We have new solution for overload, #3596