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

Replace FAB url filtering function with Airflows #27576

Merged
merged 2 commits into from Nov 9, 2022

Conversation

jedcunningham
Copy link
Member

Use our url filtering util function so there is consistency between FAB and Airflow routes.

Use our url filtering util function so there is consistency between
FAB and Airflow routes.
@ashb ashb added this to the Airflow 2.4.3 milestone Nov 9, 2022
@ephraimbuddy ephraimbuddy merged commit b33d22c into apache:main Nov 9, 2022
@ephraimbuddy ephraimbuddy deleted the swap_fab_url_filter branch November 9, 2022 19:02
@ephraimbuddy ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label Nov 9, 2022
ephraimbuddy pushed a commit that referenced this pull request Nov 9, 2022
Use our url filtering util function so there is consistency between
FAB and Airflow routes.

(cherry picked from commit b33d22c)
@IKholopov
Copy link
Contributor

This broke API unit tests, e.g. tests/api_connexion/test_auth.py::TestSessionAuth::test_success:

=================================== FAILURES ===================================                                       
_________________________ TestSessionAuth.test_success _________________________                                                                                                                                                              
                                                                                                                       
self = <tests.api_connexion.test_auth.TestSessionAuth object at 0x7f9d56d4fb10>                                        
                                                                                                                       
    def test_success(self):                                                                                                                                                                                                                   
        clear_db_pools()                                                                                               
                                                                                                                       
>       admin_user = client_with_login(self.app, username="test", password="test")                                     
                                                                                                                       
tests/api_connexion/test_auth.py:143:                                                                                  
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _                                        
tests/test_utils/www.py:27: in client_with_login                                                                                                                                                                                              
    resp = client.post("/login/", data=kwargs)                                                                                                                                                                                                
/usr/local/lib/python3.7/site-packages/werkzeug/test.py:1145: in post                                                                                                                                                                         
    return self.open(*args, **kw)                                                                                                                                                                                                             
/usr/local/lib/python3.7/site-packages/flask/testing.py:226: in open                                                                                                                                                                          
    follow_redirects=follow_redirects,                                                                                                                                                                                                        
/usr/local/lib/python3.7/site-packages/werkzeug/test.py:1094: in open                                                                                                                                                                         
    response = self.run_wsgi_app(request.environ, buffered=buffered)                                                                                                                                                                          
/usr/local/lib/python3.7/site-packages/werkzeug/test.py:961: in run_wsgi_app
    rv = run_wsgi_app(self.application, environ, buffered=buffered)
/usr/local/lib/python3.7/site-packages/werkzeug/test.py:1242: in run_wsgi_app
    app_rv = app(environ, start_response)
/usr/local/lib/python3.7/site-packages/flask/app.py:2548: in __call__
    return self.wsgi_app(environ, start_response)
/usr/local/lib/python3.7/site-packages/flask/app.py:2528: in wsgi_app
    response = self.handle_exception(e)
/usr/local/lib/python3.7/site-packages/flask/app.py:2525: in wsgi_app
    response = self.full_dispatch_request()
/usr/local/lib/python3.7/site-packages/flask/app.py:1822: in full_dispatch_request
    rv = self.handle_user_exception(e)
/usr/local/lib/python3.7/site-packages/flask/app.py:1820: in full_dispatch_request
    rv = self.dispatch_request()
/usr/local/lib/python3.7/site-packages/flask/app.py:1796: in dispatch_request
    return self.ensure_sync(self.view_functions[rule.endpoint])(**view_args)
/usr/local/lib/python3.7/site-packages/flask_appbuilder/security/views.py:524: in login
    return redirect(get_safe_redirect(next_url))
airflow/www/views.py:159: in get_safe_url
    return url_for('Airflow.index')
/usr/local/lib/python3.7/site-packages/flask/helpers.py:262: in url_for
    **values,
/usr/local/lib/python3.7/site-packages/flask/app.py:2031: in url_for
    return self.handle_url_build_error(error, endpoint, values)
/usr/local/lib/python3.7/site-packages/flask/app.py:2025: in url_for
    force_external=_external,
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <werkzeug.routing.map.MapAdapter object at 0x7f9d54a88810>
endpoint = 'Airflow.index', values = {}, method = None, force_external = False
append_unknown = True, url_scheme = None
...
        rv = self._partial_build(endpoint, values, method, append_unknown)
        if rv is None:
>           raise BuildError(endpoint, values, method, self)
E           werkzeug.routing.exceptions.BuildError: Could not build url for endpoint 'Airflow.index'. Did you mean 'IndexView.index' instead?

/usr/local/lib/python3.7/site-packages/werkzeug/routing/map.py:917: BuildError

@jedcunningham
Copy link
Member Author

Thanks @IKholopov, looking now.


def _init_extension(self, app):
app.appbuilder = self
if not hasattr(app, "extensions"):
app.extensions = {}
app.extensions["appbuilder"] = self

def _swap_url_filter(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

A side comment (not related to the test failure):

why is the private method of a class altering the global object? The content of this method doesn't use the actual object it is called upon (self) and yet this method is called as part of initialization of the AirflowAppBuilder object.

Does the builder depends on the old behavior of fab_sec_views.get_safe_redirect before initialization complete? If so, could you please leave a comment about that? If not, is the AppBuilder (which focuses on building "app" object) the right place to perform this substitution? Maybe it would be better to put static part of the initialization in a new dedicated file?

@jedcunningham
Copy link
Member Author

Fix in #27586.

Adityamalik123 pushed a commit to Adityamalik123/airflow that referenced this pull request Nov 12, 2022
Use our url filtering util function so there is consistency between
FAB and Airflow routes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:webserver Webserver related Issues type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants