tagged union disambiguation seems to ignore omit_if_default setting

[aha79]

• cattrs version: 23.1.2
• Python version: 3.10.7
• Operating System: Windows

Description

The tagged union disambiguation seems to ignore omit_if_default=True

What I Did

@dataclass(kw_only=True,frozen=True,eq=True)
class A:
    a: int
    b: Optional[int] = None

@dataclass(kw_only=True,frozen=True,eq=True)
class Derived(A):
    d: int
    c: Optional[int] = None

  def ustrat(a: Any, conv) -> Any:
      return cattrs.strategies.configure_tagged_union(a, conv, tag_name="type")

  converter = cattrs.Converter(omit_if_default=True)
  cattrs.strategies.include_subclasses(A, converter, union_strategy=ustrat)
  
  #data =  [ Derived(a=1, d=2) ]
  data =  [ A(a=1) ]
  dic = converter.unstructure(data, unstructure_as=List[A] )
 
  # >> dic = {'a': 1, 'b': None, 'type': 'a'}

Maybe this is related #402

[aha79]

I think the culprit lies here:

cattrs/src/cattrs/strategies/_subclasses.py

Line 98 in c447dfe

base_struct_hook = make_dict_structure_fn(cl, converter, **overrides)

No additional arguments are passed as for instance in the 'gen_xx' function:

• https://github.com/python-attrs/cattrs/blob/c447dfe4de2978ef1bb2974b53562e413fe28a1a/src/cattrs/converters.py#L981C1-L981C1

• cattrs/src/cattrs/converters.py

  Line 945 in c447dfe

  return make_dict_unstructure_fn(

However, I am not sure if it is a good idea at all to call 'make_dict_(un)structure_fn' directly there (even when the missing arguments are added), as it will also prevent user-defined hooks from working.

[Tinche]

You're right, I can reproduce the issue. Took me a while to understand what's going on. Two separate issues here: existing hooks aren't honored, and converter.omit_if_default isn't honored when new hooks are generated.

We could make the strategy fish out hooks out of the converter instead of generating them; that way existing hooks and converter flags would be automatically honored. The problem is the strategy was written to take overrides, implying it would generate hooks for you with these overrides, so this would be a backwards-incompatible API change. So we need to be careful here.

We could make it so the hooks are not generated if there are no overrides; I think this would work but it just complicates the strategy with a separate behavior path. Maybe it's ok?

The simplest change would be to just let the strategy pass converter.omit_if_default to gen_unstructure_hook. This would solve your particular issue.

[aha79]

Personally I like the idea of using the existing generator hook if there are no overrides and regenerate the hook if there are overrides. It doesnt seem very unclean, but I do not know if there are any not yet seen pitfalls.

But after looking at the code and thinking about what you wrote, I see another problem:

• The current approach also ignores the Converter level overrides ('type_overrides'). One would have to specify them again in 'include_subclasses'. This is maybe not intuitive to the user. In addition to that,
• contrary to 'type_overrides' the overrides in 'include_subclasses' are applied to all union member types at once. There is no way to limit the overrides to one specific type. (Fields with same name receive same override settings in all union types). This may turn out to be limiting in the future as well. It may even not be possible to get recreate the 'type_overrides' effect even if one duplicates them in the 'overrides' of 'include_subclasses'.

As a solution maybe one can augment 'gen_unstructure_attrs_fromdict' and 'gen_structure_attrs_fromdict' to accept another 'overrides ' as arguments and then call these functions from 'include_subclasses' if there are call level overrides for re-generation (and use existing hooks if there are no overrides). Just an idea

[Tinche]

Yeah I'm warming up to the idea of fishing the hook out if no overrides. Maybe that's the way to go.

The type_overrides are mostly used for customizing collections so I don't think those will be problematic, unless I'm misunderstanding your concern.

Your second bullet is valid, and would also be fixed by fishing the hook out if no overrides. You can apply your overrides prior to applying the strategy and let the strat fish em out.

[aha79]

Regarding the type_overrides: I was thinking of renaming fields. Sometimes there is no way around, e.g. if the dict key is a python reserved keyword ('class', 'if'), and the dataclass has to have altered field names ('class_', 'if_).
And that should also work if the dataclass is in a union.

[Tinche]

I've changed it so the strategy uses the converter to get the base hooks when no overrides are provided, so converter-level customizations should propagate now.