Skip to content

Commit

Permalink
Applied fix for daveshanley#142 and some other tweaks (daveshanley#143)
Browse files Browse the repository at this point in the history
* Applied fix for daveshanley#142 and some other tweaks

There was a bug in the `unused_definitions` function that was not looking in all polymorphic collections for use. Added the missing two checks to address daveshanley#142

Also, there was a bug I found when testing rules, I noticed that when running without a version, rules are called multipled times. This behavior is incorrect, there is no way to be reliable or accurate if multiple
versions of rule for different versions are operating. Added a check for a missing version and format. Without this data, the rule is skipped.

Updated some tests to include version details in sample spec snippet code.

* Tuning CI/CD config

Disabling auto-tag for main push, not everything needs a release. Also bumping version on go.
  • Loading branch information
daveshanley authored Oct 12, 2022
1 parent 283b739 commit 09dd95b
Show file tree
Hide file tree
Showing 9 changed files with 132 additions and 28 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/build.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ jobs:
- name: Set up Go 1.x
uses: actions/setup-go@v2
with:
go-version: ^1.16
go-version: ^1.18
id: go

- name: Checkout code
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/label-check.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,6 @@ jobs:
- uses: jesusvasquez333/[email protected]
with:
github-token: "${{ secrets.GITHUB_TOKEN }}"
valid-labels: "release/patch, release/minor, release/major"
valid-labels: "release/patch, release/minor, release/major, bugfix, feature"
pull-request-number: '${{ github.event.pull_request.number }}'
disable-reviews: true
2 changes: 1 addition & 1 deletion .github/workflows/tag.yaml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
name: Tag

on:
push:
workflow_dispatch:
branches:
- main

Expand Down
27 changes: 24 additions & 3 deletions functions/openapi/unused_component.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,20 @@ func (uc UnusedComponent) RunRule(nodes []*yaml.Node, context model.RuleFunction
links := context.Index.GetAllLinks()
callbacks := context.Index.GetAllCallbacks()

// create poly maps.
oneOfRefs := make(map[string]*index.Reference)
allOfRefs := make(map[string]*index.Reference)
anyOfRefs := make(map[string]*index.Reference)

// include all polymorphic references.
for _, ref := range context.Index.GetPolyAllOfReferences() {
allRefs[ref.Definition] = ref
allOfRefs[ref.Definition] = ref
}
for _, ref := range context.Index.GetPolyOneOfReferences() {
oneOfRefs[ref.Definition] = ref
}
for _, ref := range context.Index.GetPolyAnyOfReferences() {
anyOfRefs[ref.Definition] = ref
}

// if a component does not exist in allRefs, it was not referenced anywhere.
Expand All @@ -66,9 +78,18 @@ func (uc UnusedComponent) RunRule(nodes []*yaml.Node, context model.RuleFunction
// find everything that was never referenced.
for _, resultMap := range mapsToSearch {
for key, ref := range resultMap {

// check everything!
if allRefs[key] == nil {
// nothing is using this!
notUsed[key] = ref
found := false
// check poly refs if the reference can't be found
if oneOfRefs[key] != nil || allOfRefs[key] != nil || anyOfRefs[key] != nil {
found = true
}
if !found {
// nothing is using this!
notUsed[key] = ref
}
}
}
}
Expand Down
57 changes: 57 additions & 0 deletions functions/openapi/unused_component_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -163,3 +163,60 @@ components:

assert.Len(t, res, 4)
}

func TestUnusedComponent_RunRule_Success_PolymorphicCheck(t *testing.T) {

yml := `paths:
/naughty/{puppy}:
get:
responses:
"200":
description: The naughty pup
content:
application/json:
schema:
oneOf:
- $ref: '#/components/schemas/Puppy'
"404":
description: The naughty kitty
content:
application/json:
schema:
anyOf:
- $ref: '#/components/schemas/Kitty'
"500":
description: The naughty bunny
content:
application/json:
schema:
allOf:
- $ref: '#/components/schemas/Bunny'
components:
schemas:
Puppy:
type: string
description: pup
Kitty:
type: string
description: kitty
Bunny:
type: string
description: bunny`

path := "$"

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

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

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

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

assert.Len(t, res, 0)
}
6 changes: 6 additions & 0 deletions motor/rule_applicator.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/pb33f/libopenapi/index"
"github.com/pb33f/libopenapi/resolver"
"github.com/pb33f/libopenapi/utils"
"github.com/pterm/pterm"
"gopkg.in/yaml.v3"
"sync"
)
Expand Down Expand Up @@ -340,6 +341,11 @@ func buildResults(ctx ruleContext, ruleAction model.RuleAction, nodes []*yaml.No
SpecInfo: ctx.specInfo,
}

