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

Non-informative error on creation #154

Open
ghilesmeddour opened this issue Apr 4, 2022 · 3 comments
Open

Non-informative error on creation #154

ghilesmeddour opened this issue Apr 4, 2022 · 3 comments

Comments

@ghilesmeddour
Copy link

Hi,

HTTPException(422, "Key already exists") if raised for every IntegrityError on creation. This can be confusing. I was wondering whether it would be better to use CRUDGenerator _raise method to have a more specific error message.

Ghiles

@xkortex
Copy link

xkortex commented May 19, 2022

I just got bit by this. I was wracking my brain trying to figure out how I could be getting Key already exists errors with a completely empty database. So I debugged the _create function and found out the error it was eating was actually
sqlalchemy.exc.IntegrityError: (psycopg2.errors.NotNullViolation) null value in column "created_on" violates not-null constraint (I had forgot to set server_default=sa.sql.func.now() for the column).

I agree, doing a self._raise(e) just like the _update method would be an easy fix here.

@rodg
Copy link

rodg commented Dec 20, 2022

Yeah I just ran into this as well, going to +1 having the option to just raise the errors up. I get the point mentioned in other issues (like #66 ), but at the same time I find this package useful for stuff that never sees the light of day (quick tests, internal tools) so having a bonus option on the create route to have it just print the real error would be very helpful. If we could just toggle a boolean to have the "less safe but more useful" errors that would be a good change.

Something more ideal might be having the ability to pass in an error handling function to the router? I don't think it'd be 100% necessary but nice error handling customization would be cool IMO.

@rodg
Copy link

rodg commented Dec 20, 2022

In the mean time this little hack seems to work (not exhaustively tested):

from typing import Callable, Any

from fastapi_crudrouter import SQLAlchemyCRUDRouter as _SQLACRUDRouter
from sqlalchemy.orm import Session
from sqlalchemy.ext.declarative import DeclarativeMeta as Model
from sqlalchemy.exc import IntegrityError

from fastapi import Depends

CALLABLE = Callable[..., Model]


class SQLAlchemyCRUDRouter(_SQLACRUDRouter):
    def _create(self, *args: Any, **kwargs: Any) -> CALLABLE:
        def route(
            model: self.create_schema,  # type: ignore
            db: Session = Depends(self.db_func),
        ) -> Model:
            try:
                db_model: Model = self.db_model(**model.dict())
                db.add(db_model)
                db.commit()
                db.refresh(db_model)
                return db_model
            except IntegrityError as e:
                db.rollback()
                self._raise(e)

        return route

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

No branches or pull requests

3 participants