Make READONLY_PROPERTY_NOT_ALLOWED_IN_REQUEST rule less confusing

[mentat9]

Current READONLY_PROPERTY_NOT_ALLOWED_IN_REQUEST rule disallows RO properties in PUT/PATCH examples. The message indicates the RO properties should be removed, but implies that the spec is invalid and RO properties should be removed from the spec, when actually RO properties are valid and recommended in the spec.

Either the rule should be changed to allow RO properties in examples, or the message should be clarified to indicate that the error is that examples are expected to avoid RO properties in PUT/PATCH payloads, even though such requests are valid.

See below context for additional information.

The OAV code for this error is implemented at https://github.com/Azure/oav/blob/develop/lib/swaggerValidator/ajvSchemaValidator.ts#L464 I don’t think we currently have anyone super familiar with the code but you are welcome to take a look. Also, if you feel there can be some improvement, please file an issue in that repo with some details of what you feel would could be improved.

Thanks,
Wes

From: Chris Stackhouse <[email protected]>
Sent: Tuesday, July 11, 2023 2:41 PM
To: Wes Haggard <[email protected]>; Johan Stenberg <[email protected]>; Roopesh Manda <[email protected]>; Jeffrey Richter <[email protected]>; Lauren Jernigan <[email protected]>; Konrad Jamrozik <[email protected]>
Subject: RE: IBM Power Vm Swagger

Thank-you Wes:

@Lauren, this should unblock you.

I think the issue here is that multi-request examples are not supported in swagger (AFAIK), but the scenarios in question depend on multiple requests. Maybe it’s OK to flag these examples, since they don’t make a lot of sense as a single isolated request.

OTOH, as you said below we can’t tell which is correct, only that the examples mismatch the model. Except in this case, the examples actually match the model making the rule doubly confusing. It’s telling the author to remove the RO properties, but from what? The examples, or the spec?

I guess the main question in my mind is whether we could leverage messaging to at least make the issue clearer, even if we decide not to change the validation rule. I don’t know how the rule is implemented, but if it’s based on matching the model, then it must already have logic that overrides for this specific RO case. Maybe when that happens it could indicate that examples are expected to avoid RO properties even though they are allowed by the model…?

Thoughts?

Thanks,
-=Chris
From: Wes Haggard <[email protected]>
Sent: Tuesday, July 11, 2023 2:36 PM
To: Johan Stenberg <[email protected]>; Roopesh Manda <[email protected]>; Chris Stackhouse <[email protected]>; Jeffrey Richter <[email protected]>; Lauren Jernigan <[email protected]>; Konrad Jamrozik <[email protected]>
Subject: RE: IBM Power Vm Swagger

