Repository: stoplightio/spectral (ts)

Issue:
Path Parameter Definitions Not Required Across Operations
**Describe the bug**
The `path-params` rule is not triggered when a path parameter is defined under one operation but not in another, both of which are under the same path item.

**To Reproduce**
Given the following OpenAPI document:

```yaml
openapi: 3.0.3
info:
  title: API Title
  version: '1.0'
paths:
  /test/{foo}:
    get:
      parameters:
        - name: foo
          in: path
          required: true
          schema:
            type: string
      responses:
        '200':
          description: OK
    put:
      responses:
        '200':
          description: OK
```

I get this when linting:

```
$ spectral lint .\openapi.yaml
OpenAPI 3.x detected

c:/dev/openapi.yaml
  1:1  warning  oas3-api-servers       OpenAPI `servers` must be present and non-empty array.
  1:1  warning  openapi-tags           OpenAPI object should have non-empty `tags` array.
  2:6  warning  info-contact           Info object should contain `contact` object.
  2:6  warning  info-description       OpenAPI object info `description` must be present and non-empty string.
  7:9  warning  operation-description  Operation `description` must be present and non-empty string.
  7:9  warning  operation-operationId  Operation should have an `operationId`.
  7:9  warning  operation-tags         Operation should have non-empty `tags` array.
 17:9  warning  operation-description  Operation `description` must be present and non-empty string.
 17:9  warning  operation-operationId  Operation should have an `operationId`.
 17:9  warning  operation-tags         Operation should have non-empty `tags` array.

✖ 10 problems (0 errors, 10 warnings, 0 infos, 0 hints)
```

**Expected behavior**
This should have triggered the `path-params` rule for the `put`, which is what I get without the parameter definition in the `get`.

```
6:15    error  path-params            The path `/test/{foo}` uses a parameter `{foo}` that does not have a corresponding definition.
```

**Environment:**
 - Library version: 5.3.0
 - OS: Windows 10

Pull request:
fix(rule): make `path-params` oas rule detect undefined parameters accross operations
Fixes #1076.

**Checklist**

- [x] Tests added / updated
- [ ] Docs added / updated

**Does this PR introduce a breaking change?**

- [ ] Yes
- [x] No

This also brings
- extended test coverage
- more precise reported paths
- a fix to the detection of duplicated parameters algorithm

Patch:
diff --git a/src/rulesets/oas/functions/oasPathParam.ts b/src/rulesets/oas/functions/oasPathParam.ts
index ac908550..a18b7d03 100644
--- a/src/rulesets/oas/functions/oasPathParam.ts
+++ b/src/rulesets/oas/functions/oasPathParam.ts
@@ -1,7 +1,89 @@
+import { Segment } from '@stoplight/types';
 import { IFunction, IFunctionResult, Rule } from '../../../types';
 
 const pathRegex = /(\{[a-zA-Z0-9_-]+\})+/g;
 
