Fix recursive association stack overflow + $schema version location (#24

) * Update go.mod * Update helpers.go Create a helper to fetch definition key so that we don't run into inconsistent keys as easily. * Update reflect.go Fix stack overflow issue by consolidating inserted and fetched key for definitions. * Update reflect.go Fix errors/consolidate definitions key. * Add tests and fix location * Add recursion test case
discovery-digital · Apr 29, 2019 · d708398 · d708398
1 parent 9c48c65
commit d708398
Show file tree

Hide file tree

Showing 15 changed files with 72 additions and 61 deletions.
diff --git a/fixtures/allow_additional_props.json b/fixtures/allow_additional_props.json
@@ -53,8 +53,7 @@
           "type": "array"
         },
         "grand": {
-          "$ref": "#/definitions/testmodels.GrandfatherType",
-          "$schema": "http://json-schema.org/draft-07/schema#"
+          "$ref": "#/definitions/testmodels.GrandfatherType"
         },
         "id": {
           "type": "integer"

diff --git a/fixtures/case.json b/fixtures/case.json
@@ -37,8 +37,7 @@
                         }
                     },
                     "then": {
-                        "$ref": "#/definitions/testmodels.BoolPayload",
-                        "$schema": "http://json-schema.org/draft-07/schema#"
+                        "$ref": "#/definitions/testmodels.BoolPayload"
                     }
                 },
                 {
@@ -61,8 +60,7 @@
                         }
                     },
                     "then": {
-                        "$ref": "#/definitions/testmodels.IntPayload",
-                        "$schema": "http://json-schema.org/draft-07/schema#"
+                        "$ref": "#/definitions/testmodels.IntPayload"
                     }
                 },
                 {
@@ -85,8 +83,7 @@
                         }
                     },
                     "then": {
-                        "$ref": "#/definitions/testmodels.StringPayload",
-                        "$schema": "http://json-schema.org/draft-07/schema#"
+                        "$ref": "#/definitions/testmodels.StringPayload"
                     }
                 }
             ],

