Updated some unit test and address PR comments

SPSCommerce · Jul 17, 2024 · 1cd62da · 1cd62da
1 parent 83fdedb
commit 1cd62da
Show file tree

Hide file tree

Showing 4 changed files with 360 additions and 128 deletions.
diff --git a/rulesets/src/collections.ruleset.yml b/rulesets/src/collections.ruleset.yml
@@ -1,7 +1,7 @@
 rules:
   ##### General #####
   sps-no-collection-paging-capability:
-    description: Response bodies from collection endpoints must offer paging capability.
+    description: "Response bodies from collection endpoints must offer paging capability."
     severity: error
     given: $.paths[?([email protected](/.*\/\{[^}]+\}$/))].get.responses['200'].content.application/json.schema.properties
     then:
@@ -14,7 +14,7 @@ rules:
 
   ##### Root Element #####
   sps-collection-missing-results-array:
-    description: Response bodies must have a root element called results and is an array of objects.
+    description: "Response bodies must have a root element called results and is an array of objects."
     severity: error
     given: $.paths[?([email protected](/.*\/\{[^}]+\}$/))].get.responses['200'].content.application/json.schema.properties.results
     then:
@@ -29,7 +29,7 @@ rules:
 
   ##### Pagination #####
   sps-missing-pagination-query-parameters:
-    description: 'Collection GET endpoints must support pagination using query parameters: limit, offset, cursor.'
+    description: "Collection GET endpoints must support pagination using query parameters. Offset or cursor based pagination is required."
     severity: error
     given: $.paths[?([email protected](/.*\/\{[^}]+\}$/))].get
     then:
@@ -40,23 +40,29 @@ rules:
             type: array
             items:
               type: object
+            contains:
+              type: object
               properties:
                 name:
-                  type: string
-              required:
-                - name
-            minItems: 3
-            contains:
-              anyOf:
-                - properties:
+                  const: limit
+                in:
+                  const: query
+            allOf:
+            - anyOf:
+              - contains:
+                  type: object
+                  properties:
                     name:
                       const: offset
-                - properties:
-                    name:
-                      const: limit
-                - properties:
+                    in:
+                      const: query
+              - contains:
+                  type: object
+                  properties:
                     name:
                       const: cursor
+                    in:
+                      const: query
 
   sps-post-request-body-missing-paging-object:
     description: "POST collection endpoints MUST have a request body schema that includes paging parameters."
@@ -70,7 +76,7 @@ rules:
 
   ##### FILTERING #####
   sps-disallow-resource-identifier-filtering:
-    description: "Resource identifier filtering is not allowed. Use the resource identifier in the URL path."
+    description: "Resource identifier filtering is not allowed as a query parameter. Use the resource identifier in the URL path."
     severity: warn
     given: $.paths..get.parameters.[?(@.in=='query' && @.name=='id')]
     then:
@@ -79,35 +85,37 @@ rules:
       functionOptions:
         notMatch: "^id$"
 
-  sps-unreasonable-query-parameters-limit:
-    description: "Filtering query parameters SHOULD have a reasonable limit, no more than 12."
-    severity: warn
-    given: $.paths..get
-    then:
-      function: schema
-      functionOptions:
-        dialect: "draft2020-12"
-        schema:
-          type: object
-          properties:
-            parameters:
-              type: array
-              minContains: 0
-              maxContains: 12
-              contains:
-                type: object
-                properties:
-                  in:
-                    const: query
+# Commented out because 
+# https://github.com/SPSCommerce/sps-api-standards/pull/86#discussion_r1680361945
+  # sps-unreasonable-query-parameters-limit:
+  #   description: "Filtering query parameters SHOULD have a reasonable limit, no more than 12."
+  #   severity: warn
+  #   given: $.paths..get
+  #   then:
+  #     function: schema
+  #     functionOptions:
+  #       schema:
+  #         type: object
+  #         properties:
+  #           parameters:
+  #             type: array
+  #             minContains: 0
+  #             maxContains: 12
+  #             contains:
+  #               type: object
+  #               properties:
+  #                 in:
+  #                   const: query
 
+# This rule might need to be reworked.
+# Hybird filter is possible but it should error if root level filter exist with hybird filter.
   sps-multiple-filter-parameters:
-    description: "Ensures the 'filter' query parameter is specified only once."
+    description: "The 'filter' query parameter MUST be specified only once."
     severity: error
     given: $.paths..get
     then:
       function: schema
       functionOptions:
-        dialect: "draft2020-12"
         schema:
           type: object
           properties:
@@ -125,12 +133,12 @@ rules:
 
   ##### SORTING #####
   sps-sorting-parameters-only-get-requests:
-    description: "Sorting query parameters SHOULD only be used with GET endpoints."
+    description: "Non-GET endpoints MUST NOT have sorting query parameters. Parameter names such as sort, sorting, orderBy, etc."
     severity: error
     given: $.paths.*[?(@property!='get')].parameters.[?(@.in=='query')]
     then:
       field: "name"
       function: pattern
       functionOptions:
-        notMatch: "^sort|sorting|sort_by|order|ordering|order_by$"
+        notMatch: "^sort|sorting|sortBy|order|ordering|orderBy$"
 
diff --git a/rulesets/test/collections/sps-missing-pagination-query-parameters.test.js b/rulesets/test/collections/sps-missing-pagination-query-parameters.test.js
@@ -9,7 +9,55 @@ describe("sps-missing-pagination-query-parameters", () => {
     spectral = new SpectralTestHarness(ruleset);
   });
 
