Better status codes for PATCH requests

[alabou]

The current API uses the 501 Internal Server Error status for conditions that are not errors of the server or unexpected errors but known conditions that may arise in the use of the API.

I would propose using the following status code for the PATCH method for conditions that are allowed by the API and expected from some implementations.

405 Method Not Allowed Returned when the implementation does not support the annotation API for a specific resource. Client shall not retry the request.

422 Unprocessable Content Returned when the implementation does not support the annotation API request: string too long, read-only tag, too many tags values, too many tags, etc. Client shall not retry the request. It shall modify the request based on the detailed status message before retrying.

507 Insufficient Storage Returned when the implementation does not have more space for storing annotations. Client shall not retry the request.

[garethsb]

I think this is worth considering.

I'm not sure about the use case for 405 Method Not Allowed. Why not, if the Annotation API is not supported for a resource, just don't include it in the API?

422 Unprocessable Content is not used in any published AMWA NMOS spec, but is proposed to be used in IS-11. I think it could be appropriate.

507 Insufficient Storage is not defined in the HTTP spec but in the WebDAV spec, but I don't think that should stop us using it if the semantics are right - we also use 423 Locked defined in that spec.

We should make clear about the distinction between "string too long, too many tags values, too many tags, etc." and "does not have more space"... I think it's the difference between (unstated/undiscoverable because we haven't added an endpoint to the API) constraints in a spec of a tag or the spec of this product, vs. current lack of persistent storage, right?

[peterbrightwell]

Discussion 2023-06-01:

API implementation is responsible for which resources are supported, so 404 would do for cases where the Annotation API isn't supported. If it's a dynamic resource and can't be annotated, don't put it in the API. IS-13 provides a list of resources on a GET. These are not guaranteed to still exist on a a latter request, which is probably ok (and is consistent with the non-atomic nature of much of NMOS). See also #9. WA: 405 not needed.

422 and 507 ideas accepted. RAML description states (without normative language) when a particular status code is used, and the documentation says "Where the RAML specification of an API specifies explicit response codes it is expected that a client will handle these cases in a particular way. "

We should set a minimum requirement for length of string etc that implementations must support. This will help testing. Action all to agree this.

Action @garethsb draft a PR and all consider implementation. Consider what it might mean for e.g. an Ansible playbook for annotations.

[garethsb]

I'm looking at the existing language:

Read-Only Tags

Some named tags, such as those defined by BCP-002-01 and BCP-002-02, or by a particular manufacturer, are intended to have read-only values assigned to the resources by the manufacturer.

An API implementation MAY therefore reject requests to update specific named tags, with a 500 (Internal Server Error) response.

A PATCH request with tags set to null does not update read-only tags.

Additional Limitations

Some named tags have additional limitations, such as needing precisely one element in the array of values. An API implementation SHOULD reject requests which do not meet the additional limitations specified for such tags, with a 500 (Internal Server Error) response.

An API implementation MAY have additional limitations such as:

• maximum length of strings used as labels, descriptions or tag values
• maximum number of values for each named tag
• maximum number of tags (per resource)
• maximum total size of annotations per resource or across all resources

The API implementation MAY reject requests which it cannot process, with a 500 (Internal Server Error) response.

How would we divvy up these different limitations to 422 (Unprocessable Content) or 507 (Insufficient Storage)?

If they are limitations on particular tags, due to a specification for that tag, e.g. that urn:x-nmos:tag:asset:manufacturer/v1.0 is read-only, or that urn:x-manfacturer:tag:foo must meet a certain syntax or must have precisely one value, then the client error 422 would seem appropriate, even if a client couldn't be expected to know the specification of every tag that it came across.

How about maximum lengths of strings for labels or descriptions or urn:x-nmos:tag:user: tags? Or a general limitation that the vendor/device only supports single values per tag? These seem a little different, arbitrary vendor decisions, somehow?

Then there's the "maximum total size of annotations per resource or across all resources" (two separate possible conditions) for which server error 507 might be appropriate.

What do you think, @alabou @cristian-recoseanu ?

We do definitely need @timhall99 user guidance as well as vendor input on whether/what minimums we should write into the spec for label, description and tag value supported lengths (as well as actually, tag name length!) and numbers of values per tag, numbers of tags per resource, etc.

[alabou]

• For 507 I think it is not possible to know if it is per resource or total. It seems to imply that the user of the API will have to assume per-resource failure and continue and attempt to annotate the other resources. This error shall indicate that nothing other than deleting something from the resource would allow successful annotation requests.

• For 422 A user of the API would need to annotate only one item in a PATCH in order to figure out what is the limitation. It works well for string length, number of values for a tag, number of tags BUT it does not work well for read-only tags unless we consider that a failure to add a user tag with an empty array as a value indicate too many user tags while a failure to add a non-user tag with an empty array as a value would indicate either an attempt to annotate a read-only tag or too many tags.

• As a vendor I would not allow the annotation of a tag having a special syntax requirement as I don't think I want to run schemas verification or the like to check the syntax at runtime.

discovering limitations:

• for a label, description: annotate item with a string from 0 to N characters until getting 422 or 507. If getting 422 for a string of length 0 it means that the item cannot be annotated, else get the maximum number of characters (Max1) allowed for annotating item (422, 507).

• for non-user tags: annotate tag with an array of 0 to N empty strings until getting 422 or 507, If getting 422 for a count of 0 it means that the tag is read-only (422) or that there are too many tags (422, 507), else get the maximum number of values (Max2) allowed for annotating the tag (422, 507). Annotate each element of the array with a string from 0 to N character until getting 422 or 507. If getting 422 for a string of length 0 it is an unexpected error, else get the maximum number of characters (Max3) allowed for annotating each element (422, 507). Annotate a single element of the array with a string from 0 to N characters until getting 422 or 507. If getting 422 for a string of length 0 it is an unexpected error, else get the maximum number of characters (Max4) allowed for annotating a single element (422, 507). If Max4 is larger than Max3 the implementation is sharing storage across the elements of the array.

• for user tags: same as above except that read-only condition is not possible.

[peterbrightwell]

Update 2023-07-13: no discussion this time. We need to resolve next week when @alabou is available.

[peterbrightwell]

Update 2023-07-20: we can leave as only 500 in this first phase, no need to delay this iteration. For many use cases an error message will be useful as there will be a user on hand and no need for automated action to deal with specific error cases.