BeanieODM / beanie

Asynchronous Python ODM for MongoDB
http://beanie-odm.dev/
Apache License 2.0
2.02k stars 213 forks source link

Why Input model need to be init with beanie? #318

Closed nghianv19940 closed 2 years ago

nghianv19940 commented 2 years ago

Hi I'm using beanie with fastapi. this is my model:

class UserBase(Document):
    username: str | None
    parent_id: str | None
    role_id: str | None
    payment_type: str | None
    disabled: bool | None
    note: str | None
    access_token: str | None

class UserIn(UserBase):
    username: str
    password: str
    disabled: bool = False

class User(UserBase):
    username: str
    password: str
    disabled: bool = False
    salt: str

    class Settings:
        name = "users"

class UserOut(UserBase):
    note: str | None
    access_token: str | None

my routes:

USERS = APIRouter()

@USERS.post('/users', response_model=UserOut)
async def post_user(user: UserIn):
    user_to_crate = User(**user.dict(), salt=get_salt())
    await user_to_crate.save()
    user_to_response = UserOut(**user_to_crate.dict())
    return user_to_response

my beane init function:

async def init():
    # Create Motor client
    client = motor.motor_asyncio.AsyncIOMotorClient(
        f"mongodb://{getenv('MONGO_USER')}:{getenv('MONGO_PASSWORD')}"
        f"@{getenv('MONGO_HOST')}/{getenv('MONGO_DATABASE')}?"
        f"replicaSet={getenv('MONGO_REPLICA_SET')}"
        f"&authSource={getenv('MONGO_DATABASE')}"
    )

    # Init beanie
    await init_beanie(database=client[f"{getenv('MONGO_DATABASE')}"],
                      document_models=[User])

if i'm not put UserIn to the init beanie function, then call /users, i will get this error:

