fix structure for union of attr classes with default values

[dswistowski]

Structure union of attr classes when one of the fields was optional and had a default value was not deterministic.

Disambiguator function sometimes was created to use field with default value - and because it's allowed to not provide this value to __init__ method wrong class was chosen to structure.

A provided test case is a minimal deterministic test I was able to generate.

[dswistowski]

I was trying to fix broken tests - but because I'm not familiar with a hypothesis I failed.

was assuming:

diff --git a/tests/test_disambigutors.py b/tests/test_disambigutors.py
index c9078e7..f86612d 100644
--- a/tests/test_disambigutors.py
+++ b/tests/test_disambigutors.py
@@ -65,12 +65,12 @@ def test_fallback(cl_and_vals):

 @given(simple_classes(), simple_classes())
 def test_disambiguation(cl_and_vals_a, cl_and_vals_b):
-    """Disambiguation should work when there are unique fields."""
+    """Disambiguation should work when there are unique non default fields."""
     cl_a, vals_a = cl_and_vals_a
     cl_b, vals_b = cl_and_vals_b

-    req_a = {a.name for a in attr.fields(cl_a)}
-    req_b = {a.name for a in attr.fields(cl_b)}
+    req_a = {a.name for a in attr.fields(cl_a) if a.default is attr.NOTHING}
+    req_b = {a.name for a in attr.fields(cl_b) if a.default is attr.NOTHING}

     assume(len(req_a))
     assume(len(req_b))

should fix the test - but it didn't work 😿

[Tinche]

Hello,

the test is failing because the new implementation is faulty :)

This throws an error on master, but not on your branch:

from attr import define

from cattr.disambiguators import create_uniq_field_dis_func


@define
class A:
    a: int


@define
class B:
    a: int = 0


fn = create_uniq_field_dis_func(A, B)

Just because B.a has a default value doesn't mean an unstructured payload containing 'a' is automatically an instance of A.

[Tinche]

Here's a patch that does what you want:

diff --git a/src/cattr/disambiguators.py b/src/cattr/disambiguators.py
index ada129f..cdb9bcf 100644
--- a/src/cattr/disambiguators.py
+++ b/src/cattr/disambiguators.py
@@ -10,7 +10,7 @@ from typing import (  # noqa: F401, imported for Mypy.
     Type,
 )
 
-from attr import fields, NOTHING
+from attr import NOTHING, fields
 
 from cattr._compat import get_origin
 
@@ -24,11 +24,7 @@ def create_uniq_field_dis_func(*classes: Type) -> Callable:
     cls_and_attrs = [
         (
             cl,
-            set(
-                at.name
-                for at in fields(get_origin(cl) or cl)
-                if at.default is NOTHING
-            ),
+            set(at.name for at in fields(get_origin(cl) or cl)),
         )
         for cl in classes
     ]
@@ -49,7 +45,14 @@ def create_uniq_field_dis_func(*classes: Type) -> Callable:
             if not uniq:
                 m = "{} has no usable unique attributes.".format(cl)
                 raise ValueError(m)
-            uniq_attrs_dict[next(iter(uniq))] = cl
+            # We need a unique attribute with no default.
+            cl_fields = fields(get_origin(cl) or cl)
+            for attr_name in uniq:
+                if getattr(cl_fields, attr_name).default is NOTHING:
+                    break
+            else:
+                raise Exception(f"{cl} has no usable non-default attributes.")
+            uniq_attrs_dict[attr_name] = cl
         else:
             fallback = cl
 
diff --git a/tests/test_disambigutors.py b/tests/test_disambigutors.py
index c9078e7..80040b2 100644
--- a/tests/test_disambigutors.py
+++ b/tests/test_disambigutors.py
@@ -1,8 +1,8 @@
 """Tests for auto-disambiguators."""
 import attr
 import pytest
-
-from hypothesis import assume, given
+from attr import NOTHING
+from hypothesis import HealthCheck, assume, given, settings
 
 from cattr.disambiguators import create_uniq_field_dis_func
 
@@ -63,9 +63,12 @@ def test_fallback(cl_and_vals):
         fn({"xyz": 1}) is A  # Uses the fallback.
 
 
-@given(simple_classes(), simple_classes())
+@settings(
+    suppress_health_check=[HealthCheck.filter_too_much, HealthCheck.too_slow]
+)
+@given(simple_classes(min_attrs=1), simple_classes())
 def test_disambiguation(cl_and_vals_a, cl_and_vals_b):
-    """Disambiguation should work when there are unique fields."""
+    """Disambiguation should work when there are unique required fields."""
     cl_a, vals_a = cl_and_vals_a
     cl_b, vals_b = cl_and_vals_b
 
@@ -76,6 +79,10 @@ def test_disambiguation(cl_and_vals_a, cl_and_vals_b):
     assume(len(req_b))
 
     assume((req_a - req_b) or (req_b - req_a))
+    for attr_name in req_a - req_b:
+        assume(getattr(attr.fields(cl_a), attr_name).default is NOTHING)
+    for attr_name in req_b - req_a:
+        assume(getattr(attr.fields(cl_b), attr_name).default is NOTHING)
 
     fn = create_uniq_field_dis_func(cl_a, cl_b)
 

Also please add a line to the changelog with a link back here.

[codecov]

Codecov Report

Merging #108 (ba9cd71) into master (1ab1cb2) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #108      +/-   ##
==========================================
+ Coverage   97.70%   97.72%   +0.02%     
==========================================
  Files           7        7              
  Lines         435      440       +5     
==========================================
+ Hits          425      430       +5     
  Misses         10       10              

Impacted Files	Coverage Δ
src/cattr/disambiguators.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ab1cb2...ba9cd71. Read the comment docs.

[dswistowski]

you are totally right, somehow I missed that case - now the hypothesis error message makes sense.

It still failing, but I'm totally swamped with meetings today. Will try to fix it later

[dswistowski]

@Tinche I think it's ready to review. (Autoreview in this case :P)

[Tinche]

@dswistowski I caught a cold, will review soon-ish :)

[Tinche]

Looks good, thanks!