From 4c42906ba5b3b4f01b56cd06a1614504fec61a9d Mon Sep 17 00:00:00 2001 From: Dave Shanley Date: Wed, 12 Oct 2022 08:52:44 -0400 Subject: [PATCH 1/2] Applied fix for #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 #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. --- functions/openapi/unused_component.go | 27 ++++++++-- functions/openapi/unused_component_test.go | 57 ++++++++++++++++++++++ motor/rule_applicator.go | 6 +++ motor/rule_applicator_test.go | 57 ++++++++++++++-------- motor/rule_tests/openapi_rule_test.go | 5 +- rulesets/ruleset_functions.go | 2 +- 6 files changed, 129 insertions(+), 25 deletions(-) diff --git a/functions/openapi/unused_component.go b/functions/openapi/unused_component.go index 1a0ebc5c..1df6ff03 100644 --- a/functions/openapi/unused_component.go +++ b/functions/openapi/unused_component.go @@ -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. @@ -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 + } } } } diff --git a/functions/openapi/unused_component_test.go b/functions/openapi/unused_component_test.go index 9b0eb080..f8e326fa 100644 --- a/functions/openapi/unused_component_test.go +++ b/functions/openapi/unused_component_test.go @@ -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) +} diff --git a/motor/rule_applicator.go b/motor/rule_applicator.go index 3fa6ef88..9ed4954a 100644 --- a/motor/rule_applicator.go +++ b/motor/rule_applicator.go @@ -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" ) @@ -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 { diff --git a/motor/rule_applicator_test.go b/motor/rule_applicator_test.go index 857849e0..f9f1213f 100644 --- a/motor/rule_applicator_test.go +++ b/motor/rule_applicator_test.go @@ -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` @@ -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.` @@ -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 @@ -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: @@ -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: @@ -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) @@ -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, ` rules := make(map[string]*model.Rule) @@ -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` @@ -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: @@ -810,7 +819,8 @@ components: func TestRuleSet_TagsNotArray(t *testing.T) { - yml := `info: + yml := `openapi: 3.1.0 +info: contact: name: Duck tags: none @@ -839,7 +849,8 @@ components: func TestRuleSet_TagsWrongType(t *testing.T) { - yml := `info: + yml := `openapi: 3.1.0 +info: contact: name: Duck tags: @@ -869,7 +880,8 @@ components: func TestRuleSet_OperationIdInvalidInUrl(t *testing.T) { - yml := `paths: + yml := `openapi: 3.1.0 +paths: /hi: get: operationId: nice rice @@ -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: @@ -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: @@ -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 @@ -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 @@ -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 @@ -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 @@ -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/ ` diff --git a/motor/rule_tests/openapi_rule_test.go b/motor/rule_tests/openapi_rule_test.go index 1bcfc3ca..d1201eba 100644 --- a/motor/rule_tests/openapi_rule_test.go +++ b/motor/rule_tests/openapi_rule_test.go @@ -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: @@ -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) diff --git a/rulesets/ruleset_functions.go b/rulesets/ruleset_functions.go index 31ff88a3..4e571f17 100644 --- a/rulesets/ruleset_functions.go +++ b/rulesets/ruleset_functions.go @@ -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, From 9c1da96213080bddbe33e6f7086e7962d8f3d64f Mon Sep 17 00:00:00 2001 From: Dave Shanley Date: Wed, 12 Oct 2022 09:02:10 -0400 Subject: [PATCH 2/2] Tuning CI/CD config Disabling auto-tag for main push, not everything needs a release. Also bumping version on go. --- .github/workflows/build.yaml | 2 +- .github/workflows/label-check.yaml | 2 +- .github/workflows/tag.yaml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index 22f575f0..1f1b80c1 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -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 diff --git a/.github/workflows/label-check.yaml b/.github/workflows/label-check.yaml index 09db1ff7..718edd25 100644 --- a/.github/workflows/label-check.yaml +++ b/.github/workflows/label-check.yaml @@ -11,6 +11,6 @@ jobs: - uses: jesusvasquez333/verify-pr-label-action@v1.4.0 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 \ No newline at end of file diff --git a/.github/workflows/tag.yaml b/.github/workflows/tag.yaml index 0d4041fa..86702c9e 100644 --- a/.github/workflows/tag.yaml +++ b/.github/workflows/tag.yaml @@ -1,7 +1,7 @@ name: Tag on: - push: + workflow_dispatch: branches: - main