fastapi-users / fastapi-users-db-beanie

FastAPI Users database adapter for Beanie
MIT License
5 stars 5 forks source link

Superficial collections are created for base classes #10

Closed bkis closed 1 year ago

bkis commented 1 year ago

Describe the bug

When using fastapi-users-db-beanie as described in the docs, there will be two collections created per ODM model: User (which is used) and BeanieBaseUser (unused), as well as AccessToken and BeanieBaseAccessToken (I'm sticking to the naming from the docs, here).

When creating a model with Beanie by overriding the Document base model, Beanie creates a collection in MongoDB to hold this document type's data. The name of the collection is determined by the name attribute of the models inner Settings class.

If you don't set a collection name, Beanie will just name the collection after the model class name (e.g. User). Apparently, if you further extend this model class (e.g. class SpecialUser(User) and still don't set a collection name, Beanie creates two collections - one for the parent (User) and one for the subclass (SpecialUser), even if only the collection for the subclass is ever used (the other one is created because it's indexes are created at initialization).

A quick fix would be to define the name attribute in the two base classes' inner Settings class - e.g. for BeanieBaseUser:

class Settings:
        name = "users"
        # ...

Unfortunately, it doesn't work to override BeanieBaseUser.Settings in User.Settings to add that name value - at least when I tried it seems like it breaks something.

Setting name = "users" in BeanieBaseUser.Settings and setting name = "access_tokens" in BeanieBaseAccessToken.Settings could be good enough, but there are things to discuss:

  1. The used collection names will be taken. Should names with a lower collision chance be used (fastapi-users-users???)?
  2. Is there a way to make this configurable and just use user and access_tokens as defaults?

To Reproduce

Steps to reproduce the behavior:

  1. Use fastapi-users with Beanie support as described in the docs.
  2. Check the created MongoDB collections.

Expected behavior

There should only be one collection per ODM model.

Configuration

frankie567 commented 1 year ago

I'm not sure to understand which situation cause the issue:

  1. Is it when having only one model, as described in the examples?
  2. Is it when you define another sub-model SpecialUser? If so, the official Beanie solution is to use the is_root setting: https://beanie-odm.dev/tutorial/inheritance/#defining-models
bkis commented 1 year ago

Sorry for the confusion. The imaginary case with SpecialUser was just a general example. I should've sticked to what happens in fastapi-user's context: So no, no additional submodels needed:

I have the following ODM models related to fastapi-users:

class User(BeanieBaseUser[PydanticObjectId]):
    is_active: bool | None = _cfg.dev_mode
    first_name: str
    last_name: str

... and ...

class AccessToken(BeanieBaseAccessToken[PydanticObjectId]):
    pass

... and when I start the application, MongoDB shows these collections:

image

(Mongo Express is just a web frontend for MongoDB)

When registering a user and logging in, there's only data created in the User and AccessToken collections. So the other two (named after the base classes) are of no use.
Of course this doesn't create any further problems, but it feels a bit buggy :)

frankie567 commented 1 year ago

Hmm, that's weird, I can't seem to reproduce the issue. Here is what I have in my DB:

Capture d’écran 2023-01-25 à 10 27 13
@app.on_event("startup")
async def on_startup():
    await init_beanie(
        database=db,
        document_models=[
            User,
            AccessToken,
        ],
    )
bkis commented 1 year ago

That is weird indeed.

  1. Yes, clean database.
  2. Correct. That's what I do. That's why it surprises me that I still get those extra collections.

If you cannot reproduce, it might be some error on my end. I will investigate further and report back. Thank you for now!

bkis commented 1 year ago

I have found out some things:

  1. The phenomenon I described also happens with bare Beanie if a parent model has indexes defined (like BeanieBaseUser). These indexes are created both in the extraneous collection for the parent model as well as the intended collection for the inheriting model.
  2. I ran your example project from here in my environment and it shows the same behavior. Are you testing on the current versions of Beanie etc.? Maybe we could see if something changed in one of the deps we're using. See my dependencies below.
beanie==1.17.0
fastapi-users-db-beanie==1.1.4
fastapi-users==10.3.0
fastapi-users[beanie]==10.3.0
fastapi==0.89.1
frankie567 commented 1 year ago

Right, so after investigating, it seems this behavior appeared starting beanie==1.14.0, and it actually coincides with the Multi-model behavior for inherited documents feature.

After experimenting, it seems that the best solution is to make BeanieBaseUser and BeanieBaseAccessToken pure mixins that are not inheriting from Document. Inheriting Document would be the responsibility of the developer. So we would end up with this:

class User(BeanieBaseUser[PydanticObjectId], Document):
    pass

It avoids Beanie from considering BeanieBaseUser as a root document. It's a small breaking change, but I think it's a reasonable solution. What do you think?

bkis commented 1 year ago

I am relieved you could reproduce this - I ran out of ideas where to look for a bug :)
I like your idea, but doesn't BeanieBaseUser contain Beanie-related things like this Settings definition?

class Settings:
        email_collation = Collation("en", strength=2)
        indexes = [
            IndexModel("email", unique=True),
            IndexModel(
                "email", name="case_insensitive_email_index", collation=email_collation
            ),
        ]

Wouldn't this then be have to implemented by the end-user?

frankie567 commented 1 year ago

No, it works (I tested) without repeating Settings, inheritance is working with nested classes (Python considers it as an attribute).

Even in the case you need to add custom Settings, you can directly inherit from the parent Settings:

class User(BeanieBaseUser[PydanticObjectId], Document):
    class Settings(BeanieBaseUser.Settings):
        name = "users"
bkis commented 1 year ago

Ah, right. That sounds great!
In the end, you as the project owner will have to decide whether this more or less cosmetic issue is worth any breaking changes. You could also postpone it to some future release that contains breaking changes anyway. But all in all it sounds like a good solution.
BTW thanks for your work and the friendly interactions!

bkis commented 1 year ago

Just a quick update: If you don't want to make the changes mentioned above, it's also possible to just manipulate the inner Settings classes of BeanieBaseUser and BeanieBaseAccessToken in the importing project like so:

from fastapi_users_db_beanie import BeanieBaseUser
from fastapi_users_db_beanie.access_token import BeanieBaseAccessToken

# modify collection names of FastAPI-Users-DB-Beanie
BeanieBaseUser.Settings.name = "users"  # choose whatever
BeanieBaseAccessToken.Settings.name = "access_tokens"  # choose whatever

...and stick to the docs for the rest. Works just fine and is easy. It is a little hacky, so I don't know whether you'd like it hinted to in your docs, but it does solve the problem. Sure, if the internals of fastapi-users-db-beanie change for some reason, this workaround might break.

andrejridzik commented 1 year ago

Hi @frankie567 . Any update whether this issue is going to be fixed anytime soon? I would also like to get rid of the extra BeanieBaseUser collection :) Thanks a lot!

frankie567 commented 1 year ago

Shall be fixed with fastapi-users-db-beanie==2.0.0.