Skip to content

Commit d68743c

Browse files
kiwamizamuraireuvenharrison
kiwamizamurai
and
reuvenharrison
authored
Fix pattern addition detection: Change severity from WARN to ERROR for breaking changes (#720)
* feat: add pattern-added-error-comment for breaking pattern additions - Add new error message for pattern additions in English - Add Portuguese (Brazil) translation - Add Russian translation - Update pattern-changed-warn-comment to be more comprehensive * chore: regenerate localization files Regenerate localizations.go after adding pattern-added-error-comment * fix: use specific error comment for pattern additions - Add PatternAddedCommentId constant for pattern addition errors - Use PatternAddedCommentId for pattern additions instead of generic PatternChangedCommentId - Keep PatternChangedCommentId for pattern changes (WARN level) * fix: change pattern addition severity from WARN to ERR BREAKING CHANGE: Pattern additions are now classified as ERROR level instead of WARN level - RequestParameterPatternAddedId: WARN -> ERR - RequestPropertyPatternAddedId: WARN -> ERR This correctly identifies pattern additions as breaking changes since they restrict previously accepted values. * test: update tests for pattern addition severity change - Update pattern addition tests to expect ERR level instead of WARN - Update test assertions for new error messages - Verify pattern changes still use WARN level * test: fix test ordering after pattern addition severity change Pattern additions now have ERR level priority, changing the order of breaking changes in test results. Update expected order in: - TestBreaking_OperationID - TestBreaking_LinkOperationID * run make to fix build * delete duplicate rule: RequestParameterPatternAddedId * update localized messages --------- Co-authored-by: Reuven Harrison <[email protected]>
1 parent dcf4f18 commit d68743c

10 files changed

+28
-23
lines changed

‎checker/check_not_breaking_test.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -168,17 +168,17 @@ func TestBreaking_OperationID(t *testing.T) {
168168
r := d(t, diff.NewConfig(), 3, 1)
169169
require.Len(t, r, 3)
170170
require.Equal(t, checker.RequestParameterMaxLengthDecreasedId, r[0].GetId())
171-
require.Equal(t, checker.RequestParameterEnumValueRemovedId, r[1].GetId())
172-
require.Equal(t, checker.RequestParameterPatternAddedId, r[2].GetId())
171+
require.Equal(t, checker.RequestParameterPatternAddedId, r[1].GetId())
172+
require.Equal(t, checker.RequestParameterEnumValueRemovedId, r[2].GetId())
173173
}
174174

175175
// BC: changing a link to operation ID is not breaking
176176
func TestBreaking_LinkOperationID(t *testing.T) {
177177
r := d(t, diff.NewConfig(), 3, 1)
178178
require.Len(t, r, 3)
179179
require.Equal(t, checker.RequestParameterMaxLengthDecreasedId, r[0].GetId())
180-
require.Equal(t, checker.RequestParameterEnumValueRemovedId, r[1].GetId())
181-
require.Equal(t, checker.RequestParameterPatternAddedId, r[2].GetId())
180+
require.Equal(t, checker.RequestParameterPatternAddedId, r[1].GetId())
181+
require.Equal(t, checker.RequestParameterEnumValueRemovedId, r[2].GetId())
182182
}
183183

184184
// BC: adding a media-type to response is not breaking

‎checker/check_request_parameter_pattern_added_or_changed.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ const (
1010
RequestParameterPatternChangedId = "request-parameter-pattern-changed"
1111
RequestParameterPatternGeneralizedId = "request-parameter-pattern-generalized"
1212
PatternChangedCommentId = "pattern-changed-warn-comment"
13+
PatternAddedCommentId = "pattern-added-error-comment"
1314
)
1415

1516
func RequestParameterPatternAddedOrChangedCheck(diffReport *diff.Diff, operationsSources *diff.OperationsSourcesMap, config *Config) Changes {
@@ -43,7 +44,7 @@ func RequestParameterPatternAddedOrChangedCheck(diffReport *diff.Diff, operation
4344
RequestParameterPatternAddedId,
4445
config,
4546
[]any{patternDiff.To, paramLocation, paramName},
46-
PatternChangedCommentId,
47+
PatternAddedCommentId,
4748
operationsSources,
4849
operationItem.Revision,
4950
operation,

‎checker/check_request_parameter_pattern_added_or_changed_test.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ func TestRequestParameterPatternChanged(t *testing.T) {
3131
Source: load.NewSource("../data/checker/request_parameter_pattern_added_or_changed_base.yaml"),
3232
}, errs[0])
3333
require.Equal(t, "changed the pattern of the 'query' request parameter 'category' from '^\\w+$' to '^[\\w\\s]+$'", errs[0].GetUncolorizedText(checker.NewDefaultLocalizer()))
34-
require.Equal(t, "This is a warning because it is difficult to automatically analyze if the new pattern is a superset of the previous pattern (e.g. changed from '[0-9]+' to '[0-9]*')", errs[0].GetComment(checker.NewDefaultLocalizer()))
34+
require.Equal(t, "This is a warning because adding or changing a pattern may restrict the accepted values and break existing clients. For pattern changes, it is difficult to automatically analyze if the new pattern is a superset of the previous pattern (e.g. changed from '[0-9]+' to '[0-9]*')", errs[0].GetComment(checker.NewDefaultLocalizer()))
3535
}
3636

3737
// CL: generalizing pattern of request parameters
@@ -66,18 +66,18 @@ func TestRequestParameterPatternAdded(t *testing.T) {
6666

6767
d, osm, err := diff.GetWithOperationsSourcesMap(diff.NewConfig(), s1, s2)
6868
require.NoError(t, err)
69-
errs := checker.CheckBackwardCompatibilityUntilLevel(singleCheckConfig(checker.RequestParameterPatternAddedOrChangedCheck), d, osm, checker.WARN)
69+
errs := checker.CheckBackwardCompatibilityUntilLevel(singleCheckConfig(checker.RequestParameterPatternAddedOrChangedCheck), d, osm, checker.ERR)
7070
require.Len(t, errs, 1)
7171
require.Equal(t, checker.ApiChange{
7272
Id: checker.RequestParameterPatternAddedId,
7373
Args: []any{"^\\w+$", "query", "category"},
74-
Comment: checker.PatternChangedCommentId,
75-
Level: checker.WARN,
74+
Comment: checker.PatternAddedCommentId,
75+
Level: checker.ERR,
7676
Operation: "POST",
7777
Path: "/test",
7878
Source: load.NewSource("../data/checker/request_parameter_pattern_added_or_changed_base.yaml"),
7979
}, errs[0])
80-
require.Equal(t, "This is a warning because it is difficult to automatically analyze if the new pattern is a superset of the previous pattern (e.g. changed from '[0-9]+' to '[0-9]*')", errs[0].GetComment(checker.NewDefaultLocalizer()))
80+
require.Equal(t, "This is a breaking change because adding a pattern restriction to a previously unrestricted parameter will reject values that were previously accepted, breaking existing clients", errs[0].GetComment(checker.NewDefaultLocalizer()))
8181
}
8282

8383
// CL: removing pattern from request parameters

‎checker/check_request_property_pattern_added_or_changed.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ func RequestPropertyPatternUpdatedCheck(diffReport *diff.Diff, operationsSources
5454
RequestPropertyPatternAddedId,
5555
config,
5656
[]any{patternDiff.To, propName},
57-
PatternChangedCommentId,
57+
PatternAddedCommentId,
5858
operationsSources,
5959
operationItem.Revision,
6060
operation,

‎checker/check_request_property_pattern_added_or_changed_test.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ func TestRequestPropertyPatternChanged(t *testing.T) {
3131
Source: load.NewSource("../data/checker/request_property_pattern_added_or_changed_revision.yaml"),
3232
Comment: checker.PatternChangedCommentId,
3333
}, errs[0])
34-
require.Equal(t, "This is a warning because it is difficult to automatically analyze if the new pattern is a superset of the previous pattern (e.g. changed from '[0-9]+' to '[0-9]*')", errs[0].GetComment(checker.NewDefaultLocalizer()))
34+
require.Equal(t, "This is a warning because adding or changing a pattern may restrict the accepted values and break existing clients. For pattern changes, it is difficult to automatically analyze if the new pattern is a superset of the previous pattern (e.g. changed from '[0-9]+' to '[0-9]*')", errs[0].GetComment(checker.NewDefaultLocalizer()))
3535
}
3636

