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

Fix "actions" on ModelViews with composite primary keys #1493

Merged
merged 1 commit into from
Oct 25, 2020

Conversation

ashb
Copy link
Contributor

@ashb ashb commented Oct 22, 2020

The refactor in #1398 changes the get() function, and mistakenly
always applied the PK filter, even in the case of composite keys.

(The else block of if self.is_pk_composite applied it again, so it was
actually applying the same filter twice, which didn't break anything,
but was not needed)

I had a bit of trouble running the tests locally, but by ignoring some bits that weren't working for me I was able to validate that the block I added behaves as expected.

Description

Discovered in Airflow, apache/airflow#11513

The stack trace we got was:

  File "/home/airflow/.local/lib/python3.6/site-packages/flask/app.py", line 2447, in wsgi_app
    response = self.full_dispatch_request()
  File "/home/airflow/.local/lib/python3.6/site-packages/flask/app.py", line 1952, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/home/airflow/.local/lib/python3.6/site-packages/flask/app.py", line 1821, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "/home/airflow/.local/lib/python3.6/site-packages/flask/_compat.py", line 39, in reraise
    raise value
  File "/home/airflow/.local/lib/python3.6/site-packages/flask/app.py", line 1950, in full_dispatch_request
    rv = self.dispatch_request()
  File "/home/airflow/.local/lib/python3.6/site-packages/flask/app.py", line 1936, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "/home/airflow/.local/lib/python3.6/site-packages/flask_appbuilder/views.py", line 686, in action_post
    self.datamodel.get(self._deserialize_pk_if_composite(pk)) for pk in pks
  File "/home/airflow/.local/lib/python3.6/site-packages/flask_appbuilder/views.py", line 686, in <listcomp>
    self.datamodel.get(self._deserialize_pk_if_composite(pk)) for pk in pks
  File "/home/airflow/.local/lib/python3.6/site-packages/flask_appbuilder/models/sqla/interface.py", line 870, in get
    query, _filters, select_columns=select_columns
  File "/home/airflow/.local/lib/python3.6/site-packages/flask_appbuilder/models/sqla/interface.py", line 324, in apply_all
    select_columns,
  File "/home/airflow/.local/lib/python3.6/site-packages/flask_appbuilder/models/sqla/interface.py", line 272, in _apply_inner_all
    query = self.apply_filters(query, inner_filters)
  File "/home/airflow/.local/lib/python3.6/site-packages/flask_appbuilder/models/sqla/interface.py", line 162, in apply_filters
    return filters.apply_all(query)
  File "/home/airflow/.local/lib/python3.6/site-packages/flask_appbuilder/models/filters.py", line 295, in apply_all
    query = flt.apply(query, value)
  File "/home/airflow/.local/lib/python3.6/site-packages/flask_appbuilder/models/sqla/filters.py", line 137, in apply
    query, field = get_field_setup_query(query, self.model, self.column_name)
  File "/home/airflow/.local/lib/python3.6/site-packages/flask_appbuilder/models/sqla/filters.py", line 40, in get_field_setup_query
    if not hasattr(model, column_name):
TypeError: hasattr(): attribute name must be string

ADDITIONAL INFORMATION

This was noticed in Apache Airflow 2.0.0a1 where we use the "multi-select + action" on a table with composite PKs

  • Has associated issue:
  • Is CRUD MVC related.
  • Is Auth, RBAC security related.
  • Changes the security db schema.
  • Introduces new feature
  • Removes existing feature

@ashb
Copy link
Contributor Author

ashb commented Oct 22, 2020

@dpgaspar Any chance of getting this in 3.1.1? It's breaking a number of the CRUD views in Airflow. Thanks!

Copy link

@kaxil kaxil left a comment

Choose a reason for hiding this comment

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

@dpgaspar Would really appreciate if we can get this out sooner

The refactor in dpgaspar#1398 changes the `get()` function, and mistakenly
always applied the PK filter, even in the case of composite keys.

(The else block of `if self.is_pk_composite` applied it again, so it was
actually applying the same filter twice, which didn't break anything,
but was not needed)
ashb added a commit to astronomer/airflow that referenced this pull request Oct 23, 2020
This fixes apache#11513 -- and has been submitted upstream to FAB as
dpgaspar/Flask-AppBuilder#1493, once that is
merged we can remove this
ashb added a commit to apache/airflow that referenced this pull request Oct 23, 2020
This fixes #11513 -- and has been submitted upstream to FAB as
dpgaspar/Flask-AppBuilder#1493, once that is
merged we can remove this override.
@dpgaspar
Copy link
Owner

Hope to get 3.1.1 out on monday/tuesday, Thank you for the fix and sorry for this regression

@dpgaspar dpgaspar merged commit 0e5c651 into dpgaspar:master Oct 25, 2020
@ashb
Copy link
Contributor Author

ashb commented Oct 26, 2020

No worries - regressions happen, that's why I added tests :)

