Merge pull request 'schemagen: cache non-object $defs entries to avoid duplicate types' (#31) from weaselbot/weaseljson:weaselbot/issue-17 into main

Reviewed-on: weaselab/weaseljson#31

contrib/schemagen/test_schemagen.py

@@ -195,6 +195,37 @@ class SchemagenCollisionTest(unittest.TestCase):
         self.assertIn("Arr0", enumerators)
         self.assertEqual(len(enumerators), len(set(enumerators)))
 
+    def test_non_object_defs_reused_across_refs(self):
+        """Regression test for issue #17: enum and array $defs must be reused."""
+        schema = {
+            "type": "object",
+            "properties": {
+                "role1": {"$ref": "#/$defs/Role"},
+                "role2": {"$ref": "#/$defs/Role"},
+                "roles1": {"$ref": "#/$defs/Roles"},
+                "roles2": {"$ref": "#/$defs/Roles"},
+            },
+            "$defs": {
+                "Role": {"enum": ["admin", "user"]},
+                "Roles": {"type": "array", "items": {"$ref": "#/$defs/Role"}},
+            },
+        }
+        out = self.generate_and_compile(schema)
+        # Exactly one Role enum is generated.
+        self.assertIn("enum class Role : int { admin, user };", out)
+        self.assertNotIn("enum class Role1 : int", out)
+        self.assertNotIn("enum class Role2 : int", out)
+        # The array of enum is represented by a single kind.
+        m = re.search(r"enum class Kind : uint8_t \{([^}]+)\}", out)
+        self.assertIsNotNone(m)
+        enumerators = [e.strip() for e in m.group(1).split(",")]
+        self.assertEqual(enumerators.count("Arr0"), 1)
+        # All four fields use the same C++ types.
+        self.assertIn("std::optional<Role> role1;", out)
+        self.assertIn("std::optional<Role> role2;", out)
+        self.assertIn("std::optional<std::vector<Role>> roles1;", out)
+        self.assertIn("std::optional<std::vector<Role>> roles2;", out)
+
 
 class SchemagenAdditionalPropertiesTest(unittest.TestCase):
     def run_schemagen(self, schema, args=None):

contrib/schemagen/weaseljson_schemagen.py

@@ -250,7 +250,6 @@ class Builder:
         if defname not in self.defs:
             raise GenError(f"$ref to unknown def: {defname}")
         node = self.defs[defname]
-        # For objects we must register the name before recursing into fields.
         return self.build_type(node, defname, defname=defname)
 
     def build_type(self, node, hint, defname=None):
@@ -279,6 +278,10 @@ class Builder:
         if "$ref" in node:
             return self.build_def(self.ref_name(node["$ref"]))
 
+        # Non-object $defs entries must reuse the same type for every $ref.
+        if defname is not None and defname in self._building:
+            return self._building[defname]
+
         # nullability via type lists: ["string", "null"]
         nullable = False
         typ = node.get("type")
@@ -297,18 +300,29 @@ class Builder:
                 raise GenError("only non-empty string enums are supported")
             name = defname and self.unique_name(defname) or self.unique_name(hint)
             self.enums[name] = EnumType(name, list(vals))
-            return (TEnum(name), nullable)
+            result = (TEnum(name), nullable)
+            if defname is not None:
+                self._building[defname] = result
+            return result
 
         if typ == "object" or (typ is None and "properties" in node):
-            return (self._build_object(node, hint, defname), nullable)
+            return (self._build_object(node, hint, defname, nullable), nullable)
 
         if typ == "array":
             if "items" not in node or not isinstance(node["items"], dict):
                 raise GenError("arrays require a single 'items' schema")
+            # Register the array before building its items so $ref cycles back
+            # to this definition resolve to the same TArr instance.
+            t = TArr(None, False)
+            result = (t, nullable)
+            if defname is not None:
+                self._building[defname] = result
             elem, elem_nullable = self._unpack(
                 self.build_type(node["items"], hint + "Item")
             )
-            return (TArr(elem, elem_nullable), nullable)
+            t.elem = elem
+            t.elem_nullable = elem_nullable
+            return result
 
         scalar = {
             "string": "str",
@@ -317,7 +331,10 @@ class Builder:
             "boolean": "bool",
         }.get(typ)
         if scalar:
-            return (TScalar(scalar), nullable)
+            result = (TScalar(scalar), nullable)
+            if defname is not None:
+                self._building[defname] = result
+            return result
         if typ == "null":
             raise GenError("'null'-only types are not supported")
 
@@ -331,13 +348,14 @@ class Builder:
             return result
         return (result, False)
 
-    def _build_object(self, node, hint, defname):
+    def _build_object(self, node, hint, defname, nullable=False):
         name = self.unique_name(defname or hint)
         obj = ObjectType(name)
         self.objects[name] = obj
         # register for $ref cycles before building fields
+        tobj = TObj(name)
         if defname is not None:
-            self._building[defname] = TObj(name)
+            self._building[defname] = (tobj, nullable)
         ap = node.get("additionalProperties", False)
         if ap is True:
             raise GenError("additionalProperties: true is not supported")
@@ -358,7 +376,7 @@ class Builder:
                 cpp = f"{base}{i}"
             seen_cpp.add(cpp)
             obj.fields.append(Field(key, cpp, ty, key in required, nullable))
-        return TObj(name)
+        return tobj
 
 
 # ---------------------------------------------------------------------------