Perform attr order checks after field transformer. (#1401)

jamesmurphy-mc · pre-commit-ci[bot] · hynek · web-flow · commit e048efcb3954 · 2025-02-09T07:28:11.000Z
* Perform attr order checks after field transformer. Fixes: #1147 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix assert style in test_hooks --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Hynek Schlawack <hs@ox.cx>
diff --git a/changelog.d/1147.change.md b/changelog.d/1147.change.md
@@ -0,0 +1 @@
+Checking mandatory vs non-mandatory attribute order is now performed after the field transformer, since the field transformer may change attributes and/or their order.
diff --git a/src/attr/_make.py b/src/attr/_make.py
@@ -449,6 +449,10 @@ def _transform_attrs(
 
     attrs = base_attrs + own_attrs
 
+    if field_transformer is not None:
+        attrs = field_transformer(cls, attrs)
+
+    # Check attr order after executing the field_transformer.
     # Mandatory vs non-mandatory attr order only matters when they are part of
     # the __init__ signature and when they aren't kw_only (which are moved to
     # the end and can be mandatory or non-mandatory in any order, as they will
@@ -462,9 +466,6 @@ def _transform_attrs(
         if had_default is False and a.default is not NOTHING:
             had_default = True
 
-    if field_transformer is not None:
-        attrs = field_transformer(cls, attrs)
-
     # Resolve default field alias after executing field_transformer.
     # This allows field_transformer to differentiate between explicit vs
     # default aliases and supply their own defaults.
diff --git a/tests/test_hooks.py b/tests/test_hooks.py
@@ -4,6 +4,8 @@
 
 from datetime import datetime
 
+import pytest
+
 import attr
 
 
@@ -30,7 +32,7 @@ class C:
             y = attr.ib(type=int)
             z: float = attr.ib()
 
-        assert results == [("x", None), ("y", int), ("z", float)]
+        assert [("x", None), ("y", int), ("z", float)] == results
 
     def test_hook_applied_auto_attrib(self):
         """
@@ -49,7 +51,7 @@ class C:
             x: int
             y: str = attr.ib()
 
-        assert results == [("x", int), ("y", str)]
+        assert [("x", int), ("y", str)] == results
 
     def test_hook_applied_modify_attrib(self):
         """
@@ -66,7 +68,8 @@ class C:
             y: float
 
         c = C(x="3", y="3.14")
-        assert c == C(x=3, y=3.14)
+
+        assert C(x=3, y=3.14) == c
 
     def test_hook_remove_field(self):
         """
@@ -82,7 +85,7 @@ class C:
             x: int
             y: float
 
-        assert attr.asdict(C(2.7)) == {"y": 2.7}
+        assert {"y": 2.7} == attr.asdict(C(2.7))
 
     def test_hook_add_field(self):
         """
@@ -98,7 +101,7 @@ def hook(cls, attribs):
         class C:
             x: int
 
-        assert attr.asdict(C(1, 2)) == {"x": 1, "new": 2}
+        assert {"x": 1, "new": 2} == attr.asdict(C(1, 2))
 
     def test_hook_override_alias(self):
         """
@@ -118,13 +121,73 @@ class NameCase:
             1, 2, 3
         )
 
+    def test_hook_reorder_fields(self):
+        """
+        It is possible to reorder fields via the hook.
+        """
+
+        def hook(cls, attribs):
+            return sorted(attribs, key=lambda x: x.metadata["field_order"])
+
+        @attr.s(field_transformer=hook)
+        class C:
+            x: int = attr.ib(metadata={"field_order": 1})
+            y: int = attr.ib(metadata={"field_order": 0})
+
+        assert {"x": 0, "y": 1} == attr.asdict(C(1, 0))
+
+    def test_hook_reorder_fields_before_order_check(self):
+        """
+        It is possible to reorder fields via the hook before order-based errors are raised.
+
+        Regression test for #1147.
+        """
+
+        def hook(cls, attribs):
+            return sorted(attribs, key=lambda x: x.metadata["field_order"])
+
+        @attr.s(field_transformer=hook)
+        class C:
+            x: int = attr.ib(metadata={"field_order": 1}, default=0)
+            y: int = attr.ib(metadata={"field_order": 0})
+
+        assert {"x": 0, "y": 1} == attr.asdict(C(1))
+
+    def test_hook_conflicting_defaults_after_reorder(self):
+        """
+        Raises `ValueError` if attributes with defaults are followed by
+        mandatory attributes after the hook reorders fields.
+
+        Regression test for #1147.
+        """
+
+        def hook(cls, attribs):
+            return sorted(attribs, key=lambda x: x.metadata["field_order"])
+
+        with pytest.raises(ValueError) as e:
+
+            @attr.s(field_transformer=hook)
+            class C:
+                x: int = attr.ib(metadata={"field_order": 1})
+                y: int = attr.ib(metadata={"field_order": 0}, default=0)
+
+        assert (
+            "No mandatory attributes allowed after an attribute with a "
+            "default value or factory.  Attribute in question: Attribute"
+            "(name='x', default=NOTHING, validator=None, repr=True, "
+            "eq=True, eq_key=None, order=True, order_key=None, "
+            "hash=None, init=True, "
+            "metadata=mappingproxy({'field_order': 1}), type='int', converter=None, "
+            "kw_only=False, inherited=False, on_setattr=None, alias=None)",
+        ) == e.value.args
+
     def test_hook_with_inheritance(self):
         """
         The hook receives all fields from base classes.
         """
 
         def hook(cls, attribs):
-            assert [a.name for a in attribs] == ["x", "y"]
+            assert ["x", "y"] == [a.name for a in attribs]
             # Remove Base' "x"
             return attribs[1:]
 
@@ -136,7 +199,7 @@ class Base:
         class Sub(Base):
             y: int
 
-        assert attr.asdict(Sub(2)) == {"y": 2}
+        assert {"y": 2} == attr.asdict(Sub(2))
 
     def test_attrs_attrclass(self):
         """
@@ -151,7 +214,7 @@ class C:
             x: int
 
         fields_type = type(attr.fields(C))
-        assert fields_type.__name__ == "CAttributes"
+        assert "CAttributes" == fields_type.__name__
         assert issubclass(fields_type, tuple)
 
 
@@ -187,12 +250,12 @@ class Parent:
         )
 
         result = attr.asdict(inst, value_serializer=hook)
-        assert result == {
+        assert {
             "a": {"x": 1, "y": ["2020-07-01T00:00:00"]},
             "b": [{"x": 2, "y": ["2020-07-02T00:00:00"]}],
             "c": {"spam": {"x": 3, "y": ["2020-07-03T00:00:00"]}},
             "d": {"eggs": "2020-07-04T00:00:00"},
-        }
+        } == result
 
     def test_asdict_calls(self):
         """
@@ -217,12 +280,12 @@ class Parent:
         inst = Parent(a=Child(1), b=[Child(2)], c={"spam": Child(3)})
 
         attr.asdict(inst, value_serializer=hook)
-        assert calls == [
+        assert [
             (inst, "a", inst.a),
             (inst.a, "x", inst.a.x),
             (inst, "b", inst.b),
             (inst.b[0], "x", inst.b[0].x),
             (inst, "c", inst.c),
             (None, None, "spam"),
             (inst.c["spam"], "x", inst.c["spam"].x),
-        ]
+        ] == calls

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+Checking mandatory vs non-mandatory attribute order is now performed after the field transformer, since the field transformer may change attributes and/or their order.`