-  test("valid - GET endpoint has the proper pagination query parameters", async () => {
+  test("invalid - GET endpoint is missing pagination query parameters", async () => {
+    const spec = `
+      openapi: 3.1.0
+      paths:
+        /v1/users:
+          get:
+            summary: Get User by ID
+            parameters:
+            - name: officeLocation
+              in: query
+    `;
+    await spectral.validateFailure(spec, ruleName, "Error", 2);
+  });
+
+  test("valid - rule does not flag query parameters on GET endpoints that searches on a certain id", async () => {
+    const spec = `
+      openapi: 3.1.0
+      paths:
+        /v1/users/{userId}:
+          get:
+            summary: Get User by ID
+            parameters:
+            - name: userId
+              in: path
+              required: true
+            - name: limit
+              in: query
+    `;
+    await spectral.validateSuccess(spec, ruleName);
+  });
+
+  describe("offset based pagination", () => {
+    test("valid - GET endpoint has offset based pagination query parameters", async () => {
+        const spec = `
+          openapi: 3.1.0
+          paths:
+            /v1/users:
+              get:
+                summary: Get User by ID
+                parameters:
+                - name: offset
+                  in: query
+                - name: limit
+                  in: query
+        `;
+        await spectral.validateSuccess(spec, ruleName);
+    });
+
+    test("valid - GET endpoint has offset based pagination and other query parameters", async () => {
       const spec = `
         openapi: 3.1.0
         paths:
@@ -21,13 +69,13 @@ describe("sps-missing-pagination-query-parameters", () => {
                 in: query
               - name: limit
                 in: query
-              - name: cursor
+              - name: officeLocation
                 in: query
       `;
       await spectral.validateSuccess(spec, ruleName);
-  });
-    
-  test("valid - GET endpoint has the proper pagination and other query parameters", async () => {
+    });
+
+    test("invalid - GET endpoint has offset pagination parameters but not limit", async () => {
       const spec = `
         openapi: 3.1.0
         paths:
@@ -37,91 +85,59 @@ describe("sps-missing-pagination-query-parameters", () => {
               parameters:
               - name: offset
                 in: query
-              - name: limit
-                in: query
+      `;
+      await spectral.validateFailure(spec, ruleName, "Error", 1);
+    });
+  });
+
+  describe("cursor based pagination", () => {
+    test("valid - GET endpoint has cursor based pagination query parameters", async () => {
+        const spec = `
+          openapi: 3.1.0
+          paths:
+            /v1/users:
+              get:
+                summary: Get User by ID
+                parameters:
+                - name: cursor
+                  in: query
+                - name: limit
+                  in: query
+        `;
+        await spectral.validateSuccess(spec, ruleName);
+    });
+
+    test("valid - GET endpoint has cursor pagination and other query parameters", async () => {
+      const spec = `
+        openapi: 3.1.0
+        paths:
+          /v1/users:
+            get:
+              summary: Get User by ID
+              parameters:
               - name: cursor
                 in: query
+              - name: limit
+                in: query
               - name: officeLocation
                 in: query
       `;
       await spectral.validateSuccess(spec, ruleName);
-  });
-
-  test("invalid GET endpoint has 2 out of the 3 required pagination query parameters", async () => {
-    const spec = `
-      openapi: 3.1.0
-      paths:
-        /v1/users:
-          get:
-            summary: Get User by ID
-            parameters:
-            - name: offset
-              in: query
-              type: integer
-            - name: limit
-              in: query
-              type: integer
-    `;
-    await spectral.validateFailure(spec, ruleName, "Error", 1);
-  });
-
-  test("invalid - GET endpoint has 2 out of the 3 required pagination query parameters", async () => {
-    const spec = `
-      openapi: 3.1.0
-      paths:
-        /v1/users:
-          get:
-            summary: Get User by ID
-            parameters:
-            - name: offset
-              in: query
-              type: integer
-            - name: limit
-              in: query
-              type: integer
-    `;
-    await spectral.validateFailure(spec, ruleName, "Error", 1);
-  });
-
-  test("invalid - pagination query parameters on GET endpoint", async () => {
-    const spec = `
-      openapi: 3.1.0
-      paths:
-        /v1/users:
-          get:
-            summary: Get User by ID
-            parameters:
-            - name: max
-              in: query
-    `;
-    await spectral.validateFailure(spec, ruleName, "Error", 1);
-  });
+    });
 
-  test("invalid - missing pagination query parameters on GET endpoint", async () => {
-    const spec = `
-      openapi: 3.1.0
-      paths:
-        /v1/users:
-          get:
-            summary: Get User by ID
-    `;
-    await spectral.validateFailure(spec, ruleName, "Error", 1);
+    test("invalid - GET endpoint has cursor pagination parameters but not limit", async () => {
+      const spec = `
+        openapi: 3.1.0
+        paths:
+          /v1/users:
+            get:
+              summary: Get User by ID
+              parameters:
+              - name: cursor
+                in: query
+      `;
+      await spectral.validateFailure(spec, ruleName, "Error", 1);
+    });
   });
 
-  test("valid - rule does not flag query parameters on GET endpoints that searches on a certain id", async () => {
-    const spec = `
-      openapi: 3.1.0
-      paths:
-        /v1/users/{userId}:
-          get:
-            summary: Get User by ID
-            parameters:
-            - name: userId
-              in: path
-              required: true
-            - name: limit
-              in: query
-    `;
-    await spectral.validateSuccess(spec, ruleName);
-  });
 });