269 Stop multi-dimensional array and email filtering from breaking

[dantownsend]

Resolves #269

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 89.74359% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 92.59%. Comparing base (2df940d) to head (a8c213e).

Files	Patch %	Lines
piccolo_api/utils/types.py	81.25%	3 Missing ⚠️
piccolo_api/crud/endpoints.py	94.11%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #270      +/-   ##
==========================================
- Coverage   92.68%   92.59%   -0.10%     
==========================================
  Files          33       34       +1     
  Lines        2038     2052      +14     
==========================================
+ Hits         1889     1900      +11     
- Misses        149      152       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dantownsend]

Thinking about this some more - we could modify create_pydantic_model instead, so it adds something like 'multidimensional' or 'dimensions' to the JSON schema, and we read it in here.

Edit: Turns out using Body isn't a great solution - it stops FastAPI from complaining, but when actually call the API using a GET with a body, it gets rejected.

[sinisaos]

Edit: Turns out using Body isn't a great solution - it stops FastAPI from complaining, but when actually call the API using a GET with a body, it gets rejected.

@dantownsend It's the same problem we have here because Piccolo Admin depends on the Piccolo API. FastAPI does not support nested lists via Query parameters. I already tried using Body in the Piccolo Admin issue, but I get the same error as in your case. I have no idea how to solve this due to FastAPI limitations.

[dantownsend]

@sinisaos I looked into it a bit more, and it seems like OpenAPI allows arrays as query params, but not multidimensional arrays:

https://swagger.io/docs/specification/describing-parameters/#query-parameters
https://swagger.io/docs/specification/serialization/#query

Interestingly, Axios does allow multidimensional arrays (some_key[][]=1) - it's built on top of this:

https://www.npmjs.com/package/qs

So I think FastAPI only allows basic arrays because that's all that the OpenAPI JS interface supports.

I think the options are:

• Don't add filters for multidimensional arrays
• Allow a list of values to be passed in, and the filter works by looking for a match anyway in the nested array:

array_1 =  [["a", "b", "c"], ["d", "e", "f"]]
array_2 = [["g", "h", "i"], ["j", "k", "l"]]

# if the query is "a", then return array_1

I'm not concerned with getting a perfect solution - just something which kind of works without crashing, as multidimensional arrays is definitely an edgecase.

[sinisaos]

I think this should be for checking a multidimensional list

if args[0] is list:
    return True

because if args and t.get_origin(args[0]) is list: always returns False

[dantownsend]

Thanks - I need to test it.

[sinisaos]

• Don't add filters for multidimensional arrays

That would be easiest with something like this

if _is_multidimensional_array(type_=type_):
    pass
else:
    # previous parameters code unchanged

That works both GET and POST request in interactive docs.

• Allow a list of values to be passed in, and the filter works by looking for a match anyway in the nested array:

That would be great, but I don't know how to get around the FastAPI error from the previous comment

[dantownsend]

• Don't add filters for multidimensional arrays

That would be easiest with something like this

if _is_multidimensional_array(type_=type_):
    pass
else:
    # previous parameters code unchanged

That works both GET and POST request in interactive docs.

• Allow a list of values to be passed in, and the filter works by looking for a match anyway in the nested array:

That would be great, but I don't know how to get around the FastAPI error from the previous comment

Yes, that's definitely an option. If getting filters to work is too much hassle, that's definitely what we need to do.

It turns out that the SQL for filtering multidimensional arrays to check if they contain a value is fairly simple:

-- Get me all rows where the array (or sub array) contains 'a'
select * from my_table where exists (select 1 from unnest(my_column) i where i = 'a');

EDIT: even any works for multidimensional arrays, which is surprising:

select * from my_table where 'a' = any(my_column);

[sinisaos]

Filtering nested lists is not a problem on the ORM side. Filtering works with raw SQL or with ORM query like this

await MyTable.select().where(MyTable.my_column.any(1)) # this works without problem

I'm talking about the FastAPI(OpenAPI) error for using Query (Body also does not work) because I don't know how to include nested lists (or convert to one dimensional list) in FastAPI docs. Hope you understand what I mean.

[dantownsend]

@sinisaos For that we just need to do something like:

if _is_multidimensional_array(type_):
    type_ = list

[dantownsend]

This still isn't perfect - but I think I know the solution now.

Rather than changing it within FastAPIWrapper, we need to modify PiccoloCRUD.pydantic_model_optional instead, because PiccoloCRUD also uses this to validate the query params.

[dantownsend]

This PR will fail until a new version of Piccolo is released which contains Table._meta.array_columns.

[sinisaos]

@dantownsend I tried your changes and they work without problems in FastAPI docs. A clever solution, which solves the problems I tried to explain in the my previous comments. Tests will pass when you release Piccolo ORM changes. Great job :)

[dantownsend]

@sinisaos Thanks. I think I'll move get_array_base_type into the main Piccolo repo, as a method on the Array column, and then will merge it in.

[dantownsend]

Now we've added a separate Pydantic model for handling filters, I noticed that we need this not just for edge cases with multidimensional arrays, but also things like Email columns.

create_pydantic_model by default turns Email columns into the pydantic.EmailStr type. But when filtering, we might want to enter gmail which isn't a valid email address, so we get an error. For filtering, we just want Email columns to be str.

[dantownsend]

I think this is basically done now ... but it could do with tests for filtering email and multidimensional array columns in GET requests.