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

FastAPI API Add test query params standardization an reusability #42320

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

pierrejeambrun
Copy link
Member

@pierrejeambrun pierrejeambrun commented Sep 18, 2024

Related to: #42159

This will abstract the handling of common FIlters, Search and Sort query param so we do not have to copy and past the same query.where(...) on all endpoints that are using this same logic.

This allows:

  • To remove some logic from the views, and only keep http related thing. (serialization/deserialization, query param validation, etc...). Aim for a more layed and decouple AIP code. (hexagonal / onion architecture is an inspiration, but that's too early and not a priority). Cf the custom code removed from the get_dags view.
  • Initialize some reusable logic for the db layer
  • Will allow to add more complex filters that cannot be done today, especially on the order_by because we were using a sqlalchemy text clause that do not work with hybrid properties. (cf follow up PR to add ordering by dag_display_name)

The goal is to have that extended and have a library of query params that can be re-used accross all the RestAPI.

return select.order_by(getattr(DagModel, lstriped_orderby).desc())
else:
return select.order_by(getattr(DagModel, lstriped_orderby).asc())
return select
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return select

This will never be hit, the else will.

Comment on lines +150 to +154
if self.value:
conditions = [DagModel.tags.any(DagTag.name == tag) for tag in self.value]
return select.where(or_(*conditions))

return select
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if self.value:
conditions = [DagModel.tags.any(DagTag.name == tag) for tag in self.value]
return select.where(or_(*conditions))
return select
if self.value is None:
return select
conditions = [DagModel.tags.any(DagTag.name == tag) for tag in self.value]
return select.where(or_(*conditions))

Keep it consistent with the others, plus short circuiting out is easier to read imo.

Comment on lines +77 to +79
self.value = offset

return self
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.value = offset
return self
return self.set_value(offset)

Comment on lines +63 to +65
self.value = limit

return self
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.value = limit
return self
return self.set_value(limit)

Comment on lines +22 to +24
from sqlalchemy.sql import Select

from airflow.api_fastapi.parameters import BaseParam
Copy link
Member

Choose a reason for hiding this comment

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

Should move these into the if TYPE_CHECKING block below, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Would not ruff tell you so :-D

from abc import ABC, abstractmethod
from typing import Any, Generic, List, TypeVar, Union

from fastapi import Depends, HTTPException, Query as Query
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
from fastapi import Depends, HTTPException, Query as Query
from fastapi import Depends, HTTPException, Query

items:
type: string
- type: 'null'
type: array
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this chane not create the requirement to always pass at least an empty array and does not allow skipping this filter? Is this a breaking change or do I mis-interpret?

Comment on lines +138 to +140
else:
return select.order_by(getattr(DagModel, lstriped_orderby).asc())
return select
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 but then it needs to be:

Suggested change
else:
return select.order_by(getattr(DagModel, lstriped_orderby).asc())
return select
return select.order_by(getattr(DagModel, lstriped_orderby).asc())

Copy link
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

Learning some Python magic every day. That query stuff smells like black magic... but looks cool!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AIP-84 Modern Rest API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants