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

Alter endpoint context behavior. #125

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

AutumnalDream
Copy link
Contributor

@AutumnalDream AutumnalDream commented Aug 30, 2020

Currently, in GraphQLEndpoint._get_response(), it makes a new dictionary and adds data from the provided context dictionary to it. This can be an issue if a custom dictionary class is provided as the context.
In my opinion as well it makes more sense to add to the provided dict instead of replacing it.
As far as I can tell I don't see any breaking issues with this change.

Also if I remember correctly, context is a dict in GraphQL spec maybe, but I don't think it is specified as anything in the Tartiflette code itself.

Edit: Including reason for this PR.
Basically, my reason for this issue is that it seems the context is being shared across requests.

Copy link
Contributor

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, left a comment. Black 20 is out and it's breaking CI, we'd need to fix things separately.

src/tartiflette_asgi/_endpoints.py Outdated Show resolved Hide resolved
AutumnalDream and others added 2 commits August 30, 2020 15:57
Use a copy of the initial config instead of directly updating the initial config with the new values.

Co-authored-by: Florimond Manca <[email protected]>
@AutumnalDream
Copy link
Contributor Author

It was set to use config.context so fixed that and now everything is good.

@AutumnalDream
Copy link
Contributor Author

Oops, just realized I should add this behavior to the Subscriptions endpoint as well, will update with that in a bit.

@AutumnalDream
Copy link
Contributor Author

AutumnalDream commented Aug 31, 2020

So it seems this hasn't fixed my issue but my initial method does.
Basically, my reason for this issue is that it seems the context is being shared across requests.
In my use case, I made the context a customized UserDict and wrote in __setitem__ that when req is updated, to refresh the object to its initial state. Doing my initial commit with just updating the context object that was sent in fixes this issue, but copying does not.

Edit: Example code for my custom context object. For reference, I was having this issue before with just a plain dictionary but attempted this as a solution.

class GraphQLContext(UserDict):
    def __init__(self):
        super().__init__(self)
        self.refresh()

    def __setitem__(self, key, value):
        if key == "req":
            req = self.data.get("req", None)
            if req is not None:
                if req is not value:
                    self.refresh()
        self.data[key] = value

    def refresh(self) -> None:
        self.data = dict(key=value....)

@AutumnalDream
Copy link
Contributor Author

@florimondmanca Do you have any thoughts on this? I'm having trouble coming up with a solution.

@florimondmanca
Copy link
Contributor

@AutumnalDream Hi! Could you try by starting to explain what your initial problem / use case was? From the snippet of your custom context class, it seems you're trying to have a context per request, or at least "refresh" this context once a new request comes in. I'm not sure exactly what you're trying to do, but have you explored the contexvars module?

@AutumnalDream
Copy link
Contributor Author

AutumnalDream commented Sep 14, 2020

Ya that is exactly it!
My reason for the initial request was the context was being shared across requests but I'm under the impression it is supposed to be a per query context.

My initial plan was to just make sure it was making a copy when it gets sent to the engine but perhaps also using copy.copy or copy.deepcopy as I didn't find anything that specified that context has to be a dict.

So more of two separate things in this PR I guess.

I've never written anything with the contextvars module but it looks like it could be the solution.

@AutumnalDream
Copy link
Contributor Author

So after messing around with contextvars for a bit I couldn't get it to work. But, I'm also a bit of an async noob and have never used contextvars before. But I did switch the code to use copy.deepcopy, and it works.

@AutumnalDream
Copy link
Contributor Author

@florimondmanca Looks like there is a flake8 issue on protocol.py, do you want me to put flake8: PIE786 on it?

@AutumnalDream
Copy link
Contributor Author

@florimondmanca Hi just checking if you saw this.

@florimondmanca
Copy link
Contributor

@AutumnalDream Hi! Hmm yes I suppose we're having to deal with these new flake8-pie errors in this repo as well (they caught us in a few other repos I contribute to). Ideally we should go through a dedicated pull request to ignore them, to keep this one clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants