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: Make default filter, sort and split_str a no-op, plus various improvements #272

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

Conversation

bartelemi
Copy link
Contributor

what.

  • Make the base implementations of filter, sort and split_str validator a no-operation (i.e. don't modify data, just return the query or value).
  • Additional entries to .gitignore
  • Improve docstrings and their formatting.
  • Bump setup to Poetry v1.3.2
  • Change FastAPI constraint to <1.0.0

why.

The no-op change is required for custom Filter classes that do not want to implement all the methods or (which is more important) don't want to opt-in for the list query param handling offered by this library.

FastAPI (and Starlette) do handle list-like query params, but in a format of ?list=1&list=2&list=3 instead of ?list=1,2,3. For me, it was a big deal to not use the built-in behavior, for following reasons: 1) performance impact (even if minimal), 2) OpenAPI schema mismatch - if using this lib behavior you loose strong typing for list query parameters.

Also make the default filter & sort a no-op and improve doc formatting
@netlify
Copy link

netlify bot commented Feb 9, 2023

Deploy Preview for fastapi-filter ready!

Name Link
🔨 Latest commit ca7dc9e
🔍 Latest deploy log https://app.netlify.com/sites/fastapi-filter/deploys/63ea722fe410df0008072195
😎 Deploy Preview https://deploy-preview-272--fastapi-filter.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@codecov
Copy link

codecov bot commented Feb 10, 2023

Codecov Report

Merging #272 (ca7dc9e) into main (6d6f11e) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #272   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            6         6           
  Lines          175       175           
=========================================
  Hits           175       175           
Impacted Files Coverage Δ
fastapi_filter/base/filter.py 100.00% <ø> (ø)
fastapi_filter/contrib/mongoengine/filter.py 100.00% <ø> (ø)
fastapi_filter/contrib/sqlalchemy/filter.py 100.00% <ø> (ø)

@arthurio arthurio self-requested a review February 10, 2023 05:11
@arthurio
Copy link
Owner

@bartelemi Thanks for the suggestion! I guess I missed that entry in the docs (https://fastapi.tiangolo.com/tutorial/query-params-str-validations/#query-parameter-list-multiple-values). I vaguely remember that my list filters would not show in swagger, hence the _list_to_str_fields hack (which keeps the typing btw). I'll try to take some time to test things out without that hack and make ?q=1&q=2 the native behavior if it works as intended. I'd rather use the supported stuff than making something up... It would mean that the __in queries will be super verbose, like ?user_age__in=1&user_age__in=2... but I think that's fine since the front-end will most likely generate the query string?

@arthurio
Copy link
Owner

After refreshing my memory a bit, the issue is that fastapi doesn't support lists within pydantic models called with Depends. See fastapi/fastapi#2869 (comment). I'm curious to understand how you are able to do so, could you share a small example?

@bartelemi
Copy link
Contributor Author

I am not 100% sure if this is not supported or simply not documented well, but I found that the workaround posted in this comment fastapi/fastapi#4445 (comment) works just fine. I run into a problem that list-type parameters in the pydantic model were translated into a body field params (instead being a URL query parameters). Wrapping the list query param with a field like below seems to solve the issue:

 class QueryModel(BaseModel):
-    problematic: Optional[list[str]] = Query([])
+    problematic: Optional[list[str]] = Field(Query([]))

I will try to find some time to post here a small working example. Before that here's what I did to get it working with the recent version of this library:

  1. Skip using the FilterDepends and simply use the fastapi's Depends.
  2. Create my own version of the base Filter class which overrides the split_str behavior and returns unmodified fields.
  3. When defining a model field, I use the above workaround, for instance:
    from datetime import date
    from typing import Optional
    
    from fastapi import Query
    from pydantic import Field
    
    from my_project.enums import BillingStatus
    from my_project.models import Invoice
    from my_project.utils import Filter  # modified Filter class with the split_str override
    
    class InvoiceQueryParams(Filter):
        invoice_date__gte: Optional[date]
        invoice_date__lte: Optional[date]
        status__in: Optional[list[BillingStatus]] = Field(Query(None))
        order_by: Optional[list[str]] = Field(Query(default=["-invoice_date"]))
        search: Optional[str]
    
        class Constants(Filter.Constants):
            model = Invoice
            search_model_fields = ["id", "company_name"]

@arthurio
Copy link
Owner

@bartelemi I played briefly with the Field trick and it works great so I think I'll try to combine your changes with updating the _list_to_str_fields trick to instead set the list fields to Field(Query(...)) (need to look into keeping the defaults and whatnot).

@bartelemi
Copy link
Contributor Author

Cool, I'll be happy to review a PR and test it in my projects if you want me to.

Meanwhile, I resolved dependency conflicts in this PR, so if you want to merge it first then go ahead. There shouldn't be any breaking changes to logic or behavior of the lib, if anything it should make it easier to build custom Filters.

Copy link
Owner

@arthurio arthurio left a comment

Choose a reason for hiding this comment

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

As discussed, I think that making things work with Field(Query(...)) wrapper would probably be better. I know you said that having no-ops would help you define custom filters more easily but I don't really understand why. I'm not saying we shouldn't do it but if you can provide an example, that would help me a lot to understand!

Otherwise, I know that I haven't made a "contributing" template or document yet but I very much prefer small and focused PRs that address one issue. Here I'm not against keeping the .gitignore and comment changes.


- uses: snok/install-poetry@v1
with:
version: 1.2.2
version: 1.3.2
virtualenvs-create: true
virtualenvs-in-project: true
Copy link
Owner

Choose a reason for hiding this comment

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

Can we move the poetry upgrade to a separate PR?

@@ -14,23 +14,22 @@ class BaseFilterModel(BaseModel, extra=Extra.forbid):

Provides the interface for filtering and ordering.

## Ordering
Copy link
Owner

Choose a reason for hiding this comment

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

Just curious why ## instead of #?

@@ -68,7 +67,7 @@ def ordering_values(self):

@validator("*", pre=True)
def split_str(cls, value, field): # pragma: no cover
...
return value
Copy link
Owner

Choose a reason for hiding this comment

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

The reason I used ... vs. a no-op operation was to make BaseFilterModel a loose "interface". I'm curious to understand why you need to have to use a no-op in your custom filters. If you don't plan on using one of the methods, you can just not use it, no? Could you provide a use case to illustrate the need of defining a no-op instead?

@@ -47,7 +47,7 @@ classifiers = [

[tool.poetry.dependencies]
python = ">=3.8,<3.12"
fastapi = ">=0.78,<0.90"
fastapi = ">=0.78,<1.0.0"
Copy link
Owner

Choose a reason for hiding this comment

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

I prefer managing the dependency upgrades in individual PRs, can you remove the dependency upgrades?


## Query string examples:
### Query string examples::
Copy link
Owner

Choose a reason for hiding this comment

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

I have never been a docstring formatting expert, why do you use the :: here?

arthurio added a commit that referenced this pull request Feb 16, 2023
@arthurio
Copy link
Owner

@bartelemi I plan on coming back to this after #447 lands.

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.

2 participants