Fix Enum Mismatch Issues in Search Service [DO NOT MERGE]

[sarangan12]

Latest improvements:

MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.

Contribution checklist:

• I have reviewed the documentation for the workflow.
• Validation tools were run on swagger spec(s) and have all been fixed in this PR.
• The OpenAPI Hub was used for checking validation status and next steps.

ARM API Review Checklist

• Service team MUST add the "WaitForARMFeedback" label if the management plane API changes fall into one of the below categories.
• adding/removing APIs.
• adding/removing properties.
• adding/removing API-version.
• adding a new service in Azure.

Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs.

• If you are blocked on ARM review and want to get the PR merged urgently, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
  Please follow the link to find more details on API review process.

[AutorestCI]

Automation for azure-sdk-for-js

Unable to detect any generation context from this PR.

[AutorestCI]

Automation for azure-sdk-for-python

Unable to detect any generation context from this PR.

[AutorestCI]

Automation for azure-sdk-for-ruby

Unable to detect any generation context from this PR.

[AutorestCI]

Automation for azure-sdk-for-go

Unable to detect any generation context from this PR.

[AutorestCI]

Automation for azure-sdk-for-java

Unable to detect any generation context from this PR.

[azuresdkci]

Can one of the admins verify this patch?

[brjohnstmsft]

@sarangan12 The original examples actually are valid for the Azure Search REST API; We just don't have the tools to express them in the Swagger and still get the codegen result that we want.

Is the intent of your change just to satisfy the validation rules? If so, I think there should be a better way. Soon we plan to use these examples in generated documentation, and it would be a shame if we have to make our examples less useful to just to satisfy validation rules that aren't yet sufficiently expressive.

[sarangan12]

@brjohnstmsft I am trying to understand the issue here. If the examples are correct. what is the meaning of 'Collection(Edm.String)'? If this is the actual value sent by the service, then why couldn't this be one of the actual enum value in the swagger? (instead of Edm.String) Could you please help me understand?

[brjohnstmsft]

@sarangan12 That particular property represents the data type of a field in Azure Search. Our service is OData-compliant, so it uses the EDM (Entity Data Model) type system. Edm.String is the primitive string type, and Collection(Edm.String) represents a collection of strings. We could have modeled this as an enum in the generated code, but this would have had a few drawbacks:

1. It would probably result in poor names. We don't want enum values like "Collection_Edm_String". It's a much cleaner user experience if they can type DataType.Collection(DataType.String).
2. It would result in a ponderously long list of names, since we'd need to manually combine every scalar data type with "Collection()".
3. enum in .NET versions very poorly. If we introduce a new data type, that automatically requires a major version bump of the SDK.

Regarding the final reason, you'd think we'd just use modelAsString: true, but that has the drawback of a very poor Intellisense experience. Having DataType as a strong type in our client programming model instead of string is highly desirable. This is why myself and others pushed for the inclusion of an "extensible enum" mode for AutoRest, but it appears to be in a state of limbo as of now. It was implemented over a year ago, but somehow doesn't seem to be working. If it did, your validation tool would need to be aware of it so it could not be so strict with validating enum values.

We have plenty of other data types in our API that behave this way. For example, RegexFlags can be combined. In that case it just isn't practical to have every possible combination of flags in an enum. You could say that it would have been a better choice to model RegexFlags as an array of flags instead, and you'd be right, but we designed this part of the API a long time ago and are not willing to change it now as it is a breaking change that would create pain for our customers when upgrading to a new API version.

Basically, the tools as they exist are not sufficiently flexible to accommodate the sometimes conflicting needs of precise specs and usable SDKs.

[sarangan12]

@brjohnstmsft Thanks a lot for the explanation. Now, I understand the reasoning for the Collection &

1. You do not want to use it directly in enum to avoid long & illogical names (Agreed) and
2. You do not want to use modelAsString:true to have a good Intellisense Experience. (Actually you can intellisense even when you use modelAsString:true. Refer Option 2 below)

Now, I did take a look at the way the enums are implemented and here are my findings (I will address your points after presenting my findings):

Option 1
Simple enums. If you define a simple enum in the swagger (Or an enum value with modelAsString set to false), then the C# SDK will generate a enum type. For example, refer the definition of ‘status’ - here. It is defined as:

"status": {
    "description": "The status of the capability.",
    "enum": [
        "Visible",
        "Available",
        "Default",
        "Disabled"
    ],
    "type": "string",
    "readOnly": true,
    "x-ms-enum": {
        "name": "CapabilityStatus",
        "modelAsString": false
    }
} 

This definition will be translated to a enum type in C# SDK as seen here as:

[JsonConverter(typeof(StringEnumConverter))]
public enum CapabilityStatus
{
    [EnumMember(Value = "Visible")]
    Visible,
    [EnumMember(Value = "Available")]
    Available,
    [EnumMember(Value = "Default")]
    Default,
    [EnumMember(Value = "Disabled")]
    Disabled
}

Now, in the places where this ‘CapabilityStatus’ is referenced, its data type will be ‘CapabilityStatus’ as seen here:

[JsonProperty(PropertyName = "status")]
public CapabilityStatus? Status { get; private set; }

Option 2
Enum with x-ms-enum extension and modelAsString set to true, then the C# SDK will generate a static class with several constants. But the references will be of type String. For example, refer the definition of ‘include’ – here. It is defined as:

{
    "name": "include",
    "in": "query",
    "description": "If specified, restricts the response to only include the selected item.",
    "required": false,
    "type": "string",
    "enum": [
        "supportedEditions",
        "supportedElasticPoolEditions",
        "supportedManagedInstanceVersions"
    ],
    "x-ms-enum": {
        "name": "CapabilityGroup",
        "modelAsString": true
    }
}

