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

Enable compression per view #12

Closed
mik-laj opened this issue Oct 16, 2020 · 9 comments · Fixed by #14
Closed

Enable compression per view #12

mik-laj opened this issue Oct 16, 2020 · 9 comments · Fixed by #14

Comments

@mik-laj
Copy link
Contributor

mik-laj commented Oct 16, 2020

Hello,

It would be fantastic if this design only allows compression to be enabled for selected views. This could be done with a decorator.
https://github.com/apache/airflow/blob/master/airflow/www/decorators.py#L67
This will allow this library to be used in my project - Apache Airflow.

Yours faithfully.

@KelSolaar
Copy link
Member

Hi @mik-laj,

Sounds good to me and if you have the cycles to implement the feature, it would be awesome! I'm overwhelmed and won't be able to touch as that for the foreseeable future.

Cheers,

Thomas

@alexprengere
Copy link
Collaborator

alexprengere commented Oct 19, 2020

It is actually possible to do this client side, without updating the library. Here is an example using an API inspired by Flask-Caching:

import functools
from flask_compress import Compress
 
class CustomCompress(Compress):
    def compressed(self):
        def decorator(f):
            @functools.wraps(f)
            def decorated_function(*args, **kwargs):
                @after_this_request
                def compressor(response):
                    return self.after_request(response)
                return f(*args, **kwargs)
            return decorated_function
        return decorator

# app is the Flask app 
app.config["COMPRESS_REGISTER"] = False  # disable default compression of all eligible requests
compress = CustomCompress()
compress.init_app(app)

# Compress this view
@app.route("/test")
@compress.compressed()
def view():
   pass

I implemented this in the per_view branch, I could merge this once you have tested it, to make sure it fits your use case.

@mik-laj
Copy link
Contributor Author

mik-laj commented Oct 22, 2020

@alexprengere I have to think about it, so I will look at it at the weekend.

@alexprengere
Copy link
Collaborator

@mik-laj I plan to merge the PR in a few days, once rfc7232 is merged as well, before making a new release. If you have any comments, now is the time 😉

@mik-laj
Copy link
Contributor Author

mik-laj commented Nov 1, 2020

This looks fine, but I would personally create a static method to limit the side effects. Now no global object is used and no need to initialize the global object first.

from flask_compress import Compress

class CustomCompress(Compress):
    def compressed(f):
        @functools.wraps(f)
        def decorated_function(*args, **kwargs):
            @after_this_request
            def compressor(response):
                compress = current_app.compress
                return compress.after_request(response)
            return f(*args, **kwargs)
        return decorated_function

Usage:

from flask_compress import Compress

# Compress this view specifically
@app.route("/test")
@Compress.compressed
def view():
   pass

@alexprengere
Copy link
Collaborator

I think you mean return current_app.after_request(response), because:

  • current_app is a Flask object
  • compress is a method of Compress

Nonetheless, I tried this idea, but the tests are failing, it is possible that @after_this_request does not work with current_app (I have to investigate).

A downside of that approach is that it makes configuration harder, because compression behavior is driven by the values in app.config, which are set by init_app. To work properly, the user will have to define the dozen of COMPRESS_ variables manually in app.config, so in the end more code will have to be written.

@mik-laj
Copy link
Contributor Author

mik-laj commented Nov 1, 2020

I prepared small POC: #16

@alexprengere
Copy link
Collaborator

Thanks a lot for the POC, but it does not really answer my concern. The reason why this works, is because you have those two lines. If you remove them, the test fails. So effectively the implementation does not satisfy your point above: "no global object is used and no need to initialize the global object first."

The way I see it, we cannot avoid the init_app call (unless we add a lot more code to fix missing app.config values), so we have to create Compress(), so we should not make the implementation more complex to support that Compress.compressed() use case (although the app.extensions trick is clever 😉).

@mik-laj
Copy link
Contributor Author

mik-laj commented Nov 1, 2020

https://github.com/mik-laj/flask-compress/blob/14--per_view-alternative/tests/test_flask_compress.py#L337-L338
These two lines are not a problem as these objects are bind to the context local variable, so we do not have global access to this variable, but depending on what application is currently using the variable, we use the configuration for the matching application.

no global object is used and no need to initialize the global object first.

When used in a view, it does not refer to global variables, but to the @Compress.compressed static method.

 routes = Blueprint('routes', __name__)

@routes.route('/route1/')
def view_1():
    return render_template('large.html')

@routes.route('/route2/')
@Compress.compressed
def view_2():
    return render_template('large.html')

This doesn't really matter in most cases, but can cause problems if you use the factory method to create your application instance. Then you have to use either the imports in the function or keep an eye on the order of the imports.

However, I can solve this problem on my side, so if you feel that you want to merge your idea then go ahead.

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 a pull request may close this issue.

3 participants