AIP-84 Refactor Filter Query Parameters #43947

jason810496 · 2024-11-13T03:07:00Z

related: #43407

As we discussed in #43407 (review), here is the current implementation and usage for the refactored filter query parameters. I'd appreciate your feedback before proceeding with refactoring other endpoints to align with this approach.

Since parameter types need to be specified statically to generate the OpenAPI schema, it is necessary to have a corresponding filter_param_factory for each common type ( Like float_range_filter_factory and datetime_range_filter_factory for RangeFilter ).

Thanks!

pierrejeambrun

Definitely worth pursuing, thanks.

Just a bunch of early comments.

airflow/api_fastapi/common/parameters.py

airflow/api_fastapi/core_api/routes/public/event_logs.py

pierrejeambrun · 2024-11-13T16:57:06Z

I didn't run it myself but we need to make sure that the documentation is well detected and constructed. On http://localhost:29091/docs (default values, types, parameter names, schemas etc..)

jason810496 · 2024-11-16T09:44:27Z

Hi @pierrejeambrun,

What are your thoughts on the current filter_param_factory implementation? The current approach adds a new parameter, _type, to specify the type of query parameter at runtime. This implementation consolidates multiple factories like str_filter_param_factory, str_list_filter_param_factory, etc., into a single filter_param_factory.

I have checked the OpenAPI schema at http://localhost:29091/docs, and the correct type is displayed in the documentation.

Looking forward to your feedback!

pierrejeambrun · 2024-11-18T11:05:24Z

I think it's great, last time I tried exactly the same, code was working but documentation was missing the appropriate type. I think I was missing the following to make it work:

depends_filter.__annotations__["value"] = _type

But indeed having a Generic factory is the best approach, removing the need for type specific factories.

Love it.

jason810496 · 2024-11-23T15:40:10Z

Hi @pierrejeambrun,
I chose the schema with only basic functionalities defined in common/parameters.py but used solely in a single endpoint.
The refactor will not affect the OpenAPI schema or test cases. The current differences in v1-generated.yaml are caused by the Offset, Limit, and OrderBy parameters.

Follow-Up

I noticed that _SearchParam could leverage the factory pattern as well. I plan to refactor this part after this PR is merged to avoid making the scope of this PR too large.

Question

For some parameter schemas like QueryTIStateFilter, which are only used twice in task_instance.py, should such parameter schemas be placed directly in task_instance.py rather than in common/parameters?

Thanks !

pierrejeambrun · 2024-11-27T13:25:19Z

For some parameter schemas like QueryTIStateFilter, which are only used twice in task_instance.py, should such parameter schemas be placed directly in task_instance.py rather than in common/parameters?

As long as they are 'reused' and not specific to a fonction I think it's fine to factorize them into parameters.py

pierrejeambrun

Nice, should we generalize that to all routes ?

jason810496 · 2024-11-27T15:26:45Z

Nice, should we generalize that to all routes ?

Yes, I believe most routers can adopt filter_param_factory if there isn't complex logic within to_orm.

pierrejeambrun

I think we can do the full refactoring and apply that to all route.

I'm not super comfortable having both old and new parameters system in place and live together until we decide to finalize the refactoring.

A lot of endpoints are impacted, but the change is quite limited, spec and tests are not modified. I would say the change is not that big. (And that would confirm that every case can be handled with that new system)

jason810496 · 2024-11-29T08:56:28Z

Hi @pierrejeambrun, the refactor for _SearchParam is complete. Overall, only these 4 parameters remain unchanged as they don't contain common filter logic:

_TagsFilter
_OwnersFilter
_DagIdAssetReferenceFilter

Let me know if further adjustments are needed.

jason810496 · 2024-12-02T15:42:48Z

Rebased to the latest main. Looking forward to further feedback before merging 🙌

pierrejeambrun

Really nice.

Also as I understand, we keep flexibility by either:

using filter_param_factory for classic orm operation that are already handled
can still subclass FilterParam and implemet a full custom filter if needed.

- QueryLastDagRunStateFilter - dag_ids of get_dag_stats

pierrejeambrun · 2024-12-03T16:03:22Z

Rebasing, merging on green CI.

* Add FilterParam and type_filter_param_factory * Refactor Get Event Logs with filter_param_factory * Refactor add type option for filter_param_factory * Fix Get Event Logs with latest paginated_select * Refactor Get Assets Event * Refactor List Dag Warnings * Refactor DagRun related - QueryLastDagRunStateFilter - dag_ids of get_dag_stats * Remove unused parameters * Refactor on Dag parameters * Add any_equal to FilterParam * Refactor Task Instance * Fix Get Event Logs type * Fix after rebase * Refactor with search_param_factory * Refactor QueryLastDagRunStateFilter * Fix get_list_dag_runs_batch

jason810496 requested review from bbovenzi, pierrejeambrun, ryanahamilton, jscheffl and ephraimbuddy as code owners November 13, 2024 03:07

