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

Allow change class_id and use name settings in UnionDoc #466

Merged

Conversation

wonjoonSeol-WS
Copy link
Contributor

@wonjoonSeol-WS wonjoonSeol-WS commented Dec 29, 2022

Beanie is all sound and good until there is a need to connect to existing DB where beanie is/was not used.
For instance, one might have SNS -> SQS -> lambda (that uses any other client other than beanie) -> DocDB / Mongo.
In case like this, the existing data would not have beanie's internal data field such as '_class_id".

This makes switching to Beanie difficult as it requires full DB migration, especially if DB is used for event-sourcing.

This PR allows (1) custom internal data field name to be set and (2) custom name to be set in Union doc children classes.

Usage:

class Parent(UnionDoc):
    class Settings:
        class_id = "event_type" <-
        name = "eventsource"

class ChildCreated(Document):
    event_type: Literal["created"] = "created" <- event_type is used instead of _class_id 
    sent_at: datetime

    class Settings:
        name = "created" <- This was option was ignored before
        union_doc = Parent

class ChildDeleted(Document):
    event_type: Literal["created"] = "deleted" <- event_type is used instead of _class_id 
    sent_at: datetime

    class Settings:
        name = "deleted" <- This was option was ignored before
        union_doc = Parent

Example:

Schema

class Parent(UnionDoc):
    class Settings:
        class_id = "event_type"
        name = "collection"


class One(Document):
    test: int

    class Settings:
        name = "two"
        class_id = "event_type"
        union_doc = Parent
        
 class Two(Document): <- without union_doc
    test: int

    class Settings:
        name = "three"
        class_id = "event_type"

Test

In [1]: await db.One(test=1).insert()
Out[1]: One(id=ObjectId('63ae1890edab780d45df0b07'), revision_id=None, test=1)

pymongo
In [17]: list(c["db"]["collection"].find({'test': 1}))
Out[17]: [{'_id': ObjectId('63ae1890edab780d45df0b07'), 'event_type': 'two', 'test': 1}]

In [1]: await db.Two(test=3).insert()
Out[1]: Two(id=ObjectId('63ae19cd8e684a4cd769fb1f'), revision_id=None, test=3)

pymongo
In [23]: list(c["db"]["three"].find({'test': 3}))
Out[23]: [{'_id': ObjectId('63ae19cd8e684a4cd769fb1f'), 'test': 3}]


FYI, #206 this PR was not reviewed for a year so I closed it..

@@ -233,8 +233,9 @@ async def init_document_collection(self, cls):
# register in the Union Doc

if document_settings.union_doc is not None:
name = cls.get_settings().name or cls.__name__
Copy link
Contributor Author

@wonjoonSeol-WS wonjoonSeol-WS Dec 29, 2022

Choose a reason for hiding this comment

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

if cls.get_settings().name always returns original class name instead of None when name is not set, we wouldn't need this line.

@@ -24,7 +24,7 @@ def parse_obj(
raise UnionHasNoRegisteredDocs

if isinstance(data, dict):
class_name = data["_class_id"]
class_name = data[model.get_settings().class_id]
else:
class_name = data._class_id
Copy link
Contributor Author

@wonjoonSeol-WS wonjoonSeol-WS Dec 29, 2022

Choose a reason for hiding this comment

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

Does it ever reach line 29 for UnionDoc? it didn't for my (small) dataset

@@ -24,7 +24,7 @@ def parse_obj(
raise UnionHasNoRegisteredDocs

if isinstance(data, dict):
class_name = data["_class_id"]
class_name = data[model.get_settings().class_id]
else:
class_name = data._class_id
Copy link
Contributor Author

@wonjoonSeol-WS wonjoonSeol-WS Dec 29, 2022

Choose a reason for hiding this comment

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

        if class_name not in model._document_models:  # type: ignore
            raise DocWasNotRegisteredInUnionClass

This line seems too strict - especially in event source DB. One MSA can add a new event and boom we now have an error. if there is schema that beanie doesn't yet know perhaps gracefully skip?

@wonjoonSeol-WS wonjoonSeol-WS changed the title Allow change class_id and use name settings in UnionDoc Allow change class_id and use name settings Dec 29, 2022
@wonjoonSeol-WS wonjoonSeol-WS changed the title Allow change class_id and use name settings Allow change class_id and use name settings in UnionDoc Dec 29, 2022
@roman-right
Copy link
Member

Hi @wonjoonSeol-WS ,
Sounds very reasonable. I'll check the PR soon this week

@roman-right roman-right self-assigned this Jan 9, 2023
@github-actions
Copy link
Contributor

This PR is stale because it has been open 45 days with no activity.

@github-actions github-actions bot added the Stale label Feb 24, 2023
@roman-right roman-right added bug Something isn't working and removed Stale labels Feb 24, 2023
@roman-right roman-right merged commit c561478 into BeanieODM:main Mar 31, 2023
@roman-right
Copy link
Member

Merged. Thank you very mush for the PR.
It will be published in the next release (beta or normal) today-tomorrow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants