New error says "shall" where spec says "should"

DMTF / Redfish-Protocol-Validator

The Redfish Protocol Validator tests the HTTP protocol behavior of a Redfish service to validate that it conforms to the Redfish specification.

Other

14 stars 12 forks source link

New error says "shall" where spec says "should" #66

Closed blakehilliard closed 11 months ago

blakehilliard commented 11 months ago

In the last couple of days, I've started getting this new error: REQ_PATCH_ODATA_PROPS: "A client only provides OData annotations: Services shall return the HTTP 400 Bad Request status code with the NoOperation message from the Base Message Registry or one of the modification success responses."

But when I look for this in the spec, I see "shall" not "should". Maybe it's OK to still flag errors for services not doing what the spec says they "should" do, but the spec does make a clear distinction between the meaning of "shall" and "should".

mraineri commented 11 months ago

Do you have the test report? Are you returning 400 Bad Request with a message other an NoOperation? The passing criteria for this test is..

400 Bad Request with NoOperation
200 OK
202 Accepted
204 No Content

The reason it's a "should" is older versions of the spec would map this type of request to a 200 OK due to the "The service shall ignore OData annotations in the request body", meaning that the client essentially told the service to do nothing. Since then, we added in a "should" to avoid this fall-through case to give guidance that you really should call out the client for only giving OData annotations.

mraineri commented 11 months ago

Looking at the spec history a bit more...

In 1.0, providing only OData annotations would only map to 200 OK since all OData annotations are ignored.
In 1.7.0, we introduced stronger language, if you only provide OData annotations, it's either 400 Bad Request with NoOperation OR a success response (leaving compatibility with older versions of the spec).
In 1.13.0 (or one of the errata versions of 1.12), we wanted to remove the monolithic "exception" tables in the PATCH clause, and inadvertently opened some new interpretations here.

Really the spec's intent here is a "shall" for either 400 Bad Request (with NoOperation) or the less preferred "success" response.

blakehilliard commented 11 months ago

Are you returning 400 Bad Request with a message other an NoOperation?

Yes.

mraineri commented 11 months ago

That's not correct behavior. Going back to the 1.12.0 text, this is what the spec states:

We did a cleanup of this section in 1.12.1 to make it more readable and inadvertently introduced a new opening for interpretation. We had no intent to loosen the language for this exception case.

mraineri commented 11 months ago

Closing; test is functioning as designed and the requirement on the service is correct.