Skip to content

Commit

Permalink
check new required props for default value (Tufin#551)
Browse files Browse the repository at this point in the history
  • Loading branch information
@reuvenharrison
reuvenharrison authored Jun 5, 2024
1 parent 8aa8c09 commit 7d9a529
Show file tree
Hide file tree
Showing 12 changed files with 218 additions and 105 deletions.
16 changes: 16 additions & 0 deletions checker/check_breaking_property_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -591,6 +591,7 @@ func TestBreaking_Body(t *testing.T) {
require.NotEmpty(t, errs)
require.Len(t, errs, 1)
require.Equal(t, checker.RequestPropertyBecameRequiredId, errs[0].GetId())
require.Equal(t, []interface{}{"id"}, errs[0].GetArgs())
}

// BC: changing an existing property in request body items to required is breaking
Expand All @@ -607,6 +608,21 @@ func TestBreaking_Items(t *testing.T) {
require.NotEmpty(t, errs)
require.Len(t, errs, 1)
require.Equal(t, checker.RequestPropertyBecameRequiredId, errs[0].GetId())
require.Equal(t, []interface{}{"/items/id"}, errs[0].GetArgs())
}

// BC: changing an existing property in request body items to required with a default value is not breaking
func TestBreaking_ItemsWithDefault(t *testing.T) {
s1, err := open(getReqPropFile("items1.yaml"))
require.NoError(t, err)

s2, err := open(getReqPropFile("items3.yaml"))
require.NoError(t, err)

d, osm, err := diff.GetWithOperationsSourcesMap(diff.NewConfig(), s1, s2)
require.NoError(t, err)
errs := checker.CheckBackwardCompatibility(checker.NewConfig(), d, osm)
require.Empty(t, errs)
}

// BC: changing an existing property in request body anyOf to required is breaking
Expand Down
141 changes: 57 additions & 84 deletions checker/check_request_property_required_updated.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@ import (
)

const (
RequestPropertyBecameRequiredId = "request-property-became-required"
RequestPropertyBecameOptionalId = "request-property-became-optional"
RequestPropertyBecameRequiredId = "request-property-became-required"
RequestPropertyBecameRequiredWithDefaultId = "request-property-became-required-with-default"
RequestPropertyBecameOptionalId = "request-property-became-optional"
)

func RequestPropertyRequiredUpdatedCheck(diffReport *diff.Diff, operationsSources *diff.OperationsSourcesMap, config *Config) Changes {
Expand All @@ -33,92 +34,38 @@ func RequestPropertyRequiredUpdatedCheck(diffReport *diff.Diff, operationsSource
continue
}

if mediaTypeDiff.SchemaDiff.RequiredDiff != nil {
for _, changedRequiredPropertyName := range mediaTypeDiff.SchemaDiff.RequiredDiff.Added {
if mediaTypeDiff.SchemaDiff.Base.Properties[changedRequiredPropertyName] == nil {
// it is a new property, checked by the new-required-request-property check
continue
}
if mediaTypeDiff.SchemaDiff.Revision.Properties[changedRequiredPropertyName] == nil {
// property was removed, checked by request-property-removed
continue
}
if mediaTypeDiff.SchemaDiff.Revision.Properties[changedRequiredPropertyName].Value.ReadOnly {
continue
}
result = append(result, ApiChange{
Id: RequestPropertyBecameRequiredId,
Level: ERR,
Args: []any{changedRequiredPropertyName},
Operation: operation,
OperationId: operationItem.Revision.OperationID,
Path: path,
Source: load.NewSource(source),
})
}
for _, changedRequiredPropertyName := range mediaTypeDiff.SchemaDiff.RequiredDiff.Deleted {
if mediaTypeDiff.SchemaDiff.Base.Properties[changedRequiredPropertyName] == nil {
// it is a new property, checked by the new-required-request-property check
continue
}
if mediaTypeDiff.SchemaDiff.Revision.Properties[changedRequiredPropertyName] == nil {
// property was removed, checked by request-property-removed
continue
}
if mediaTypeDiff.SchemaDiff.Revision.Properties[changedRequiredPropertyName].Value.ReadOnly {
continue
}
result = append(result, ApiChange{
Id: RequestPropertyBecameOptionalId,
Level: INFO,
Args: []any{changedRequiredPropertyName},
Operation: operation,
OperationId: operationItem.Revision.OperationID,
Path: path,
Source: load.NewSource(source),
})
}
}

CheckModifiedPropertiesDiff(
mediaTypeDiff.SchemaDiff,
func(propertyPath string, propertyName string, propertyDiff *diff.SchemaDiff, parent *diff.SchemaDiff) {
requiredDiff := propertyDiff.RequiredDiff
if requiredDiff == nil {
return
}
for _, changedRequiredPropertyName := range requiredDiff.Added {
if propertyDiff.Revision.Properties[changedRequiredPropertyName] == nil {
continue
}
if propertyDiff.Revision.Properties[changedRequiredPropertyName].Value.ReadOnly {
continue
}
if propertyDiff.Base.Properties[changedRequiredPropertyName] == nil {
// it is a new property, checked by the new-required-request-property check
processRequestPropertyRequiredDiff := func(schemaDiff *diff.SchemaDiff, propertyPath string, propertyName string) {
if schemaDiff.RequiredDiff != nil {
for _, changedRequiredPropertyName := range schemaDiff.RequiredDiff.Added {
if !changedRequiredPropertyRelevant(schemaDiff, changedRequiredPropertyName) {
continue
}

result = append(result, ApiChange{
Id: RequestPropertyBecameRequiredId,
Level: ERR,
Args: []any{propertyFullName(propertyPath, propertyFullName(propertyName, changedRequiredPropertyName))},
Operation: operation,
OperationId: operationItem.Revision.OperationID,
Path: path,
Source: load.NewSource(source),
})
}

for _, changedRequiredPropertyName := range requiredDiff.Deleted {
if propertyDiff.Revision.Properties[changedRequiredPropertyName] == nil {
continue
if schemaDiff.Revision.Properties[changedRequiredPropertyName].Value.Default == nil {
result = append(result, ApiChange{
Id: RequestPropertyBecameRequiredId,
Level: ERR,
Args: []any{propertyFullName(propertyPath, propertyFullName(propertyName, changedRequiredPropertyName))},
Operation: operation,
OperationId: operationItem.Revision.OperationID,
Path: path,
Source: load.NewSource(source),
})
} else {
// property has a default value, so making it required is not a breaking change
result = append(result, ApiChange{
Id: RequestPropertyBecameRequiredWithDefaultId,
Level: INFO,
Args: []any{propertyFullName(propertyPath, propertyFullName(propertyName, changedRequiredPropertyName))},
Operation: operation,
OperationId: operationItem.Revision.OperationID,
Path: path,
Source: load.NewSource(source),
})
}
if propertyDiff.Revision.Properties[changedRequiredPropertyName].Value.ReadOnly {
continue
}
if propertyDiff.Base.Properties[changedRequiredPropertyName] == nil {
// it is a new property, checked by the new-required-request-property check
}
for _, changedRequiredPropertyName := range schemaDiff.RequiredDiff.Deleted {
if !changedRequiredPropertyRelevant(schemaDiff, changedRequiredPropertyName) {
continue
}

Expand All @@ -132,9 +79,35 @@ func RequestPropertyRequiredUpdatedCheck(diffReport *diff.Diff, operationsSource
Source: load.NewSource(source),
})
}
}
}

processRequestPropertyRequiredDiff(mediaTypeDiff.SchemaDiff, "", "")

CheckModifiedPropertiesDiff(
mediaTypeDiff.SchemaDiff,
func(propertyPath string, propertyName string, propertyDiff *diff.SchemaDiff, _ *diff.SchemaDiff) {
processRequestPropertyRequiredDiff(propertyDiff, propertyPath, propertyName)
})
}
}
}
return result
}

func changedRequiredPropertyRelevant(schemaDiff *diff.SchemaDiff, changedRequiredPropertyName string) bool {
if schemaDiff.Base.Properties[changedRequiredPropertyName] == nil {
// it is a new property, checked by the new-required-request-property check
return false
}
if schemaDiff.Revision.Properties[changedRequiredPropertyName] == nil {
// property was removed, checked by request-property-removed
return false
}
if schemaDiff.Revision.Properties[changedRequiredPropertyName].Value.ReadOnly {
// property is read-only, not relevant in requests
return false
}

return true
}
24 changes: 24 additions & 0 deletions checker/check_request_property_required_updated_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,27 @@ func TestRequestPropertyMarkedOptional(t *testing.T) {
OperationId: "addProduct",
}, errs[0])
}

// CL: making request property required, while also giving it a default value
func TestRequestPropertyWithDefaultMarkedRequired(t *testing.T) {
s1, err := open("../data/checker/request_property_became_required_base.yaml")
require.NoError(t, err)
s2, err := open("../data/checker/request_property_became_required_base.yaml")
require.NoError(t, err)

s1.Spec.Paths.Value("/products").Post.RequestBody.Value.Content["application/json"].Schema.Value.Required = []string{""}
s2.Spec.Paths.Value("/products").Post.RequestBody.Value.Content["application/json"].Schema.Value.Properties["name"].Value.Default = "default"
d, osm, err := diff.GetWithOperationsSourcesMap(diff.NewConfig(), s1, s2)
require.NoError(t, err)
errs := checker.CheckBackwardCompatibilityUntilLevel(singleCheckConfig(checker.RequestPropertyRequiredUpdatedCheck), d, osm, checker.INFO)
require.Len(t, errs, 1)
require.Equal(t, checker.ApiChange{
Id: checker.RequestPropertyBecameRequiredWithDefaultId,
Args: []any{"name"},
Level: checker.INFO,
Operation: "POST",
Path: "/products",
Source: load.NewSource("../data/checker/request_property_became_required_base.yaml"),
OperationId: "addProduct",
}, errs[0])
}
37 changes: 25 additions & 12 deletions checker/check_request_property_updated.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@ import (
)

const (
RequestPropertyRemovedId = "request-property-removed"
NewRequiredRequestPropertyId = "new-required-request-property"
NewOptionalRequestPropertyId = "new-optional-request-property"
RequestPropertyRemovedId = "request-property-removed"
NewRequiredRequestPropertyId = "new-required-request-property"
NewRequiredRequestPropertyWithDefaultId = "new-required-request-property-with-default"
NewOptionalRequestPropertyId = "new-optional-request-property"
)

func RequestPropertyUpdatedCheck(diffReport *diff.Diff, operationsSources *diff.OperationsSourcesMap, config *Config) Changes {
Expand Down Expand Up @@ -57,15 +58,27 @@ func RequestPropertyUpdatedCheck(diffReport *diff.Diff, operationsSources *diff.
propName := propertyFullName(propertyPath, propertyName)

if slices.Contains(parent.Revision.Required, propertyName) {
result = append(result, ApiChange{
Id: NewRequiredRequestPropertyId,
Level: ERR,
Args: []any{propName},
Operation: operation,
OperationId: operationItem.Revision.OperationID,
Path: path,
Source: load.NewSource(source),
})
if propertyItem.Default == nil {
result = append(result, ApiChange{
Id: NewRequiredRequestPropertyId,
Level: ERR,
Args: []any{propName},
Operation: operation,
OperationId: operationItem.Revision.OperationID,
Path: path,
Source: load.NewSource(source),
})
} else {
result = append(result, ApiChange{
Id: NewRequiredRequestPropertyWithDefaultId,
Level: INFO,
Args: []any{propName},
Operation: operation,
OperationId: operationItem.Revision.OperationID,
Path: path,
Source: load.NewSource(source),
})
}
} else {
result = append(result, ApiChange{
Id: NewOptionalRequestPropertyId,
Expand Down
22 changes: 22 additions & 0 deletions checker/check_request_property_updated_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,3 +106,25 @@ func TestRequiredRequestPropertyRemoved(t *testing.T) {
OperationId: "addProduct",
}, errs[0])
}

// CL: adding a new required request property with a default value
func TestRequiredRequestPropertyAddedWithDefault(t *testing.T) {
s1, err := open("../data/checker/request_property_added_base.yaml")
require.NoError(t, err)
s2, err := open("../data/checker/request_property_added_with_default.yaml")
require.NoError(t, err)

d, osm, err := diff.GetWithOperationsSourcesMap(diff.NewConfig(), s1, s2)
require.NoError(t, err)
errs := checker.CheckBackwardCompatibilityUntilLevel(singleCheckConfig(checker.RequestPropertyUpdatedCheck), d, osm, checker.INFO)
require.Len(t, errs, 1)
require.Equal(t, checker.ApiChange{
Id: checker.NewRequiredRequestPropertyWithDefaultId,
Args: []any{"description"},
Level: checker.INFO,
Operation: "POST",
Path: "/products",
Source: load.NewSource("../data/checker/request_property_added_with_default.yaml"),
OperationId: "addProduct",
}, errs[0])
}
12 changes: 9 additions & 3 deletions checker/localizations/localizations.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit 7d9a529

Please sign in to comment.