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

Fix optional embedded model definition #273

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

art049
Copy link
Owner

@art049 art049 commented Oct 10, 2022

Fixes #272

@michallowasrzechonek-silvair

FYI I think there is another issue here:

async def test_embedded_model_with_filled_optional(aio_engine: AIOEngine):
    class In(EmbeddedModel):
        a: int

    class Out(Model):
        inner: Union[In, None]

    instance = Out(inner=In(a=3))
    await aio_engine.save(instance)
    fetched = await aio_engine.find_one(Out)
    assert instance == fetched

fails on revision 002f698 with

aio_engine = <odmantic.engine.AIOEngine object at 0x7f58da0b5b10>

    async def test_embedded_model_with_filled_optional(aio_engine: AIOEngine):
        class In(EmbeddedModel):
            a: int
    
        class Out(Model):
            inner: Union[In, None]
    
        instance = Out(inner=In(a=3))
>       await aio_engine.save(instance)

tests/integration/test_embedded_model.py:340: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
odmantic/engine.py:588: in save
    await self._save(instance, local_session)
odmantic/engine.py:534: in _save
    doc = instance.doc(include=fields_to_update)
odmantic/model.py:784: in doc
    doc = self.__doc(raw_doc, type(self), include)
odmantic/model.py:755: in __doc
    self.__doc(item, field.model) for item in raw_doc[field_name]
odmantic/model.py:755: in <listcomp>
    self.__doc(item, field.model) for item in raw_doc[field_name]
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = Out(id=ObjectId('635bb09c1116fe2dbfe2fd2a'), inner=In(a=3)), raw_doc = 'a', model = <class 'tests.integration.test_embedded_model.test_embedded_model_with_filled_optional.<locals>.In'>, include = None

    def __doc(
        self,
        raw_doc: Dict[str, Any],
        model: Type["_BaseODMModel"],
        include: Optional["AbstractSetIntStr"] = None,
    ) -> Dict[str, Any]:
        doc: Dict[str, Any] = {}
        for field_name, field in model.__odm_fields__.items():
            if include is not None and field_name not in include:
                continue
            if isinstance(field, ODMReference):
                doc[field.key_name] = raw_doc[field_name][field.model.__primary_field__]
            elif isinstance(field, ODMEmbedded):
                doc[field.key_name] = self.__doc(raw_doc[field_name], field.model, None)
            elif isinstance(field, ODMEmbeddedGeneric):
                if raw_doc[field_name] is None:
                    doc[field.key_name] = None
                elif field.generic_origin is dict:
                    doc[field.key_name] = {
                        item_key: self.__doc(item_value, field.model)
                        for item_key, item_value in raw_doc[field_name].items()
                    }
                else:
                    doc[field.key_name] = [
                        self.__doc(item, field.model) for item in raw_doc[field_name]
                    ]
            elif field_name in model.__bson_serialized_fields__:
                doc[field.key_name] = model.__fields__[field_name].type_.__bson__(
                    raw_doc[field_name]
                )
            else:
>               doc[field.key_name] = raw_doc[field_name]
E               TypeError: string indices must be integers

@michallowasrzechonek-silvair

Moreover:

async def test_parse_embedded_model_with_filled_optional():
    class In(EmbeddedModel):
        a: int

    class Out(Model):
        inner: Union[In, None]

    instance = Out.parse_doc({"_id": "5a6a43a5ca40d8defe2bf4e0", "inner": {"a": 3}})

also fails:

cls = <class 'tests.integration.test_embedded_model.test_parse_embedded_model_with_filled_optional.<locals>.Out'>, raw_doc = {'_id': '5a6a43a5ca40d8defe2bf4e0', 'inner': {'a': 3}}

    @classmethod
    def parse_doc(cls: Type[BaseT], raw_doc: Dict) -> BaseT:
        """Parse a BSON document into an instance of the Model
    
        Args:
            raw_doc: document to parse (as Dict)
    
        Raises:
            DocumentParsingError: the specified document is invalid
    
        Returns:
            an instance of the Model class this method is called on.
        """
        errors, obj = cls._parse_doc_to_obj(raw_doc)
        if len(errors) > 0:
            raise DocumentParsingError(
                errors=[errors],
                model=cls,
>               primary_value=raw_doc.get("_id", "<unknown>"),
            )
E           odmantic.exceptions.DocumentParsingError: 1 validation error for Out
E           inner
E             incorrect generic embedded model value (type=value_error.incorrectgenericembeddedmodelvalue; value={'a': 3})
E           (Out instance details: id='5a6a43a5ca40d8defe2bf4e0')

@adeelsohailahmed
Copy link
Contributor

adeelsohailahmed commented Jan 4, 2023

Hi @art049,

As always, thank you for all the work you do.

The issues (as described in the comments here), as well the original #272 issue are still preventing us from upgrading ODMantic to the latest version.

@gsakkis
Copy link

gsakkis commented Mar 21, 2023

@art049 @michallowasrzechonek-silvair @adeelsohailahmed I submitted a new PR for this. It works for me, please let me know if you find any issues with it.

@art049
Copy link
Owner Author

art049 commented Mar 21, 2023

Hey, thanks a lot. I'll have a look!

@art049 art049 force-pushed the master branch 2 times, most recently from 5bd1656 to a99a258 Compare November 3, 2023 20:39
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.

Engine not saving the model if it contains an Optional EmbeddedModel set to None
4 participants