if ctx.specInfo.SpecFormat == "" && ctx.specInfo.Version == "" {
pterm.Warning.Printf("Specification version not detected, cannot apply rule `%s`\n", ctx.rule.Id)
return ctx.ruleResults
}

// validate the rule is configured correctly before running it.
res, errs := model.ValidateRuleFunctionContextAgainstSchema(ruleFunction, rfc)
if !res {
Expand Down
57 changes: 38 additions & 19 deletions motor/rule_applicator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -618,7 +618,8 @@ func TestApplyRulesToRuleSet_CircularReferences(t *testing.T) {

func TestRuleSet_ContactProperties(t *testing.T) {

yml := `info:
yml := `openapi: 3.1.0
info:
contact:
name: pizza
email: monkey`
Expand All @@ -638,7 +639,8 @@ func TestRuleSet_ContactProperties(t *testing.T) {

func TestRuleSet_InfoContact(t *testing.T) {

yml := `info:
yml := `openapi: 3.1.0
info:
title: Terrible API Spec
description: No operations, no contact, useless.`

Expand All @@ -657,7 +659,8 @@ func TestRuleSet_InfoContact(t *testing.T) {

func TestRuleSet_InfoDescription(t *testing.T) {

yml := `info:
yml := `openapi: 3.1.0
info:
title: Terrible API Spec
contact:
name: rubbish
Expand All @@ -678,7 +681,8 @@ func TestRuleSet_InfoDescription(t *testing.T) {

func TestRuleSet_InfoLicense(t *testing.T) {

yml := `info:
yml := `openapi: 3.1.0
info:
title: Terrible API Spec
description: really crap
contact:
Expand All @@ -700,7 +704,8 @@ func TestRuleSet_InfoLicense(t *testing.T) {

func TestRuleSet_InfoLicenseUrl(t *testing.T) {

yml := `info:
yml := `openapi: 3.1.0
info:
title: Terrible API Spec
description: really crap
contact:
Expand All @@ -724,7 +729,8 @@ func TestRuleSet_InfoLicenseUrl(t *testing.T) {

func TestRuleSet_NoEvalInMarkdown(t *testing.T) {

yml := `info:
yml := `openapi: 3.1.0
info:
description: this has no eval('alert(1234') impact in vacuum, but JS tools might suffer.`

rules := make(map[string]*model.Rule)
Expand All @@ -742,7 +748,8 @@ func TestRuleSet_NoEvalInMarkdown(t *testing.T) {

func TestRuleSet_NoScriptInMarkdown(t *testing.T) {

yml := `info:
yml := `openapi: 3.1.0
info:
description: this has no impact in vacuum, <script>alert('XSS for you')</script>`

rules := make(map[string]*model.Rule)
Expand All @@ -761,7 +768,8 @@ func TestRuleSet_NoScriptInMarkdown(t *testing.T) {

func TestRuleSet_TagsAlphabetical(t *testing.T) {

yml := `tags:
yml := `openapi: 3.1.0
tags:
- name: zebra
- name: chicken
- name: puppy`
Expand All @@ -782,7 +790,8 @@ func TestRuleSet_TagsAlphabetical(t *testing.T) {

func TestRuleSet_TagsMissing(t *testing.T) {

yml := `info:
yml := `openapi: 3.1.0
info:
contact:
name: Duck
paths:
Expand Down Expand Up @@ -810,7 +819,8 @@ components:

func TestRuleSet_TagsNotArray(t *testing.T) {

yml := `info:
yml := `openapi: 3.1.0
info:
contact:
name: Duck
tags: none
Expand Down Expand Up @@ -839,7 +849,8 @@ components:

func TestRuleSet_TagsWrongType(t *testing.T) {

yml := `info:
yml := `openapi: 3.1.0
info:
contact:
name: Duck
tags:
Expand Down Expand Up @@ -869,7 +880,8 @@ components:

func TestRuleSet_OperationIdInvalidInUrl(t *testing.T) {

yml := `paths:
yml := `openapi: 3.1.0
paths:
/hi:
get:
operationId: nice rice
Expand All @@ -893,7 +905,8 @@ func TestRuleSet_OperationIdInvalidInUrl(t *testing.T) {

func TestRuleSetGetOperationTagsRule(t *testing.T) {

yml := `paths:
yml := `openapi: 3.1.0
paths:
/hi:
get:
tags:
Expand All @@ -918,7 +931,8 @@ func TestRuleSetGetOperationTagsRule(t *testing.T) {

func TestRuleSetGetOperationTagsMultipleRule(t *testing.T) {

yml := `paths:
yml := `openapi: 3.1.0
paths:
/hi:
get:
tags:
Expand Down Expand Up @@ -949,7 +963,8 @@ func TestRuleSetGetOperationTagsMultipleRule(t *testing.T) {

func TestRuleSetGetPathDeclarationsMustExist(t *testing.T) {

yml := `paths:
yml := `openapi: 3.1.0
paths:
/hi/{there}:
get:
operationId: a
Expand All @@ -974,7 +989,8 @@ func TestRuleSetGetPathDeclarationsMustExist(t *testing.T) {

func TestRuleSetNoPathTrailingSlashTest(t *testing.T) {

yml := `paths:
yml := `openapi: 3.1.0
paths:
/hi/{there}/:
get:
operationId: a
Expand All @@ -1000,7 +1016,8 @@ func TestRuleSetNoPathTrailingSlashTest(t *testing.T) {

func TestRuleSetNoPathQueryString(t *testing.T) {

yml := `paths:
yml := `openapi: 3.1.0
paths:
/hi/{there}?oh=yeah:
get:
operationId: a
Expand All @@ -1026,7 +1043,8 @@ func TestRuleSetNoPathQueryString(t *testing.T) {

func TestRuleTagDescriptionRequiredRule(t *testing.T) {

yml := `tags:
yml := `openapi: 3.1.0
tags:
- name: pizza
description: nice
- name: cinnamon
Expand Down Expand Up @@ -1245,7 +1263,8 @@ func TestRuleOAS3HostTrailingSlashRule(t *testing.T) {

func TestRuleOAS3HostTrailingSlashRule_Fail(t *testing.T) {

yml := `servers:
yml := `openapi: 3.1.0
servers:
- url: https://quobix.com/
- url: https://pb33f.io/
`
Expand Down
5 changes: 3 additions & 2 deletions motor/rule_tests/openapi_rule_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ func Benchmark_DefaultOpenAPI(b *testing.B) {

func Test_Default_OpenAPIRuleSet_FireABunchOfIssues(t *testing.T) {

badDoc := `paths:
badDoc := `openapi: 3.0.3
paths:
/curry/{hurry}/{salsa}:
get:
tags:
Expand All @@ -71,7 +72,7 @@ func Test_Default_OpenAPIRuleSet_FireABunchOfIssues(t *testing.T) {
rules := rs.GenerateOpenAPIDefaultRuleSet()
lintExecution := motor.ApplyRulesToRuleSet(&motor.RuleSetExecution{RuleSet: rules, Spec: []byte(badDoc)})
assert.Len(t, lintExecution.Errors, 0)
assert.Len(t, lintExecution.Results, 29)
assert.Len(t, lintExecution.Results, 27)

for n := 0; n < len(lintExecution.Results); n++ {
assert.NotNil(t, lintExecution.Results[n].Path)
Expand Down
2 changes: 1 addition & 1 deletion rulesets/ruleset_functions.go
Original file line number Diff line number Diff line change
Expand Up @@ -1010,7 +1010,7 @@ func GetOAS2UnusedComponentRule() *model.Rule {
Name: "Check for unused definitions",
Id: oas2UnusedDefinition,
Formats: model.OAS2Format,
Description: "Check for unused components and bad references",
Description: "Check for unused definitions and bad references",
Given: "$",
Resolved: false,
Recommended: true,
Expand Down

0 comments on commit 09dd95b

Please sign in to comment.