Merge pull request #342 from snyk/fix/error-on-conflict

fix: throw error when conflicting components are found

collator.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"regexp"
 	"sort"
+	"unicode"
 
 	"github.com/getkin/kin-openapi/openapi3"
 	"github.com/google/go-cmp/cmp"
@@ -241,10 +242,19 @@ var cmpComponents = cmp.Options{
 		openapi3.Schema{},
 		openapi3.SchemaRef{},
 	),
-	// Refs themselves can mutate during relocation, so they are excluded from
-	// content comparison.
 	cmp.FilterPath(func(p cmp.Path) bool {
-		return p.Last().String() == ".Ref"
+		// We can't reflect on non-exported members, this will only be relevant
+		// if there are fields on the source structures that are unexpected -
+		// eg invalid openapi properties.
+		isPrivate := false
+		member := p.Last().String()
+		if len(member) > 1 {
+			isPrivate = unicode.IsLower(rune(member[1]))
+		}
+		// Refs themselves can mutate during relocation, so they are excluded
+		// from content comparison.
+		isRef := p.Last().String() == ".Ref"
+		return isPrivate || isRef
 	}, cmp.Ignore()),
 }
 

internal/compiler/compiler.go

@@ -184,10 +184,16 @@ func (c *Compiler) Build(ctx context.Context, apiName string) error {
 
 			// Merge all overlays
 			for _, doc := range api.overlayIncludes {
-				vervet.Merge(spec, doc.T, true)
+				err = vervet.Merge(spec, doc.T, true)
+				if err != nil {
+					return buildErr(err)
+				}
 			}
 			for _, doc := range api.overlayInlines {
-				vervet.Merge(spec, doc, true)
+				err = vervet.Merge(spec, doc, true)
+				if err != nil {
+					return buildErr(err)
+				}
 			}
 
 			// Write the compiled spec to JSON and YAML

merge.go

@@ -1,6 +1,7 @@
 package vervet
 
 import (
+	"errors"
 	"sort"
 
 	"github.com/getkin/kin-openapi/openapi3"
@@ -19,15 +20,20 @@ import (
 //   - This function is suitable for overlay merging scenarios only.
 //   - Component merging should be removed. Use Collator for safe component
 //     merging.
-func Merge(dst, src *openapi3.T, replace bool) {
-	mergeComponents(dst, src, replace)
+func Merge(dst, src *openapi3.T, replace bool) error {
+	err := mergeComponents(dst, src, replace)
+	if err != nil {
+		return err
+	}
 	mergeExtensions(dst, src, replace)
 	mergeInfo(dst, src, replace)
 	mergeOpenAPIVersion(dst, src, replace)
 	mergePaths(dst, src, replace)
 	mergeSecurityRequirements(dst, src, replace)
 	mergeServers(dst, src, replace)
 	mergeTags(dst, src, replace)
+
+	return nil
 }
 
 func mergeOpenAPIVersion(dst, src *openapi3.T, replace bool) {
@@ -87,9 +93,9 @@ func initDestinationComponents(dst, src *openapi3.T) {
 	}
 }
 
-func mergeComponents(dst, src *openapi3.T, replace bool) {
+func mergeComponents(dst, src *openapi3.T, replace bool) error {
 	if src.Components == nil {
-		return
+		return nil
 	}
 
 	if dst.Components == nil {
@@ -98,23 +104,50 @@ func mergeComponents(dst, src *openapi3.T, replace bool) {
 
 	initDestinationComponents(dst, src)
 
-	mergeMap(dst.Components.Schemas, src.Components.Schemas, replace)
-	mergeMap(dst.Components.Parameters, src.Components.Parameters, replace)
-	mergeMap(dst.Components.Headers, src.Components.Headers, replace)
-	mergeMap(dst.Components.RequestBodies, src.Components.RequestBodies, replace)
-	mergeMap(dst.Components.Responses, src.Components.Responses, replace)
-	mergeMap(dst.Components.SecuritySchemes, src.Components.SecuritySchemes, replace)
-	mergeMap(dst.Components.Examples, src.Components.Examples, replace)
-	mergeMap(dst.Components.Links, src.Components.Links, replace)
-	mergeMap(dst.Components.Callbacks, src.Components.Callbacks, replace)
+	err := mergeMap(dst.Components.Schemas, src.Components.Schemas, replace)
+	if err != nil {
+		return err
+	}
+	err = mergeMap(dst.Components.Parameters, src.Components.Parameters, replace)
+	if err != nil {
+		return err
+	}
+	err = mergeMap(dst.Components.Headers, src.Components.Headers, replace)
+	if err != nil {
+		return err
+	}
+	err = mergeMap(dst.Components.RequestBodies, src.Components.RequestBodies, replace)
+	if err != nil {
+		return err
+	}
+	err = mergeMap(dst.Components.Responses, src.Components.Responses, replace)
+	if err != nil {
+		return err
+	}
+	err = mergeMap(dst.Components.SecuritySchemes, src.Components.SecuritySchemes, replace)
+	if err != nil {
+		return err
+	}
+	err = mergeMap(dst.Components.Examples, src.Components.Examples, replace)
+	if err != nil {
+		return err
+	}
+	err = mergeMap(dst.Components.Links, src.Components.Links, replace)
+	if err != nil {
+		return err
+	}
+	return mergeMap(dst.Components.Callbacks, src.Components.Callbacks, replace)
 }
 
-func mergeMap[T any](dst, src map[string]T, replace bool) {
+func mergeMap[T any](dst, src map[string]T, replace bool) error {
 	for k, v := range src {
-		if _, ok := dst[k]; !ok || replace {
-			dst[k] = v
+		existing, exists := dst[k]
+		if exists && !replace && !componentsEqual(v, existing) {
+			return errors.New("conflicting component: " + k)
 		}
+		dst[k] = v
 	}
+	return nil
 }
 
 func mergeExtensions(dst, src *openapi3.T, replace bool) {

merge_test.go

@@ -24,11 +24,18 @@ var openapiCmp = qt.CmpEquals(cmpopts.IgnoreUnexported(
 
 func TestMergeComponents(t *testing.T) {
 	c := qt.New(t)
+	c.Run("component without replace and conflict", func(c *qt.C) {
+		src := mustLoadFile(c, "merge_test_conflict.yaml")
+		dst := mustLoadFile(c, "merge_test_dst.yaml")
+		err := vervet.Merge(dst, src, false)
+		c.Assert(err, qt.IsNotNil)
+	})
 	c.Run("component without replace", func(c *qt.C) {
 		src := mustLoadFile(c, "merge_test_src.yaml")
 		dstOrig := mustLoadFile(c, "merge_test_dst.yaml")
 		dst := mustLoadFile(c, "merge_test_dst.yaml")
-		vervet.Merge(dst, src, false)
+		err := vervet.Merge(dst, src, false)
+		c.Assert(err, qt.IsNil)
 
 		c.Assert(dst.Components.Schemas["Foo"], openapiCmp, dstOrig.Components.Schemas["Foo"])
 		c.Assert(dst.Components.Schemas["Bar"], openapiCmp, src.Components.Schemas["Bar"])
@@ -62,7 +69,8 @@ func TestMergeComponents(t *testing.T) {
 		src := mustLoadFile(c, "merge_test_src.yaml")
 		dstOrig := mustLoadFile(c, "merge_test_dst.yaml")
 		dst := mustLoadFile(c, "merge_test_dst.yaml")
-		vervet.Merge(dst, src, true)
+		err := vervet.Merge(dst, src, true)
+		c.Assert(err, qt.IsNil)
 
 		c.Assert(dst.Components.Schemas["Foo"], openapiCmp, src.Components.Schemas["Foo"])
 		c.Assert(dst.Components.Schemas["Bar"], openapiCmp, src.Components.Schemas["Bar"])
@@ -96,7 +104,8 @@ func TestMergeComponents(t *testing.T) {
 		src := mustLoadFile(c, "merge_test_src.yaml")
 		dstOrig := mustLoadFile(c, "merge_test_dst_missing_components.yaml")
 		dst := mustLoadFile(c, "merge_test_dst_missing_components.yaml")
-		vervet.Merge(dst, src, true)
+		err := vervet.Merge(dst, src, true)
+		c.Assert(err, qt.IsNil)
 
 		c.Assert(dst.Components.Schemas["Foo"], openapiCmp, src.Components.Schemas["Foo"])
 		c.Assert(dst.Components.Schemas["Bar"], openapiCmp, src.Components.Schemas["Bar"])
@@ -147,7 +156,8 @@ tags:
 	c.Run("tags without replace", func(c *qt.C) {
 		src := mustLoad(c, srcYaml)
 		dst := mustLoad(c, dstYaml)
-		vervet.Merge(dst, src, false)
+		err := vervet.Merge(dst, src, false)
+		c.Assert(err, qt.IsNil)
 		c.Assert(dst.Tags, qt.DeepEquals, openapi3.Tags{{
 			Extensions:  map[string]interface{}{},
 			Name:        "bar",
@@ -165,7 +175,8 @@ tags:
 	c.Run("tags with replace", func(c *qt.C) {
 		src := mustLoad(c, srcYaml)
 		dst := mustLoad(c, dstYaml)
-		vervet.Merge(dst, src, true)
+		err := vervet.Merge(dst, src, true)
+		c.Assert(err, qt.IsNil)
 		c.Assert(dst.Tags, qt.DeepEquals, openapi3.Tags{{
 			Extensions:  map[string]interface{}{},
 			Name:        "bar",
@@ -225,7 +236,8 @@ x-extension:
 	c.Run("without replace", func(c *qt.C) {
 		src := mustLoad(c, srcYaml)
 		dst := mustLoad(c, dstYaml)
-		vervet.Merge(dst, src, false)
+		err := vervet.Merge(dst, src, false)
+		c.Assert(err, qt.IsNil)
 		c.Assert(dst.Info, qt.DeepEquals, &openapi3.Info{
 			Extensions: map[string]interface{}{},
 			Title:      "Dst",
@@ -255,7 +267,8 @@ x-extension:
 	c.Run("with replace", func(c *qt.C) {
 		src := mustLoad(c, srcYaml)
 		dst := mustLoad(c, dstYaml)
-		vervet.Merge(dst, src, true)
+		err := vervet.Merge(dst, src, true)
+		c.Assert(err, qt.IsNil)
 		c.Assert(dst.Info, qt.DeepEquals, &openapi3.Info{
 			Extensions: map[string]interface{}{},
 			Title:      "Src",
@@ -303,7 +316,8 @@ paths:
 `
 	src := mustLoad(c, srcYaml)
 	dst := &openapi3.T{}
-	vervet.Merge(dst, src, false)
+	err := vervet.Merge(dst, src, false)
+	c.Assert(err, qt.IsNil)
 	c.Assert(dst.Paths, qt.HasLen, 1)
 }
 

spec.go

@@ -88,7 +88,7 @@ func (sv *SpecVersions) At(v Version) (*openapi3.T, error) {
 	return doc, nil
 }
 
-func (sv *SpecVersions) resolveOperations() {
+func (sv *SpecVersions) resolveOperations() error {
 	type operationKey struct {
 		path, operation string
 	}
@@ -151,7 +151,14 @@ func (sv *SpecVersions) resolveOperations() {
 			if currentOp == nil {
 				// The added operation may reference components from its source
 				// document; import those that are missing here.
-				mergeComponents(doc, opValue.src, false)
+				err := mergeComponents(doc, opValue.src, false)
+				if err != nil {
+					return fmt.Errorf(`
+Cannot rollup endpoints as there are conflicts in components."
+"This is a known problem with Vervet.
+Please rename conflicting components as a workaround:
+	%w`, err)
+				}
 				setOperationByName(currentPathItem, opKey.operation, opValue.operation)
 			}
 		}
@@ -161,6 +168,7 @@ func (sv *SpecVersions) resolveOperations() {
 			currentActiveOps[opKey] = nextOpValue
 		}
 	}
+	return nil
 }
 
 var operationNames = []string{
@@ -250,8 +258,8 @@ func newSpecVersions(specs resourceVersionsSlice) (*SpecVersions, error) {
 		index:     NewVersionIndex(maps.Keys(documentVersions)),
 		documents: documentVersions,
 	}
-	sv.resolveOperations()
-	return sv, nil
+	err := sv.resolveOperations()
+	return sv, err
 }
 
 func findResources(root string) ([]string, error) {

testdata/merge_test_conflict.yaml

@@ -0,0 +1,45 @@
+components:
+  schemas:
+    Foo:
+      type: object
+      properties:
+        color:
+          type: string
+          enum: ["red", "green", "blue"]
+        dimension:
+          type: array
+          items:
+            type: number
+        strange:
+          type: boolean
+  parameters:
+    Foo:
+      in: path
+      name: foo
+      required: true
+      schema:
+        type: string
+  headers:
+    Foo:
+      schema:
+        type: string
+  requestBodies:
+    Foo:
+     required: true
+     content:
+       application/json:
+        schema:
+          $ref: '#/components/schemas/Foo'
+  responses:
+    '200':
+      content:
+        application/vnd.api+json:
+          schema:
+            $ref: '#/components/schemas/Foo'
+  securitySchemes:
+    Foo:
+     type: http
+     scheme: basic
+  examples:
+    Foo:
+     value: foo

testdata/merge_test_dst.yaml

@@ -5,13 +5,9 @@ components:
       properties:
         color:
           type: string
-          enum: ["red", "green", "blue"]
-        dimension:
-          type: array
-          items:
-            type: number
-        strange:
-          type: boolean
+          enum: ["red", "green", "blue", "alpha"]
+        quantity:
+          type: number
     Baz:
       type: object
       properties:
@@ -20,7 +16,7 @@ components:
   parameters:
     Foo:
       in: path
-      name: foo2
+      name: foo
       required: true
       schema:
         type: string
@@ -70,6 +66,6 @@ components:
      scheme: basic
   examples:
     Foo:
-     value: foo-natured
+     value: foo
     Baz:
      value: bazil