Skip to content

Commit

Permalink
(fix): path_parameters function missing some checks #207
Browse files Browse the repository at this point in the history 
A missing check for available path params has been added.
  • Loading branch information
@daveshanley
daveshanley committed Dec 16, 2022
1 parent 35c63de commit 2413115
Show file tree
Hide file tree
Showing 3 changed files with 160 additions and 26 deletions.
1 change: 0 additions & 1 deletion functions/openapi/examples.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -559,7 +559,6 @@ func analyzeExample(nameNodeValue string, mediaTypeNode *yaml.Node, basePath str
modifyExampleResults(results, &z)
return results
}

}

//return results
Expand Down
80 changes: 57 additions & 23 deletions functions/openapi/path_parameters.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@ package openapi
import (
"fmt"
"github.com/daveshanley/vacuum/model"
v3 "github.com/pb33f/libopenapi/datamodel/low/v3"
"github.com/pb33f/libopenapi/utils"
"gopkg.in/yaml.v3"
"regexp"
"strings"
)

// PathParameters is a rule that checks path level and operation level parameters for correct paths. The rule is
Expand Down Expand Up @@ -41,7 +43,7 @@ func (pp PathParameters) RunRule(nodes []*yaml.Node, context model.RuleFunctionC

var results []model.RuleFunctionResult

opNodes := context.Index.GetPathsNode()
pathNodes := context.Index.GetPathsNode()
paramRegex := `(\{;?\??[a-zA-Z0-9_-]+\*?\})`
rx, _ := regexp.Compile(paramRegex)

Expand All @@ -53,25 +55,25 @@ func (pp PathParameters) RunRule(nodes []*yaml.Node, context model.RuleFunctionC
pathElements := make(map[string]bool)
topLevelParams := make(map[string]map[string][]string)

if opNodes == nil {
if pathNodes == nil {
return results
}
for j, operationNode := range opNodes.Content {
for j, pathNode := range pathNodes.Content {

if utils.IsNodeStringValue(operationNode) {
if utils.IsNodeStringValue(pathNode) {
// replace any params with an invalid char (%) so we can perform a path
// equality check. /hello/{fresh} and /hello/{fish} are equivalent to OpenAPI.
currentPath = operationNode.Value
currentPath = pathNode.Value
currentPathNormalized := rx.ReplaceAllString(currentPath, "%")

// check if it's been seen
if seenPaths[currentPathNormalized] != "" {
res := model.BuildFunctionResultString(
fmt.Sprintf("Paths `%s` and `%s` must not be equivalent, paths must be unique",
seenPaths[currentPathNormalized], currentPath))
res.StartNode = operationNode
res.EndNode = operationNode
res.Path = fmt.Sprintf("$.paths.%s", currentPath)
res.StartNode = pathNode
res.EndNode = pathNode
res.Path = fmt.Sprintf("$.paths.['%s']", currentPath)
res.Rule = context.Rule
results = append(results, res)
} else {
Expand All @@ -87,9 +89,9 @@ func (pp PathParameters) RunRule(nodes []*yaml.Node, context model.RuleFunctionC
res := model.BuildFunctionResultString(
fmt.Sprintf("Path `%s` must not use the parameter `%s` multiple times",
currentPath, param))
res.StartNode = operationNode
res.EndNode = operationNode
res.Path = fmt.Sprintf("$.paths.%s", currentPath)
res.StartNode = pathNode
res.EndNode = pathNode
res.Path = fmt.Sprintf("$.paths.['%s']", currentPath)
res.Rule = context.Rule
results = append(results, res)
} else {
Expand All @@ -99,7 +101,7 @@ func (pp PathParameters) RunRule(nodes []*yaml.Node, context model.RuleFunctionC
}
}

if utils.IsNodeMap(operationNode) {
if utils.IsNodeMap(pathNode) {

topLevelParametersNode := context.Index.GetOperationParameterReferences()[currentPath]["top"]
//_, topLevelParametersNode := utils.FindKeyNodeTop("parameters", operationNode.Content)
Expand Down Expand Up @@ -134,7 +136,7 @@ func (pp PathParameters) RunRule(nodes []*yaml.Node, context model.RuleFunctionC
c := 0
verbLevelParams := make(map[string]map[string][]string)

for _, verbMapNode := range operationNode.Content {
for _, verbMapNode := range pathNode.Content {
if utils.IsNodeStringValue(verbMapNode) && utils.IsHttpVerb(verbMapNode.Value) {
currentVerb = verbMapNode.Value
} else {
Expand Down Expand Up @@ -186,15 +188,39 @@ func (pp PathParameters) RunRule(nodes []*yaml.Node, context model.RuleFunctionC
}
}

startNode := operationNode
startNode := pathNode
endNode := utils.FindLastChildNode(startNode)
if j+1 < len(opNodes.Content) {
endNode = opNodes.Content[j+1]
if j+1 < len(pathNodes.Content) {
endNode = pathNodes.Content[j+1]
}

r := ""
for i, verbMapNode := range pathNode.Content {
if i%2 == 0 {
r = verbMapNode.Value
continue
}
if isVerb(r) {
if len(topLevelParams) <= 0 {
if verbLevelParams[r] == nil {
for pe := range pathElements {
err := fmt.Sprintf("`%s` must define parameter `%s` as expected by path `%s`",
strings.ToUpper(r), pe, currentPath)
res := model.BuildFunctionResultString(err)
res.StartNode = startNode
res.EndNode = endNode
res.Path = fmt.Sprintf("$.paths.['%s'].%s", currentPath, r)
res.Rule = context.Rule
results = append(results, res)
}
}
}
pp.ensureAllExpectedParamsInPathAreDefined(currentPath, allPathParams,
pathElements, &results, startNode, endNode, context, r)
}
}
pp.ensureAllDefinedPathParamsAreUsedInPath(currentPath, allPathParams,
pathElements, &results, startNode, endNode, context)
pp.ensureAllExpectedParamsInPathAreDefined(currentPath, allPathParams,
pathElements, &results, startNode, endNode, context)

// reset for the next run.
pathElements = make(map[string]bool)
Expand All @@ -220,6 +246,14 @@ func (pp PathParameters) RunRule(nodes []*yaml.Node, context model.RuleFunctionC

}

func isVerb(verb string) bool {
switch strings.ToLower(verb) {
case v3.GetLabel, v3.PostLabel, v3.PutLabel, v3.PatchLabel, v3.DeleteLabel, v3.HeadLabel, v3.OptionsLabel, v3.TraceLabel:
return true
}
return false
}

func (pp PathParameters) ensureAllDefinedPathParamsAreUsedInPath(path string, allPathParams map[string]map[string][]string,
pathElements map[string]bool, results *[]model.RuleFunctionResult, startNode, endNode *yaml.Node,
context model.RuleFunctionContext) {
Expand All @@ -238,7 +272,7 @@ func (pp PathParameters) ensureAllDefinedPathParamsAreUsedInPath(path string, al
res := model.BuildFunctionResultString(err)
res.StartNode = startNode
res.EndNode = endNode
res.Path = fmt.Sprintf("$.paths.%s", path)
res.Path = fmt.Sprintf("$.paths.['%s']", path)
res.Rule = context.Rule
*results = append(*results, res)
}
Expand All @@ -248,7 +282,7 @@ func (pp PathParameters) ensureAllDefinedPathParamsAreUsedInPath(path string, al

func (pp PathParameters) ensureAllExpectedParamsInPathAreDefined(path string, allPathParams map[string]map[string][]string,
pathElements map[string]bool, results *[]model.RuleFunctionResult, startNode, endNode *yaml.Node,
context model.RuleFunctionContext) {
context model.RuleFunctionContext, verb string) {
var top map[string][]string

if allPathParams != nil {
Expand All @@ -260,11 +294,11 @@ func (pp PathParameters) ensureAllExpectedParamsInPathAreDefined(path string, al
}
for p := range pathElements {
if !pp.segmentExistsInPathParams(p, e, top) {
err := fmt.Sprintf("Operation must define parameter `%s` as expected by path `%s`", p, path)
err := fmt.Sprintf("`%s` must define parameter `%s` as expected by path `%s`", strings.ToUpper(verb), p, path)
res := model.BuildFunctionResultString(err)
res.StartNode = startNode
res.EndNode = endNode
res.Path = fmt.Sprintf("$.paths.%s", path)
res.Path = fmt.Sprintf("$.paths.['%s']", path)
res.Rule = context.Rule
*results = append(*results, res)
}
Expand Down Expand Up @@ -317,7 +351,7 @@ func (pp PathParameters) isPathParamNamedAndRequired(in, required, name *yaml.No
res := model.BuildFunctionResultString(errMsg)
res.StartNode = required
res.EndNode = required
res.Path = fmt.Sprintf("$.paths.%s.%s.parameters", currentPath, currentVerb)
res.Path = fmt.Sprintf("$.paths.['%s'].%s.parameters", currentPath, currentVerb)
res.Rule = context.Rule

if utils.IsNodeBoolValue(required) {
Expand Down
105 changes: 103 additions & 2 deletions functions/openapi/path_parameters_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ func TestPathParameters_RunRule_TopParameterCheck_MissingParamDefInOp(t *testing
res := def.RunRule(nodes, ctx)

assert.Len(t, res, 1)
assert.Equal(t, "Operation must define parameter `cake` as expected by path `/musical/{melody}/{pizza}/{cake}`",
assert.Equal(t, "`GET` must define parameter `cake` as expected by path `/musical/{melody}/{pizza}/{cake}`",
res[0].Message)
}

Expand Down Expand Up @@ -313,6 +313,107 @@ paths:
res := def.RunRule([]*yaml.Node{&rootNode}, ctx)

assert.Len(t, res, 1)
assert.Equal(t, "Operation must define parameter `cake` as expected by path `/musical/{melody}/{pizza}/{cake}`",
assert.Equal(t, "`GET` must define parameter `cake` as expected by path `/musical/{melody}/{pizza}/{cake}`",
res[0].Message)
}

func TestPathParameters_RunRule_NoParamsDefined(t *testing.T) {

yml := `
paths:
/update/{somethings}:
post:
operationId: postSomething
summary: Post something
tags:
- tag1
responses:
'200':
description: Post OK
get:
operationId: getSomething
summary: Get something
tags:
- tag1
responses:
'200':
description: Get OK
components:
securitySchemes:
basicAuth:
type: http
scheme: basic`

path := "$"

var rootNode yaml.Node
mErr := yaml.Unmarshal([]byte(yml), &rootNode)
assert.NoError(t, mErr)

nodes, _ := utils.FindNodes([]byte(yml), path)

rule := buildOpenApiTestRuleAction(path, "path_parameters", "", nil)
ctx := buildOpenApiTestContext(model.CastToRuleAction(rule.Then), nil)
ctx.Index = index.NewSpecIndex(&rootNode)

def := PathParameters{}
res := def.RunRule(nodes, ctx)

assert.Len(t, res, 2)
assert.Equal(t, "`POST` must define parameter `somethings` as expected by path `/update/{somethings}`",
res[0].Message)
assert.Equal(t, "`GET` must define parameter `somethings` as expected by path `/update/{somethings}`",
res[1].Message)
}

func TestPathParameters_RunRule_NoParamsDefined_TopExists(t *testing.T) {

yml := `paths:
/update/{somethings}:
parameters:
- in: path
name: somethings
schema:
type: string
example: something nice
description: this is something nice.
required: true
post:
operationId: postSomething
summary: Post something
tags:
- tag1
responses:
'200':
description: Post OK
get:
operationId: getSomething
summary: Get something
tags:
- tag1
responses:
'200':
description: Get OK
components:
securitySchemes:
basicAuth:
type: http
scheme: basic`

path := "$"

var rootNode yaml.Node
mErr := yaml.Unmarshal([]byte(yml), &rootNode)
assert.NoError(t, mErr)

nodes, _ := utils.FindNodes([]byte(yml), path)

rule := buildOpenApiTestRuleAction(path, "path_parameters", "", nil)
ctx := buildOpenApiTestContext(model.CastToRuleAction(rule.Then), nil)
ctx.Index = index.NewSpecIndex(&rootNode)

def := PathParameters{}
res := def.RunRule(nodes, ctx)

assert.Len(t, res, 0)
}

0 comments on commit 2413115

Please sign in to comment.