From ab3ea181d17a9250bf4f92eda0bf27d048521931 Mon Sep 17 00:00:00 2001
From: Chris Riccomini <criccomini@apache.org>
Date: Mon, 12 Feb 2024 10:12:45 -0800
Subject: [PATCH] RecapType.is_nullable should iterate on union types

There was a bug in #423. Null types in a union with `extra_attrs` or other
attributes set would result in `False` returned when there was in fact a
NullType in the UnionType's `types` list. `__eq__` checks all attributes, so you
must iterate over the UnionType's `types` attribute and look for any type with
`isinstance(..., NullType) is True`. I updated the logic accordingly.

In doing so, I discovered that the JSON converter logic was converting JSON
schema fields of `null` type to a UnionType with a single nested NullType type.
This seems wrong; I updated the test to validate that `null` JSON fields are
returned as `NullType` with a default of `None`.
---
 recap/converters/json_schema.py           | 10 ++++---
 recap/types.py                            | 12 +++++++-
 tests/unit/converters/test_json_schema.py | 14 ++++-----
 tests/unit/test_types.py                  | 36 +++++++++++++++++++++++
 4 files changed, 58 insertions(+), 14 deletions(-)

diff --git a/recap/converters/json_schema.py b/recap/converters/json_schema.py
index ffdac9b..24721ae 100644
--- a/recap/converters/json_schema.py
+++ b/recap/converters/json_schema.py
@@ -54,8 +54,8 @@ def _parse(
         alias_strategy: AliasStrategy,
     ) -> RecapType:
         extra_attrs = {}
-        # Check if json_schema is just a string representing a basic type, and convert
-        # to a dict with a "type" property if so
+        # Check if json_schema is just a string representing a basic type, and
+        # convert to a dict with a "type" property if so
         if isinstance(json_schema, str):
             json_schema = {"type": json_schema}
         if "description" in json_schema:
@@ -79,11 +79,13 @@ def _parse(
                 fields = []
                 for name, prop in properties.items():
                     field = self._parse(prop, alias_strategy)
-                    # If not explicitly required, make optional by ensuring the field is
-                    # nullable, and has a default
+                    # If not explicitly required, make optional by ensuring the
+                    # field is nullable, and has a default
                     if name not in json_schema.get("required", []):
                         if not field.is_nullable():
                             field = field.make_nullable()
+                        # is_nullable doesn't guarantee a default of none, so
+                        # set it here.
                         if "default" not in field.extra_attrs:
                             field.extra_attrs["default"] = None
 
diff --git a/recap/types.py b/recap/types.py
index 5e5ad99..7773800 100644
--- a/recap/types.py
+++ b/recap/types.py
@@ -73,7 +73,17 @@ def is_nullable(self) -> bool:
         :return: True if the type is nullable.
         """
 
-        return isinstance(self, UnionType) and NullType() in self.types
+        if isinstance(self, UnionType):
+            # Can't do `NullType() in type_copy.types` because equality checks
+            # extra_attrs, which can vary. Instead, just look for any NullType
+            # instance.
+            for t in self.types:
+                if isinstance(t, NullType):
+                    return True
+
+            return False
+
+        return isinstance(self, NullType)
 
     def validate(self) -> None:
         # Default to valid type
diff --git a/tests/unit/converters/test_json_schema.py b/tests/unit/converters/test_json_schema.py
index 8912b74..3a4fd84 100644
--- a/tests/unit/converters/test_json_schema.py
+++ b/tests/unit/converters/test_json_schema.py
@@ -39,29 +39,25 @@ def test_all_basic_types():
     assert struct_type.fields == [
         UnionType(
             [NullType(), StringType()],
-            name="a_string",
             default=None,
+            name="a_string",
         ),
         UnionType(
             [NullType(), FloatType(bits=64)],
-            name="a_number",
             default=None,
+            name="a_number",
         ),
         UnionType(
             [NullType(), IntType(bits=32, signed=True)],
-            name="an_integer",
             default=None,
+            name="an_integer",
         ),
         UnionType(
             [NullType(), BoolType()],
-            name="a_boolean",
-            default=None,
-        ),
-        UnionType(
-            [NullType()],
-            name="a_null",
             default=None,
+            name="a_boolean",
         ),
+        NullType(default=None, name="a_null"),
     ]
 
 
diff --git a/tests/unit/test_types.py b/tests/unit/test_types.py
index 04cb98e..b85c041 100644
--- a/tests/unit/test_types.py
+++ b/tests/unit/test_types.py
@@ -1639,3 +1639,39 @@ def test_make_nullable_of_default_none():
         ],
         default=None,
     )
+
+
+def test_is_nullable():
+    assert NullType().is_nullable() is True
+    assert NullType(doc="Something", foo=123).is_nullable() is True
+    assert IntType(bits=32).is_nullable() is False
+    assert (
+        UnionType(
+            types=[
+                NullType(),
+                IntType(bits=32),
+            ],
+            default=None,
+        ).is_nullable()
+        is True
+    )
+    assert (
+        UnionType(
+            types=[
+                IntType(bits=32),
+                NullType(doc="Something", foo=123),
+            ],
+            default=123,
+        ).is_nullable()
+        is True
+    )
+    assert (
+        UnionType(
+            types=[
+                IntType(bits=32),
+                StringType(bytes_=50),
+            ],
+            default=123,
+        ).is_nullable()
+        is False
+    )