Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support to ignore api component errors #394

Merged
merged 6 commits into from
Nov 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions BREAKING-CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,21 +40,24 @@ Example:
oasdiff allows you to [deprecate APIs gracefully](API-DEPRECATION.md) without triggering a breaking-change error.

### Ignoring Specific Breaking Changes
Sometimes, you may want to ignore certain breaking changes.
The new Breaking Changes method allows you define breaking changes that you want to ignore in a configuration file.
Sometimes, you want to allow certain breaking changes, for example, when your spec and service are out-of-sync and you need to correct the spec.
Oasdiff allows you define breaking changes that you want to ignore in a configuration file.
You can specify the configuration file name in the oasdiff command-line with the `--warn-ignore` flag for WARNINGS or the `--err-ignore` flag for ERRORS.
Each line in the configuration file should contain two parts:
1. method and path (the first field in the line beginning with slash)
1. method and path (the first field in the line beginning with slash) to ignore a change to an endpoint, or the keyword 'components' to ignore a change in components
2. description of the breaking change

For example:
```
GET /api/{domain}/{project}/badges/security-score removed the success response with the status '200'
components removed the schema 'rules'
```

The line may contain additional info, like this:
```
- 12.01.2023 In the GET /api/{domain}/{project}/badges/security-score, we removed the success response with the status '200'
- 31.10.2023 In components, removed the schema 'network-policies'

```

The configuration files can be of any text type, e.g., Markdown, so you can use them to document breaking changes and other important changes.
Expand Down
10 changes: 4 additions & 6 deletions checker/api_change.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,13 @@ type ApiChange struct {
SourceColumnEnd int `json:"-" yaml:"-"`
}

func (c ApiChange) getUncolorizedText() string {
uncolorizedText := strings.ReplaceAll(c.Text, color.Bold, "")
return strings.ReplaceAll(uncolorizedText, color.Reset, "")
}

func (c ApiChange) MatchIgnore(ignorePath, ignoreLine string) bool {
if ignorePath == "" {
return false
}
return ignorePath == strings.ToLower(c.Path) &&
strings.Contains(ignoreLine, strings.ToLower(c.Operation+" "+c.Path)) &&
strings.Contains(ignoreLine, strings.ToLower(c.getUncolorizedText()))
strings.Contains(ignoreLine, strings.ToLower(GetUncolorizedText(c)))
}

func (c ApiChange) GetId() string {
Expand Down
12 changes: 12 additions & 0 deletions checker/change_utils.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package checker

import (
"strings"

"github.com/TwiN/go-color"
)

func GetUncolorizedText(c Change) string {
uncolorizedText := strings.ReplaceAll(c.GetText(), color.Bold, "")
return strings.ReplaceAll(uncolorizedText, color.Reset, "")
}
2 changes: 1 addition & 1 deletion checker/check-components-schemas-removed.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ func APIComponentsSchemaRemovedCheck(diffReport *diff.Diff, operationsSources *d
Id: APISchemasRemovedId,
Level: config.getLogLevel(APISchemasRemovedId, INFO),
Text: config.Localize(APISchemasRemovedId, ColorizedValue(deletedSchema)),
Source: "", // TODO: get the file name
Source: "Components",
})
}
return result
Expand Down
6 changes: 4 additions & 2 deletions checker/component_change.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package checker

import (
"fmt"
"strings"

"github.com/TwiN/go-color"
)
Expand All @@ -21,8 +22,9 @@ type ComponentChange struct {
SourceColumnEnd int `json:"-" yaml:"-"`
}

