DPG generator performs more restrictive validation for path parameters

[christothes]

I came across a DPG generator issue that is a breaking behavior change from the HLC generator. It appears that we generate a AssertNotNullOrEmpty where we used to generate only a null check for required path parameters.

Here is the one I encountered: DPG method

HLC method

Parameter

Although this is rare, some services like Azure Table Storage allow for empty string parameters. Ex:

GET https://foo.table.core.windows.net/MyTable(PartitionKey='3162e1ef-f4a7-4d18-9ed8-dfe08c6d7d7a',RowKey='')

[lirenhe]

Have a discussion for this topic in https://github.com/Azure/autorest.csharp/issues/2468 Need to check the general implementation for MPG and HLC first

[archerzz]

This is not the same topic we've discussed in #2468 It's about the logic to generate method parameter validation.

Parameter Definition

rowKeyParameter is a special case. It's not used like ordinary path parameters. There is the swagger sample.

"odata.id": "https://myaccount.table.core.windows.net/Customers(PartitionKey=Customer',RowKey='Name')",

DPG

In our DPG generation logic, we'll add AssertNullOrEmpty if the parameter is a path parameter, a string and doesn't skip URL encoding. rowKeyParameter follow the criteria. https://github.com/Azure/autorest.csharp/blob/849394266e905156b708523eb1115630f0c30c6f/src/AutoRest.CSharp/Common/Output/Models/Shared/Parameter.cs#L88

HLC

In HLC logic, we only generate null check. See: https://github.com/Azure/autorest.csharp/blob/849394266e905156b708523eb1115630f0c30c6f/src/AutoRest.CSharp/Common/Generation/Writers/RestClientWriter.cs#L99

Solution

I think we should consider if we drop the empty validation on string type path parameters. It looks like in our current service definitions, empty string type path parameters are not rare cases. Here is another one parentResourcePath in policy assignment:

"description": "The parent resource path. Use empty string if there is none.",

[archerzz]

I did some experiments, and I found if we dropped the check there will be too many changes in the existing generated codes, include MPG. For example, this line will be changed: https://github.com/Azure/azure-sdk-for-net/blob/5cc66b069706f5e47d2ac1fed7104b87ddea43bd/sdk/compute/Azure.ResourceManager.Compute/src/Generated/AvailabilitySetCollection.cs#L118

The problem is that the interface is also changed, e.g.

Maybe we just write customization codes for cases that path parameters can be empty strings.

[archerzz]

Need to change this back to GA milestone, since it could involve breaking changes. For MPG, Feng also showed me a service which defines the API like that. MPG doesn't hit DPG problem, because the problematic path parameter is avoided due to the MPG API hierarchical structure.

Looks like we need a different behavior for DPG here, following HLC.

[chunyu3]

We will consolidate the same behavior between DPG and HLC. Need typespec to add a decorator (e.g @AllowEmpty) for path parameter to indicate if the path parameter allow empty or not. Then codegen will apply right validation according to the decorator. By default, we will validate path paramter NotNullOrEmpty. Now we will maintain the validation as it is.

[chunyu3]

Move it to DPG/RLC v2.1. After the typespec add decorator, we will update the validation.

[archerzz]

Filed https://github.com/Azure/typespec-azure/issues/3416