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

What's the recommended way of passing information to middlewares #2246

Closed
argaen opened this issue Sep 6, 2017 · 14 comments
Closed

What's the recommended way of passing information to middlewares #2246

argaen opened this issue Sep 6, 2017 · 14 comments

Comments

@argaen
Copy link
Member

argaen commented Sep 6, 2017

Today we've come to a situation where we are not sure on how to solve it in a clean way using aiohttp, let me give some context:

For all endpoints we have, we want to send an event. This event is only sent depending on some values that come as an output from our view that uses json_response. We also want this event to contain some data generated in the view.

Since there is no field extras in the response object were we can store raw data so it can be used in the middlewares, we've thought of different approaches without any of them feeling "clean".

  1. Send the event in each view explicitly (there is no middleware involved here) -> We haven't gone this way because it doesn't scale, imagine if we reach a point where we have lots of views and we change the data the event sends or how it is being sent.

  2. Propagate the information through headers -> Super ugly, we are modifying our output to the clients just to be able to send an internal event.

  3. Hack the response object once built to add a new field called extras or something like that so then we are able in the middleware to access response.extras and work from there.

Below is a pseudocode for our middleware:

async def apicall_handler(request):

  response = await handler(request)

  if some_response_value is True
    send_metric_with_some_response_data()
    return response  

  return apicall_handler

Right now we are going for number 3 but it still feels hacky.

So my question is, is there a clean way of doing this? If not, would it make sense to add an extras (or whatever name you prefer) in the Response object so users can propagate information to the middlewares?

@asvetlov
Copy link
Member

asvetlov commented Sep 6, 2017

Hi.
What's about storing your data in request?
Request supports mutable mapping interface (request['key'] = value), the feature is intended to be used as per-request storage.
Middleware has access to request anyway.

@asvetlov
Copy link
Member

asvetlov commented Sep 6, 2017

Now I see no point for adding the same functionality to response objects because request already has it and response is tightly coupled with request in one-one relation.

@argaen
Copy link
Member Author

argaen commented Sep 6, 2017

That's a good point, also crossed my mind but feels a bit weird to modify the request once there is nothing else to do in the view. To show in code so the effect is more clear:

async def view(request):
  data = get_args(request)
  data = await do_some_work(data)
  
  response_data = {
    'x': data.x,
    'something': data.something
  }

  request['something'] = data.something    # ?????
  return json_response(response_data)

Someone that doesn't know the code may think, why on earth is the request being modified if the view is already done? and until you don't see that middleware it feels like magic or something wrong. Quite implicit.

Something less confusing would be: return json_response(response_data, extras=whatever). This already gives a clue that something else is happening with the response.

Anyway, maybe I'm just biased. If that's the way of doing it and you feel there is no more discussion around the topic feel free to close the issue :)

@asvetlov
Copy link
Member

asvetlov commented Sep 6, 2017

Well, modifying request on sending response is not weird from my perspective because request and response are pretty tight coupled.

But I like to get comments from other aiohttp committers.

@samuelcolvin
Copy link
Member

I think this makes sense, it's what I would do.

I would add a comment to the source # request['foobar'] is now available as it was set by the handler and that would be enough to be clear to me.

@asvetlov
Copy link
Member

asvetlov commented Sep 6, 2017

@samuelcolvin please do.
Also http://aiohttp.readthedocs.io/en/stable/web.html#data-sharing-aka-no-singletons-please is the only mention for pushing data into request object.
Would you consider PR for promoting request as local storage for issues like this?

@fafhrd91
Copy link
Member

fafhrd91 commented Sep 6, 2017

usage of request as shared dictionary seems like ad hoc solution. should this pattern be more structured?
similar to pyramid's https://docs.pylonsproject.org/projects/pyramid/en/latest/narr/hooks.html#adding-methods-or-properties-to-a-request-object

@argaen
Copy link
Member Author

argaen commented Sep 6, 2017

I would add a comment to the source # request['foobar'] is now available as it was set by the handler and that would be enough to be clear to me.

I have strong opinion that if you need comments to explain what the code is doing, that there is something wrong about it. That's why the solution kind of doesn't convince me 100%

Well, modifying request on sending response is not weird from my perspective because request and response are pretty tight coupled.

I think this makes sense, it's what I would do.

Fair enough, if two maintainers agree on this I'm OK with it :)

@samuelcolvin
Copy link
Member

one option would be to add dictionary functionality to responses. I don't know how difficult that would be or whether it would have any performance consequences?

But if it's not too difficult and doesn't effect performance I would say it would make sense, then all the major objects in the aiohttp landscape app request and response would work in consistent ways.

It would also be clearer what's happening in middleware.

@asvetlov
Copy link
Member

asvetlov commented Sep 7, 2017

Well, supporting MutableMapping interface by responses is easy task, I don't see any logical/performance problems.

If you feel it makes sense -- I'm ok with proposal.
Any volunteer for Pull Request?

@argaen
Copy link
Member Author

argaen commented Sep 8, 2017

I'm going first for #2197, once finished if no one is taking it I'll go for it.

@argaen
Copy link
Member Author

argaen commented Nov 9, 2017

@samuelcolvin @asvetlov MR is ready

@asvetlov
Copy link
Member

Fixed by #2494

@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