AutoRest azure-validator + client generation erroneously fails on core version ~2.0.4413 but not ^3.2.0

[forteddyt]

Before filling a bug

• have you checked the faq for known issues.
• have you checked existing issues

Describe the bug
This issue was originally posted at Azure/azure-rest-api-specs#15654, but I was advised to post the issue here.

PR in question: Azure/azure-rest-api-specs#15439
LintDiff Logs in question: https://github.com/Azure/azure-rest-api-specs/pull/15439/checks?check_run_id=3333578400
Fails with: "Text": "Error: '$.definitions.Identity.properties.type.enum' has incompatible values (---\n- SystemAssigned\n- 'SystemAssigned,UserAssigned'\n- UserAssigned\n- None\n, ---\n- SystemAssigned\n)."

The issue lies in machineLearningServices' ResourceIdentityType enum sharing the same name as common-types' ResourceIdentityType enum. However, common-types' Identity definition is not referenced (neither directly nor indirectly) anywhere in any of the specs defined in the readme.md. Interestingly, removing the reference to Identity from ResourceModelWithAllowedPropertySet resolves this issue. ResourceModelWithAllowedPropertySet is not referenced in any of the specs.

Running the validation command with --v3 (which uses AutoRest core version ^3.2.0, as opposed to AutoRest core version ~2.0.4413) does not produce this failure.

autorest --validation --azure-validator --v3 --semantic-validator=false --model-validator=false --message-format=json --openapi-type=arm [email protected]/[email protected] [email protected]/[email protected] --tag=package-2021-10-01 readme.md

Because of these reasons, I think this might be an issue with autorest core version ~2.0.4413. ~2.0.4413 is used in the LintDiff pipeline. The client generation gates also does not seem to use a 3.x version, as they are failing with the same issue.

Expected behavior
Validation/client generation to succeed, as it would with the 3.x version.

Additional context
To temporarily alleviate this issue in Azure/azure-rest-api-specs#15439, I have removed the reference to Identity from ResourceModelWithAllowedPropertySet. If you're planning on reproducing this issue, make sure to add that back in.
THIS IS NOT AN ACCEPTABLE SOLUTION; common-types definitions are used across practically every modern spec in the repo. Making changes to side-step this bug would have unintended consequences to other, already-merged specs.

[timotheeguerin]

Has far as I can tell this is a design issue with autorest v2 and can't be fixed easily. It is due to how autorest v2 merged swagger models with the same names and the logic for including external models is a bit complex(one rule that applies here is if a model is referenced via allOf it will be included - explain why removing that link fixes it) and changing it would introduce a great chance for breaking existing specs. Autorest V2 not being actively maintained anymore this will most likely not get changed.

There is another solution than updating the types.json(which I agree is not a solution):

Rename the Identity defintiions in the spec and use x-ms-client-name to rename it to Identity if wanted.
For example this is how Cosmos DB has it for their Identity type.

[forteddyt]

Unfortunately, this did not seem to solve the issue. Let me know if I implemented the fix incorrectly (zhaomuzhi/azure-rest-api-specs@ced11a7 + zhaomuzhi/azure-rest-api-specs@7d80193)
LintDiff w/ fix: https://github.com/Azure/azure-rest-api-specs/pull/15439/checks?check_run_id=3344550243
.NET generation gate w/ fix: https://github.com/Azure/azure-rest-api-specs/pull/15439/checks?check_run_id=3344560763

I'm now getting R4005 error, as well

[timotheeguerin]

Yeah that looks good to me, but I guess it does fail for that other type. So after digging more into the code base the reason this gets included is autorest v2 will include any models that reference via allOf on of the included model(no idea whats the reasoning behind it but as said above this will most likely not change).
In this case it is Sku that trigger ResourceModelWithAllowedPropertySet from being included which in turn include "Identity`.

I tried changing the Sku reference from types.json and using a copied version and this resolve the issue(Shouldn't even need that 1st fix). There is still some issue in mfe.json where Sku is defined manually as well.

I don't know if the azure-specs-rest-api teams has some docs on how to use types.json but it kinda seems like we either want all or nothing with v2(and all their tooling...)

[forteddyt]

ok... so the solution is to copy the Sku definition verbatim in our specs, and reference that instead of referencing the common-type?

The reason being that since we reference the common-type Sku, through some mumbo-jumbo in autorest v2, Sku being referenced by ResourceModelWithAllowedPropertySet.sku causes all of ResourceModelWithAllowedPropertySet to be generated, and since ResourceModelWithAllowedPropertySet references Identity, then Identity will be included and BAM that collides with our specs' internal definition of Identity?

[timotheeguerin]

yeah basically...

[forteddyt]

Lol... so azure-rest-api-specs can't update to AutoRest v3 "because the autorest v3 has many breaking changes against v2" Azure/azure-rest-api-specs#15654 (comment)

And the AutoRest v2 can't be updated because v2 isn't being maintained anymore

Wonderful 🤦‍♂️

My grumblings aside, your solution worked and has unblocked the code gen gates (.NET gate, for example). Thanks :)