feat(types): improve order by type safety #255
Conversation
Codecov Report
@@ Coverage Diff @@
## main #255 +/- ##
=======================================
Coverage 96.74% 96.75%
=======================================
Files 111 111
Lines 5412 5420 +8
Branches 319 320 +1
=======================================
+ Hits 5236 5244 +8
Misses 130 130
Partials 46 46
Continue to review full report at Codecov.
|
|
If you need to order by multiple fields you can pass a list to posts = await Post.prisma().find_many(
order=[
{'created_at': 'desc'},
{'views': 'desc'},
],
) |
|
The solution is extremely inflexible and broke our code in development/testing branches, because it requires a def some_endpoint(sorty_by_field: str):
posts = await Post.prisma().find_many(
order=[
{sort_by_field: 'desc'}
],
)Does not work. We tried to make Literals, Finals, but it fails on |
|
@danigosa oh that's unfortunate, I didn't realise that wouldn't work. Does this work for you? from prisma.types import PostScalarFieldKeys
def some_endpoint(sorty_by_field: PostScalarFieldKeys) -> None:
posts = await Post.prisma().find_many(
order=[
{sort_by_field: 'desc'}
],
) |
|
I can't find any
Where this type is generated? |
|
@danigosa sorry I should've been more clear, that type is generated for every model in your schema file. So Are there other types like |
Yes! I know documenting is hard, but this is really awesome and undocumented 😆 |
Unfortunately with mypy it does not work: @router.get(
"",
response_model=Paginated[CenterWithoutRelations],
response_class=ORJSONResponse,
)
async def list_centers(
take: int = None,
skip: int = None,
sort_by: CenterScalarFieldKeys = "id",
order: SortOrder = "asc",
db: Prisma = Depends(db_from_request),
) -> Paginated[CenterWithoutRelations]:
"""LIST"""
if sort_by not in CenterWithoutRelations.__fields__:
return ORJSONResponse(
status_code=400, content={"message": "Incorrect sort_by field"}
)
data = await db.center.find_many(
take=take,
skip=skip,
where={"deleted": False},
order={sort_by: order},
)
if skip is not None:
count = await db.center.count()
else:
count = len(data)
return {"data": data, "count": count}➜ care-api git:(1446-care-api-dev-deploy) ✗ make stylecheck
Loading .env environment variables...
src/robbie/care_api/v1/centers.py:46: error: Argument "order" to "find_many" of "CenterActions" has incompatible type "Dict[Literal['id', 'org_id', 'name', 'created_at', 'modified_at', 'deleted'], Literal['asc', 'desc']]"; expected "Union[_Center_id_OrderByInput, _Center_org_id_OrderByInput, _Center_name_OrderByInput, _Center_created_at_OrderByInput, _Center_modified_at_OrderByInput, _Center_deleted_OrderByInput, List[Union[_Center_id_OrderByInput, _Center_org_id_OrderByInput, _Center_name_OrderByInput, _Center_created_at_OrderByInput, _Center_modified_at_OrderByInput, _Center_deleted_OrderByInput]], None]"
src/robbie/care_api/v1/centers.py:46: error: Expected TypedDict key to be string literalThe types code: CenterScalarFieldKeys = Literal[
'id',
'org_id',
'name',
'created_at',
'modified_at',
'deleted',
]I think the problem is using |
|
Looking at https://peps.python.org/pep-0589/#rejected-alternatives I can see why is not working, |
|
Adding |
Yeah the documentation needs some more love to be honest... Any suggestions are more than welcome :)
I can replicate the errors you're seeing with both pyright and mypy. It saddens me to say this but I think this is just one of those cases where you'll have to ignore the error unfortunately. If you do this in multiple places I'd recommend extracting it to a function that returns the corresponding @danigosa Thanks for looking into this and raising it, I was not aware that this did not work! This should probably also be raised on typing-sig / mypy / pyright to see if it would be feasible to support officially. If I receive signal from other users that this change should be reverted, I'll look into it but for the time being I think this change provides a good amount of value as it signals to users directly writing the |
Yeah we are bypassing it with After a talk with some team mates it's really a limitation on |
|
Yeah casting is the best solution.
As far as I'm aware, the list of dictionaries is required as the order of sorting matters when you're sorting by multiple fields. If it was sent in a single dictionary then the ordering of the fields is not strictly defined which could lead to some unexpected results. |
Change Summary
Improves the
ordertype to signify that only one field can be present at any given time.Checklist
Agreement
By submitting this pull request, I confirm that you can use, modify, copy and redistribute this contribution, under the terms of your choice.