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

Simplify CRUD definitions, make a clearer distinction between schemas and models #23

Merged
merged 33 commits into from
Jan 19, 2020

Conversation

ebreton
Copy link
Contributor

@ebreton ebreton commented May 3, 2019

This PR facilitates the creation of the basic CRUD operations for new db_models objects.

Motivation: get DRYer

If you don't care for IDE embedded auto-completion, you willl not need to copy/paste app.crud.items.py into app.crud.your_model.py file anymore. One line is sufficient in app.crud.__init__.py (with appropriate import):

your_model = CrudBase(YourModel)

If you care about "intellisense", then you will still have a bit of copy-pasting for the sake of auto-completion, and leverage your favorite IDE. But you will still gain time (and lines of code)

Solution
The PR therefore defines a CrudBase class, which instances will provide the get/get_multi/create/udpate/delete/remove operations for a given SQLAlchemy object. I have tried to be as detailed as possible in the documentation of app.crud.base.py

Along with this new tool, I have added a new SubItem model that showcases the usage of CrudBase. I have also modified Item with the same objective. Tests have been added or updated consequently.

because we should be DRY 🚀

Bonus 1: reflect the fastapi documentation by using schemas (when pydantic) and models (when sqlalchemy)

I have updated the tree structure to reflect the latest documentation from fastapi (https://fastapi.tiangolo.com/tutorial/sql-databases/). Hence renaming models to schemas and db_models to models.

Bonus 2: run the test more easily

when runing sh tests.sh from the root directory, arguments are forward to scripts/test.sh and then to .../test-start.sh.

Thats allows the user to pass whatever argument he would pass to pytest, like -x -v -k filter --pdb (pick your favorite)

when sh tests.sh fails and you want to quicky run the test again and again (without deleting the entire testing-projectand generating again (including docker image), then you can user sh test-again.sh that will rsync the testing-project src code with your latest modifications.

Bonus 3: easy casting from schema to model (and vice versa)

Not only we I have tried to be very explicit on which kind object is used, i.e

from app.models import subitem as models_subitem
from app.schemas import subitem as schemas_subitem

subitem = CrudBase(models_subitem.SubItem, schemas_subitem.SubItem)

But the models provide 2 functions to ease the casting_

class CustomBase(object):

    def to_schema(self, schema_cls):
        return schema_cls(**self.__dict__)

    @classmethod
    def from_schema(cls, schema_obj):
        return cls(**jsonable_encoder(schema_obj))

which allows you to easily cast a model instance to any schema you provide, or create the model object from a schema instance.

Those 2 functions are used in the base crud definitions of update and create, and makes sure that all types will be properly cast (in particular non directly JSONable ones like datetime or enums)

Bonus 4: normal user for tests

without the need to create a new one in each and every tests...

That basically comes from PR #20

@tiangolo
Copy link
Member

Very interesting ideas! I'll check it thoroughly soon.

Merge in tiangolo branch
@wassname
Copy link

Just to chime in, I'm not a contributor but been using the contents of this PR in my project and it works fine and seems cleaner.

@wassname
Copy link

wassname commented Aug 27, 2019

Sorry for all the comments @ebreton, they are because I liked and used the PR :)

@ebreton
Copy link
Contributor Author

ebreton commented Aug 27, 2019

no worries @wassname , its awesome to see you comment and improve :)
I am just deep underwater right now, but I appreciate 👍

@wassname
Copy link

wassname commented Sep 5, 2019

A little bit tangential, but on the topic of crud, here's an approach I came across using sqlalchemy mixins.

@ebreton
Copy link
Contributor Author

ebreton commented Sep 6, 2019

A little bit tangential, but on the topic of crud, here's an approach I came across using sqlalchemy mixins.

I will need to dive a bit further into SQLAlchemy, I have not used their MixIns yet

@omrihar
Copy link

omrihar commented Sep 6, 2019

I've been using sqlalchemy_mixins for a while now, and actually it made me make some substantial changes to the template. For example, it's not necessary to pass the database as a dependency injection anymore, because you can set the session on the base class. It's quite convenient I must say. I still use the crud object though because sometimes the operations necessary can be quite complex.

@wassname
Copy link

wassname commented Sep 7, 2019

I don't suppose you could share an example, or a gist few anonymized files? It's always interesting to see minor implementation details of something we're working on.

from app.models import subitem as models_subitem
from app.schemas import subitem as schemas_subitem

subitem = CrudBase(models_subitem.SubItem, schemas_subitem.SubItem)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a good idea to use module variables (effectively globals/singletons)? What happens if you get multiple requests and one of them is using this object?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those variables are just "gateways" from/into the DB. It will be the DB which will make sure that there is no conflict when multiple requests come in.

@tiangolo
Copy link
Member

Thank you @ebreton for your contribution! 🍰

