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

views: apply decorators once #41

Open
ppanero opened this issue Jun 19, 2020 · 8 comments
Open

views: apply decorators once #41

ppanero opened this issue Jun 19, 2020 · 8 comments
Labels
enhancement New feature or request

Comments

@ppanero
Copy link
Member

ppanero commented Jun 19, 2020

The correct use case would be:

  • The method/view gets decorated once
  • The decorator is called each time the function is invoked

The current behaviour:

  • The method/view is decorated each time a function (get, post, etc.) is invoked. This happens because it is being decorated in the dispatch_request.

The ideal would be to decorate each method once, to avoid having the same chain of decorators in every module (e.g. content negotiation happens always, request loading also...)

EDIT
Goal of this issue is to refactor BaseView to use decorators to hold the common decorators. This will:

  • decorate in one place the common decorators
  • decorate once when as_view is called

EDIT
As per comment below, we can't use decorators. The remaining issue is "decorating once". IMHO this is a nice to have, but not necessary now: the performance cost of this should be felt before. I suggest we delay this using my release management powers ⚡ 😉

@ppanero ppanero added this to Triage in InvenioRDM June Board via automation Jun 19, 2020
@ppanero ppanero moved this from Triage to Backend To do in InvenioRDM June Board Jun 19, 2020
@ppanero ppanero removed this from Backend To do in InvenioRDM June Board Jun 26, 2020
@ppanero ppanero added this to Backend To do in InvenioRDM July Board Jun 26, 2020
@fenekku
Copy link
Contributor

fenekku commented Jun 29, 2020

@ppanero I think this is solved. We decorate in one place (the BaseView) with decorators that apply to all Resource methods and more spericifc method-related decorators can be added on each method separately.

Did you have something else in mind?

@fenekku fenekku added the question Further information is requested label Jun 29, 2020
@ppanero
Copy link
Member Author

ppanero commented Jun 29, 2020

@fenekku The issue still applies, the main problem is that the decoration happens inside the dispatch_request. Which means that we are decorating on each request rather than once (at start time of the web app).

However, if the function invokation is moved to the __init__ the decorator is not applied to each method but to each class, and therefore not work.

So it is half done:

  • ✅ Decorate only in one place
  • ❌ Decorate at start time

@fenekku
Copy link
Contributor

fenekku commented Jun 29, 2020

Ok, then we should use the decorators class variable from https://github.com/pallets/flask/blob/024f0d384cf5bb65c76ac59f8ddce464b2dc2ca1/src/flask/views.py#L56 . Will amend the issue to this.

@fenekku fenekku removed the question Further information is requested label Jun 29, 2020
@ppanero
Copy link
Member Author

ppanero commented Jun 29, 2020

@fenekku when using that variable is the second point I was mentioning. Decorators are applied per class, and then not used in every function (get, post, etc). This would need some more research on the inner guts of flask/werkzeug I guess.

@fenekku
Copy link
Contributor

fenekku commented Jun 29, 2020

Decorators are applied per class, and then not used in every function (get, post, etc).

I don't think I understand something here... With View's decorators the decorators are applied per class and used for get, post, ... When are they not used?

If the issue is about decorators only for some methods and not others, then that is already possible by putting a decorator on some methods and not others. In some cases, we could give extra support such as the 2nd option here: #59 .

@ppanero
Copy link
Member Author

ppanero commented Jun 30, 2020

Theoretically what you say is right. However, when Alex and I tested it, this:

I don't think I understand something here... With View's decorators the decorators are applied per class and used for get, post, ... When are they not used?

Did not work. That is why I say it requires more research. Maybe we did something wrong, or maybe it is not working as intended.

@fenekku
Copy link
Contributor

fenekku commented Jun 30, 2020

Strange, 🤔 I will check it out this week.

@fenekku fenekku self-assigned this Jun 30, 2020
@fenekku fenekku moved this from Backend To do to In progress in InvenioRDM July Board Jun 30, 2020
@fenekku
Copy link
Contributor

fenekku commented Jul 6, 2020

This is what is preventing us from using the built-in decorators:

https://github.com/pallets/flask/blob/master/src/flask/app.py#L1852

return self.view_functions[rule.endpoint](**req.view_args)

because self is not passed in for MethodView decorators and we need it, we will do without it.

@fenekku fenekku closed this as completed Jul 6, 2020
@fenekku fenekku reopened this Jul 6, 2020
@fenekku fenekku moved this from In progress to Triage in InvenioRDM July Board Jul 6, 2020
@fenekku fenekku removed this from Triage in InvenioRDM July Board Jul 6, 2020
@fenekku fenekku changed the title views: decorators are applied on each call views: apply decorators once Jul 6, 2020
@fenekku fenekku added the enhancement New feature or request label Jul 27, 2020
@fenekku fenekku removed their assignment Sep 1, 2020
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
None yet
Development

No branches or pull requests

2 participants