Removed internal variable pass_context_arg_name #1568
Conversation
|
Thanks @leonardofesta! |
|
Hi @leonardofesta, did you have a chance to have another look at this? Let me know if you need support. |
|
Sorry @RobbeSneyders I was on vacation and I didn't saw the emails. Now it should pass all tests. Should i rebase and force push? |
There was a problem hiding this comment.
LGTM apart from the thing that Ruwann said about collisions, could you maybe add a test where there is a parameter called context? Also converting the constants to all uppercase seems like a refactor we should do in a separate PR. Let's see what Ruwann and Robbe say.
Pull Request Test Coverage Report for Build 3108587472
💛 - Coveralls |
There was a problem hiding this comment.
Thanks @LeenaBhegade
You've removed some functionality that we need to keep. I added some comments to indicate where.
This means that we need the context_kw both in the parameter decorator and the security handler factory. We can probably just define it on both separately, as they are quite decoupled from each other.
I would also propose to use context_ as the default context_kw. The underscore lowers the chance of collision and indicates that it's a special variable.
| @@ -124,10 +119,6 @@ def wrapper(request): | |||
| else: | |||
| logger.debug("Context parameter '%s' not in function arguments", key) | |||
|
|
|||
| # attempt to provide the request context to the function | |||
There was a problem hiding this comment.
We still want to pass the context as an argument to the view function, so we can't just remove this. We need to include the context in kwargs using context_kw.
| @@ -23,11 +23,6 @@ def get_arguments(self, *args, **kwargs): | |||
| parameter_to_arg(Op(), handler)(request) | |||
| func.assert_called_with(p1="123") | |||
|
|
|||
| parameter_to_arg(Op(), handler, pass_context_arg_name="framework_request_ctx")( | |||
There was a problem hiding this comment.
We want to keep this behaviour, so should keep testing for it
tests/fakeapi/hello/__init__.py
Outdated
| def get_bye_secure_from_connexion(req_context): | ||
| return "Goodbye {user} (Secure!)".format(user=req_context["user"]) | ||
| def get_bye_secure_from_connexion(): | ||
| return "Goodbye {user} (Secure!)".format(user=context["user"]) |
There was a problem hiding this comment.
We want to check that the context is passed to the function, so we shouldn't use the global context here.
now the function accept a pass_context_arg boolean parameter, instead of the pass_context_arg_name value
|
Any update @leonardofesta? |
Let me know |
There was a problem hiding this comment.
- I basically restored the decorator functionality by using a boolean with default == False to maintain the same logic
The reason to remove the pass_context_arg_name variable, was that it needs to be passed around a lot for a small benefit. Replacing it with a boolean doesn't really solve this.
Instead it would be nice if the context is passed automatically if the context_ parameter is added to the view function.
| need_context = self.pass_context_arg_name and ( | ||
| has_kwargs or self.pass_context_arg_name in arguments | ||
| ) | ||
| need_context = has_kwargs or self.context_kw in arguments |
There was a problem hiding this comment.
We could do this by removing the has_kwargs here.
There was a problem hiding this comment.
Ok i'll fix the two things as suggested
fixed security handler as suggested removed pass_context_arg as before
|
Thanks @leonardofesta, looks good to me. I pushed a change to the test, as it wasn't actually testing the new behavior. |
Fixes #1532 .
Changes proposed in this pull request:
I don't know if it is better to put in uppercase both values (context_kw and required_scopes_kw), now they are constants in the end.
I left as the other one to have consistency, but if you prefer I can change them both.