boring-cyborg bot added the area:UI Related to UI/UX. For Frontend Developers. label Nov 13, 2024

pierrejeambrun reviewed Nov 13, 2024

View reviewed changes

jason810496 force-pushed the feature/AIP-84/refactor-filter-query-parameters branch from cabb68d to 0b291bb Compare November 16, 2024 09:29

jason810496 force-pushed the feature/AIP-84/refactor-filter-query-parameters branch 3 times, most recently from 9b6c29f to 28c34e2 Compare November 22, 2024 14:48

jason810496 marked this pull request as draft November 22, 2024 16:39

jason810496 force-pushed the feature/AIP-84/refactor-filter-query-parameters branch 4 times, most recently from ae70027 to d329025 Compare November 23, 2024 15:28

jason810496 marked this pull request as ready for review November 23, 2024 15:38

pierrejeambrun reviewed Nov 27, 2024

View reviewed changes

pierrejeambrun reviewed Nov 28, 2024

View reviewed changes

jason810496 marked this pull request as draft November 29, 2024 07:09

jason810496 force-pushed the feature/AIP-84/refactor-filter-query-parameters branch 4 times, most recently from 88817ac to 7d57e77 Compare November 29, 2024 08:53

jason810496 marked this pull request as ready for review November 29, 2024 08:53

jason810496 force-pushed the feature/AIP-84/refactor-filter-query-parameters branch 2 times, most recently from adc3de0 to 03d0c56 Compare December 2, 2024 15:40

pierrejeambrun approved these changes Dec 3, 2024

View reviewed changes

jason810496 added 16 commits December 4, 2024 00:03

Add FilterParam and type_filter_param_factory

d1ff864

Refactor Get Event Logs with filter_param_factory

62b0165

Refactor add type option for filter_param_factory

57c3700

Fix Get Event Logs with latest paginated_select

8fae068

Refactor Get Assets Event

c5acbdf

Refactor List Dag Warnings

593e684

Refactor DagRun related

2721388

- QueryLastDagRunStateFilter - dag_ids of get_dag_stats

Remove unused parameters

894982b

Refactor on Dag parameters

1a7a6ec

Add any_equal to FilterParam

695a0d7

Refactor Task Instance

acdf9f0

Fix Get Event Logs type

5fa943e

Fix after rebase

7ea3559

Refactor with search_param_factory

ead3215

Refactor QueryLastDagRunStateFilter

2d728c3

Fix get_list_dag_runs_batch

b2d01db

pierrejeambrun force-pushed the feature/AIP-84/refactor-filter-query-parameters branch from 03d0c56 to b2d01db Compare December 3, 2024 16:03

pierrejeambrun merged commit 31ba41e into apache:main Dec 3, 2024
49 checks passed

jason810496 mentioned this pull request Dec 9, 2024

AIP-84 Filter task instances list by task id and display name #44736

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AIP-84 Refactor Filter Query Parameters #43947

AIP-84 Refactor Filter Query Parameters #43947

jason810496 commented Nov 13, 2024 •

edited

Loading

pierrejeambrun left a comment

pierrejeambrun commented Nov 13, 2024

jason810496 commented Nov 16, 2024

pierrejeambrun commented Nov 18, 2024 •

edited

Loading

jason810496 commented Nov 23, 2024 •

edited

Loading

pierrejeambrun commented Nov 27, 2024

pierrejeambrun left a comment

jason810496 commented Nov 27, 2024

pierrejeambrun left a comment •

edited

Loading

jason810496 commented Nov 29, 2024

jason810496 commented Dec 2, 2024

pierrejeambrun left a comment

pierrejeambrun commented Dec 3, 2024

AIP-84 Refactor Filter Query Parameters #43947

AIP-84 Refactor Filter Query Parameters #43947

Conversation

jason810496 commented Nov 13, 2024 • edited Loading

pierrejeambrun left a comment

Choose a reason for hiding this comment

pierrejeambrun commented Nov 13, 2024

jason810496 commented Nov 16, 2024

pierrejeambrun commented Nov 18, 2024 • edited Loading

jason810496 commented Nov 23, 2024 • edited Loading

Follow-Up

Question

pierrejeambrun commented Nov 27, 2024

pierrejeambrun left a comment

Choose a reason for hiding this comment

jason810496 commented Nov 27, 2024

pierrejeambrun left a comment • edited Loading

Choose a reason for hiding this comment

jason810496 commented Nov 29, 2024

jason810496 commented Dec 2, 2024

pierrejeambrun left a comment

Choose a reason for hiding this comment

pierrejeambrun commented Dec 3, 2024

jason810496 commented Nov 13, 2024 •

edited

Loading

pierrejeambrun commented Nov 18, 2024 •

edited

Loading

jason810496 commented Nov 23, 2024 •

edited

Loading

pierrejeambrun left a comment •

edited

Loading