fix: operation_success sensitive to swagger/openapi types #214

Rule was not behaving correctly between different response key types. The rule now reports any incorrect integer use of response keys in OpenAPI 3+

Signed-off-by: Dave Shanley <dave@quobix.com>

functions/openapi/success_response.go

@@ -56,18 +56,34 @@ func (sr SuccessResponse) RunRule(nodes []*yaml.Node, context model.RuleFunction
 					}
 					verbDataNode := operationNode.Content[h+1]
 
-					fieldNode, valNode := utils.FindFirstKeyNode(context.RuleAction.Field, verbDataNode.Content, 0)
+					fieldNode, valNode := utils.FindKeyNodeTop(context.RuleAction.Field, verbDataNode.Content)
+
 					if fieldNode != nil && valNode != nil {
 						var responseSeen bool
+						var responseInvalidType bool
+						var responseCode int
+						var invalidCodes []int
 						for _, response := range valNode.Content {
-							if response.Tag == "!!str" {
-								responseCode, _ := strconv.Atoi(response.Value)
+							if utils.IsNodeStringValue(response) {
+								responseCode, _ = strconv.Atoi(response.Value)
 								if responseCode >= 200 && responseCode < 400 {
 									responseSeen = true
 								}
 							}
+
+							// check for an integer instead of a string, and check if this is an OpenAPI 3+ doc,
+							// if so, throw an error about using the wrong type
+							// https://github.com/daveshanley/vacuum/issues/214
+							if context.SpecInfo.SpecType == utils.OpenApi3 {
+								if utils.IsNodeIntValue(response) {
+									responseInvalidType = true
+									responseSeen = true
+									c, _ := strconv.Atoi(response.Value)
+									invalidCodes = append(invalidCodes, c)
+								}
+							}
 						}
-						if !responseSeen {
+						if !responseSeen || responseInvalidType {
 
 							// see if we can extract a name from the operationId
 							_, g := utils.FindKeyNode("operationId", verbDataNode.Content)
@@ -83,13 +99,29 @@ func (sr SuccessResponse) RunRule(nodes []*yaml.Node, context model.RuleFunction
 								endNode = operationNode.Content[j+1]
 							}
 
-							results = append(results, model.RuleFunctionResult{
-								Message:   fmt.Sprintf("Operation `%s` must define at least a single `2xx` or `3xx` response", name),
-								StartNode: fieldNode,
-								EndNode:   endNode,
-								Path:      fmt.Sprintf("$.paths.%s.%s.%s", currentPath, currentVerb, context.RuleAction.Field),
-								Rule:      context.Rule,
-							})
+							if !responseSeen {
+								results = append(results, model.RuleFunctionResult{
+									Message:   fmt.Sprintf("Operation `%s` must define at least a single `2xx` or `3xx` response", name),
+									StartNode: fieldNode,
+									EndNode:   endNode,
+									Path:      fmt.Sprintf("$.paths.%s.%s.%s", currentPath, currentVerb, context.RuleAction.Field),
+									Rule:      context.Rule,
+								})
+							}
+
+							if responseInvalidType {
+								for i := range invalidCodes {
+									results = append(results, model.RuleFunctionResult{
+										Message: fmt.Sprintf("Operation `%s` uses an `integer` instead of a `string` "+
+											"for response code `%d`", name, invalidCodes[i]),
+										StartNode: fieldNode,
+										EndNode:   endNode,
+										Path:      fmt.Sprintf("$.paths.%s.%s.%s", currentPath, currentVerb, context.RuleAction.Field),
+										Rule:      context.Rule,
+									})
+								}
+							}
+
 						}
 					}
 

functions/openapi/success_response_test.go

@@ -2,6 +2,7 @@ package openapi
 
 import (
 	"github.com/daveshanley/vacuum/model"
+	"github.com/pb33f/libopenapi/datamodel"
 	"github.com/pb33f/libopenapi/index"
 	"github.com/pb33f/libopenapi/utils"
 	"github.com/stretchr/testify/assert"
@@ -25,15 +26,17 @@ func TestSuccessResponse_RunRule_Success(t *testing.T) {
 
 	sampleYaml, _ := os.ReadFile("../../model/test_files/burgershop.openapi.yaml")
 
+	info, _ := datamodel.ExtractSpecInfo([]byte(sampleYaml))
+
 	nodes, _ := utils.FindNodes(sampleYaml, "$")
 
 	rule := buildOpenApiTestRuleAction(GetAllOperationsJSONPath(), "xor", "responses", nil)
 
 	ctx := buildOpenApiTestContext(model.CastToRuleAction(rule.Then), nil)
 
 	def := SuccessResponse{}
+	ctx.SpecInfo = info
 	res := def.RunRule(nodes, ctx)
-
 	assert.Len(t, res, 0)
 }
 
@@ -50,22 +53,39 @@ paths:
 
 	path := "$"
 
-	var rootNode yaml.Node
-	mErr := yaml.Unmarshal([]byte(yml), &rootNode)
-	assert.NoError(t, mErr)
-
+	info, _ := datamodel.ExtractSpecInfo([]byte(yml))
 	rule := buildOpenApiTestRuleAction(path, "success_response", "responses", nil)
 	ctx := buildOpenApiTestContext(model.CastToRuleAction(rule.Then), nil)
-	ctx.Index = index.NewSpecIndex(&rootNode)
-
+	ctx.Index = index.NewSpecIndex(info.RootNode)
+	ctx.SpecInfo = info
 	def := SuccessResponse{}
-	res := def.RunRule(rootNode.Content, ctx)
+	res := def.RunRule(info.RootNode.Content, ctx)
 
 	assert.Len(t, res, 1)
 	assert.Equal(t, "Operation `fresh` must define at least a single `2xx` or `3xx` response", res[0].Message)
 
 }
 
+func TestSuccessResponse_NoPaths(t *testing.T) {
+
+	yml := `swagger: 2.0
+definitions:
+  something:
+    description: hello`
+
+	path := "$"
+
+	info, _ := datamodel.ExtractSpecInfo([]byte(yml))
+	rule := buildOpenApiTestRuleAction(path, "success_response", "responses", nil)
+	ctx := buildOpenApiTestContext(model.CastToRuleAction(rule.Then), nil)
+	ctx.Index = index.NewSpecIndex(info.RootNode)
+	ctx.SpecInfo = info
+	def := SuccessResponse{}
+	res := def.RunRule(info.RootNode.Content, ctx)
+
+	assert.Len(t, res, 0)
+}
+
 func TestSuccessResponse_TriggerFailure_NoId(t *testing.T) {
 
 	yml := `swagger: 2.0
@@ -78,20 +98,50 @@ paths:
 
 	path := "$"
 
+	info, _ := datamodel.ExtractSpecInfo([]byte(yml))
+	rule := buildOpenApiTestRuleAction(path, "success_response", "responses", nil)
+	ctx := buildOpenApiTestContext(model.CastToRuleAction(rule.Then), nil)
+	ctx.Index = index.NewSpecIndex(info.RootNode)
+	ctx.SpecInfo = info
+
+	def := SuccessResponse{}
+	res := def.RunRule(info.RootNode.Content, ctx)
+
+	assert.Len(t, res, 1)
+	assert.Equal(t, "Operation `undefined operation (no operationId)` must define at least a"+
+		" single `2xx` or `3xx` response", res[0].Message)
+
+}
+
+func TestSuccessResponse_TriggerFailure_IntVsString(t *testing.T) {
+
+	yml := `openapi: 3.1
+paths:
+  /melody:
+    post:
+      responses:
+        500:
+          description: hello`
+
+	path := "$"
+
 	var rootNode yaml.Node
 	mErr := yaml.Unmarshal([]byte(yml), &rootNode)
 	assert.NoError(t, mErr)
 
+	info, _ := datamodel.ExtractSpecInfo([]byte(yml))
+
 	rule := buildOpenApiTestRuleAction(path, "success_response", "responses", nil)
 	ctx := buildOpenApiTestContext(model.CastToRuleAction(rule.Then), nil)
-	ctx.Index = index.NewSpecIndex(&rootNode)
+	ctx.Index = index.NewSpecIndex(info.RootNode)
+	ctx.SpecInfo = info
 
 	def := SuccessResponse{}
 	res := def.RunRule(rootNode.Content, ctx)
 
 	assert.Len(t, res, 1)
-	assert.Equal(t, "Operation `undefined operation (no operationId)` must define at least a"+
-		" single `2xx` or `3xx` response", res[0].Message)
+	assert.Equal(t, "Operation `undefined operation (no operationId)` uses an `integer` instead of a "+
+		"`string` for response code `500`", res[0].Message)
 
 }