-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
Add REST API query sort and order to some endpoints #14895
Conversation
00178a2
to
4a20594
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a common enough pattern that I think it should be extracted out to something like this:
def apply_sorting(model, query, sort, order_by):
if order_by not in (i.name for i in model.__table__.columns):
raise BadRequest(
detail=f"{type(model).__name__} has no attribute '{order_by}' specified in order_by parameter",
)
if sort == "asc":
return = query.order_by(asc(order_by))
else:
return query.order_by(desc(order_by))
(I have used the param names you did, but see my previous comment.)
4a20594
to
74aecc2
Compare
9a367c8
to
fa4fed7
Compare
total_entries = session.query(func.count(Connection.id)).scalar() | ||
query = session.query(Connection) | ||
connections = query.order_by(Connection.id).offset(offset).limit(limit).all() | ||
query = apply_sorting(Connection, query, order_by, to_replace, allowed_filter_attrs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel there should be a way to avoid repeating Connection
here. Same goes for other calls below.
fa4fed7
to
c020425
Compare
The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*. |
The Workflow run is cancelling this PR. Building image for the PR has been cancelled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_dag_runs_batch
in dag_run_endpoint should have ordering too.
5fdf19d
to
142457f
Compare
The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*. |
The Workflow run is cancelling this PR. Building image for the PR has been cancelled |
142457f
to
5b714ec
Compare
The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*. |
5b714ec
to
9efd7db
Compare
Co-authored-by: Kamil Breguła <mik-laj@users.noreply.github.com>
This change adds ordering to the REST API endpoints.
The get taskinstances and dags endpoint are excluded in this PR
Part of #14803
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.