3737
// CL: generalizing request property pattern
@@ -66,18 +66,18 @@ func TestRequestPropertyPatternAdded(t *testing.T) {
6666

6767
d, osm, err := diff.GetWithOperationsSourcesMap(diff.NewConfig(), s1, s2)
6868
require.NoError(t, err)
69-
errs := checker.CheckBackwardCompatibilityUntilLevel(singleCheckConfig(checker.RequestPropertyPatternUpdatedCheck), d, osm, checker.INFO)
69+
errs := checker.CheckBackwardCompatibilityUntilLevel(singleCheckConfig(checker.RequestPropertyPatternUpdatedCheck), d, osm, checker.ERR)
7070
require.Len(t, errs, 1)
7171
require.Equal(t, checker.ApiChange{
7272
Id: checker.RequestPropertyPatternAddedId,
7373
Args: []any{"^\\w+$", "name"},
74-
Level: checker.WARN,
74+
Level: checker.ERR,
7575
Operation: "POST",
7676
Path: "/test",
7777
Source: load.NewSource("../data/checker/request_property_pattern_added_or_changed_base.yaml"),
78-
Comment: checker.PatternChangedCommentId,
78+
Comment: checker.PatternAddedCommentId,
7979
}, errs[0])
80-
require.Equal(t, "This is a warning because it is difficult to automatically analyze if the new pattern is a superset of the previous pattern (e.g. changed from '[0-9]+' to '[0-9]*')", errs[0].GetComment(checker.NewDefaultLocalizer()))
80+
require.Equal(t, "This is a breaking change because adding a pattern restriction to a previously unrestricted parameter will reject values that were previously accepted, breaking existing clients", errs[0].GetComment(checker.NewDefaultLocalizer()))
8181
}
8282

