Feat/custom class ID

[volfpeter]

Closes #1132

For reference, this is a re-implementation of #1133 using settings to set the class ID value (as requested in that PR by @staticxterm ).

This solution has some quirks in comparison with #1132, notably the need for creating a base settings class and using subclasses in every model in the inheritance chain.

[volfpeter]

It seems there's still some flakiness in the test suite.

[staticxterm]

It seems there's still some flakiness in the test suite.

Yeah, don't have any control over that, it's due to rate limits when pulling the Mongo Docker image. Perhaps we can look into caching it somehow... I have rerun the failing job.

[staticxterm]

@volfpeter Regarding the PR and the functionality, I have just quickly glanced over it, it looks good to me. Will try it out and test it when I get the time.
Just one kind of important comment, we should also update the docs. Could you please add notes about this usage to the documentation (either in this PR or as a separate PR if you wish)?

[volfpeter]

@volfpeter Regarding the PR and the functionality, I have just quickly glanced over it, it looks good to me. Will try it out and test it when I get the time. Just one kind of important comment, we should also update the docs. Could you please add notes about this usage to the documentation (either in this PR or as a separate PR if you wish)?

Thanks for the quick feedback. When the code change itself is accepted, I'll add the related documentation.

Honestly, I'm not too happy with the class_id_val name (class_id_field and class_id would be better for example, but that would be a breaking change), maybe a better idea will pop up.

I'm also not happy about settings inheritance, it's easy to use incorrectly and not that intuitive, mainly because of settings names. There is one alternative I can think of (besides the one I implemented in my earlier PR), but first I implemented what was requested. The alternative is using __init_subclass__() with a class_id keyword only argument. Usage would look like this:

class MyDocument(Document, class_id="value-for-class-id-field"):
    ...

This pattern is also used by SQLModel for example. It feels better to me. Let me know what you think.

[staticxterm]

Again, I have no strong opinions here.
I share your sentiment that the naming may be unclear too. If I were designing this feature from scratch, I would change the names to mention "discriminator" something instead of the "class_id" naming.
The community wishes that this functionality (setting of custom "ID" to differentiate documents in the inheritance chain) is extended. I'm open for anything that feels like a good API.

[volfpeter]

