fix(expand): parameters & responses should properly follow remote doc…

… resolution when SKipSchema (go-openapi#183)

* fix(expand): parameters & responses should properly follow remote doc resolution when SKipSchema

* fixes go-openapi#182
* contributes go-swagger/go-swagger#2743

Signed-off-by: Frederic BIDON <[email protected]>

* fixed assertion in test for windows paths

Signed-off-by: Frederic BIDON <[email protected]>

---------

Signed-off-by: Frederic BIDON <[email protected]>

expander.go

@@ -213,7 +213,19 @@ func expandSchema(target Schema, parentRefs []string, resolver *schemaLoader, ba
 	}
 
 	if target.Ref.String() != "" {
-		return expandSchemaRef(target, parentRefs, resolver, basePath)
+		if !resolver.options.SkipSchemas {
+			return expandSchemaRef(target, parentRefs, resolver, basePath)
+		}
+
+		// when "expand" with SkipSchema, we just rebase the existing $ref without replacing
+		// the full schema.
+		rebasedRef, err := NewRef(normalizeURI(target.Ref.String(), basePath))
+		if err != nil {
+			return nil, err
+		}
+		target.Ref = denormalizeRef(&rebasedRef, resolver.context.basePath, resolver.context.rootID)
+
+		return &target, nil
 	}
 
 	for k := range target.Definitions {
@@ -525,21 +537,25 @@ func getRefAndSchema(input interface{}) (*Ref, *Schema, error) {
 }
 
 func expandParameterOrResponse(input interface{}, resolver *schemaLoader, basePath string) error {
-	ref, _, err := getRefAndSchema(input)
+	ref, sch, err := getRefAndSchema(input)
 	if err != nil {
 		return err
 	}
 
-	if ref == nil {
+	if ref == nil && sch == nil { // nothing to do
 		return nil
 	}
 
 	parentRefs := make([]string, 0, 10)
-	if err = resolver.deref(input, parentRefs, basePath); resolver.shouldStopOnError(err) {
-		return err
+	if ref != nil {
+		// dereference this $ref
+		if err = resolver.deref(input, parentRefs, basePath); resolver.shouldStopOnError(err) {
+			return err
+		}
+
+		ref, sch, _ = getRefAndSchema(input)
 	}
 
-	ref, sch, _ := getRefAndSchema(input)
 	if ref.String() != "" {
 		transitiveResolver := resolver.transitiveResolver(basePath, *ref)
 		basePath = resolver.updateBasePath(transitiveResolver, basePath)
@@ -551,6 +567,7 @@ func expandParameterOrResponse(input interface{}, resolver *schemaLoader, basePa
 		if ref != nil {
 			*ref = Ref{}
 		}
+
 		return nil
 	}
 
@@ -560,38 +577,29 @@ func expandParameterOrResponse(input interface{}, resolver *schemaLoader, basePa
 			return ern
 		}
 
-		switch {
-		case resolver.isCircular(&rebasedRef, basePath, parentRefs...):
+		if resolver.isCircular(&rebasedRef, basePath, parentRefs...) {
 			// this is a circular $ref: stop expansion
 			if !resolver.options.AbsoluteCircularRef {
 				sch.Ref = denormalizeRef(&rebasedRef, resolver.context.basePath, resolver.context.rootID)
 			} else {
 				sch.Ref = rebasedRef
 			}
-		case !resolver.options.SkipSchemas:
-			// schema expanded to a $ref in another root
-			sch.Ref = rebasedRef
-			debugLog("rebased to: %s", sch.Ref.String())
-		default:
-			// skip schema expansion but rebase $ref to schema
-			sch.Ref = denormalizeRef(&rebasedRef, resolver.context.basePath, resolver.context.rootID)
 		}
 	}
 
+	// $ref expansion or rebasing is performed by expandSchema below
 	if ref != nil {
 		*ref = Ref{}
 	}
 
 	// expand schema
-	if !resolver.options.SkipSchemas {
-		s, err := expandSchema(*sch, parentRefs, resolver, basePath)
-		if resolver.shouldStopOnError(err) {
-			return err
-		}
-		if s == nil {
-			// guard for when continuing on error
-			return nil
-		}
+	// yes, we do it even if options.SkipSchema is true: we have to go down that rabbit hole and rebase nested $ref)
+	s, err := expandSchema(*sch, parentRefs, resolver, basePath)
+	if resolver.shouldStopOnError(err) {
+		return err
+	}
+
+	if s != nil { // guard for when continuing on error
 		*sch = *s
 	}
 

fixtures/bugs/2743/not-working/minimal.yaml

@@ -0,0 +1,7 @@
+swagger: '2.0'
+info:
+  version: 0.0.0
+  title: Simple API
+paths:
+  /bar:
+    $ref: 'swagger/paths/bar.yml'

fixtures/bugs/2743/not-working/spec.yaml

@@ -0,0 +1,11 @@
+swagger: '2.0'
+info:
+  version: 0.0.0
+  title: Simple API
+paths:
+  /foo:
+    $ref: 'swagger/paths/foo.yml'
+  /bar:
+    $ref: 'swagger/paths/bar.yml'
+  /nested:
+    $ref: 'swagger/paths/nested.yml#/response'

fixtures/bugs/2743/not-working/swagger/definitions.yml

@@ -0,0 +1,9 @@
+ErrorPayload:
+  title: Error Payload
+  required:
+    - errors
+  properties:
+    errors:
+      type: array
+      items:
+        $ref: './items.yml#/ErrorDetailsItem'

fixtures/bugs/2743/not-working/swagger/items.yml

@@ -0,0 +1,15 @@
+ErrorDetailsItem:
+  title: Error details item
+  description: Represents an item of the list of details of an error.
+  required:
+    - message
+    - code
+  properties:
+    message:
+      type: string
+    code:
+      type: string
+    details:
+      type: array
+      items:
+        type: string

fixtures/bugs/2743/not-working/swagger/paths/bar.yml

@@ -0,0 +1,8 @@
+get:
+  responses:
+    200:
+      description: OK
+      schema:
+        type: array
+        items:
+          $ref: '../user/index.yml#/User' ## this doesn't work

fixtures/bugs/2743/not-working/swagger/paths/foo.yml

@@ -0,0 +1,8 @@
+get:
+  responses:
+    200:
+      description: OK
+    500:
+      description: OK
+      schema:
+        $ref: '../definitions.yml#/ErrorPayload'

fixtures/bugs/2743/not-working/swagger/paths/index.yml

@@ -0,0 +1,6 @@
+/foo:
+  $ref: ./foo.yml
+/bar:
+  $ref: ./bar.yml
+/nested:
+  $ref: ./nested.yml#/response

fixtures/bugs/2743/not-working/swagger/paths/nested.yml

@@ -0,0 +1,14 @@
+response:
+  get:
+    responses:
+      200:
+        description: OK
+        schema:
+          $ref: '#/definitions/SameFileReference'
+
+definitions:
+  SameFileReference:
+    type: object
+    properties:
+      name:
+        type: string

fixtures/bugs/2743/not-working/swagger/user/index.yml

@@ -0,0 +1,2 @@
+User:
+  $ref: './model.yml'

fixtures/bugs/2743/not-working/swagger/user/model.yml

@@ -0,0 +1,4 @@
+type: object
+properties:
+  name:
+    type: string

fixtures/bugs/2743/working/spec.yaml

@@ -0,0 +1,11 @@
+swagger: '2.0'
+info:
+  version: 0.0.0
+  title: Simple API
+paths:
+  /foo:
+    $ref: 'swagger/paths/foo.yml'
+  /bar:
+    $ref: 'swagger/paths/bar.yml'
+  /nested:
+    $ref: 'swagger/paths/nested.yml#/response'

fixtures/bugs/2743/working/swagger/definitions.yml

@@ -0,0 +1,9 @@
+ErrorPayload:
+  title: Error Payload
+  required:
+    - errors
+  properties:
+    errors:
+      type: array
+      items:
+        $ref: './items.yml#/ErrorDetailsItem'

fixtures/bugs/2743/working/swagger/items.yml

@@ -0,0 +1,15 @@
+ErrorDetailsItem:
+  title: Error details item
+  description: Represents an item of the list of details of an error.
+  required:
+    - message
+    - code
+  properties:
+    message:
+      type: string
+    code:
+      type: string
+    details:
+      type: array
+      items:
+        type: string

fixtures/bugs/2743/working/swagger/paths/bar.yml

@@ -0,0 +1,8 @@
+get:
+  responses:
+    200:
+      description: OK
+      schema:
+        type: array
+        items:
+          $ref: './swagger/user/index.yml#/User' ## this works

fixtures/bugs/2743/working/swagger/paths/foo.yml

@@ -0,0 +1,8 @@
+get:
+  responses:
+    200:
+      description: OK
+    500:
+      description: OK
+      schema:
+        $ref: '../definitions.yml#/ErrorPayload'

fixtures/bugs/2743/working/swagger/paths/index.yml

@@ -0,0 +1,6 @@
+/foo:
+  $ref: ./foo.yml
+/bar:
+  $ref: ./bar.yml
+/nested:
+  $ref: ./nested.yml#/response

fixtures/bugs/2743/working/swagger/paths/nested.yml

@@ -0,0 +1,14 @@
+response:
+  get:
+    responses:
+      200:
+        description: OK
+        schema:
+          $ref: '#/definitions/SameFileReference'
+
+definitions:
+  SameFileReference:
+    type: object
+    properties:
+      name:
+        type: string

fixtures/bugs/2743/working/swagger/user/index.yml

@@ -0,0 +1,2 @@
+User:
+  $ref: './model.yml'

fixtures/bugs/2743/working/swagger/user/model.yml

@@ -0,0 +1,4 @@
+type: object
+properties:
+  name:
+    type: string

spec_test.go

@@ -25,6 +25,38 @@ import (
 )
 
 // Test unitary fixture for dev and bug fixing
+
+func TestSpec_Issue2743(t *testing.T) {
+	t.Run("should expand but produce unresolvable $ref", func(t *testing.T) {
+		path := filepath.Join("fixtures", "bugs", "2743", "working", "spec.yaml")
+		sp := loadOrFail(t, path)
+		require.NoError(t,
+			spec.ExpandSpec(sp, &spec.ExpandOptions{RelativeBase: path, SkipSchemas: true, PathLoader: testLoader}),
+		)
+
+		t.Run("all $ref do not resolve when expanding again", func(t *testing.T) {
+			err := spec.ExpandSpec(sp, &spec.ExpandOptions{RelativeBase: path, SkipSchemas: false, PathLoader: testLoader})
+			require.Error(t, err)
+			require.ErrorContains(t, err, filepath.FromSlash("swagger/paths/swagger/user/index.yml"))
+		})
+	})
+
+	t.Run("should expand and produce resolvable $ref", func(t *testing.T) {
+		path := filepath.Join("fixtures", "bugs", "2743", "not-working", "spec.yaml")
+		sp := loadOrFail(t, path)
+		require.NoError(t,
+			spec.ExpandSpec(sp, &spec.ExpandOptions{RelativeBase: path, SkipSchemas: true, PathLoader: testLoader}),
+		)
+
+		t.Run("all $ref properly reolve when expanding again", func(t *testing.T) {
+			require.NoError(t,
+				spec.ExpandSpec(sp, &spec.ExpandOptions{RelativeBase: path, SkipSchemas: false, PathLoader: testLoader}),
+			)
+			require.NotContainsf(t, asJSON(t, sp), "$ref", "all $ref's should have been expanded properly")
+		})
+	})
+}
+
 func TestSpec_Issue1429(t *testing.T) {
 	path := filepath.Join("fixtures", "bugs", "1429", "swagger.yaml")