Review new behavior of the LintDiff PatchBodyParameterSchema rule

[konrad-jamrozik]

In this PR @mikekistler fixed a bug with the rule:

• Fix patch body schema azure-openapi-validator#661

This led to the rule behavior change, making it more strict.

We need to review its new behavior.

Relevant Teams discussion

Relevant comment:

• Codesigning RP: adding first 2024-02-05 public preview version  azure-rest-api-specs#27888 (comment)

[konrad-jamrozik]

@mikekistler could you provide more context on the new rule behavior?

[mikekistler]

Previously the rule checked a PATCH request body for any properties that are 1) required, 2) have a default value, or 2) have x-ms-mutability of ["create"].

My change simply enforces these same rules on nested properties as well.

The updated rule flagged a PATCH operation that, two-levels deep in the body, has a "sku" property whose schema is an object with a single property, "name", that is required.

I think that flagging a required property anywhere in a PATCH request body is consistent with the spirit of this rule.

[rkmanda]

yup, agree that Patch request body must not have a required property at any level

[rkmanda]

I actually have a question that popped up regarding this. When a property is marked required at a particular nesting level in swagger, does it actually make it required for the entire payload or does it make it required only when the parent property is present on the wire? I will check what the open API spec language says about this and get back. @konrad-jamrozik , @mikekistler

[rkmanda]

[rkmanda]

However, from a patch standpoint, the argument is that marking a property "required" at a particular level, makes it mandatory for that property to always be specified when a Patch is being used to update its siblings. So I guess we are still good with the change we made. One issue I see with this is a case where a set of properties are always required together at the same level, then theres no way to express that for Patch APIs.

[mikekistler]

@rkmanda I had to think through this exact scenario too. I think the general principle is that you can't use validation on the request payload alone to ensure that the request is valid -- it will depend on the state of the service as well. These are just special cases of this.

[konrad-jamrozik]

This rule is causing some trouble. See email thread with subject: RE: [LintDiff] common-types causes PatchBodyParametersSchema.

[mikeharder]

Spec authors continue to be confused by failures of this rule, and are starting to call the errors "false positives":
#7802

@markcowl: In Azure/typespec-azure#383, you seem to suggest this is a bug in typespec-autorest, rather than azure-openapi-validator. If so, I think this issue and #7802 should both be closed, and we track this only as a bug in TypeSpec? Do you agree?

If so, can we prioritize fixing the bug in TypeSpec to reduce confusion? Or is there anything we can improve in azure-openapi-validator to help, maybe a better error message?

[mikekistler]

+1 to closing this issue and #7802.

Fixing the TypeSpec issue will help but I expect some teams will still get tripped up on this.

[konrad-jamrozik]

@mikekistler so if I understand correctly your recommendation is:

• Close as "nothing more to do here" the following issues:
  • Review new behavior of the LintDiff PatchBodyParameterSchema rule #7771
  • Swagger LintDiff false positives for PatchBodyParametersSchema #7802
• Consider the new strict behavior as given by Fix patch body schema azure-openapi-validator#661 as the correct one.

?

Question 2: what do you mean by:

but I expect some teams will still get tripped up on this.

even after TypeSpec fixes this?

[mikeharder]

From this discussion with @markcowl:

https://teams.microsoft.com/l/message/19:4f661242c446452e895359cc3ef45125@thread.tacv2/1718209098071?tenantId=72f988bf-86f1-41af-91ab-2d7cd011db47&groupId=0cab4ce9-7691-42ae-82e3-460d4346a710&parentMessageId=1718209098071&teamName=ARM%20API%20Reviewers&channelName=ARM%20linter%20rules&createdTime=1718209098071

I believe the root cause is between azure-openapi-validator and the swagger in common-types. According to Mark, this issue can impact specs using either TypeSpec or handwritten-swagger, so I think we can simplify by removing TypeSpec from the repro.

I believe the root cause can be summarized as "Rule PatchBodyParameterSchema is incompatible with the shared swagger in common-types"? So either the rule or common-types needs to change. We think the correct fix is to change common-types, but as a temporary workaround, reverting the rule change will be easier.

[mikekistler]

I'm skeptical that all the instances of this rule firing are caused by the common-types.

And when they are caused by the common types, can't we simply use suppressions until the common types are fixed?