+interface IParam {
+  in?: string;
+  name?: string;
+  required?: boolean;
+}
+
+interface INamedPathParam {
+  in: 'path';
+  name: string;
+  required?: boolean;
+}
+
+const isNamedPathParam = (p: IParam): p is INamedPathParam => {
+  return p.in !== void 0 && p.in === 'path' && p.name !== void 0;
+};
+
+const isUnknownNamedPathParam = (
+  p: IParam,
+  path: Segment[],
+  results: IFunctionResult[],
+  seens: Array<Record<string, unknown>>,
+): p is INamedPathParam => {
+  if (!isNamedPathParam(p)) {
+    return false;
+  }
+
+  if (!p.required) {
+    results.push(generateResult(requiredMessage(p.name), path));
+  }
+
+  for (const seen of seens) {
+    if (p.name in seen) {
+      results.push(generateResult(uniqueDefinitionMessage(p.name), path));
+      return false;
+    }
+  }
+
+  return true;
+};
+
+const ensureAllDefinedPathParamsAreUsedInPath = (
+  path: string,
+  params: Record<string, Segment[]>,
+  expected: Record<string, unknown>,
+  results: IFunctionResult[],
+) => {
+  for (const p in params) {
+    if (!params[p]) {
+      continue;
+    }
+
+    if (!(p in expected)) {
+      const resPath = params[p];
+      results.push(generateResult(`Parameter \`${p}\` is not used in the path \`${path}\`.`, resPath));
+    }
+  }
+};
+
+const ensureAllExpectedParamsinPathAreDefined = (
+  path: string,
+  params: Record<string, Segment[]>,
+  expected: Record<string, unknown>,
+  operationPath: Segment[],
+  results: IFunctionResult[],
+) => {
+  for (const p in expected) {
+    if (!expected[p]) {
+      continue;
+    }
+
+    if (!(p in params)) {
+      results.push(
+        generateResult(
+          `The operation does not define the parameter \`{${p}}\` expected by path \`${path}\`.`,
+          operationPath,
+        ),
+      );
+    }
+  }
+};
+
 export const oasPathParam: IFunction<Rule> = (targetVal, _options, paths, vals) => {
   const results: IFunctionResult[] = [];
 
@@ -62,28 +144,18 @@ export const oasPathParam: IFunction<Rule> = (targetVal, _options, paths, vals)
     }
 
     // find parameters set within the top-level 'parameters' object
