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

Added error for lookup by primary key #759

Merged
merged 7 commits into from
Jun 12, 2024

Conversation

spacemanspiff2007
Copy link
Contributor

This PR allows to generate nice error messages with a central exception handler (e.g. in FastAPI) - see #741 for initial idea.
I've changed the exception to ObjectDoesNotExistError since the user explicitly specifies a primary key which makes this a little different than the filter function.
If this will get merged, I'll add something to the docs on how to use it.

async def run():
    try:
        await MyModel[99]
    except ObjectDoesNotExistError as e:
        print(e.model)
        print(e.pk_name)
        print(e.pk_val)
        print(e)

Output:

<class '__main__.MyModel'>
id
99
models.MyModel has no object with id=99

@long2ice
Copy link
Member

There is DoesNotExist already, why new one?

@spacemanspiff2007
Copy link
Contributor Author

This one is only thrown when an item gets accessed by primary key.
If no item is found the error contains the primary key name and value.

That way it is possible to register a central error handler (e.g. in FastAPI) and
return a http response 404 which can contain the Model and pk.

e.g.

{
    "item": "MyModel",
    "id": 99,
    "msg": "MyModel with id 99 does not exist"
}

which is way better than just

{
    "msg": "Object not found"
}

Additionally it is not necessary any more to handle the ObjectNotFoundError in every route.
This leads to simpler and easier to read code.

Imho it makes sense to raise different errors for accessing an item by primary key and for filtering and searching an item.
For the first one I am expecting it to exist (otherwise I wouldn't pass a pk) while with the other one I am searching for an occurrence.

@spacemanspiff2007
Copy link
Contributor Author

spacemanspiff2007 commented May 27, 2021

I have a FastApi application where the user interacts with the db.
With this PR I don't have to check every time I get something passed in:

@router.get(
    path="/filter",
    response_model=List[MyModelApiOut],
    status_code=status.HTTP_200_OK,
)
async def get_models_of_type(type: int, category: int):
    type = await MyModelType[type]
    category= await MyModelCategory[category]

    return [MyModelApiOut.from_orm(k) for k in await MyModel.filter(type=type, category=category)]

Without this PR I have to do this:

@router.get(
    path="/filter",
    response_model=List[MyModelApiOut],
    status_code=status.HTTP_200_OK,
)
async def get_models_of_type(type: int, category: int):
    type = await MyModelType.get_or_none(pk=type)
    if type is None:
        return Response404(pk_name='id', pk_value=type, msg=f'MyModelType with id {type} does not exist')

    category = await MyModelCategory.get_or_none(pk=category)
    if category is None:
        return Response404(pk_name='id', pk_value=category, msg=f'MyModelCategory with id {category} does not exist')

    return [MyModelApiOut.from_orm(k) for k in await MyModel.filter(type=type, category=category)]

I have to explicitly check for every item that get's passed in by the user and issue a corresponding error message.
This may not be much for one endpoint but it quickly adds up and makes the code unnecessarily complex, especially if the user passes in multiple primary keys in the route.

I really think differentiating between accessing and item with the primary key and searching for an item with the filter function is a smart design choice and I would really like to see tortoise-orm go that way.

@spacemanspiff2007
Copy link
Contributor Author

@long2ice
It would really simply things if we can get the pk name, pk value and model on the Exception.
What do I have to do to get this or something similar merged?
I created a new exception because in this context we expect a single object that is queried by pk in contrast to the query by filter values.

@abondar
Copy link
Member

abondar commented Jun 6, 2024

@spacemanspiff2007 would you like to update this PR?
I am okay with merging it after update

But probably need to additionally inherit this error from KeyError for compatibility and because it is expected of __getitem__ to return key error

- added 2 commits June 11, 2024 06:21
# Conflicts:
#	tortoise/exceptions.py
#	tortoise/models.py
@spacemanspiff2007
Copy link
Contributor Author

@abondar like this?

@coveralls
Copy link

coveralls commented Jun 11, 2024

Pull Request Test Coverage Report for Build 9459828839

Details

  • 9 of 9 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 87.854%

Totals Coverage Status
Change from base Build 9345308446: 0.01%
Covered Lines: 5786
Relevant Lines: 6488

💛 - Coveralls

@abondar abondar merged commit d1e4be0 into tortoise:develop Jun 12, 2024
7 checks passed
@spacemanspiff2007 spacemanspiff2007 deleted the error_pk branch June 12, 2024 08:51
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.

4 participants