Breaking Changes PR check not flagging type change as breaking

[mikekistler]

The Breaking Changes PR check in the azure-rest-api-specs repo is failing to flag a type change as breaking and requiring a Breaking Change review.

REST API PR: Azure/azure-rest-api-specs#19468

The Breaking Changes-Cross Version PR check issued these two warnings for change of type:

Here is one of the changes:

Azure/azure-rest-api-specs@ed2c211

According to the Azure Versioning and Breaking Change Policy, Table 3 (Changes requiring a review with Azure’s breaking Change Review Board), this should be reported as an error and require review/approval by the Azure Breaking Changes review board before the PR can be merged.

cc: @JeffreyRichter @srmantha

[mikekistler]

@akning-ms Please give this top priority as it is allowing breaking changes to evade review and impact customers.

[akning-ms]

@jianyexi to triage the issue

[jianyexi]

I think it's because we flagged this rule as waring temporary. The background is that
currently there are a lot of schema definitions which are actually objects but don't contain the 'type:object' and autorest team want to differentiate the schema with or without 'type:object', as they have different meaning. Therefore, we added a lint rule to force the 'type:object'. But fixing it will cause a breaking change false alarm and a lot of service teams complain about it and considering the SDK breaking change already reported it, we marked this rule as warning. I think it's time to change it back to 'error' again to fix this risk

[mikekistler]

Can we special-case adding type: object -- make that a warning -- but return to reporting a change of type to be an error?

[jianyexi]

Can we special-case adding type: object -- make that a warning -- but return to reporting a change of type to be an error?
But if we honor the autorest/OAS2 semantic, then adding 'type: object' is an 'error', so we don't need to do any change on the current implementation ,we just need to change it back to 'error' in the configuration without any code change

[mikekistler]

Are you suggesting that autorest does something different when a schema has type: object vs when it does not? I know that technically these are different, as discussed in this issue. Maybe even more specifically -- if a schema has "properties", does autorest behavior change if type: object is added? If not then I think a warning is sufficient for this special case.

[jianyexi]

if a schema has "properties", does autorest behavior change if type: object is added?

yes, actually it's motivation that autorest want to add the linting rule , after adding 'type:object' , the model would be a object, but if without 'type:object' it would be an any type

[mikekistler]

@jianyexi But I really don't think the code generated by autorest changes when type: object is added.

I just tested this with the appconfiguration service in the dotnet mgmt SDK (link to test branch). In my test, I removed type: object from many of the schemas that also have properties (commit b1f253d). Then I updated the autorest.md to point to this version and regenerated the client library. The regeneration threw a bunch of warning messages like:

      - https://github.com/mikekistler/azure-rest-api-specs/blob/b1f253d70547f975650612226919c73da808b015/specification/appconfiguration/resource-manager/Microsoft.AppConfiguration/preview/2021-10-01-preview/appconfiguration.json:1203:5
  warning | PreCheck/SchemaMissingType | The schema 'ConfigurationStoreProperties' with an undefined type and declared properties is a bit ambiguous. This has been auto-corrected to 'type:object'
      - https://github.com/mikekistler/azure-rest-api-specs/blob/b1f253d70547f975650612226919c73da808b015/specification/appconfiguration/resource-manager/Microsoft.AppConfiguration/preview/2021-10-01-preview/appconfiguration.json:1251:5
  warning | PreCheck/SchemaMissingType | The schema 'ConfigurationStoreUpdateParameters' with an undefined type and declared properties is a bit ambiguous. This has been auto-corrected to 'type:object'

but there was no change to the actual generated code (see the branch I pushed here).

Is there some other language or perhaps dotnet dataplane that does generate different code based on whether type: object is present or absent in a schema that has defined properties?

[jianyexi]

I think it has no change today is because of below flag in the M4 ,https://github.com/Azure/autorest/tree/main/packages/extensions/modelerfour

[mikekistler]

Perfect. So please treat this as a special case -- adding type: object next to properties is only a warning -- and all other additions or changes of type are error. We need this fix ASAP.