-    const topParams: object = {};
-    if (object.paths[path].parameters) {
-      for (const p of object.paths[path].parameters) {
-        if (p.in && p.in === 'path' && p.name) {
-          if (!p.required) {
-            results.push(generateResult(requiredMessage(p.name), [...paths.given, 'paths', path, 'parameters']));
-          }
-
-          if (topParams[p.name]) {
-            // name has already been specified
-            results.push(
-              generateResult(uniqueDefinitionMessage(p.name), [...paths.given, 'paths', path, 'parameters']),
-            );
-            continue;
-          }
-          topParams[p.name] = [...paths.given, 'paths', path, 'parameters'];
-        }
+    const topParams = {};
+    for (const i in object.paths[path].parameters) {
+      if (!object.paths[path].parameters[i]) continue;
+
+      const p: IParam = object.paths[path].parameters[i];
+      const fullParameterPath = [...paths.given, 'paths', path, 'parameters', i];
+
+      if (isUnknownNamedPathParam(p, fullParameterPath, results, [topParams])) {
+        topParams[p.name] = fullParameterPath;
       }
     }
 
-    // find parameters set at the operation level
-    const operationParams = {};
     for (const op in object.paths[path]) {
       if (!object.paths[path][op]) continue;
 
@@ -91,63 +163,25 @@ export const oasPathParam: IFunction<Rule> = (targetVal, _options, paths, vals)
         continue;
       }
 
+      const operationParams = {};
       const parameters = object.paths[path][op].parameters;
-      if (parameters) {
-        // temporary store for tracking parameters specified across operations (to make sure
-        // parameters are not defined multiple times under the same operation)
-        const tmp = {};
-
-        for (const i in parameters) {
-          if (!parameters.hasOwnProperty(i)) continue;
-
-          const p = parameters[i];
-          if (p.in && p.in === 'path' && p.name) {
-            const parameterPath = ['paths', path, op, 'parameters', i];
-            if (!p.required) {
-              results.push(generateResult(requiredMessage(p.name), [...paths.given, ...parameterPath]));
-            }
-
-            if (tmp[p.name]) {
-              results.push(generateResult(uniqueDefinitionMessage(p.name), [...paths.given, ...parameterPath]));
-              continue;
-            } else if (operationParams[p.name]) {
-              continue;
-            }
-
-            tmp[p.name] = {};
-            operationParams[p.name] = parameterPath;
-          }
-        }
-      }
-    }
 
-    // verify templated path elements are present in either top-level or operation parameters
-    for (const p in pathElements) {
-      if (!pathElements[p]) continue;
-
-      if (!topParams[p] && !operationParams[p]) {
-        results.push(
-          generateResult(
-            `The path \`${path}\` uses a parameter \`{${p}}\` that does not have a corresponding definition.`,
-            [...paths.given, 'paths', path],
-          ),
-        );
-      }
-    }
+      const operationPath = [...paths.given, 'paths', path, op];
 
-    // verify parameters defined in either top-level or operation parameters are set in path
-    // template
-    for (const paramObj of [topParams, operationParams]) {
-      for (const p in paramObj) {
-        if (!paramObj[p]) continue;
+      for (const i in parameters) {
+        if (!parameters.hasOwnProperty(i)) continue;
 
-        if (!pathElements[p]) {
-          const resPath = paramObj[p];
-          results.push(
-            generateResult(`Parameter \`${p}\` is not used in the path \`${path}\`.`, [...paths.given, ...resPath]),
-          );
+        const p: IParam = parameters[i];
+        const fullParameterPath = [...operationPath, 'parameters', i];
+
+        if (isUnknownNamedPathParam(p, fullParameterPath, results, [topParams, operationParams])) {
+          operationParams[p.name] = fullParameterPath;
         }
       }
+
+      const definedParams = { ...topParams, ...operationParams };
+      ensureAllDefinedPathParamsAreUsedInPath(path, definedParams, pathElements, results);
+      ensureAllExpectedParamsinPathAreDefined(path, definedParams, pathElements, operationPath, results);
     }
   }
 


Test patch:
diff --git a/src/__tests__/spectral.jest.test.ts b/src/__tests__/spectral.jest.test.ts
index 0165c031..91250d53 100644
--- a/src/__tests__/spectral.jest.test.ts
+++ b/src/__tests__/spectral.jest.test.ts
@@ -154,15 +154,15 @@ describe('Spectral', () => {
         }),
         expect.objectContaining({
           code: 'path-params',
-          path: ['paths', '/todos/{todoId}'],
+          path: ['paths', '/todos/{todoId}', 'get'],
           range: {
             end: {
-              character: 5,
-              line: 34,
+              character: 7,
+              line: 33,
             },
             start: {
-              character: 23,
-              line: 10,
+              character: 13,
+              line: 11,
             },
           },
           source: documentUri,
diff --git a/src/rulesets/oas/functions/__tests__/oasPathParam.test.ts b/src/rulesets/oas/functions/__tests__/oasPathParam.test.ts
index ba8c02bc..931f5f00 100644
--- a/src/rulesets/oas/functions/__tests__/oasPathParam.test.ts
+++ b/src/rulesets/oas/functions/__tests__/oasPathParam.test.ts
@@ -14,14 +14,15 @@ describe('oasPathParam', () => {
   });
 
   test('No error if templated path is not used', async () => {
-    const results = s.run({
+    const results = await s.run({
       paths: {
         '/foo': {
           get: {},
         },
       },
     });
-    expect(results).resolves.toHaveLength(0);
+
+    expect(results).toHaveLength(0);
   });
 
   test('Error if no path parameter definition', async () => {
@@ -32,19 +33,20 @@ describe('oasPathParam', () => {
         },
       },
     });
+
     expect(results).toEqual([
       {
         code: 'path-params',
-        message: 'The path `/foo/{bar}` uses a parameter `{bar}` that does not have a corresponding definition.',
-        path: ['paths', '/foo/{bar}'],
+        message: 'The operation does not define the parameter `{bar}` expected by path `/foo/{bar}`.',
+        path: ['paths', '/foo/{bar}', 'get'],
         range: {
           end: {
             character: 15,
             line: 3,
           },
           start: {
-            character: 17,
-            line: 2,
+            character: 12,
+            line: 3,
           },
         },
         severity: DiagnosticSeverity.Error,
@@ -53,7 +55,7 @@ describe('oasPathParam', () => {
   });
 
   test('No error if path parameter definition is used (at the path level)', async () => {
-    const results = s.run({
+    const results = await s.run({
       paths: {
         '/foo/{bar}': {
           parameters: [
@@ -67,11 +69,12 @@ describe('oasPathParam', () => {
         },
       },
     });
-    expect(results).resolves.toHaveLength(0);
+
+    expect(results).toHaveLength(0);
   });
 
   test('No error if $ref path parameter definition is used (at the path level)', async () => {
-    const results = s.run({
+    const results = await s.run({
       paths: {
         '/foo/{bar}': {
           parameters: [
@@ -90,11 +93,12 @@ describe('oasPathParam', () => {
         },
       },
     });
-    expect(results).resolves.toHaveLength(0);
+
+    expect(results).toHaveLength(0);
   });
 
   test('No error if path parameter definition is set (at the operation level)', async () => {
-    const results = s.run({
+    const results = await s.run({
       paths: {
         '/foo/{bar}': {
           get: {
@@ -109,7 +113,36 @@ describe('oasPathParam', () => {
         },
       },
     });
-    expect(results).resolves.toHaveLength(0);
+
+    expect(results).toHaveLength(0);
+  });
+
+  test('Error if path parameter definition is set (at the operation level) for a method, but forgotten for another one', async () => {
+    const results = await s.run({
+      paths: {
+        '/foo/{bar}': {
+          get: {
+            parameters: [
+              {
+                name: 'bar',
+                in: 'path',
+                required: true,
+              },
+            ],
+          },
+          put: {},
+        },
+      },
+    });
+
+    expect(results).toEqual([
+      expect.objectContaining({
+        code: 'path-params',
+        message: 'The operation does not define the parameter `{bar}` expected by path `/foo/{bar}`.',
+        path: ['paths', '/foo/{bar}', 'put'],
+        severity: DiagnosticSeverity.Error,
+      }),
+    ]);
   });
 
   test('Error if duplicate path parameters with same name are used', async () => {
@@ -127,6 +160,7 @@ describe('oasPathParam', () => {
         },
       },
     });
+
     expect(results).toEqual([
       {
         code: 'path-params',
@@ -172,15 +206,15 @@ describe('oasPathParam', () => {
       {
         code: 'path-params',
         message: `Path parameter \`bar\` must have a \`required\` property that is set to \`true\`.`,
-        path: ['paths', '/foo/{bar}', 'parameters'],
+        path: ['paths', '/foo/{bar}', 'parameters', '0'],
         range: {
           end: {
             character: 42,
             line: 5,
           },
           start: {
-            character: 19,
-            line: 3,
+            character: 8,
+            line: 4,
           },
         },
         severity: DiagnosticSeverity.Error,
@@ -188,6 +222,38 @@ describe('oasPathParam', () => {
     ]);
   });
 
+  test('Error if $ref operation parameter definition is not required', async () => {
+    const results = await s.run({
+      paths: {
+        '/foo/{bar}': {
+          get: {
+            parameters: [
+              {
+                $ref: '#/definitions/barParam',
+              },
+            ],
+          },
+        },
+      },
+      definitions: {
+        barParam: {
+          name: 'bar',
+          in: 'path',
+          required: false,
+        },
+      },
+    });
+
+    expect(results).toEqual([
+      expect.objectContaining({
+        code: 'path-params',
+        message: `Path parameter \`bar\` must have a \`required\` property that is set to \`true\`.`,
+        path: ['paths', '/foo/{bar}', 'get', 'parameters', '0'],
+        severity: DiagnosticSeverity.Error,
+      }),
+    ]);
+  });
+
   test('Error if paths are functionally equivalent', async () => {
     const results = await s.run({
       paths: {
@@ -233,4 +299,171 @@ describe('oasPathParam', () => {
       },
     ]);
   });
+
+  test('Error if path parameter definition is set (at the global and/or operation level), but unused', async () => {
+    const results = await s.run({
+      paths: {
+        '/foo': {
+          parameters: [
+            {
+              name: 'boo',
+              in: 'path',
+              required: true,
+            },
+          ],
+          get: {
+            parameters: [
+              {
+                name: 'bar',
+                in: 'path',
+                required: true,
+              },
+            ],
+          },
+          put: {
+            parameters: [
+              {
+                name: 'baz',
+                in: 'path',
+                required: true,
+              },
+              {
+                name: 'qux',
+                in: 'path',
+                required: true,
+              },
+            ],
+          },
+        },
+      },
+    });
+
+    expect(results).toEqual([
+      expect.objectContaining({
+        code: 'path-params',
+        message: 'Parameter `boo` is not used in the path `/foo`.',
+        path: ['paths', '/foo', 'parameters', '0'],
+        severity: DiagnosticSeverity.Error,
+      }),
+      expect.objectContaining({
+        code: 'path-params',
+        message: 'Parameter `bar` is not used in the path `/foo`.',
+        path: ['paths', '/foo', 'get', 'parameters', '0'],
+        severity: DiagnosticSeverity.Error,
+      }),
+      expect.objectContaining({
+        code: 'path-params',
+        message: 'Parameter `baz` is not used in the path `/foo`.',
+        path: ['paths', '/foo', 'put', 'parameters', '0'],
+        severity: DiagnosticSeverity.Error,
+      }),
+      expect.objectContaining({
+        code: 'path-params',
+        message: 'Parameter `qux` is not used in the path `/foo`.',
+        path: ['paths', '/foo', 'put', 'parameters', '1'],
+        severity: DiagnosticSeverity.Error,
+      }),
+    ]);
+  });
+
+  test('Error if path parameter are defined multiple times', async () => {
+    const results = await s.run({
+      paths: {
+        '/foo/{boo}/{bar}/{qux}': {
+          parameters: [
+            {
+              name: 'boo',
+              in: 'path',
+              required: true,
+            },
+            {
+              name: 'boo',
+              in: 'path',
+              required: true,
+            },
+            {
+              name: 'bar',
+              in: 'path',
+              required: true,
+            },
+          ],
+          get: {
+            parameters: [
+              {
+                name: 'bar',
+                in: 'path',
+                required: true,
+              },
+              {
+                name: 'qux',
+                in: 'path',
+                required: true,
+              },
+            ],
+          },
+          put: {
+            parameters: [
+              {
+                name: 'qux',
+                in: 'path',
+                required: true,
+              },
+              {
+                name: 'qux',
+                in: 'path',
+                required: true,
+              },
+            ],
+          },
+        },
+      },
+    });
+
+    expect(results).toEqual([
+      expect.objectContaining({
+        code: 'path-params',
+        message: 'Path parameter `boo` is defined multiple times. Path parameters must be unique.',
+        path: ['paths', '/foo/{boo}/{bar}/{qux}', 'parameters', '1'],
+        severity: DiagnosticSeverity.Error,
+      }),
+      expect.objectContaining({
+        code: 'path-params',
+        message: 'Path parameter `bar` is defined multiple times. Path parameters must be unique.',
+        path: ['paths', '/foo/{boo}/{bar}/{qux}', 'get', 'parameters', '0'],
+        severity: DiagnosticSeverity.Error,
+      }),
+      expect.objectContaining({
+        code: 'path-params',
+        message: 'Path parameter `qux` is defined multiple times. Path parameters must be unique.',
+        path: ['paths', '/foo/{boo}/{bar}/{qux}', 'put', 'parameters', '1'],
+        severity: DiagnosticSeverity.Error,
+      }),
+    ]);
+  });
+
+  test('No error if two parameters bear the same name but target different locations', async () => {
+    const results = await s.run({
+      paths: {
+        '/foo/{boo}': {
+          parameters: [
+            {
+              name: 'boo',
+              in: 'path',
+              required: true,
+            },
+          ],
+          get: {
+            parameters: [
+              {
+                name: 'boo',
+                in: 'header',
+              },
+            ],
+          },
+        },
+      },
+    });
+
+    expect(results).toEqual([]);
+  });
 });