Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Media Type value is incorrect #2

Open
Roanmh opened this issue Oct 17, 2024 · 5 comments
Open

Media Type value is incorrect #2

Roanmh opened this issue Oct 17, 2024 · 5 comments
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@Roanmh
Copy link

Roanmh commented Oct 17, 2024

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
Copy link
Owner

jokiefer commented Oct 17, 2024

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.

@jokiefer jokiefer added bug Something isn't working good first issue Good for newcomers labels Oct 17, 2024
@Roanmh
Copy link
Author

Roanmh commented Oct 17, 2024

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
Copy link

sliverc commented Oct 18, 2024

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
Copy link
Author

Roanmh commented Oct 18, 2024

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
Copy link
Author

Roanmh commented Oct 20, 2024

wrote an issue about our question regarding the media type syntax

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants