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

json request for certain media types fail #2206

Closed
iherman opened this issue Aug 15, 2017 · 15 comments
Closed

json request for certain media types fail #2206

iherman opened this issue Aug 15, 2017 · 15 comments

Comments

@iherman
Copy link

iherman commented Aug 15, 2017

Long story short

The resp.json() call reports a bug if the return type is something like application/XXX+json (as opposed to application/json). However, application/XXX+json is a perfectly fine media type, "extending" the semantics (not the syntax) of the core json syntax, ie, should be parsed as bona fide json...

Your environment

OS X 10.11.6, Python 3.6.2

@jscholes
Copy link

You can pass a custom content_type parameter to the ClientResponse.json method, although I agree that this isn't perhaps the most sensible API. I just switched some code from using Requests to using this library and while Requests could decode the JSON without a problem, aiohttp reported an error which will trip some people up.

@asvetlov
Copy link
Member

Related to #2174

@asvetlov
Copy link
Member

Extending the list of accepted json content types is fair easy change.
Please propose a Pull Request. application/XXX+json is not acceptable but I pretty sure you could grab a list of wide spread json types from requests library.

@asvetlov asvetlov added this to the 3.0 milestone Oct 19, 2017
@wuan wuan mentioned this issue Dec 6, 2017
5 tasks
@asvetlov asvetlov modified the milestones: 3.0, 3.1 Feb 12, 2018
@asvetlov
Copy link
Member

use content_type=None for disabling the check

@iherman
Copy link
Author

iherman commented Feb 17, 2018

@asvetlov, unfortunately I missed the reply on Sep 11; probably my fault.

However, I am not sure I agree with your statement that "application/XXX+json is not acceptable". In my view, it is perfectly acceptable (and future proof) if the code simply ignores the XXX+ part, whatever XXX is. Looking at RFC 6839:

At the same time, using the suffix allows receivers of the media types to do generic processing of the underlying representation in cases where

  • they do not need to perform special handling of the particular semantics of the exact media type, and

  • there is no special knowledge needed by such a generic processor in order to parse that underlying representation other than what would be needed to parse any example of that underlying representation.

I.e., there is no reason for aiohttp to maintain whatever specific list. This is valid for any conversion step, not only for JSON.


Yes, I agree that using content_type=None can be done, but that is clearly just a hack...

@asvetlov
Copy link
Member

What to do?
On one hand we want to prevent processing non-JSON data in .json() method.
On other we don't want to be too strict.

Maybe you can pass a custom content type when fetching a data from your server?
Or create a custom json reading function on top of resp.read()?

@iherman
Copy link
Author

iherman commented Feb 18, 2018

@asvetlov,

The fundamental fact is that data coming with XXX+json is JSON. From the point of view of aiohttp that all that matters. A properly registered XXX+json media type means that the content is JSON but it may have some additional syntactic or structural constraint within the framework of JSON. This may mean imposing a particular JSON Schema, or some more complicated specification on top of what JSON means, or may be no extra syntactic restriction whatsoever.

To take a specific example. application/ld+json is a bona fide media type defined through the structural syntax; the content is defined via the JSON-LD Recommendation. That specification adds some additional semantics to individual terms, requires some specific structure to the JSON data, but the essential point is that a JSON-LD content is a perfectly fine JSON content. (This is probably one of the most complex examples. Others may be a few paragraphs only.)

In my view, this means that the handling of the XXX+json (or XXX+xml, etc.) is simple, as far as aiohttp goes: strip the XXX+ string altogether (the + character is not allowed elsewhere in a media type name), and handle the content as json (or xml). You are done. All other processing is in the application domain.

(You will find an abundance of such media types on the media type registry.)

@asvetlov
Copy link
Member

You would say that any application/xxx+json is a valid JSON type.
Or parsing all JSON registered types from the media registry is better?

@webknjaz
Copy link
Member

I'd say both statements are true.

@webknjaz
Copy link
Member

By sending application/xxx+json client explicitly tells server to expect valid json payload and vice versa

@iherman
Copy link
Author

iherman commented Feb 19, 2018

@asvetlov : parsing the json types from the media registry is of course nicer and better but... it is not that obvious. Would aiohttp check that registry any time the "read" function is called? Would set up some sort of a local cache for the registry that has a finite lifetime? The registry is a changing target after all...

I believe just saying that "any application/xxx+json is a valid JSON type" is good enough.

@asvetlov asvetlov reopened this Feb 19, 2018
@asvetlov
Copy link
Member

Reopen the issue.
@iherman would you provide a Pull Request with the first option (accepting application/xxx+json)?

@iherman
Copy link
Author

iherman commented Feb 19, 2018

@asvetlov : I have no idea when I could do that; I have never looked into the internal of the aiohttp code, ie, it would take me quite a while to familiarize myself with it to produce a proper PR. I can try at some point unless somebody beats me into it, but it may take a while:-(

@iherman
Copy link
Author

iherman commented Feb 19, 2018

@asvetlov, I have looked at the code and I think I have found the few changes that are required. However, I simply will not have the time to make a proper pull request, ie, running all the necessary tests on the code overall. It would require a major setup on my machine, understanding how the test harness works, etc. Sorry, but that will not work.

I will look at the code again tomorrow just to be sure, and I could then forward you (tomorrow or on Wednesday) the file where I have made some changes: client_reqrep.py. Would that work?

@lock
Copy link

lock bot commented Oct 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants