sts-sponsors team mailing list archive

[Adam Collard <mp+438678@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> diff --git a/backend/msm/db/queries.py b/backend/msm/db/queries.py
> index ce2bd93..19ba033 100644
> --- a/backend/msm/db/queries.py
> +++ b/backend/msm/db/queries.py
> @@ -15,6 +25,87 @@ from ._tables import (
>  )
>  
>  
> +def filters_from_arguments(
> +    table: Table,
> +    **kwargs: list[str] | None,

**kwargs is a dict[str, ...] not a list of strs

> +) -> ColumnElement[bool]:
> +    """
> +    Joins all keys of the kwargs by AND and all entries for a single arg by OR.
> +    This enables to convert query params such as
> +
> +      ?name=name1&name=name2&city=city
> +
> +    to a where clause such as
> +
> +      (name ilike %name1% OR name ilike %name2%) AND city ilike %city%
> +
> +    :param table: the table to create the WHERE clause for
> +    :param kwargs: the parameters matching the table's column name
> +                   as keys and lists of Strings that will be matched
> +                   via ilike
> +    :returns: a where clause that joins all queries per column
> +                  with OR and all columns with AND
> +    """
> +    ands: list[ColumnElement[Any]] = []
> +    for key in kwargs:
> +        entries = kwargs[key]

for key, entries in kwargs.items()

> +        if entries and len(entries) == 1:

feels like a good match (pun intended) for Python's match statement https://docs.python.org/3/tutorial/controlflow.html#match-statements

> +            ands.append(
> +                cast(
> +                    ColumnElement[Any],
> +                    table.c[key].icontains(entries[0], autoescape=True),
> +                )
> +            )
> +        if entries and len(entries) > 1:
> +            ors: list[ColumnElement[Any]] = [
> +                cast(
> +                    ColumnElement[Any],
> +                    table.c[key].icontains(entry, autoescape=True),
> +                )
> +                for entry in entries
> +            ]
> +            ands.append(or_(true(), *ors))
> +    return and_(true(), *ands)
> +
> +
> +async def get_filtered_sites(
> +    session: AsyncSession,
> +    city: list[str] | None = [],
> +    name: list[str] | None = [],
> +    note: list[str] | None = [],
> +    region: list[str] | None = [],
> +    street: list[str] | None = [],
> +    timezone: list[str] | None = [],
> +    url: list[str] | None = [],
> +) -> Iterable[dict[str, Any]]:

can we be stricter about the types here - we should know what the types of the keys and values are for the returned dict

> +    where = filters_from_arguments(
> +        Site,
> +        city=city,
> +        name=name,
> +        note=note,
> +        region=region,
> +        street=street,
> +        timezone=timezone,
> +        url=url,
> +    )
> +
> +    stmt = select(
> +        Site.c.id,
> +        Site.c.name,
> +        Site.c.identifier,
> +        Site.c.city,
> +        Site.c.latitude,
> +        Site.c.longitude,
> +        Site.c.note,
> +        Site.c.region,
> +        Site.c.street,
> +        Site.c.timezone,
> +        Site.c.url,
> +    ).where(where)
> +    result = await session.execute(stmt)
> +    return (row._asdict() for row in result.all())
> +
> +
>  async def get_sites(session: AsyncSession) -> Iterable[dict[str, Any]]:
>      stmt = select(
>          Site.c.id,


-- 
https://code.launchpad.net/~thorsten-merten/maas-site-manager/+git/maas-site-manager/+merge/438678
Your team MAAS Committers is subscribed to branch ~thorsten-merten/maas-site-manager:MAASENG-1290-add-filter-to-sites.