@ashb ashb deleted the sqla-interface-get-composite-pk branch October 26, 2020 09:46
michalmisiewicz pushed a commit to michalmisiewicz/airflow that referenced this pull request Oct 30, 2020
This fixes apache#11513 -- and has been submitted upstream to FAB as
dpgaspar/Flask-AppBuilder#1493, once that is
merged we can remove this override.
szn pushed a commit to szn/airflow that referenced this pull request Nov 1, 2020
This fixes apache#11513 -- and has been submitted upstream to FAB as
dpgaspar/Flask-AppBuilder#1493, once that is
merged we can remove this override.
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Sep 15, 2021
This fixes #11513 -- and has been submitted upstream to FAB as
dpgaspar/Flask-AppBuilder#1493, once that is
merged we can remove this override.

GitOrigin-RevId: 28229e990894531d0aaa3f29fe68682c8b01430a
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Sep 17, 2021
This fixes #11513 -- and has been submitted upstream to FAB as
dpgaspar/Flask-AppBuilder#1493, once that is
merged we can remove this override.

GitOrigin-RevId: 28229e990894531d0aaa3f29fe68682c8b01430a
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Sep 23, 2021
This fixes #11513 -- and has been submitted upstream to FAB as
dpgaspar/Flask-AppBuilder#1493, once that is
merged we can remove this override.

GitOrigin-RevId: 28229e990894531d0aaa3f29fe68682c8b01430a
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Nov 26, 2021
This fixes #11513 -- and has been submitted upstream to FAB as
dpgaspar/Flask-AppBuilder#1493, once that is
merged we can remove this override.

GitOrigin-RevId: 28229e990894531d0aaa3f29fe68682c8b01430a
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Mar 9, 2022
This fixes #11513 -- and has been submitted upstream to FAB as
dpgaspar/Flask-AppBuilder#1493, once that is
merged we can remove this override.

GitOrigin-RevId: 28229e990894531d0aaa3f29fe68682c8b01430a
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jun 4, 2022
This fixes #11513 -- and has been submitted upstream to FAB as
dpgaspar/Flask-AppBuilder#1493, once that is
merged we can remove this override.

GitOrigin-RevId: 28229e990894531d0aaa3f29fe68682c8b01430a
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jun 7, 2022
This fixes #11513 -- and has been submitted upstream to FAB as
dpgaspar/Flask-AppBuilder#1493, once that is
merged we can remove this override.

GitOrigin-RevId: 28229e990894531d0aaa3f29fe68682c8b01430a
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jul 9, 2022
This fixes #11513 -- and has been submitted upstream to FAB as
dpgaspar/Flask-AppBuilder#1493, once that is
merged we can remove this override.

GitOrigin-RevId: 28229e990894531d0aaa3f29fe68682c8b01430a
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Aug 27, 2022
This fixes #11513 -- and has been submitted upstream to FAB as
dpgaspar/Flask-AppBuilder#1493, once that is
merged we can remove this override.

GitOrigin-RevId: 28229e990894531d0aaa3f29fe68682c8b01430a
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Oct 4, 2022
This fixes #11513 -- and has been submitted upstream to FAB as
dpgaspar/Flask-AppBuilder#1493, once that is
merged we can remove this override.

GitOrigin-RevId: 28229e990894531d0aaa3f29fe68682c8b01430a
aglipska pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Oct 7, 2022
This fixes #11513 -- and has been submitted upstream to FAB as
dpgaspar/Flask-AppBuilder#1493, once that is
merged we can remove this override.

GitOrigin-RevId: 28229e990894531d0aaa3f29fe68682c8b01430a
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Dec 7, 2022
This fixes #11513 -- and has been submitted upstream to FAB as
dpgaspar/Flask-AppBuilder#1493, once that is
merged we can remove this override.

GitOrigin-RevId: 28229e990894531d0aaa3f29fe68682c8b01430a
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jan 27, 2023
This fixes #11513 -- and has been submitted upstream to FAB as
dpgaspar/Flask-AppBuilder#1493, once that is
merged we can remove this override.

GitOrigin-RevId: 28229e990894531d0aaa3f29fe68682c8b01430a
kosteev pushed a commit to kosteev/composer-airflow-test-copybara that referenced this pull request Sep 11, 2024
This fixes #11513 -- and has been submitted upstream to FAB as
dpgaspar/Flask-AppBuilder#1493, once that is
merged we can remove this override.

GitOrigin-RevId: 28229e990894531d0aaa3f29fe68682c8b01430a
kosteev pushed a commit to kosteev/composer-airflow-test-copybara that referenced this pull request Sep 12, 2024
This fixes #11513 -- and has been submitted upstream to FAB as
dpgaspar/Flask-AppBuilder#1493, once that is
merged we can remove this override.

GitOrigin-RevId: 28229e990894531d0aaa3f29fe68682c8b01430a
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Sep 17, 2024
This fixes #11513 -- and has been submitted upstream to FAB as
dpgaspar/Flask-AppBuilder#1493, once that is
merged we can remove this override.

GitOrigin-RevId: 28229e990894531d0aaa3f29fe68682c8b01430a
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.

4 participants