python-attrs / cattrs

Composable custom class converters for attrs, dataclasses and friends.
https://catt.rs
MIT License
791 stars 110 forks source link

Omit = True is ignored when unstructuring nested attrs classes #437

Closed fortysix2ahead closed 10 months ago

fortysix2ahead commented 10 months ago

Description

See attached test case: I expect __internal_ids__ to not appear in the unstructured dict.

What I expect when printing the unstructured result:

{
    'inners': [
        {
            'ids': [1, 2, 3],
        }
    ],
    'ids': [10, 20],
}

But I get this:

{
    'inners': [
        {
            'ids': [1, 2, 3],
            '__internal_ids__': [4, 5, 6]
        }
    ],
    'ids': [10, 20]
}

What I Did

Create a test case for it.

from typing import List

from attrs import define, field
from cattrs.gen import make_dict_unstructure_fn, override
from cattrs.preconf.orjson import make_converter

@define
class Inner:

    ids: List[int] = field( factory=list )
    __internal_ids__: List[int] = field( factory=list, alias='__internal_ids__' )

@define
class Outer:

    inners: List[Inner] = field( factory=list )
    ids: List[int] = field( factory=list )
    __internal_ids__: List[int] = field( factory=list, alias='__internal_ids__' )

# make converter

conv = make_converter()

# converter configuration

inner_to_dict = make_dict_unstructure_fn(
    Inner,
    conv,
    _cattrs_omit_if_default=True,
    __internal_ids__=override( omit=True ),
)

outer_to_dict = make_dict_unstructure_fn(
    Outer,
    conv,
    _cattrs_omit_if_default=True,
    __internal_ids__=override( omit=True ),
)

conv.register_unstructure_hook( Inner, inner_to_dict )
conv.register_unstructure_hook( Outer, outer_to_dict )

def test_cattrs():
    outer = Outer( ids = [10, 20], __internal_ids__ = [ 30, 40 ] )

    result = conv.unstructure( outer )
    assert 'ids' in result and '__internal_ids__' not in result

    inner = Inner( ids=[1, 2, 3], __internal_ids__=[ 4, 5, 6 ] )
    outer = Outer( inners=[inner], ids = [10, 20], __internal_ids__ = [ 30, 40 ] )

    result = conv.unstructure( outer )
    assert 'ids' in result and not '__internal_ids__' in result
    assert len( result.get( 'inners' ) ) == 1
    inner_result = result.get( 'inners' )[0]
    assert 'ids' in inner_result

    # the next line fails ...
    assert not '__internal_ids__' in inner_result
Tinche commented 10 months ago

Because of how cattrs composes hooks, you just need to intersperse the hook generation and registration:

inner_to_dict = make_dict_unstructure_fn(
    Inner,
    conv,
    _cattrs_omit_if_default=True,
    __internal_ids__=override( omit=True ),
)

conv.register_unstructure_hook( Inner, inner_to_dict )

outer_to_dict = make_dict_unstructure_fn(
    Outer,
    conv,
    _cattrs_omit_if_default=True,
    __internal_ids__=override( omit=True ),
)

conv.register_unstructure_hook( Outer, outer_to_dict )

To give a little bit of context, when you call make_dict_unstructure_fn, the converter argument is used to fish out handlers for all the fields. So if you don't register your custom handlers on the converter first, the call

outer_to_dict = make_dict_unstructure_fn(
    Outer,
    conv,
    _cattrs_omit_if_default=True,
    __internal_ids__=override( omit=True ),
)

will fetch the default handler for Inner, which is the behavior you're seeing here.

Hope this helps!

fortysix2ahead commented 10 months ago

Thanks a lot. Indeed this explains what I have seen when running stepping through the debugger. Unfortunately, it does not help: rearranging the code and creating/registering the Inner before the outer does not make any difference. The result is the same.

Tinche commented 10 months ago

Really? I slightly modified your example, and this works for me (but on 3.11, that was the virtual environment I have set up):

from typing import List

from attrs import define, field

from cattrs.gen import make_dict_unstructure_fn, override
from cattrs.preconf.orjson import make_converter

@define
class Inner:
    ids: List[int] = field(factory=list)
    __internal_ids__: List[int] = field(factory=list, alias="__internal_ids__")

@define
class Outer:
    inners: List[Inner] = field(factory=list)
    ids: List[int] = field(factory=list)
    __internal_ids__: List[int] = field(factory=list, alias="__internal_ids__")

# make converter

conv = make_converter()

# converter configuration

inner_to_dict = make_dict_unstructure_fn(
    Inner, conv, _cattrs_omit_if_default=True, __internal_ids__=override(omit=True)
)
conv.register_unstructure_hook(Inner, inner_to_dict)
outer_to_dict = make_dict_unstructure_fn(
    Outer, conv, _cattrs_omit_if_default=True, __internal_ids__=override(omit=True)
)
conv.register_unstructure_hook(Outer, outer_to_dict)

outer = Outer(ids=[10, 20], __internal_ids__=[30, 40])

result = conv.unstructure(outer)
assert "ids" in result and "__internal_ids__" not in result

inner = Inner(ids=[1, 2, 3], __internal_ids__=[4, 5, 6])
outer = Outer(inners=[inner], ids=[10, 20], __internal_ids__=[30, 40])

result = conv.unstructure(outer)
assert "ids" in result and "__internal_ids__" not in result
assert len(result.get("inners")) == 1
inner_result = result.get("inners")[0]
assert "ids" in inner_result

# the next line fails ...
assert "__internal_ids__" not in inner_result
fortysix2ahead commented 10 months ago

That's strange. I used your modified code (plus a main statement ...) and it works as expected. And there's no difference between Python 3.9 and 3.11. However, when running from within pytest it does not. I'll try to find the reason for that ...