full-stack-fastapi-template icon indicating copy to clipboard operation
full-stack-fastapi-template copied to clipboard

Default crud uses hardcoded id over general get from SQLAlchemy

Open Hultner opened this issue 4 years ago • 2 comments

SQLAlchemy supports a built in get this is used in crud.delete but note crud.get, changing this will greatly enhance usability of the base.crud as it will work with tables where the primary key isn't named id as well.

Suggestion

Change this: https://github.com/tiangolo/full-stack-fastapi-postgresql/blob/490c554e23343eec0736b06e59b2108fdd057fdc/%7B%7Bcookiecutter.project_slug%7D%7D/backend/app/app/crud/base.py#L26-L27

To work like this: https://github.com/tiangolo/full-stack-fastapi-postgresql/blob/490c554e23343eec0736b06e59b2108fdd057fdc/%7B%7Bcookiecutter.project_slug%7D%7D/backend/app/app/crud/base.py#L62-L66

I've made this change a couple of weeks back in my project and it works great, no drawbacks but lots of benefits.

Hultner avatar Oct 06 '20 11:10 Hultner

@tiangolo I've suggested a couple of improvements based on changes I've made in my own project. I could make a PR implementing all or some of these changes, if they are desirable in the upstream template. But I don't want to pollute your PR queue if they aren't desirable.

Hultner avatar Oct 06 '20 11:10 Hultner

It's better to use Session.get because Query.get is deprecated since SQLAlchemy 1.4.

def get(self, db: Session, id: Any) -> Optional[ModelType]: 
     return db.get(self.model, id)
def remove(self, db: Session, *, id: Any) -> ModelType: 
     obj = db.get(self.model, id) 
     db.delete(obj) 
     db.commit() 
     return obj

Ae-Mc avatar Jan 04 '22 16:01 Ae-Mc