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

Inconsistent Application of serialization_strategy #93

Closed
peterallenwebb opened this issue Jan 5, 2023 · 4 comments · Fixed by #94
Closed

Inconsistent Application of serialization_strategy #93

peterallenwebb opened this issue Jan 5, 2023 · 4 comments · Fixed by #94

Comments

@peterallenwebb
Copy link
Contributor

peterallenwebb commented Jan 5, 2023

  • mashumaro version: 3.2 and 3.3
  • Python version: 3.10
  • Operating System: macOS

Description

There is an apparent regression in serialization behavior when the serialization_strategy object is being used and there are multiple properties of the same type on the class being serialized. It worked as I expected in 3.1.1, but fails in 3.2 and 3.3.

If the existing unit test test_config.test_serialization_strategy is modified to include a third property called c of type str, it will fail to properly deserialize c. Here's what the modified test looks like in full:

def test_serialization_strategy():
    class TestSerializationStrategy(SerializationStrategy):
        def serialize(self, value):
            return [value]

        def deserialize(self, value):
            return value[0]

    @dataclass
    class DataClass(DataClassDictMixin):
        a: int
        b: str
        c: str

        class Config(BaseConfig):
            serialization_strategy = {
                int: TestSerializationStrategy(),
                str: {
                    "serialize": lambda v: [v],
                    "deserialize": lambda v: v[0],
                },
            }

    instance = DataClass(a=123, b="abc", c="abc")
    assert DataClass.from_dict({"a": [123], "b": ["abc"], "c": ["abc"]}) == instance
    assert instance.to_dict() == {"a": [123], "b": ["abc"], "c": ["abc"]}

It appears to fail because the custom serialize/deserialize methods are called for b, but not for c.

AssertionError: assert test_serialization_strategy.<locals>.DataClass(a=123, b='abc', c=['abc']) == test_serialization_strategy.<locals>.DataClass(a=123, b='abc', c='abc')

@peterallenwebb
Copy link
Contributor Author

peterallenwebb commented Jan 6, 2023

This appears to be happening because mashumaro.core.meta.code.builder.CodeBuilder.__iter_serialization_strategies() is decorated with @lru_cache. The generator returned by this function is being cached and then iterated, but the same generator is returned again on subsequent calls. Removing @lru_cache is an easy fix and I would be happy to prepare a PR, but I'm not sure of the overall performance implications.

@Fatal1ty
Copy link
Owner

Fatal1ty commented Jan 7, 2023

Using a decorator @lru_cache for a generator function was a stupid mistake. I wanted to cache result because iter_serialization_strategies is called for each dataclass field on compilation time but now I see what it led to. We should replace a generator function __iter_serialization_strategies with a function returning first not-none strategy found and keep it cached:

@lru_cache()
@typing.no_type_check
def __get_serialization_strategy(self, ftype: typing.Type) -> typing.Optional[SerializationStrategyValueType]

I can return back to coding next week when my vacation ended. You can prepare your existing PR or let me fix this issue myself. Anyway thank you for spotting this bug and I hope it didn’t do anything bad in your production code.

@Fatal1ty
Copy link
Owner

Fatal1ty commented Jan 9, 2023

@peterallenwebb I decided to accept your PR instead of relying on the type objects to always be hashable. Thank you for your contribution!

@Fatal1ty
Copy link
Owner

Fixed in 3.3.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants