Add json_response funciton

[asvetlov]

Should be derived from aiohttp.web.Response.

Constructor signature is:
def __init__(self, data, *, status=200, reason=None, headers=None)

Should pack data arg as json.dumps() and set content type to application/json.

People forget to specify proper content type on sending json data.

[sloria]

Any preference as to if/how the json module should be specified?

• Fallback

try:
    from simplejson as json  # or anyjson? ujson?
except ImportError:
    import json

• Class variable

import json

class JSONResponse(Response):
   json_module = json
    #...

[asvetlov]

I think constructor parameter dumps=json.dumps (like https://github.com/KeepSafe/aiohttp/blob/master/aiohttp/client_reqrep.py#L728) should be enough for the issue.

Another pull request may try to solve global configuration problem. See discussion for #336 also.

[sloria]

Any reason not to pass in the json_module=json, just in case we ever need to deserialize JSON.

Hypothetical use case:

class JSONResponse(Response):

    @classmethod
    def from_json(cls, json_string: str, **kwargs):
        return cls(data=self.json_module.loads(json_string), **kwargs)

[asvetlov]

I don't see use case for deserialization of response.
web.Request has .json() coroutine for deserializing request https://github.com/KeepSafe/aiohttp/blob/master/aiohttp/web_reqrep.py#L326

Different libraries may have different functions for unpacking data but I pretty sure all of them provides callable accepting dict and returning string.
Also I may want to use custom encoder/decoder: https://docs.python.org/3/library/json.html#encoders-and-decoders

[ludovic-gasc]

Hi,

If you could do something like that: https://github.com/Eyepea/API-Hour/blob/master/api_hour/plugins/aiohttp/__init__.py

It means we'll can remove this file: less code in API-Hour, better is ;-)

We use a lot this helper.

In this file, we have also an helper to output HTML files, be my guest to do something similar.

[asvetlov]

I'm not reluctant for adding HTMLResponse too.
Perhaps we need another issue for that.
@GMLudo would you file this one?

[jashandeep-sohi]

@asvetlov
Why not just leave JSON serialization to the user, like in HTMLResponse?
Like you said, the problem is that people forget to set content-type: application/json, not serialization.
IMHO, a simple class that forces the content-type to application/json should suffice.

[asvetlov]

@jashandeep-sohi I believe JsonResponse(json.dumps(data)) looks uglier than JsonResponse(data).

Advanced user may provide serializer is needed.

[kxepal]

Should pack data arg as json.dumps() and set content type to application/json.

I only note, that content-type should be easily customized and not just as constant value, but with the respect of Accept header. Cases:

1. When we develop API and use own JSON MIME type like application/vnd.my-company+json;
2. When we need to return text/plain for browsers in order to let us safely open API url in it and see the response instead of being asked to download some cryptic file. However, for explicit Accept application/json we must return expected application/json Content-Type. Tradeoffs for usability are hard.

These details makes life a bit complicated, but not too much.

[kxepal]

I believe JsonResponse(json.dumps(data)) looks uglier than JsonResponse(data).

Agree. It's also quite awkward that JsonResponse doesn't know how to transform data into JSON.

[jashandeep-sohi]

@kxepal
Well, neither will HTMLResponse, if that's ever added.

@asvetlov
Depends on you perspective, I suppose. I wouldn't say its uglier, but more verbose.

So, how would one go about returning a cached JSON string using this scheme?

[kxepal]

@jashandeep-sohi basically, all these *Response is about to get some data, pass it through render function and form HTTPResponse with proper related metadata (status, headers, payload etc.).

For JsonResponse this renderer is json.dump or whatever, for HTML one is jinja.render or special bridge to php or whatever. So both objects shares similar protocol and workflow. What you only need is to be able customize their behavior and be sure to not violate it.

[jashandeep-sohi]

Okay, so HTMLResponse would have a similar dumps argument to the constructor?

[asvetlov]

@jashandeep-sohi I take you proposal for adding dumps arg to HTMLResponse and a joke to stress your opinion.

But yes, we can relax parameter list a bit to make both content_type and data optional. User may override content type if needed and pass non data positional arg but text or even body named args.

Anyway, I feel we need Pull Request for further discussion.

Volunteers?

[jashandeep-sohi]

I take you proposal for adding dumps arg to HTMLResponse and a joke to stress your opinion.

Sorry, something must be getting lost in translation/typing, but I wasn't joking. I was genuinely asking how the HTMLResponse class would look like.

[asvetlov]

@jashandeep-sohi
Sorry. My English is a far from fluent.

What dumps parameter for HTMPResponse should do?
Rendering by jinja2 or mako requires a lot of work for setting up renderer (thus we've created aiohttp_jinja2 and aiohttp_mako libraries).
But for json you can use just dumps function from json, simplejson or ujson in most cases.
For complex usage scenarios you may create custom callable.

[jashandeep-sohi]

Okay, understood.

[popravich]

Hi, guys!
Sorry for stuck in the middle, but why would you need another class instead of using same technic as aiohttp_jinja2 use?
I mean to simply provide a shourcut for creating instance of Response class with proper headers set to proper values.

def json_response(data):
    resp = web.Response()
    resp.text = json.dumps(data)
    resp.content_type = 'application/json'
    return resp

[asvetlov]

@popravich I feel no big difference between function and class, but class approach looks a bit better.
You should accept multiple parameters with reasonable defaults and return response instance anyway.

[popravich]

My consern is that another class can evolve into something completle different from its parent, ie new mehods/attribues appear or else...
And also the description of this issue states that users forget to set proper content-type,
so it sounded clear for me that a simple function (with Response's signature + defaults) would be enough.

Adding another type of response it just like confusing me as a user, like:
"hmm, I dump'ed my data with json and now I need to return it to user. So I instantiate web.Response and... Wait a minute, there's a JSONResponse, hmm... Can I still use web.Response or I must use JSONResponse?.."
Such a cases make me look into code and try to understand what is the difference between those two...
I think it (documentation) must clearly state that JSONResponse class is a simply a shortcut.

But this all is just my conserns I just needed to say it aloud)

[asvetlov]

The same issue appears with HTTP exceptions, right? You may use web.Response with 404 status code or web.HTTPNotFound

[popravich]

Well, not exactly) You just named it -- those are exceptions so having separate classes works fine for me.
Also you can't raise web.Response

[asvetlov]

fixed by #599 json_response function wins.

[jashandeep-sohi]

If the functional way is the way to go, can we move json_response as a classmethod to the Response class? Might be cleaner/semantic way to organize the code. And then all the special response types can be implemented in a similar way:

class Response:
  ...
  @classmethod
  def json(cls, ...):
    ...
    return cls(...)

  @classmethod
  def html(cls, ...):
    ...
    return cls(...)

[asvetlov]

Class methods is too broad.
E.g. I may derive from Response to introduce new constructor parameters.
MyResponse.json() is still allowed but most likely crashes.
@staticmethod is safer and stricter approach but I don't see any benefits from it against first-class function.

[jashandeep-sohi]

Yeah, I forgot about inheritance.

I know @staticmethod doesn't add anything special, but perhaps it makes a bit more explicit you are still returning a Response class (for me at least):

def handler(req):
  return Response.json(...)

vs

def handler(req):
  return json_response(...)

Doesn't matter really, it's up to you.

[asvetlov]

But I don't like to have HTTPNotFound.json(...)

[jashandeep-sohi]

Yea, that is an ugly side effect. `json_response` is fine then.

[socketpair]

So, why not to have

return web.Response(json={'answer_of_the_life': 42})

?

It is straighforward, easy and can not be misunderstood (as in Tornado, where it understand only dicts as json data). And also not require to use auxilary function like json_response(). We already have Response(data=...) and Response=(text=...)

[AraHaan]

Well, as much as I like json and I use it a lot. I also hate having to use BeutifulSoup and lxml just to convert simple xml responces to json data. It ais also pain as well as I wanted to use the xml.dom.minidom in the standard library for the conversion of it to json. Hopefully an future version of aiohttp would allow me to convert xml responces to json so I can kill two dependencies with one stone (in this case aiohttp doing it for me). I can use json on request on most sites but some sites respond with xml or html even if you request to them with json.

@socketpair do you agree we need some sort of conversion from xml to json in aiohttp that uses the standard library json and standard library xml to convert the responses to json when it is in xml (not html) form?

[socketpair]

Regarding XML question, it sounds like someone takes heavy drugs. So, I think we MUST NOT do such things in aiohttp.

[kxepal]

@AraHaan does stdlib has any analogue of json.dumps for xml? I guess, only xmlrpc has.

[popravich]

web.Response(json=....) has one small problem:
how would you encode datetime object, for instance?
json_response function provides dumps parameter that allow you to specify
custom serializer, basically its partial(json.dumps, default=my_datetime_serializer).
So adding json parameter to web.Response would require to add another parameter json_dumps or whatever.

[popravich]

Also, client API now has the same problem — client.post(json={...}) does not allow custom types in json object, if you need to encode datetime objects or anything else, you'll have to either monkey-patch json.dumps or use the old way — client.post(data=json.dumps({...}, default=...)).

[kxepal]

Great points, @popravich.

[socketpair]

Maybe we should add Application-scope configuration, that allows to specify json (de-)serialiser globally ? I think real-life applications use single way of (de-)serialisation across all requests and responses. The rare cases when same objects should be (de-)serialised in different way are covered with client.post(data=json.dumps({...}, default=...))

[AraHaan]

Wait what @kxepal is xmlrpc even in the standard library? I have no found anything in it that would allow me to convert xml to json just by using the standard library. (And even then I would only want only specific parts of the xml data be in the json and the other stuff from the xml response discarded for my usage.

[kxepal]

@AraHaan

is xmlrpc even in the standard library?

Yes

I have no found anything in it that would allow me to convert xml to json just by using the standard library.

That's basically not possible because XML is very rich format. JSONx may guarantee you such kind of conversion, but nothing else.

[popravich]

Maybe we should add Application-scope configuration, that allows to specify json (de-)serialiser globally ?

I'm afraid this may not work well — real-life applications may need to handle several versions of self,
so upgrading to new serialization logic can became non trivial task (first that comes in mind — one would put versioning logic inside default function and add version argument in a some weird way...).
I'm, probably, neither good with it nor against this idea, need to think over other cases.

[popravich]

...anyway, this all can be easily achieved with serialization middleware without need to modify aiohttp.

[fafhrd91]

Serialization makes total sens.

But I don't like middleware idea. I understand it is right solution, but I prefer ergonomics of the framework.

[lock]

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.