zalando / zally

A minimalistic, simple-to-use API linter
https://zalando.github.io/zally
MIT License
905 stars 145 forks source link

JSON Patch is not considered a standard media type #950

Closed whiskeysierra closed 5 years ago

whiskeysierra commented 5 years ago

Screenshot from 2019-03-14 15-43-31

https://tools.ietf.org/html/rfc6902

I guess the same applies to JSON Merge Patch.

roxspring commented 5 years ago

This is an implementation of guideline 172 where the wording is very restrictive in what mime types should be allowed, clearly excluding application/json-patch+json and application/merge-patch+json:

Instead, just use the standard media type name application/json (or application/problem+json for 176)

Shouldn't the change be negotiated in https://github.com/zalando/restful-api-guidelines before considering a code change here?

(btw modifying the check to allow these is really easy, assuming the guidelines team agree to the change)

whiskeysierra commented 5 years ago

This is an implementation of guideline 172 where the wording is very restrictive in what mime types should be allowed, clearly excluding application/json-patch+json and application/merge-patch+json:

You're quoting rule 172 out of context. That part clearly refers to the previous allowed usage of custom media types:

Previously, this guideline allowed the use of custom media types like application/x.zalando.article+json. This usage is not recommended anymore and should be avoided

In addition the guidelines clearly mention both patch media types:

  1. use PATCH with partial objects to only update parts of a resource, whenever possible. (This is basically JSON Merge Patch, a specialized media type application/merge-patch+json that is a partial resource representation.)

  2. use PATCH with JSON Patch, a specialized media type application/json-patch+json that includes instructions on how to change the resource.

https://opensource.zalando.com/restful-api-guidelines/#patch

Summary of my perspective:

Allowing for both patch media types is not just a logical consequence, since they are explicitly mentioned they are allowed.

The only thing that I'd change in the guidelines is the explicit reference of application/problem+json in rule 172. It should rather state something like:

Instead, just use the standard media type name application/json (or any standardized variant as dictated by other rules)

We may want to refer to https://tools.ietf.org/html/rfc6838.

roxspring commented 5 years ago

For me the persuading bit of additional context is this:

  • 148 allows and mentions both media types

PR #982 should introduce the Zally change. I'll leave clarifying the guideline as an exercise for somebody else.