diff --git a/fixtures/defaults.json b/fixtures/defaults.json
@@ -53,8 +53,7 @@
           "type": "array"
         },
         "grand": {
-          "$ref": "#/definitions/testmodels.GrandfatherType",
-          "$schema": "http://json-schema.org/draft-07/schema#"
+          "$ref": "#/definitions/testmodels.GrandfatherType"
         },
         "id": {
           "type": "integer"

diff --git a/fixtures/defaults_expanded_toplevel.json b/fixtures/defaults_expanded_toplevel.json
@@ -50,8 +50,7 @@
           "type": "array"
         },
         "grand": {
-          "$ref": "#/definitions/testmodels.GrandfatherType",
-          "$schema": "http://json-schema.org/draft-07/schema#"
+          "$ref": "#/definitions/testmodels.GrandfatherType"
         },
         "id": {
           "type": "integer"
@@ -152,8 +151,7 @@
       "type": "array"
     },
     "grand": {
-      "$ref": "#/definitions/testmodels.GrandfatherType",
-      "$schema": "http://json-schema.org/draft-07/schema#"
+      "$ref": "#/definitions/testmodels.GrandfatherType"
     },
     "id": {
       "type": "integer"

diff --git a/fixtures/if_then_else.json b/fixtures/if_then_else.json
@@ -5,8 +5,7 @@
     "testmodels.Application": {
       "additionalProperties": false,
       "else": {
-        "$ref": "#/definitions/testmodels.MobileApp",
-        "$schema": "http://json-schema.org/draft-07/schema#"
+        "$ref": "#/definitions/testmodels.MobileApp"
       },
       "if": {
         "properties": {
@@ -22,8 +21,7 @@
       },
       "required": ["type"],
       "then": {
-        "$ref": "#/definitions/testmodels.WebApp",
-        "$schema": "http://json-schema.org/draft-07/schema#"
+        "$ref": "#/definitions/testmodels.WebApp"
       },
       "type": "object"
     },

diff --git a/fixtures/override_jsonschema_tag.json b/fixtures/override_jsonschema_tag.json
@@ -1,4 +1,5 @@
 {
+  "$schema": "http://json-schema.org/draft-07/schema#",
   "definitions": {
     "testmodels.Desktop": {
       "additionalProperties": false,
@@ -29,8 +30,7 @@
           ]
         },
         "hardware": {
-          "$ref": "#/definitions/testmodels.Hardware",
-          "$schema": "http://json-schema.org/draft-07/schema#"
+          "$ref": "#/definitions/testmodels.Hardware"
         },
         "language": {
           "pattern": "\\S+",
@@ -50,11 +50,9 @@
     "testmodels.Hardware": {
       "additionalProperties": false,
       "oneOf": [{
-        "$ref": "#/definitions/testmodels.Laptop",
-        "$schema": "http://json-schema.org/draft-07/schema#"
+        "$ref": "#/definitions/testmodels.Laptop"
       }, {
-        "$ref": "#/definitions/testmodels.Desktop",
-        "$schema": "http://json-schema.org/draft-07/schema#"
+        "$ref": "#/definitions/testmodels.Desktop"
       }],
       "properties": {
         "brand": {
@@ -101,10 +99,8 @@
     }
   },
   "oneOf": [{
-    "$ref": "#/definitions/testmodels.Tester",
-    "$schema": "http://json-schema.org/draft-07/schema#"
+    "$ref": "#/definitions/testmodels.Tester"
   }, {
-    "$ref": "#/definitions/testmodels.Developer",
-    "$schema": "http://json-schema.org/draft-07/schema#"
+    "$ref": "#/definitions/testmodels.Developer"
   }]
 }
diff --git a/fixtures/required_from_jsontags.json b/fixtures/required_from_jsontags.json
@@ -53,8 +53,7 @@
           "type": "array"
         },
         "grand": {
-          "$ref": "#/definitions/testmodels.GrandfatherType",
-          "$schema": "http://json-schema.org/draft-07/schema#"
+          "$ref": "#/definitions/testmodels.GrandfatherType"
         },
         "id": {
           "type": "integer"

diff --git a/fixtures/test_min_max_items.json b/fixtures/test_min_max_items.json
@@ -1,4 +1,5 @@
 {
+  "$schema": "http://json-schema.org/draft-07/schema#",
   "items": {
     "type": "string"
   },

diff --git a/fixtures/test_one_of_default.json b/fixtures/test_one_of_default.json
@@ -1,4 +1,5 @@
 {
+  "$schema": "http://json-schema.org/draft-07/schema#",
   "definitions": {
     "testmodels.Desktop": {
       "additionalProperties": false,
@@ -29,8 +30,7 @@
           ]
         },
         "hardware": {
-          "$ref": "#/definitions/testmodels.Hardware",
-          "$schema": "http://json-schema.org/draft-07/schema#"
+          "$ref": "#/definitions/testmodels.Hardware"
         },
         "language": {
           "pattern": "\\S+",
@@ -50,11 +50,9 @@
     "testmodels.Hardware": {
       "additionalProperties": false,
       "oneOf": [{
-        "$ref": "#/definitions/testmodels.Laptop",
-        "$schema": "http://json-schema.org/draft-07/schema#"
+        "$ref": "#/definitions/testmodels.Laptop"
       }, {
-        "$ref": "#/definitions/testmodels.Desktop",
-        "$schema": "http://json-schema.org/draft-07/schema#"
+        "$ref": "#/definitions/testmodels.Desktop"
       }],
       "properties": {
         "brand": {
@@ -101,10 +99,8 @@
     }
   },
   "oneOf": [{
-    "$ref": "#/definitions/testmodels.Tester",
-    "$schema": "http://json-schema.org/draft-07/schema#"
+    "$ref": "#/definitions/testmodels.Tester"
   }, {
-    "$ref": "#/definitions/testmodels.Developer",
-    "$schema": "http://json-schema.org/draft-07/schema#"
+    "$ref": "#/definitions/testmodels.Developer"
   }]
 }
diff --git a/fixtures/test_recursion.json b/fixtures/test_recursion.json
@@ -0,0 +1,21 @@
+{
+  "$ref": "#/definitions/testmodels.TestFamilyMember",
+  "$schema": "http://json-schema.org/draft-07/schema#",
+  "definitions": {
+    "testmodels.TestFamilyMember": {
+      "additionalProperties": false,
+      "properties": {
+        "children": {
+          "items": {
+            "$ref": "#/definitions/testmodels.TestFamilyMember"
+          },
+          "type": "array"
+        }
+      },
+      "required": [
+        "children"
+      ],
+      "type": "object"
+    }
+  }
+}
diff --git a/fixtures/test_versioned_packages.json b/fixtures/test_versioned_packages.json
@@ -1,4 +1,5 @@
 {
+  "$schema": "http://json-schema.org/draft-07/schema#",
   "definitions": {
     "testmodels.DeveloperPackage": {
       "additionalProperties": false,
@@ -12,12 +13,10 @@
           }]
         },
         "hardware": {
-          "$ref": "#/definitions/v1.Hardware",
-          "$schema": "http://json-schema.org/draft-07/schema#"
+          "$ref": "#/definitions/v1.Hardware"
         },
         "hardware1": {
-          "$ref": "#/definitions/v2.Hardware",
-          "$schema": "http://json-schema.org/draft-07/schema#"
+          "$ref": "#/definitions/v2.Hardware"
         },
         "language": {
           "oneOf": [{
@@ -48,11 +47,9 @@
     "v1.Hardware": {
       "additionalProperties": false,
       "oneOf": [{
-        "$ref": "#/definitions/v1.Laptop",
-        "$schema": "http://json-schema.org/draft-07/schema#"
+        "$ref": "#/definitions/v1.Laptop"
       }, {
-        "$ref": "#/definitions/v1.PC",
-        "$schema": "http://json-schema.org/draft-07/schema#"
+        "$ref": "#/definitions/v1.PC"
       }],
       "properties": {
         "brand": {
@@ -110,10 +107,8 @@
     }
   },
   "oneOf": [{
-    "$ref": "#/definitions/testmodels.TesterPackage",
-    "$schema": "http://json-schema.org/draft-07/schema#"
+    "$ref": "#/definitions/testmodels.TesterPackage"
   }, {
-    "$ref": "#/definitions/testmodels.DeveloperPackage",
-    "$schema": "http://json-schema.org/draft-07/schema#"
+    "$ref": "#/definitions/testmodels.DeveloperPackage"
   }]
 }
diff --git a/helpers.go b/helpers.go
@@ -82,6 +82,11 @@ func getPackageNameFromPath(path string) string {
 	return pathSlices[len(pathSlices)-1]
 }
 
+func getDefinitionKeyFromType(t reflect.Type) string {
+	packageName := getPackageNameFromPath(t.PkgPath())
+	return packageName + "." + t.Name()
+}
+
 // bool2bytes serializes bool to JSON
 func bool2bytes(val bool) []byte {
 	if val {

diff --git a/internal/testmodels/recursion.go b/internal/testmodels/recursion.go
@@ -0,0 +1,5 @@
+package testmodels
+
+type TestFamilyMember struct {
+	Children []TestFamilyMember `json:"children"`
+}
diff --git a/reflect.go b/reflect.go
@@ -131,8 +131,11 @@ func (r *Reflector) ReflectFromType(t reflect.Type) *Schema {
 		return &Schema{Type: st, Definitions: definitions}
 	}
 
+	rootType := r.reflectTypeToSchema(definitions, t)
+	rootType.Version = Version
+
 	s := &Schema{
-		Type:        r.reflectTypeToSchema(definitions, t),
+		Type:        rootType,
 		Definitions: definitions,
 	}
 	return s
@@ -173,8 +176,9 @@ var maxItemsType = reflect.TypeOf((*maxItems)(nil)).Elem()
 
 func (r *Reflector) reflectTypeToSchema(definitions Definitions, t reflect.Type) (schema *Type) {
 	// Already added to definitions?
-	if _, ok := definitions[t.Name()]; ok {
-		return &Type{Ref: "#/definitions/" + getPackageNameFromPath(t.PkgPath()) + "." + t.Name()}
+	definitionsKey := getDefinitionKeyFromType(t)
+	if _, ok := definitions[definitionsKey]; ok {
+		return &Type{Ref: "#/definitions/" + definitionsKey}
 	}
 
 	// jsonpb will marshal protobuf enum options as either strings or integers.
@@ -297,15 +301,12 @@ func (r *Reflector) reflectStruct(definitions Definitions, t reflect.Type) *Type
 		AdditionalProperties: bool2bytes(r.AllowAdditionalProperties),
 	}
 
-	packageName := getPackageNameFromPath(t.PkgPath())
-	definitions[packageName+"."+t.Name()] = st
+	definitionsKey := getDefinitionKeyFromType(t)
+	definitions[definitionsKey] = st
 	r.reflectStructFields(st, definitions, t)
 	r.addSubschemasForConditionalCases(st, definitions, t)
 
-	return &Type{
-		Version: Version,
-		Ref:     "#/definitions/" + packageName + "." + t.Name(),
-	}
+	return &Type{Ref: "#/definitions/" + definitionsKey}
 }
 
 func (r *Reflector) reflectStructFields(st *Type, definitions Definitions, t reflect.Type) {

diff --git a/reflect_test.go b/reflect_test.go
@@ -27,6 +27,7 @@ var schemaGenerationTests = []testSet{
 	{&jsonschema.Reflector{}, "fixtures/if_then_else.json", testmodels.Application{}},
 	{&jsonschema.Reflector{}, "fixtures/case.json", testmodels.ExampleCase{}},
 	{&jsonschema.Reflector{}, "fixtures/test_min_max_items.json", testmodels.SliceTestType{}},
+	{&jsonschema.Reflector{}, "fixtures/test_recursion.json", testmodels.TestFamilyMember{}},
 }
 
 func TestSchemaGeneration(t *testing.T) {