8383
// CL: removing request property pattern

‎checker/localizations/localizations.go‎

Lines changed: 5 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎checker/localizations_src/en/messages.yaml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,8 @@ request-parameter-became-optional: the %s request parameter %s became optional
5757
request-parameter-became-enum: the %s request parameter %s was restricted to a list of enum values
5858
request-parameter-enum-value-removed: removed the enum value %s from the %s request parameter %s
5959
request-parameter-enum-value-added: added the new enum value %s to the %s request parameter %s
60-
pattern-changed-warn-comment: "This is a warning because it is difficult to automatically analyze if the new pattern is a superset of the previous pattern (e.g. changed from '[0-9]+' to '[0-9]*')"
61-
request-parameter-x-extensible-enum-value-removed: removed the x-extensible-enum value %s from the %s request parameter %s
60+
pattern-changed-warn-comment: "This is a warning because adding or changing a pattern may restrict the accepted values and break existing clients. For pattern changes, it is difficult to automatically analyze if the new pattern is a superset of the previous pattern (e.g. changed from '[0-9]+' to '[0-9]*')"
61+
pattern-added-error-comment: "This is a breaking change because adding a pattern restriction to a previously unrestricted parameter will reject values that were previously accepted, breaking existing clients"
6262
request-parameter-max-decreased: for the %s request parameter %s, the max was decreased from %s to %s
6363
request-parameter-max-increased: for the %s request parameter %s, the max was increased from %s to %s
6464
request-parameter-default-value-added: for the %s request parameter %s, default value %s was added

‎checker/localizations_src/pt-br/messages.yaml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ request-parameter-became-enum: o parâmetro de requisição do tipo %s e nome %s
5858
request-parameter-enum-value-removed: valor %s do enum removido do parâmetro de requisição do tipo %s e nome %s
5959
request-parameter-enum-value-added: valor %s do enum adicionado ao parâmetro de requisição do tipo %s e nome %s
6060
pattern-changed-warn-comment: "Este é um aviso porque é difícil analisar automaticamente se o novo padrão é um superconjunto do padrão anterior (por exemplo, alterado de '[0-9]+' para '[0-9]*')"
61+
pattern-added-error-comment: "Esta é uma alteração crítica porque adicionar uma restrição de padrão a um parâmetro anteriormente irrestrito rejeitará valores que eram aceitos anteriormente, quebrando clientes existentes"
6162
request-parameter-x-extensible-enum-value-removed: valor x-extensible-enum %s removido do parâmetro de requisição do tipo %s e nome %s
6263
request-parameter-max-decreased: no parâmetro de requisição do tipo %s e nome %s teve seu valor máximo foi reduzido de %s para %s
6364
request-parameter-max-increased: no parâmetro de requisição do tipo %s e nome %s teve seu valor máximo foi aumentado de %s para %s

