changelog: cover not required request params (#296)

* changelog: cover optional non-path request params * git --------- Co-authored-by: Reuven <[email protected]>
Tufin · Jun 13, 2023 · 87f156b · 87f156b
1 parent 9de6f17
commit 87f156b
Show file tree

Hide file tree

Showing 9 changed files with 143 additions and 14 deletions.
diff --git a/BREAKING-CHANGES-EXAMPLES.md b/BREAKING-CHANGES-EXAMPLES.md
@@ -138,6 +138,7 @@ These examples are automatically generated from unit tests.
 [changing an existing header param from required to optional](checker/checker_request_parameter_required_value_updated_test.go?plain=1#L36)  
 [changing an existing header param to optional](checker/checker_not_breaking_test.go?plain=1#L116)  
 [deprecating an operation with sunset greater than min](checker/checker_not_breaking_test.go?plain=1#L182)  
+[new header, query and cookie request params](checker/check-new-request-non-path-parameter_test.go?plain=1#L11)  
 [new paths or path operations](checker/check-api-added_test.go?plain=1#L11)  
 [path operations that became deprecated](checker/checker_deprecation_test.go?plain=1#L324)  
 [path operations that were re-activated](checker/checker_deprecation_test.go?plain=1#L344)  
diff --git a/...ew-request-required-non-path-parameter.go → ...r/check-new-request-non-path-parameter.go b/...ew-request-required-non-path-parameter.go → ...r/check-new-request-non-path-parameter.go
@@ -6,7 +6,7 @@ import (
 	"github.com/tufin/oasdiff/diff"
 )
 
-func NewRequiredRequestNonPathParameterCheck(diffReport *diff.Diff, operationsSources *diff.OperationsSourcesMap, config BackwardCompatibilityCheckConfig) []BackwardCompatibilityError {
+func NewRequestNonPathParameterCheck(diffReport *diff.Diff, operationsSources *diff.OperationsSourcesMap, config BackwardCompatibilityCheckConfig) []BackwardCompatibilityError {
 	result := make([]BackwardCompatibilityError, 0)
 	if diffReport.PathsDiff == nil {
 		return result
@@ -28,18 +28,23 @@ func NewRequiredRequestNonPathParameterCheck(diffReport *diff.Diff, operationsSo
 				for _, paramName := range paramItems {
 					for _, param := range operationItem.Revision.Parameters {
 						if param.Value.Name == paramName {
-							if param.Value.Required {
-								source := (*operationsSources)[operationItem.Revision]
-								result = append(result, BackwardCompatibilityError{
-									Id:          "new-required-request-parameter",
-									Level:       ERR,
-									Text:        fmt.Sprintf(config.i18n("new-required-request-parameter"), ColorizedValue(paramLocation), ColorizedValue(paramName)),
-									Operation:   operation,
-									OperationId: operationItem.Revision.OperationID,
-									Path:        path,
-									Source:      source,
-								})
+							id := "new-required-request-parameter"
+							level := ERR
+							if !param.Value.Required {
+								id = "new-optional-request-parameter"
+								level = INFO
 							}
+							source := (*operationsSources)[operationItem.Revision]
+							result = append(result, BackwardCompatibilityError{
+								Id:          id,
+								Level:       level,
+								Text:        fmt.Sprintf(config.i18n(id), ColorizedValue(paramLocation), ColorizedValue(paramName)),
+								Operation:   operation,
+								OperationId: operationItem.Revision.OperationID,
+								Path:        path,
+								Source:      source,
+							})
+
 							break
 						}
 					}

diff --git a/checker/check-new-request-non-path-parameter_test.go b/checker/check-new-request-non-path-parameter_test.go
@@ -0,0 +1,43 @@
+package checker_test
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"github.com/tufin/oasdiff/checker"
+	"github.com/tufin/oasdiff/diff"
+)
+
+// CL: new header, query and cookie request params
+func TestNewRequestNonPathParameter_DetectsNewPathsAndNewOperations(t *testing.T) {
+	s1, err := open("../data/request_params/base.yaml")
+	require.NoError(t, err)
+
+	s2, err := open("../data/request_params/optional-request-params.yaml")
+	require.NoError(t, err)
+
+	d, osm, err := diff.GetWithOperationsSourcesMap(&diff.Config{}, s1, s2)
+	require.NoError(t, err)
+
+	errs := checker.CheckBackwardCompatibilityUntilLevel(singleCheckConfig(checker.NewRequestNonPathParameterCheck), d, osm, checker.INFO)
+	require.NotEmpty(t, errs)
+	require.Len(t, errs, 3)
+
+	require.Equal(t, "new-optional-request-parameter", errs[0].Id)
+	require.Equal(t, "GET", errs[0].Operation)
+	require.Equal(t, "/api/test1", errs[0].Path)
+	require.Equal(t, checker.INFO, errs[0].Level)
+	require.Contains(t, errs[0].Text, "X-NewRequestHeaderParam")
+
+	require.Equal(t, "new-optional-request-parameter", errs[1].Id)
+	require.Equal(t, "GET", errs[1].Operation)
+	require.Equal(t, "/api/test2", errs[1].Path)
+	require.Equal(t, checker.INFO, errs[1].Level)
+	require.Contains(t, errs[1].Text, "newQueryParam")
+
+	require.Equal(t, "new-optional-request-parameter", errs[2].Id)
+	require.Equal(t, "GET", errs[2].Operation)
+	require.Equal(t, "/api/test3", errs[2].Path)
+	require.Equal(t, checker.INFO, errs[2].Level)
+	require.Contains(t, errs[2].Text, "csrf-token")
+}
diff --git a/checker/default_checks.go b/checker/default_checks.go
@@ -82,7 +82,7 @@ func defaultChecks() []BackwardCompatibilityCheck {
 		ResponseSuccessStatusRemoved,
 		ResponseMediaTypeRemoved,
 		NewRequestPathParameterCheck,
-		NewRequiredRequestNonPathParameterCheck,
+		NewRequestNonPathParameterCheck,
 		NewRequiredRequestHeaderPropertyCheck,
 		ResponseRequiredPropertyRemovedCheck,
 		UncheckedRequestAllOfWarnCheck,

diff --git a/checker/localizations/localizations.go b/checker/localizations/localizations.go
diff --git a/checker/localizations_src/en/messages.yaml b/checker/localizations_src/en/messages.yaml
@@ -22,6 +22,7 @@ api-schema-removed: removed the schema %s from openapi components
 sunset-deleted: api sunset date deleted, but deprecated=true kept
 api-sunset-date-changed-too-small: api sunset date changed to earlier date from %s to %s, new sunset date must be not earlier than %s at least %d days from now
 new-required-request-parameter: added the new required %s request parameter %s
+new-optional-request-parameter: added the new optional %s request parameter %s
 new-required-request-property: added the new required request property %s
 new-required-request-header-property: added the new required %s request header's property %s
 request-body-became-required: request body became required

diff --git a/checker/localizations_src/ru/messages.yaml b/checker/localizations_src/ru/messages.yaml
@@ -22,6 +22,7 @@ api-schema-removed: удалена схема %s из компонентов ope
 sunset-deleted: удалена дата sunset date у API, но сохранён deprecated=true
 api-sunset-date-changed-too-small: дата sunset у API изменена на более раннюю с %s на %s, новая дата sunset должна быть либо не раньше %s, либо, как минимум, %d дней от текущего дня
 new-required-request-parameter: добавлен новый обязательный %s параметр зароса %s
+new-optional-request-parameter: добавлен новый необязательный %s параметр зароса %s
 new-required-request-property: добавлено новле обязательное поле запроса %s
 new-required-request-header-property: в заголовке запроса %s добавлено новое обязательное поле %s
 request-body-became-required: тело запроса стало обязательным

diff --git a/data/request_params/base.yaml b/data/request_params/base.yaml
@@ -0,0 +1,29 @@
+info:
+  title: Tufin
+  version: 1.0.0
+openapi: 3.0.3
+paths:
+  /api/test1:
+    get:
+      operationId: getTest
+      responses:
+        200:
+          description: OK
+    post:
+      responses:
+        201:
+          description: OK
+
+  /api/test2:
+    get:
+      operationId: getTest
+      responses:
+        200:
+          description: OK
+
+  /api/test3:
+    get:
+      operationId: getTest
+      responses:
+        200:
+          description: OK
diff --git a/data/request_params/optional-request-params.yaml b/data/request_params/optional-request-params.yaml
@@ -0,0 +1,47 @@
+info:
+  title: Tufin
+  version: 1.0.0
+openapi: 3.0.3
+paths:
+  /api/test1:
+    get:
+      operationId: getTest
+      parameters:
+        - in: header
+          name: X-NewRequestHeaderParam
+          required: false
+          schema:
+            type: string
+      responses:
+        200:
+          description: OK
+    post:
+      responses:
+        201:
+          description: OK
+
+  /api/test2:
+    get:
+      operationId: getTest
+      parameters:
+        - in: query
+          name: newQueryParam
+          required: false
+          schema:
+            type: string
+      responses:
+        200:
+          description: OK
+
+  /api/test3:
+    get:
+      operationId: getTest
+      parameters:
+        - in: cookie
+          name: csrf-token
+          required: false
+          schema:
+            type: string
+      responses:
+        200:
+          description: OK