forbid_extra_keys or hooks get loss with Converter.copy()

[philpep]

• cattrs version: 23.1.2
• Python version: 3.11
• Operating System: Debian bookworm

Description

Hi, I noticed some issues with Converter.copy() where in some case forbid_extra_keys or hooks get loss.

What I Did

Here's some code reproducing the issue:

from typing import Any

import attrs
import cattrs

@attrs.frozen
class M:
    some_attr: int

base = cattrs.Converter(forbid_extra_keys=True)
camel = base.copy()

def to_camel(string: str) -> str:
    parts = string.split("_")
    return parts[0] + "".join(word.capitalize() for word in parts[1:])

def camel_structure(cls: Any) -> Any:
    return cattrs.gen.make_dict_structure_fn(
        cls,
        camel,
        **{
            a.name: cattrs.gen.override(rename=to_camel(a.name))
            for a in attrs.fields(cls)
        },
    )

camel.register_structure_hook_factory(attrs.has, camel_structure)

print(camel.structure({"someAttr": 42}, M))
# OK: M(some_attr=42)

print(camel.structure({"someAttr": 42, "extra": "forbid"}, M))
# NOK: M(some_attr=42)
# should fail with cattrs.errors.ForbiddenExtraKeysError
# looks this can be workaround by adding _cattrs_forbid_extra_keys=True to make_dict_structure_fn()

camel2 = camel.copy()
print(camel2.structure({"someAttr": 42}, M))
# NOK cattrs.errors.ForbiddenExtraKeysError: Extra fields in constructor for M: someAttr
# should sucess and return M(some_attr=42)
# BUT seems ok with cattrs current master branch

[Tinche]

Hm you're right for forbid_extra_keys, if you don't override it in copy() it'll use the default value, instead of the value set on the origin converter. Should be an easy fix, taking a look at this now.

[Tinche]

Actually I was wrong, it should be using the value from the origin converter (https://github.com/python-attrs/cattrs/blob/992b137223faf3890e9e0d4307f49045b1cf57b7/src/cattrs/converters.py#L1065). Will take a deeper look.

[Tinche]

Ah I see the issue, it's unrelated to copying though.

def camel_structure(cls: Any) -> Any:
    return cattrs.gen.make_dict_structure_fn(
        cls,
        camel,
        **{
            a.name: cattrs.gen.override(rename=to_camel(a.name))
            for a in attrs.fields(cls)
        },
    )

I think you were assuming the make_dict_structure_fn will take the value of forbid_extra_keys from the camel converter, but that's not the case. The converter parameter there is for getting hooks for all the attributes only. Like this:

def camel_structure(cls: Any) -> Any:
    return cattrs.gen.make_dict_structure_fn(
        cls,
        camel,
        _cattrs_forbid_extra_keys=camel.forbid_extra_keys,
        **{
            a.name: cattrs.gen.override(rename=to_camel(a.name))
            for a in attrs.fields(cls)
        },
    )

We could make the _cattrs_forbid_extra_keys be a bool | None, with None meaning take the value from the converter. I'm not sure about changing the default though (from False to None) due to backwards compatibility :/ I could be persuaded though, to make the default behavior less surprising.

[Tinche]

Having given this some more thought I think it does make sense to change the defaults, long term. I'm preparing a PR to make this change.