Thanks everyone for the PR feedback.

@omrihar I suggest you don't add the session to your models instead of passing the DB session around. That would mean that you would have to resort to SQLAlchemy's thread-local handling (or the equivalent from some other library). Those techniques assume a framework that assigns exactly one single thread per request. But as FastAPI is async, a single thread could handle more than one request concurrently, and those models using the same session would create conflicts and errors. Also, as FastAPI could run some parts of the code in a threadpool (normal def functions), those sessions that were local to one thread would not be available for those other functions.

If you really want to use a technique like those, you would have to use contextvars, but you would have to use Python 3.7, make sure you set the ContextVar in a middleware, and open and close the sessions in a dependency with yield, etc. It's possible, but it might be a bit fragile. And as it works kinda "magically", understanding it while implementing it is quite mind-bending. For more details about it, see the section about Peewee, that does that same thing, more or less: https://fastapi.tiangolo.com/advanced/sql-databases-peewee/

Anyway, I suggest you just keep the session around. It's a lot more explicit and easy to understand and debug.


I liked several of the ideas here @ebreton ! 🚀

It was a bit difficult to review as there were a lot of changes that were not related to the PR, so it took me a while to get around it 😅. But I just finished updating it to leave it in a state where it's just the proposed change.

I updated the CRUDBase to simplify it and use Generics, that way the other sub-classes can still have all the types and editor support without having to override all the methods.

I want to have just the minimum code that can work as a base for almost any project. So I removed several things that aren't necessarily always needed. I removed the sub-items models, and some methods. I removed the custom logs, as logging personal information that ends up stored in logs somewhere would make it difficult to be conformant with GDPR and similar privacy regulations. I'm not sure about adopting a specific format for docstrings, but I prefer to delay that decision until later and merge the main idea here.

Any other of the changes can be implemented/reviewed in independent PRs, it's a lot easier like that. And that way we can have the CRUDBase here now.

Thanks for your effort! 🌮 🎉

@tiangolo tiangolo merged commit ab46165 into fastapi:master Jan 19, 2020
@ebreton
Copy link
Contributor Author

ebreton commented Jan 20, 2020

Thanks @tiangolo , that's awesome !!

I apologize for the heaviness of the PR, and I will surely make the other PR smaller in the future. 🔬 😄

Thank you so much for fastAPI and this sibling project. We are using it on daily basis 🌟 💎

ebreton pushed a commit to ebreton/full-stack-fastapi-postgresql that referenced this pull request Jan 26, 2020
…-postgresql into tiangolo-master

* 'master' of git://github.com/tiangolo/full-stack-fastapi-postgresql:
  📝 Update release notes
  ✨ Add base class to simplify CRUD (fastapi#23)
  📝 Update release notes
  ✅ Add normal-user fixture for testing (fastapi#20)
ebreton pushed a commit to ebreton/full-stack-fastapi-postgresql that referenced this pull request Jan 26, 2020
* tiangolo-master:
  📝 Update release notes
  ✨ Add base class to simplify CRUD (fastapi#23)
  📝 Update release notes
  ✅ Add normal-user fixture for testing (fastapi#20)
@ebreton ebreton deleted the feature-add-crud-base branch January 26, 2020 18:28
paul121 added a commit to paul121/farmOS-aggregator that referenced this pull request Jan 29, 2020
paul121 added a commit to paul121/farmOS-aggregator that referenced this pull request Feb 13, 2020
paul121 added a commit to farmOS/farmOS-aggregator that referenced this pull request Feb 13, 2020
@timhughes
Copy link

@ebreton @tiangolo Just a thought on this Pull Request as someone who is learning. This PR has taken the example application away from the examples in the FastAPI documentation. It would be good if the example application and the docs were somewhat similar so that people could see real world examples. Specifically I am talking about the CRUD changes and things like UserInDB which appears to be no longer in use. I do appreciate that the code is cleaner in it's current incantation (and I have learnt a thing ot two about generics - thanks) but for someone who is following the documentation and trying to work out how everything fits together it makes understanding more difficult.

I am not overly concerned from my own point of view because just checked out the revision before the merge and that helped me understand a lot better. I just think the the old style of code was simpler to understand for someone coming at this for the first time.

Thanks for all the awesome work.

@ebreton
Copy link
Contributor Author

ebreton commented Mar 22, 2020

You are right: I have focused on documenting the PR, and I have failed at updating the doc.

I guess it will come, but any help from you would be more than welcomed 😇 .

Would you please make a try at a PR where you could share your experience ? I would say that it would make the learning process much simpler, even simplest than the previous one once you get the trick (and one can always stick to the first version if it remains an option in the doc too)

gusevyaroslove pushed a commit to gusevyaroslove/fastapi-template that referenced this pull request Aug 4, 2024
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.

8 participants