search

jokiefer / drf-json-api-atomic-operations

extensions to support atomic operations
https://drf-json-api-atomic-operations.readthedocs.io
MIT License
6 stars 3 forks source link

Media Type value is incorrect #2

Open Roanmh opened 1 month ago

Roanmh commented 1 month ago

The media type

ATOMIC_MEDIA_TYPE = 'vnd.api+json;ext="https://jsonapi.org/ext/atomic'

is incorrect. This value forces the client to provide the header accept: vnd.api+json;ext="https://jsonapi.org/ext/atomic which is unexpected. Notice the missing ".

The user should be able to provide accept: vnd.api+json;ext="https://jsonapi.org/ext/atomic" but this currently results in a 406 error.

In addition, it would be helpful to note that the usual value for the accept header */* is not valid for this view. The client must provide the ext parameter.

jokiefer commented 1 month ago

After crawling the json:api spec v1.1 again, it seems to me, that the correct naming would be vnd.api+json; ext="https://jsonapi.org/ext/atomic". Notice the missing space-separated (U+0020 SPACE, “ “) described in 5.4. The example leads in to a wrong documented content-type. The Content-Type is not explicit defined inside the extension docs. It also not needed, cause 5.4 from the v1.1 specs is well defined for this case i think.

@sliverc am i right at this point?

In addition, it would be helpful to note that the usual value for the accept header / is not valid for this view. The client must provide the ext parameter.

I do not handle the accept header in my package explicitly. So it seems not a problem of this package.

Roanmh commented 1 month ago

Thanks for the relevant spec sections, I didn't find those.

As for the compatibility with accept: */*: The revelant code is in django-rest-framework. I'm not sure if the behavior is desired or not; The logic requires the client to provide all the parameters (ext="foo") of the renderer before it considers the wildcards.

I can open a bug there if we think this behavior isn't desired, but either way it seems like it would be helpful to document in this library because most developers won't have had to think about the accept header until using this library.

sliverc commented 1 month ago

I am bit confused about the space. In the spec 5.4 it states there needs to be a space when there is more than one ext or profile defined. In the example of 5.4.1 there is clearly a space although there is only one ext. But in the atomic documentation there is no space. Best ask upstream what the right content type is, as this is certainly a bug in the specification one way or the other.

In terms of Accept */* have a look at the DRF documentation where it explains how */* is interpreted.

Roanmh commented 4 weeks ago

If a client underspecifies the representations it can accept, such as sending an Accept: / header, or not including an Accept header at all, then REST framework will select the first renderer in the list to use for the response

This makes it seem like the behavior i found was undesired. I started a discussion about it.

Roanmh commented 4 weeks ago

wrote an issue about our question regarding the media type syntax