This definition will be translated to a static class with constants in C# SDK as seen here as:

public static class CapabilityGroup
{
    public const string SupportedEditions = "supportedEditions";
    public const string SupportedElasticPoolEditions = "supportedElasticPoolEditions";
    public const string SupportedManagedInstanceVersions = "supportedManagedInstanceVersions";
}

But, in the places where CapabilityGroup is referenced, the type will String and NOT CapabilityGroup as seen here as:

public async Task<AzureOperationResponse<LocationCapabilities>> ListByLocationWithHttpMessagesAsync(string locationName, string include = default(string), Dictionary<string, List<string>> customHeaders = null, CancellationToken cancellationToken = default(CancellationToken))
{
    ………………
    ………………
}

As you can see, the type of ‘include’ is a string. So, you may use one of the constants defined in the static class (Eg: CapabilityGroup.SupportedEditions) or you may pass your own values.

Option 3
Enums with opt-in-extensible-enums as true amd x-ms-enum. For example, refer the opt-in here:

openapi-type: arm
tag: package-2018-07
opt-in-extensible-enums: true

After the optin, the enums can be simply defined with x-ms-enum/modelAsString. For example, refer the definition of ‘unit’ here as:

"unit": {
    "type": "string",
    "enum": [
        "Bytes",
        "Count",
        "Milliseconds"
    ],
    "x-ms-enum": {
        "name": "MetricUnit",
        "values": [
            {
                "value": "Bytes",
                "description": "The number of bytes."
            },
            {
                "value": "Count",
                "description": "The count."
            },
            {
                "value": "Milliseconds",
                "description": "The number of milliseconds."
            }
        ],
        "modelAsString": true
    },
    "description": "The metric unit",
    "readOnly": true,
    "x-nullable": false
}

In C# SDK, the ‘unit’ will be generated as a struct as here:

[JsonConverter(typeof(MetricUnitConverter))]
public struct MetricUnit : System.IEquatable<MetricUnit>
{
    private MetricUnit(string underlyingValue)
    {
        UnderlyingValue=underlyingValue;
    }

    public static readonly MetricUnit Bytes = "Bytes";
    public static readonly MetricUnit Count = "Count";
    public static readonly MetricUnit Milliseconds = "Milliseconds";
    ……………………
    ……………………
}

In places where the ‘MetricUnit’ is referenced, the data type will be MetricUnit as here:

[JsonProperty(PropertyName = "unit")]
public MetricUnit Unit { get; private set; }

Here you could use the values defined in the MetricUnit or pass in your own values.

Option 4 & 5 (No Longer in use)
Earlier, 2 new extensions: ‘oldModelAsString’ & ‘modelAsExtensible’ are introduced. Then they have been reverted back and it is no longer in use or supported.

Now, Let us come back to your points.

1. This is why myself and others pushed for the inclusion of an "extensible enum" mode for AutoRest, but it appears to be in a state of limbo as of now. It was implemented over a year ago, but somehow doesn't seem to be working.
   I believe when you ask for "extensible enum" mode for Autorest, You are specifically asking for the Option 3 presented above. With the option 3, I believe the Collection(...) or anything like that could easily be handled. At the same time, Intellisense also could be achieved. Could you please confirm if this satisfies your needs to represent your service correctly in Swagger?

2. If it did, your validation tool would need to be aware of it so it could not be so strict with validating enum values
   Update: I believe you are correct that the tool should not be reporting this as an error. But, based on the suggestion from point 1, if you change your swagger to modelAsString:true and opt-in in the readme file, then these errors will never be reported. Please let me know if you are Ok with that.

[brjohnstmsft]

@sarangan12 Thanks for the analysis. It looks like option 3 (opt-in-extensible-enums) is what we need. However, we are not ready to adopt it yet as the generated code doesn't quite match our requirements. We'll work with the AutoRest team after //build to make the necessary modifications.

By the way, is opt-in-extensible-enums documented anywhere? I looked for something like that but couldn't find it (FYI @fearthecowboy).

[sarangan12]

@brjohnstmsft Thanks for the reply. I will also update @shahabhijeet to document opt-in-extensible-enums. I will keep this PR open to remind me of this issue and will contact you after build. Once the changes have been completed, We can work together and check the model validator issues. Thanks

[brjohnstmsft]

@sarangan12 Sounds good, thanks!

[shahabhijeet]

So search team will add opt-in flag in Readme to start using extensible enums.
If yes I am approving this PR.

[brjohnstmsft]

@shahabhijeet This PR makes our examples less useful -- I would prefer that you not merge it.

As discussed, we will opt-in to extensible enums after //build, and after we've made the feature in AutoRest do what we actually need.

[sarangan12]

@brjohnstmsft Is this a good time to start our conversation back on the option 3?

[brjohnstmsft]

@sarangan12 Not yet. We have some bug fixes and incremental changes we need to ship. As soon as that's out, I'll be working on adapting our .NET SDK to the new source tree structure and build pipeline, then I'll look at this.

[sarangan12]

@brjohnstmsft Is this a good time to start our conversation back on the option 3?

[brjohnstmsft]

@sarangan12 We don't have bandwidth right now to work on this, but might later in the year. I am tracking this and haven't forgotten about it. I'll follow up when it's time to revisit this. In the meantime, is this blocking anybody or causing any persistent issues with engineering systems infrastructure?

[sarangan12]

@brjohnstmsft No blockers. Just wanted to remind you....

[yungezz]

I'll close the PR for now since service team is not ready for it, and lots of conflicts already.. pls feel free to reopen it for discussion if ready.

[openapi-sdkautomation]

In Testing, Please Ignore

[Logs] (Generated from 97374c1, Iteration 1)

[brjohnstmsft]

Just to close the loop, we are now working on this.