Well, here is how it would work with settings (as requested in #1133):

class VehicleSettings:
    is_root = True
    name = "vehicles"
    class_id = "type"

class VehicleWithCustomClassId(Document):
    class Settings(VehicleSettings): ...

class BicycleWithCustomClassId(VehicleWithCustomClassId):
    class Settings(VehicleSettings):
        class_id_value = "bicycle"
    ...

class CarWithCustomClassId(VehicleWithCustomClassId, Fuelled):
    class Settings(VehicleSettings):
        class_id_value = "car"
    ...

And here is the same with __init_subclass__():

class VehicleWithCustomClassId(Document):
    class VehicleSettings:
        is_root = True
        name = "vehicles"
        class_id = "type"

class BicycleWithCustomClassId(VehicleWithCustomClassId, id="bicycle"):
    ...

class CarWithCustomClassId(VehicleWithCustomClassId, Fuelled, id="car"):
    ...

I feel the __init_subclass__() version is cleaner and less confusing. I wish I thought about it when we discussed #1133 , because the necessary would be essentially the same... Let me know which version you'd prefer, and I'll move forward accordingly (and add the docs for that version).

[staticxterm]

My preference is for the configuration option in the model Settings subclass, as it follows Beanie's current design and API.
Introducing this setting in the "class arguments" may be confusing to the user (even if it places it "up front" to the user): "do I set this in Settings or in the base class arguments?", "why is class_id configured through Settings but class_id_value/id in class arguments?". Also, if this other option would be introduced then we would need to maintain these two "configuration systems".
If settings inheritance is a problem, we can always write some more code to not inherit everything from the parent settings (such as is_root = True if set on the parent) but do manual merging instead.
With that, we would get something like this:

class Vehicle(Document):
    class Settings:
        is_root = True
        class_id = "type"
        class_id_value = "vehicle"


class Bicycle(Vehicle):
    class Settings:
        class_id_value = "bicycle"

which IMHO looks good.

[volfpeter]

Okay, I'm happy with this, I don't need to rewrite the PR another time.

I'm not sure your example would work, because class_id may be missing in Bicycle.Settings. I'll test that case. If that works, then using settings is definitely the good API.

[volfpeter]

My preference is for the configuration option in the model Settings subclass, as it follows Beanie's current design and API. Introducing this setting in the "class arguments" may be confusing to the user (even if it places it "up front" to the user): "do I set this in Settings or in the base class arguments?", "why is class_id configured through Settings but class_id_value/id in class arguments?". Also, if this other option would be introduced then we would need to maintain these two "configuration systems". If settings inheritance is a problem, we can always write some more code to not inherit everything from the parent settings (such as is_root = True if set on the parent) but do manual merging instead. With that, we would get something like this:

class Vehicle(Document):
    class Settings:
        is_root = True
        class_id = "type"
        class_id_value = "vehicle"


class Bicycle(Vehicle):
    class Settings:
        class_id_value = "bicycle"

which IMHO looks good.

Your example code doesn't work, because if a document class declares its own settings, then it fully overwrites the settings declaration of the parent class. So if you don't use explicit settings inheritance as in my example, the you need to set class_id="type" as well in all document classes. Changing this behavior would be a significant, potentially breaking change - I don't know the internals of beanie well enough to know for sure. I'm not sure about making class_id an exception is a good idea, although there's already a bit of similar code for is_root handling.

[staticxterm]

Your example code doesn't work, because if a document class declares its own settings, then it fully overwrites the settings declaration of the parent class. So if you don't use explicit settings inheritance as in my example, the you need to set class_id="type" as well in all document classes. Changing this behavior would be a significant, potentially breaking change - I don't know the internals of beanie well enough to know for sure. I'm not sure about making class_id an exception is a good idea, although there's already a bit of similar code for is_root handling.

This sentence from my previous reply is key:

If settings inheritance is a problem, we can always write some more code to not inherit everything from the parent settings (such as is_root = True if set on the parent) but do manual merging instead.

Otherwise yes, because you are using class inheritance, you would have to repeat/override the wanted parameters. Beanie does not anywhere state that Settings inheritance should (or shouldn't) be used, in all examples there is no inheriting of Settings class shown.

Regarding this:

Changing this behavior would be a significant, potentially breaking change[...]

This would be that __init_subclass__ implementation but with this custom "settings merging". I don't think it's backwards breaking as Beanie never stated that Settings inheritance is supported. Basically, in my opinion, we can do whatever here as long as we state it in the docs what is supported, which is the preferred way and which is not.

[volfpeter]

Oh, my bad. Apparently the current implementation works without explicit settings inheritance. I updated the test models to use that version. It's quite good this way (except for the settings names, but that's a fix for the next major version).

[volfpeter]

I've added a bit of documentation. There wasn't much documentation for the class_id setting itself, so I hope the added docs will be enough for class_id_value.

@staticxterm Do you have an idea why all Pydantic v1 jobs are failing? I'm not too familiar with the test suite or the Pydantic v1 support status. It's strange that the Pydantic TypeAdapter import is attempted with Pydantic v1.

This is in the install log (copied from a failed job:

ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
fastapi 0.128.0 requires pydantic>=2.7.0, but you have pydantic 1.10.18 which is incompatible.

[volfpeter]

@staticxterm Could you maybe review the PR?

The actual code change is about 10 lines, plus documentation with examples. The remaining 80% is testing: I mirrored the standard inheritance models and tests, but with a mix of user-defined and auto-calculated class ID values to make sure everything is tested properly.

[littlebeeper]

@staticxterm @volfpeter
Any progress on this issue?

[roman-right]

I see there has been discussion here. The feature is useful but the current approach (requiring settings subclasses in every model) is not ideal ergonomically. Will revisit this after the current batch of bug fixes lands.

[littlebeeper]

Thanks @roman-right. Appreciate it

[volfpeter]

Thanks @roman-right

Other ideas that came up so far:

• Simply let the user set _class_id (just don't blindly overwrite its value. This was implemented in fix: do not override Document._class_id if it was explicitly set on the document class #1133. It feels simpler and the changes are minimal, but I understand the arguments against that solution.
• Using __init_subclass__() to let users pass the class ID value directly in the class declaration (class MyDocument(RootDocument, id="custom-class-id")). The argument against this option was that it would bring a new pattern to beanie, which may be undesired. The implementation would be basically the same as what's in fix: do not override Document._class_id if it was explicitly set on the document class #1133
• Try to infer the class ID value from the discriminator field (e.g. from the default value). The downsides are: it would be a significant breaking change in the lib, it's implicit behavior that would add "magic", and it may be hard to properly explain the details to users.

Let me know what you think. I'm happy to spend time on alternative implementations if a different path is chosen.

[roman-right]

Looks good, thank you!

[volfpeter]

Thanks a lot!