func (ComponentChange) MatchIgnore(ignorePath, ignoreLine string) bool {
return false
func (c ComponentChange) MatchIgnore(ignorePath, ignoreLine string) bool {
return strings.Contains(ignoreLine, strings.ToLower(GetUncolorizedText(c))) &&
strings.Contains(ignoreLine, "components")
Copy link
Collaborator

@reuvenharrison reuvenharrison Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to require the keyword "components" to ignore a change in components.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll need to remove this based on the json output it wouldn't work:

[{"id":"api-schema-removed","text":"removed the schema 'SchemaValue'","level":3,"source":"Components"}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what you mean by "based on the json output it wouldn't work".
Can you explain the use case in more detail?

}

func (c ComponentChange) GetId() string {
Expand Down
4 changes: 0 additions & 4 deletions checker/ignore.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,7 @@ func ProcessIgnoredBackwardCompatibilityErrors(level Level, errs Changes, ignore
ignoredErrs := make([]bool, len(errs))
for ignoreScanner.Scan() {
ignoreLine := strings.ToLower(ignoreScanner.Text())

ignorePath := ignoreLinePath(ignoreLine)
if ignorePath == "" {
continue
}

for errIndex, err := range errs {
if err.GetLevel() != level {
Expand Down
15 changes: 15 additions & 0 deletions checker/ignore_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"github.com/stretchr/testify/require"
"github.com/tufin/oasdiff/checker"
"github.com/tufin/oasdiff/diff"
"github.com/tufin/oasdiff/utils"
)

func TestIgnore(t *testing.T) {
Expand Down Expand Up @@ -52,3 +53,17 @@ func TestIgnoreOnlyIncludedSubpaths(t *testing.T) {
e0 := errs[0].(checker.ApiChange)
require.Contains(t, e0.Path, "/resource/new") //see that new breaking change was kept even though it is a substring of newest
}

func TestIgnoreComponent(t *testing.T) {
s1 := l(t, 1)
s2 := l(t, 3)

d, osm, err := diff.GetWithOperationsSourcesMap(getConfig(), &s1, &s2)
require.NoError(t, err)
errs := checker.CheckBackwardCompatibility(checker.GetChecks(utils.StringList{"api-schema-removed"}), d, osm)
require.Equal(t, 8, len(errs))

errs, err = checker.ProcessIgnoredBackwardCompatibilityErrors(checker.ERR, errs, "../data/ignore-err-example.txt")
require.NoError(t, err)
require.Equal(t, 5, len(errs))
}
8 changes: 5 additions & 3 deletions checker/security_change.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,12 @@ package checker

import (
"fmt"
"strings"

"github.com/TwiN/go-color"
)

// SecurityChange represnts a change in the Security Section (not to be confised with components/securitySchemes)
// SecurityChange represents a change in the Security Section (not to be confised with components/securitySchemes)
type SecurityChange struct {
Id string `json:"id,omitempty" yaml:"id,omitempty"`
Text string `json:"text,omitempty" yaml:"text,omitempty"`
Expand All @@ -21,8 +22,9 @@ type SecurityChange struct {
SourceColumnEnd int `json:"-" yaml:"-"`
}

func (SecurityChange) MatchIgnore(ignorePath, ignoreLine string) bool {
return false
func (c SecurityChange) MatchIgnore(ignorePath, ignoreLine string) bool {
return strings.Contains(ignoreLine, strings.ToLower(GetUncolorizedText(c))) &&
strings.Contains(ignoreLine, "security")
}

func (c SecurityChange) GetId() string {
Expand Down
3 changes: 3 additions & 0 deletions data/ignore-err-example.txt
Original file line number Diff line number Diff line change
@@ -1 +1,4 @@
GET /api/{domain}/{project}/badges/security-score removed the success response with the status '200'
in components removed the schema 'rules'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to require the keyword "components" to ignore a component.

removed the schema 'network-policies' from components

8 changes: 8 additions & 0 deletions internal/run_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,14 @@ func Test_BreakingChangesIgnoreErrsAndWarns(t *testing.T) {
require.Len(t, bc, 4)
}

func Test_BreakingChangesIgnoreErrsApiSchemaOptional(t *testing.T) {
var stdout bytes.Buffer
require.Zero(t, internal.Run(cmdToArgs("oasdiff breaking ../data/openapi-test1.yaml ../data/openapi-test3.yaml --err-ignore ../data/ignore-err-example.txt --warn-ignore ../data/ignore-warn-example.txt --include-checks api-schema-removed --format json"), &stdout, io.Discard))
bc := []checker.ApiChange{}
require.NoError(t, json.Unmarshal(stdout.Bytes(), &bc))
require.Len(t, bc, 4)
}

func Test_BreakingChangesInvalidIgnoreFile(t *testing.T) {
require.Equal(t, 121, internal.Run(cmdToArgs("oasdiff breaking ../data/openapi-test1.yaml ../data/openapi-test3.yaml --err-ignore no-file"), io.Discard, io.Discard))
}
Expand Down