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

Unify how we allow to set headers and status code #1579

Open
patrick91 opened this issue Jan 15, 2022 · 7 comments
Open

Unify how we allow to set headers and status code #1579

patrick91 opened this issue Jan 15, 2022 · 7 comments
Labels
enhancement New feature or request
Milestone

Comments

@patrick91
Copy link
Member

patrick91 commented Jan 15, 2022

So at the moment we have a few integrations and they have a slightly different way to setting headers and status code.

The general idea is the same, we provide a response object in info.context. This object is based on the current integration, this is where the differences between integrations come into play.

I think we should provide a new object (unfortunately we can't replace the current one due to backwards compatibility) that allows to:

  1. set/update response headers
  2. set the status code

Having this could also help us prevent some mistakes, for example, should we allow to set the status code multiple times?

What do you think?

Upvote & Fund

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@patrick91 patrick91 added the enhancement New feature or request label Jan 15, 2022
@patrick91 patrick91 added this to the Version 1 milestone Jan 15, 2022
@jkimbo
Copy link
Member

jkimbo commented Jan 16, 2022

Maybe providing some methods on context? set_status_code, add_headers etc.

Also can we be sure that we can abstract away different server implementations? I know for example that ASGI running background tasks but Django doesn't.

@patrick91
Copy link
Member Author

Maybe providing some methods on context? set_status_code, add_headers etc.

I was thinking of having a new property that had those methods

Also can we be sure that we can abstract away different server implementations? I know for example that ASGI running background tasks but Django doesn't.

I'd just go for headers and status code for now, we can keep access to the framework's request object if people need to do something more specific :)

@jkimbo
Copy link
Member

jkimbo commented Jan 16, 2022

Had a thought: if we construct our own info object then we could attach this object/method to the info object rather than through context? I think that makes more sense because context is defined mostly in user land.

@patrick91
Copy link
Member Author

something like:

def a_resolver(info: Info) -> str:
    info.request.headers["example"]
    info.response.set_header("X-Framework", "Strawberry")
    return "ABC"

?

@patrick91
Copy link
Member Author

We should do this for cookies as well (even if they are just headers :))

@jkimbo
Copy link
Member

jkimbo commented Jan 17, 2022

something like:

def a_resolver(info: Info) -> str:
    info.request.headers["example"]
    info.response.set_header("X-Framework", "Strawberry")
    return "ABC"

?

Yes I think that looks good.

@jthorniley
Copy link
Member

something like:

def a_resolver(info: Info) -> str:
    info.request.headers["example"]
    info.response.set_header("X-Framework", "Strawberry")
    return "ABC"

?

I have two questions re this:

  1. If the resolver is called from a websocket (generally a subscription, but it could be a single operation called over ws), this is fairly meaningless since you can't set cookies/headers at this point. So should it error? Silently ignore? (Or there might be other things you want to do with a websocket, like close it immediately with a websocket-specific status code)
  2. How to test? At the moment, we test with something like:
    result: ExecutionResult = await execute(query, context_value=FakeContext)
    assert result.data == <blah>
    assert FakeContext.response.headers == <blah> # if needed
    
    So we can test any status code set on the django integration but this is obviously user defined. The GraphQL ExecutionResult doesn't define a place for it.

So I was just thinking maybe define two abstract base classes, like HttpContext and WebsocketContext and make them both (optionally) available:

def a_resolver(info: Info) -> str:
  http_context  = info.http_context  # HttpContext | None
  if http_context is None:
    # ... behaviour if called from websocket or non-http ...
  else:
    if http_context.response.status_code is not strawberry.DEFAULT:
      # ... behaviour if some other resolver has already set the status code ...
    else:
      http_context.response.status_code = 403
  return "value"

So then it would be up to integrations to implement and provide http_context and websocket_context on the info object (or if they don't support it, leave as None). Likewise in testing you can add them to the execute call (or not) and use e.g. a fake/mock object to check behaviours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: No status
Development

No branches or pull requests

3 participants