INFO:     172.16.16.7:55512 - "POST /users HTTP/1.0" 500 Internal Server Error
ERROR:    Exception in ASGI application
Traceback (most recent call last):
  File "/usr/local/lib/python3.10/site-packages/uvicorn/protocols/http/h11_impl.py", line 403, in run_asgi
    result = await app(self.scope, self.receive, self.send)
  File "/usr/local/lib/python3.10/site-packages/uvicorn/middleware/proxy_headers.py", line 78, in __call__
    return await self.app(scope, receive, send)
  File "/usr/local/lib/python3.10/site-packages/fastapi/applications.py", line 269, in __call__
    await super().__call__(scope, receive, send)
  File "/usr/local/lib/python3.10/site-packages/starlette/applications.py", line 124, in __call__
    await self.middleware_stack(scope, receive, send)
  File "/usr/local/lib/python3.10/site-packages/starlette/middleware/errors.py", line 184, in __call__
    raise exc
  File "/usr/local/lib/python3.10/site-packages/starlette/middleware/errors.py", line 162, in __call__
    await self.app(scope, receive, _send)
  File "/usr/local/lib/python3.10/site-packages/starlette/exceptions.py", line 93, in __call__
    raise exc
  File "/usr/local/lib/python3.10/site-packages/starlette/exceptions.py", line 82, in __call__
    await self.app(scope, receive, sender)
  File "/usr/local/lib/python3.10/site-packages/fastapi/middleware/asyncexitstack.py", line 21, in __call__
    raise e
  File "/usr/local/lib/python3.10/site-packages/fastapi/middleware/asyncexitstack.py", line 18, in __call__
    await self.app(scope, receive, send)
  File "/usr/local/lib/python3.10/site-packages/starlette/routing.py", line 670, in __call__
    await route.handle(scope, receive, send)
  File "/usr/local/lib/python3.10/site-packages/starlette/routing.py", line 266, in handle
    await self.app(scope, receive, send)
  File "/usr/local/lib/python3.10/site-packages/starlette/routing.py", line 65, in app
    response = await func(request)
  File "/usr/local/lib/python3.10/site-packages/fastapi/routing.py", line 217, in app
    solved_result = await solve_dependencies(
  File "/usr/local/lib/python3.10/site-packages/fastapi/dependencies/utils.py", line 557, in solve_dependencies
    ) = await request_body_to_args(  # body_params checked above
  File "/usr/local/lib/python3.10/site-packages/fastapi/dependencies/utils.py", line 692, in request_body_to_args
    v_, errors_ = field.validate(value, values, loc=loc)
  File "pydantic/fields.py", line 857, in pydantic.fields.ModelField.validate
  File "pydantic/fields.py", line 1074, in pydantic.fields.ModelField._validate_singleton
  File "pydantic/fields.py", line 1121, in pydantic.fields.ModelField._apply_validators
  File "pydantic/class_validators.py", line 313, in pydantic.class_validators._generic_validator_basic.lambda12
  File "pydantic/main.py", line 686, in pydantic.main.BaseModel.validate
  File "/usr/local/lib/python3.10/site-packages/beanie/odm/documents.py", line 138, in __init__
    self.get_motor_collection()
  File "/usr/local/lib/python3.10/site-packages/beanie/odm/interfaces/getters.py", line 13, in get_motor_collection
    return cls.get_settings().motor_collection
  File "/usr/local/lib/python3.10/site-packages/beanie/odm/documents.py", line 779, in get_settings
    raise CollectionWasNotInitialized
beanie.exceptions.CollectionWasNotInitialized

Then i put UserIn in the beanie init function. Everything is working.

    # Init beanie
    await init_beanie(database=client[f"{getenv('MONGO_DATABASE')}"],
                      document_models=[User,UserIn])

I just want the UserIn model to validate input data and beanie try to find it in the db. Models not mapped to db should not have anything to do with the db. Right?

RobertRosca commented 2 years ago

Document objects are used to define the mapping between Beanie and a collection in the database, so the error about the collection not being initialized (not existing) is correct, since all Documents should be mapped to the db.

The problem is that you define UserBase as:

class UserBase(Document):
    ...

And then all of your other classes inherit from it, so all of the other classes are also Documents.

If you want to use the other classes just for validation, then they shouldn't inherit Document, only a single class should be a Document in this case, so you can do something like:

from beanie import Document
from pydantic import BaseModel

#  Now inherits from pydantic `BaseModel`, not document
class UserBase(BaseModel):
    username: str | None
    parent_id: str | None
    role_id: str | None
    payment_type: str | None
    disabled: bool | None
    note: str | None
    access_token: str | None

# User inherits `UserBase` and `Document`
class User(UserBase, Document):
    username: str
    password: str
    disabled: bool = False
    salt: str

    class Settings:
        name = "users"

# Rest of the classes inherit `UserBase` so they're
# only pydantic models, not documents
class UserIn(UserBase):
    username: str
    password: str
    disabled: bool = False

class UserOut(UserBase):
    note: str | None
    access_token: str | None

This should solve the issue you're having.

Also, you're repeating some of the fields where the field is already inherited, for example UserIn has username and disabled which are both in UserBase, so password is needed there. And by marking all of the fields in UserBase as Optional you end up losing the validation of required fields being present where you'd probably like to have them.

This page from the FastAPI docs shows a user model as an example: https://fastapi.tiangolo.com/tutorial/extra-models/?h=userout#reduce-duplication

RobertRosca commented 2 years ago

Main thing to note from the page I linked above is that:

Usually the Base class has just the fields you always require and always return in all models. So in your case UserIn inherits from UserBase, so somebody logging in (I guess?) could provide any of the fields from base, including parent, access_token, and disabled, letting them change potentially sensitive fields (I'm not sure if that's how you're actually using the models so that might be wrong).

nghianv19940 commented 2 years ago

Thanks @RobertRosca,

Document objects are used to define the mapping between Beanie and a collection in the database, so the error about the collection not being initialized (not existing) is correct, since all Documents should be mapped to the db.

The problem is that you define UserBase as:

class UserBase(Document):
    ...

And then all of your other classes inherit from it, so all of the other classes are also Documents.

If you want to use the other classes just for validation, then they shouldn't inherit Document, only a single class should be a Document in this case, so you can do something like:

from beanie import Document
from pydantic import BaseModel

#  Now inherits from pydantic `BaseModel`, not document
class UserBase(BaseModel):
    username: str | None
    parent_id: str | None
    role_id: str | None
    payment_type: str | None
    disabled: bool | None
    note: str | None
    access_token: str | None

# User inherits `UserBase` and `Document`
class User(UserBase, Document):
    username: str
    password: str
    disabled: bool = False
    salt: str

    class Settings:
        name = "users"

# Rest of the classes inherit `UserBase` so they're
# only pydantic models, not documents
class UserIn(UserBase):
    username: str
    password: str
    disabled: bool = False

class UserOut(UserBase):
    note: str | None
    access_token: str | None

This should solve the issue you're having.

This info should be on the tutorial page https://roman-right.github.io/beanie/tutorial/defining-a-document/

Main thing to note from the page I linked above is that:

  • UserBase just has the bare common fields used/required by all subclasses
  • UserIn has password as that's what's provided by the user
  • UserInDB has hashed_password as the password itself isn't stored in the DB
  • UserOut just does pass since no information in addition to what is in UserBase is returned, and all the stuff in UserBase is safe to return

Usually the Base class has just the fields you always require and always return in all models. So in your case UserIn inherits from UserBase, so somebody logging in (I guess?) could provide any of the fields from base, including parent, access_token, and disabled, letting them change potentially sensitive fields (I'm not sure if that's how you're actually using the models so that might be wrong).

When creating a user, I need all the required fields, including password. Then I return the newly created user info to the creator without the password. I think it's a good way to put password in User and UserIn to not have password in UserOut. (I hope there is a better way for a child class to exclude fields from the parent class). The same thing applies to disabled.

All field in UserBase should be optional because there are multiple API route that need the user data. Each should have its own UserIn model (inherited from UserBase), with different requirements in fields. When a user creates his/her account, username and password are required. I rewrote the username to make it required in UserIn.