For the error messages it does look like it is pointing at the correct place in the sample but interestingly enough for the spec it is pointing at the correct line 14 but in the wrong file (https://github.com/Azure/azure-rest-api-specs/blob/main/specification/common-types/resource-management/v5/types.json#L14), as that definitions is coming from the common-types and not the spec. I don’t know how difficult that is to fix but it is something we should consider improving.

As for this particular PR it sounds like the right thing to do is remove those readonly fields from the examples.

Thanks,
Wes

From: Johan Stenberg <[email protected]>
Sent: Tuesday, July 11, 2023 1:51 PM
To: Roopesh Manda <[email protected]>; Chris Stackhouse <[email protected]>; Jeffrey Richter <[email protected]>; Wes Haggard <[email protected]>; Lauren Jernigan <[email protected]>; Konrad Jamrozik <[email protected]>
Subject: Re: IBM Power Vm Swagger

It is the latter/intended to illustrate correct usage of the REST API. More specifically validation was originally put in place to make sure that the examples given do not contradict the schema definition given in the service specification.

I am curious; what is the purpose/customer value of documenting request properties that the service is going to ignore? For historical context, these rules have been in place for as long as I can remember (e.g. 5+ years).

I don’t know why the error messages point to the wrong line, but they do point to both the example and the model. It is not possible to, through static analysis, determine which one is correct – we only know that the example and the model schema contradict each other.

From: Roopesh Manda <[email protected]>
Date: Tuesday, July 11, 2023 at 1:38 PM
To: Chris Stackhouse <[email protected]>, Johan Stenberg <[email protected]>, Jeffrey Richter <[email protected]>, Wes Haggard <[email protected]>, Lauren Jernigan <[email protected]>, Konrad Jamrozik <[email protected]>
Subject: RE: IBM Power Vm Swagger
I agree that its confusing. Are the examples meant to simulate the SDK behaviors or the direct REST usage. I always thought it’s the latter and if we agree on that, then the examples should allow readOnly properties to be specified in request payloads?

Roopesh

From: Chris Stackhouse <[email protected]>
Sent: Tuesday, July 11, 2023 1:04 PM
To: Johan Stenberg <[email protected]>; Jeffrey Richter <[email protected]>; Roopesh Manda <[email protected]>; Wes Haggard <[email protected]>; Lauren Jernigan <[email protected]>; Konrad Jamrozik <[email protected]>
Subject: RE: IBM Power Vm Swagger

OK, thanks for digging into this.

Just want to be sure I understand: the model SHOULD specify the read-only properties, but in they are DISALLOWED in the examples.

If that’s correct, I find it pretty confusing and unexpected. Is there a way we could make the messages clearer?

When applying my feedback, Lauren reasonably concluded the model was being flagged, since the examples matched the model (also, from what I saw, the locations the errors pointed at were unrelated parts of the spec, not the examples).

Thanks,
-=Chris
From: Johan Stenberg <[email protected]>
Sent: Tuesday, July 11, 2023 12:32 PM
To: Jeffrey Richter <[email protected]>; Roopesh Manda <[email protected]>; Chris Stackhouse <[email protected]>; Wes Haggard <[email protected]>; Lauren Jernigan <[email protected]>; Konrad Jamrozik <[email protected]>
Subject: Re: IBM Power Vm Swagger

A client is expected to not include readOnly properties in requests (that is the semantic as defined in OpenApi). As @Jeffrey Richter called out, in order to simplify life for direct REST usage, we recommend that including client side unmodified/untouched values should not trigger a service error.

This holds true for ARM as well as for data plane.

The validation rule is correct. If you include readOnly properties in your request examples, it signals that the property is, in fact, not readOnly, but it has some semantic meaning in requests. If the value is simply ignored by the service, the example should not show it sent in the request.

From: Jeffrey Richter <[email protected]>
Date: Tuesday, July 11, 2023 at 12:15 PM
To: Roopesh Manda <[email protected]>, Chris Stackhouse <[email protected]>, Wes Haggard <[email protected]>, Lauren Jernigan <[email protected]>, Konrad Jamrozik <[email protected]>, Johan Stenberg <[email protected]>
Subject: Re: IBM Power Vm Swagger
Data Plane guidance is for the swagger to define 1 (and only 1 model) for POST/PUT/PATCH input/output and GET output so that 1 model is defined by SDKs and all these operation methods accept/return this 1 model.
And yes, this allows for the round trip scenarios: GET--> modify --> PATCH/PUT (for update/replace respectively).

We recommend that clients/SDKs send read-only fields to the service and that the service return a 400-BadRequest if a passed readonly field value doesn't match what the resource already has.
This lets the client know that the request is bad if they try to CHANGE a readonly value but it allows the model returned from GET to be passed as input to other operations.

In addition, many swaggers have incorrectly marked fields as readonly when they are not or vice versa. Some SDK languages strip the readonly fields from the JSON during serialization. If a field is incorrectly marked as read-only, this results in data loss as the SDK won't ever send the value to the service. I've asked all the SDK languages to change this behavior and to always send fields marked as read-only - this will require that services not return 400 if the read-only value is the same or it is breaking. For the SDK languages that do not strip read-only fields, so far, there have been no customer issues reported. Services may be ignoring the read-only fields entirely regardless of their value - this would not be ideal as customers would think they are setting a value which is not being set -- this would be considered data corruption.

• Jeffrey Richter (see Architecting Distributed Cloud Apps & Designing HTTP APIs)

Azure API Stewardship: Guidelines | Review process | Recordings | Calendar | Join the Interest Group
Azure SDK Architecture: Guidelines | Review process | Recordings | Calendar | Join the Interest Group

---

From: Roopesh Manda <[email protected]>
Sent: Tuesday, July 11, 2023 12:04 PM
To: Chris Stackhouse <[email protected]>; Wes Haggard <[email protected]>; Lauren Jernigan <[email protected]>; Konrad Jamrozik <[email protected]>; Johan Stenberg <[email protected]>; Jeffrey Richter <[email protected]>
Subject: RE: IBM Power Vm Swagger

ARM guidelines allow for readonly properties to be specified in the request payloads for the reason Chris mentioned. Adding @Jeffrey Richter to check if this aligns with data plane guidance and if so, we can make the change in the model validation to remove this check.
Open API Specifications Guidelines | ARM Wiki

Thanks
Roopesh

From: Chris Stackhouse <[email protected]>
Sent: Tuesday, July 11, 2023 12:01 PM
To: Wes Haggard <[email protected]>; Lauren Jernigan <[email protected]>; Konrad Jamrozik <[email protected]>; Johan Stenberg <[email protected]>; Roopesh Manda <[email protected]>
Subject: RE: IBM Power Vm Swagger

+@Roopesh Manda for visibility

The scenario GET--PUT is a core scenario for ARM resource types and should always be supported/allowed.

For some reason, having the same payload in GET, PUT request and PUT response in this PR it’s getting flagged, even though it’s the “normal” way to declare ARM resource types.

Thanks,
-=Chris
From: Wes Haggard <[email protected]>
Sent: Tuesday, July 11, 2023 11:36 AM
To: Chris Stackhouse <[email protected]>; Lauren Jernigan <[email protected]>; Konrad Jamrozik <[email protected]>; Johan Stenberg <[email protected]>
Subject: RE: IBM Power Vm Swagger

I definitely don’t have a lot of the history on the linting rule but I think it is flagging things correctly here for the examples. Is there any reason these read-only fields cannot be removed from the example files?

@Johan Stenberg what is your take on this validation rule? Should the linter not be flagging readonly properties in the request?

Thanks,
Wes

From: Chris Stackhouse <[email protected]>
Sent: Tuesday, July 11, 2023 10:54 AM
To: Lauren Jernigan <[email protected]>; Wes Haggard <[email protected]>; Konrad Jamrozik <[email protected]>
Subject: RE: IBM Power Vm Swagger

Ugh, sorry. This is the PR link: IBM Power swagger. by ljernigan · Pull Request #12766 · Azure/azure-rest-api-specs-pr (github.com).

Thanks,
-=Chris
From: Chris Stackhouse
Sent: Tuesday, July 11, 2023 10:54 AM
To: Lauren Jernigan <[email protected]>; Wes Haggard <[email protected]>; Konrad Jamrozik <[email protected]>
Subject: RE: IBM Power Vm Swagger

+@Wes Haggard, @Konrad Jamrozik - This isn’t expected: it looks like something is incorrectly triggering these failures.

When Lauren includes the ARM resource envelope in the PUT request, it causes these linter errors.

Any ideas?

Thanks,
-=Chris
From: Lauren Jernigan <[email protected]>
Sent: Tuesday, July 11, 2023 7:37 AM
To: Chris Stackhouse <[email protected]>
Subject: RE: IBM Power Vm Swagger

Hi Chris,

This is the error I get from the swagger model validation when I made the request and response the same. Let me know if I’m just implementing this incorrectly, but I’m not sure how to get around these errors.

Lauren J.

From: Chris Stackhouse <[email protected]>
Sent: Monday, July 10, 2023 8:44 PM
To: Lauren Jernigan <[email protected]>
Subject: RE: IBM Power Vm Swagger

Sorry about that. Not sure what’s behind it: that shouldn’t be disallowed unless there was a linter bug.

Can you try it again to see specifically which linter failures it triggers?

Thanks,
-=Chris
From: Lauren Jernigan <[email protected]>
Sent: Monday, July 10, 2023 1:41 PM
To: Chris Stackhouse <[email protected]>
Subject: RE: IBM Power Vm Swagger

Hi Chris,

I initially tried to make them the same based on your advice. When I switched to using tracked resource for both response and request, the swagger validations gave me an error and said you could not put read only properties in the request. I understand that our backend could filter these fields out, but is there a way to pass all the swagger validations with a read only field in the request?

Lauren J.

From: Chris Stackhouse <[email protected]>
Sent: Monday, July 10, 2023 4:04 PM
To: Lauren Jernigan <[email protected]>; Azure Resource Manager RP API Review <[email protected]>
Subject: RE: IBM Power Vm Swagger

Hi Lauren:

It’s normal and expected that PUT request and response payloads for ARM resource type CRUD operations have the same definitions. The backend should ignore read-only property values that arrive in the PUT request.

A client must be able to do a GET, modify a property in the response payload and then PUT it to make an update without needing to understand which properties are read-only and filter them out.

Thanks,
-=Chris
From: Lauren Jernigan <[email protected]>
Sent: Thursday, July 6, 2023 7:00 AM
To: Azure Resource Manager RP API Review <[email protected]>
Subject: IBM Power Vm Swagger

Hi Chris,

I had Roopesh look over my pr last week, and he also wanted me to use the same definition for request and response on put, but I had some issues. When you use the tracked resource for the requests, those fields are read only. You can't put read only fields in the request unless there's a work around that I'm unaware of. Logically I was confused how the request and response could be the same. Our response includes fields such as provisioning state and sub interface id, which haven't been assigned when the request is sent. Does it still make sense for the request and response to be the same in this case? Let me know if any other changes are needed for the pr.
https://github.com/Azure/azure-rest-api-specs-pr/pull/12766

Thanks,
Lauren J.

[mikeharder]

@rkmanda has suggested disabling this rule completely, at least for resource-manager specs, and possibly for data-plane as well.

Latest Teams conversation:
https://teams.microsoft.com/l/message/19:[email protected]/1723558290666?tenantId=72f988bf-86f1-41af-91ab-2d7cd011db47&groupId=0cab4ce9-7691-42ae-82e3-460d4346a710&parentMessageId=1723558290666&teamName=ARM%20API%20Reviewers&channelName=ARM%20swagger%20reviews&createdTime=1723558290666