‎checker/localizations_src/ru/messages.yaml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ request-parameter-enum-value-removed: удалено значение enum %s у
5555
request-parameter-enum-value-added: добавлено значение enum %s у %s параметра запроса %s
5656
pattern-changed-warn-comment: Это предупреждение, потому что сложно автоматически проанализировать, является ли новый шаблон надмножеством предыдущего шаблона (например, изменен с '[0-9]+' на '[0-9]*').
5757
request-parameter-x-extensible-enum-value-removed: удалено из x-extensible-enum значение %s у %s параметра запроса %s
58+
pattern-added-error-comment: Это критическое изменение, потому что добавление ограничения шаблона к ранее неограниченному параметру отклонит значения, которые ранее принимались, сломав существующих клиентов
5859
request-parameter-max-decreased: в %s параметре запроса %s, max уменьшен с %s до %s
5960
request-parameter-max-increased: в %s параметре запроса %s, max увеличен с %s до %s
6061
request-parameter-default-value-added: для параметра запроса %s добавлено значение по умолчанию %s

‎checker/rules.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ func GetAllRules() BackwardCompatibilityRules {
193193
newBackwardCompatibilityRule(RequestParameterMinIncreasedId, ERR, RequestParameterMinUpdatedCheck, DirectionRequest, LocationParameters, ActionIncrease),
194194
newBackwardCompatibilityRule(RequestParameterMinDecreasedId, INFO, RequestParameterMinUpdatedCheck, DirectionRequest, LocationParameters, ActionDecrease),
195195
// RequestParameterPatternAddedOrChangedCheck
196-
newBackwardCompatibilityRule(RequestParameterPatternAddedId, WARN, RequestParameterPatternAddedOrChangedCheck, DirectionRequest, LocationParameters, ActionAdd),
196+
newBackwardCompatibilityRule(RequestParameterPatternAddedId, ERR, RequestParameterPatternAddedOrChangedCheck, DirectionRequest, LocationParameters, ActionAdd),
197197
newBackwardCompatibilityRule(RequestParameterPatternRemovedId, INFO, RequestParameterPatternAddedOrChangedCheck, DirectionRequest, LocationParameters, ActionRemove),
198198
newBackwardCompatibilityRule(RequestParameterPatternChangedId, WARN, RequestParameterPatternAddedOrChangedCheck, DirectionRequest, LocationParameters, ActionChange),
199199
newBackwardCompatibilityRule(RequestParameterPatternGeneralizedId, INFO, RequestParameterPatternAddedOrChangedCheck, DirectionRequest, LocationParameters, ActionGeneralize),
@@ -286,7 +286,7 @@ func GetAllRules() BackwardCompatibilityRules {
286286
newBackwardCompatibilityRule(RequestPropertyOneOfRemovedId, ERR, RequestPropertyOneOfUpdatedCheck, DirectionRequest, LocationProperties, ActionRemove),
287287
// RequestPropertyPatternUpdatedCheck
288288
newBackwardCompatibilityRule(RequestPropertyPatternRemovedId, INFO, RequestPropertyPatternUpdatedCheck, DirectionRequest, LocationProperties, ActionRemove),
289-
newBackwardCompatibilityRule(RequestPropertyPatternAddedId, WARN, RequestPropertyPatternUpdatedCheck, DirectionRequest, LocationProperties, ActionAdd),
289+
newBackwardCompatibilityRule(RequestPropertyPatternAddedId, ERR, RequestPropertyPatternUpdatedCheck, DirectionRequest, LocationProperties, ActionAdd),
290290
newBackwardCompatibilityRule(RequestPropertyPatternChangedId, WARN, RequestPropertyPatternUpdatedCheck, DirectionRequest, LocationProperties, ActionChange),
291291
newBackwardCompatibilityRule(RequestPropertyPatternGeneralizedId, INFO, RequestPropertyPatternUpdatedCheck, DirectionRequest, LocationProperties, ActionGeneralize),
292292
// RequestPropertyRequiredUpdatedCheck

0 commit comments

Comments
 (0)