Repository: stoplightio/spectral
Pull request #737 (2019-11-05 11:05:34)

chore: extract a result sorting function
From https://github.com/stoplightio/spectral/pull/681#discussion_r334885416

nulltoken: @P0lip Ready for review
nulltoken: @P0lip Thanks for this very thorough review. I think I've dealt with (or discussed) every comment.

Fixup commits have been pushed to make it easier for you to see the introduced changes.

Thoughts?

Patch:
diff --git a/src/cli/services/linter/utils/deduplicateResults.ts b/src/cli/services/linter/utils/deduplicateResults.ts
index ca275a3f..88a76cd0 100644
--- a/src/cli/services/linter/utils/deduplicateResults.ts
+++ b/src/cli/services/linter/utils/deduplicateResults.ts
@@ -1,26 +1,26 @@
-import { IRange } from '@stoplight/types/dist';
+import { compareResults } from '../../../../formatters/utils/sortResults';
 import { IRuleResult } from '../../../../types';
 
-type Dictionary<T, K extends PropertyKey> = { [key in K]: T };
+export const deduplicateResults = (results: IRuleResult[]): IRuleResult[] => {
+  const filtered: IRuleResult[] = [];
 
-const ARTIFICIAL_ROOT = Symbol('root');
+  const totalResults = results.length;
 
-const serializeRange = ({ start, end }: IRange) => `${start.line}:${start.character}:${end.line}:${end.character}`;
-const getIdentifier = (result: IRuleResult) => `${result.path.join('/')}${result.code}${serializeRange(result.range)}`;
+  if (totalResults < 2) {
+    return [...results];
+  }
 
-export const deduplicateResults = (results: IRuleResult[]) => {
-  const seen: Dictionary<Dictionary<string, string>, symbol> = {};
+  const sorted = [...results].sort(compareResults);
 
-  return results.filter(result => {
-    const source = result.source === void 0 ? ARTIFICIAL_ROOT : result.source;
-    const identifier = getIdentifier(result);
-    if (!(source in seen)) {
-      seen[source] = {};
-    } else if (identifier in seen[source]) {
-      return false;
+  filtered.push(sorted[0]);
+
+  for (let i = 1; i < totalResults; i++) {
+    if (compareResults(sorted[i], sorted[i - 1]) === 0) {
+      continue;
     }
 
-    seen[source][identifier] = true;
-    return true;
-  });
+    filtered.push(sorted[i]);
+  }
+
+  return filtered;
 };
diff --git a/src/formatters/utils/sortResults.ts b/src/formatters/utils/sortResults.ts
index b096f768..02c5bd37 100644
--- a/src/formatters/utils/sortResults.ts
+++ b/src/formatters/utils/sortResults.ts
@@ -1,13 +1,79 @@
 import { IRuleResult } from '../../types';
 
-export const sortResults = (results: IRuleResult[]) => {
-  return [...results].sort((resultA, resultB) => {
-    const diff = resultA.range.start.line - resultB.range.start.line;
+const compareCode = (left: string | number | undefined, right: string | number | undefined): number => {
+  if (left === void 0 && right === void 0) {
+    return 0;
+  }
+
+  if (left === void 0) {
+    return -1;
+  }
+
+  if (right === void 0) {
+    return 1;
+  }
+
+  return String(left).localeCompare(String(right), void 0, { numeric: true });
+};
+
+const compareSource = (left: string | undefined, right: string | undefined): number => {
+  if (left === void 0 && right === void 0) {
+    return 0;
+  }
+
+  if (left === void 0) {
+    return -1;
+  }
 
-    if (diff === 0) {
-      return resultA.range.start.character - resultB.range.start.character;
-    }
+  if (right === void 0) {
+    return 1;
+  }
+
+  return left.localeCompare(right);
+};
+
+const normalize = (value: number): -1 | 0 | 1 => {
+  if (value < 0) {
+    return -1;
+  }
+
+  if (value > 0) {
+    return 1;
+  }
+
+  return 0;
+};
 
-    return diff;
-  });
+export const compareResults = (left: IRuleResult, right: IRuleResult): -1 | 0 | 1 => {
+  const diffSource = compareSource(left.source, right.source);
+
+  if (diffSource !== 0) {
+    return normalize(diffSource);
+  }
+
+  const diffLine = left.range.start.line - right.range.start.line;
+
+  if (diffLine !== 0) {
+    return normalize(diffLine);
+  }
+
+  const diffChar = left.range.start.character - right.range.start.character;
+
+  if (diffChar !== 0) {
+    return normalize(diffChar);
+  }
+
+  const diffCode = compareCode(left.code, right.code);
+
+  if (diffCode !== 0) {
+    return normalize(diffCode);
+  }
+
+  const diffPath = left.path.join().localeCompare(right.path.join());
+
+  return normalize(diffPath);
+};
+
+export const sortResults = (results: IRuleResult[]) => {
+  return [...results].sort(compareResults);
 };


Test patch:
diff --git a/src/cli/services/__tests__/linter.test.ts b/src/cli/services/__tests__/linter.test.ts
index 34309bc7..4e5b4856 100644
--- a/src/cli/services/__tests__/linter.test.ts
+++ b/src/cli/services/__tests__/linter.test.ts
@@ -563,6 +563,13 @@ describe('Linter service', () => {
           range: expect.any(Object),
           source: expect.stringContaining('__tests__/__fixtures__/draft-ref.oas2.json'),
         }),
+        expect.objectContaining({
+          code: 'openapi-tags',
+          message: 'OpenAPI object should have non-empty `tags` array.',
+          path: [],
+          range: expect.any(Object),
+          source: expect.stringContaining('__tests__/__fixtures__/draft-ref.oas2.json'),
+        }),
         expect.objectContaining({
           code: 'oas2-schema',
           message: '/info Property foo is not expected to be here',
@@ -595,13 +602,6 @@ describe('Linter service', () => {
           },
           source: expect.stringContaining('__tests__/__fixtures__/refs/info.json'),
         }),
-        expect.objectContaining({
-          code: 'openapi-tags',
-          message: 'OpenAPI object should have non-empty `tags` array.',
-          path: [],
-          range: expect.any(Object),
-          source: expect.stringContaining('__tests__/__fixtures__/draft-ref.oas2.json'),
-        }),
       ]);
     });
 
@@ -614,6 +614,13 @@ describe('Linter service', () => {
           range: expect.any(Object),
           source: expect.stringContaining('__tests__/__fixtures__/draft-nested-ref.oas2.json'),
         }),
+        expect.objectContaining({
+          code: 'openapi-tags',
+          message: 'OpenAPI object should have non-empty `tags` array.',
+          path: [],
+          range: expect.any(Object),
+          source: expect.stringContaining('__tests__/__fixtures__/draft-nested-ref.oas2.json'),
+        }),
         expect.objectContaining({
           code: 'oas2-schema',
           message: "/info should have required property 'title'",
@@ -646,13 +653,6 @@ describe('Linter service', () => {
           },
           source: expect.stringContaining('__tests__/__fixtures__/refs/contact.json'),
         }),
-        expect.objectContaining({
-          code: 'openapi-tags',
-          message: 'OpenAPI object should have non-empty `tags` array.',
-          path: [],
-          range: expect.any(Object),
-          source: expect.stringContaining('__tests__/__fixtures__/draft-nested-ref.oas2.json'),
-        }),
         expect.objectContaining({
           code: 'operation-description',
           message: 'Operation `description` must be present and non-empty string.',
diff --git a/src/cli/services/linter/utils/__tests__/deduplicateResults.spec.ts b/src/cli/services/linter/utils/__tests__/deduplicateResults.spec.ts
index b0cd2ee1..bdf4d024 100644
--- a/src/cli/services/linter/utils/__tests__/deduplicateResults.spec.ts
+++ b/src/cli/services/linter/utils/__tests__/deduplicateResults.spec.ts
@@ -6,10 +6,10 @@ describe('deduplicateResults util', () => {
   it('deduplicate exact validation results', () => {
     expect(deduplicateResults(duplicateValidationResults)).toEqual([
       expect.objectContaining({
-        code: 'valid-schema-example-in-content',
+        code: 'valid-example-in-schemas',
       }),
       expect.objectContaining({
-        code: 'valid-example-in-schemas',
+        code: 'valid-schema-example-in-content',
       }),
     ]);
   });
@@ -22,11 +22,22 @@ describe('deduplicateResults util', () => {
 
     expect(deduplicateResults(duplicateValidationResultsWithNoSource)).toEqual([
       expect.objectContaining({
-        code: 'valid-schema-example-in-content',
+        code: 'valid-example-in-schemas',
       }),
       expect.objectContaining({
-        code: 'valid-example-in-schemas',
+        code: 'valid-schema-example-in-content',
       }),
     ]);
   });
+
+  it('deduplicate list of only duplicates', () => {
+    const onlyDuplicates = [
+      { ...duplicateValidationResults[0] },
+      { ...duplicateValidationResults[0] },
+      { ...duplicateValidationResults[0] },
+      { ...duplicateValidationResults[0] },
+    ];
+
+    expect(deduplicateResults(onlyDuplicates).length).toBe(1);
+  });
 });
diff --git a/src/formatters/__tests__/sortResults.test.ts b/src/formatters/__tests__/sortResults.test.ts
new file mode 100644
index 00000000..59250077
--- /dev/null
+++ b/src/formatters/__tests__/sortResults.test.ts
@@ -0,0 +1,206 @@
+import { DiagnosticSeverity } from '@stoplight/types';
+import { IRuleResult } from '../../types';
+import { compareResults, sortResults } from '../utils/sortResults';
+
+const results: IRuleResult[] = [
+  {
+    code: 'code 01',
+    path: ['a', 'b', 'c', 'd'],
+    source: 'source 01',
+    range: {
+      start: { line: 1, character: 1 },
+      end: { line: 99, character: 99 },
+    },
+    message: '99',
+    severity: DiagnosticSeverity.Error, // or any other level, it's irrelevant
+  },
+  {
+    code: 'code 01',
+    path: ['a', 'b', 'c', 'd'],
+    source: 'source 02',
+    range: {
+      start: { line: 1, character: 1 },
+      end: { line: 99, character: 99 },
+    },
+    message: '99',
+    severity: DiagnosticSeverity.Error, // or any other level, it's irrelevant
+  },
+  {
+    code: 'code 01',
+    path: ['a', 'b', 'c', 'd'],
+    source: 'source 02',
+    range: {
+      start: { line: 2, character: 1 },
+      end: { line: 99, character: 99 },
+    },
+    message: '99',
+    severity: DiagnosticSeverity.Error, // or any other level, it's irrelevant
+  },
+  {
+    code: 'code 01',
+    path: ['a', 'b', 'c', 'd'],
+    source: 'source 02',
+    range: {
+      start: { line: 2, character: 2 },
+      end: { line: 99, character: 99 },
+    },
+    message: '99',
+    severity: DiagnosticSeverity.Error, // or any other level, it's irrelevant
+  },
+  {
+    code: 'code 02',
+    path: ['a', 'b', 'c', 'd'],
+    source: 'source 02',
+    range: {
+      start: { line: 2, character: 2 },
+      end: { line: 99, character: 99 },
+    },
+    message: '99',
+    severity: DiagnosticSeverity.Error, // or any other level, it's irrelevant
+  },
+  {
+    code: 'code 02',
+    path: ['a', 'b', 'c', 'e'],
+    source: 'source 02',
+    range: {
+      start: { line: 2, character: 2 },
+      end: { line: 99, character: 99 },
+    },
+    message: '99',
+    severity: DiagnosticSeverity.Error, // or any other level, it's irrelevant
+  },
+  {
+    code: 'code 02',
+    path: ['a', 'b', 'c', 'f'],
+    source: 'source 02',
+    range: {
+      start: { line: 2, character: 2 },
+      end: { line: 99, character: 99 },
+    },
+    message: '99',
+    severity: DiagnosticSeverity.Error, // or any other level, it's irrelevant
+  },
+  {
+    code: 'code 03',
+    path: ['a', 'b', 'c', 'f'],
+    source: 'source 02',
+    range: {
+      start: { line: 2, character: 2 },
+      end: { line: 99, character: 99 },
+    },
+    message: '99',
+    severity: DiagnosticSeverity.Error, // or any other level, it's irrelevant
+  },
+  {
+    code: 'code 03',
+    path: ['a', 'b', 'c', 'f'],
+    source: 'source 02',
+    range: {
+      start: { line: 2, character: 3 },
+      end: { line: 99, character: 99 },
+    },
+    message: '99',
+    severity: DiagnosticSeverity.Error, // or any other level, it's irrelevant
+  },
+  {
+    code: 'code 03',
+    path: ['a', 'b', 'c', 'f'],
+    source: 'source 02',
+    range: {
+      start: { line: 3, character: 3 },
+      end: { line: 99, character: 99 },
+    },
+    message: '99',
+    severity: DiagnosticSeverity.Error, // or any other level, it's irrelevant
+  },
+  {
+    code: 'code 03',
+    path: ['a', 'b', 'c', 'f'],
+    source: 'source 03',
+    range: {
+      start: { line: 3, character: 3 },
+      end: { line: 99, character: 99 },
+    },
+    message: '99',
+    severity: DiagnosticSeverity.Error, // or any other level, it's irrelevant
+  },
+];
+
+describe('sortResults', () => {
+  const shuffleBy = (arr: IRuleResult[], indices: number[]): IRuleResult[] => {
+    expect(indices).toHaveLength(arr.length);
+
+    const shuffled = results
+      .map((v, i) => ({ ...v, pos: indices[i] }))
+      .sort((a, b) => a.pos - b.pos)
+      .map((v, i) => {
+        delete v.pos;
+        return v;
+      });
+
+    return shuffled;
+  };
+
+  test('should properly order results', () => {
+    const randomlySortedIndices = [5, 4, 1, 10, 8, 6, 3, 9, 2, 0, 7];
+
+    const shuffled = shuffleBy(results, randomlySortedIndices);
+
+    expect(sortResults(shuffled)).toEqual(results);
+  });
+});
+
+describe('compareResults', () => {
+  test('should properly order results source', () => {
+    const input = {
+      code: 'code 01',
+      path: ['a', 'b', 'c', 'd'],
+      range: {
+        start: { line: 1, character: 1 },
+        end: { line: 99, character: 99 },
+      },
+      message: '99',
+      severity: DiagnosticSeverity.Error, // or any other level, it's irrelevant
+    };
+
+    [
+      { one: undefined, another: undefined, expected: 0 },
+      { one: 'a', another: undefined, expected: 1 },
+      { one: undefined, another: 'a', expected: -1 },
+      { one: 'a', another: 'a', expected: 0 },
+      { one: 'a', another: 'b', expected: -1 },
+    ].forEach(tc => {
+      expect(compareResults({ ...input, source: tc.one }, { ...input, source: tc.another })).toEqual(tc.expected);
+    });
+  });
+
+  test('should properly order results code', () => {
+    const input = {
+      source: 'somewhere',
+      path: ['a', 'b', 'c', 'd'],
+      range: {
+        start: { line: 1, character: 1 },
+        end: { line: 99, character: 99 },
+      },
+      message: '99',
+      severity: DiagnosticSeverity.Error, // or any other level, it's irrelevant
+    };
+
+    [
+      { one: undefined, another: undefined, expected: 0 },
+      { one: 'a', another: undefined, expected: 1 },
+      { one: undefined, another: 'a', expected: -1 },
+      { one: 'a', another: 'a', expected: 0 },
+      { one: 'a', another: 'b', expected: -1 },
+      { one: '2', another: '10', expected: -1 },
+      { one: 1, another: undefined, expected: 1 },
+      { one: undefined, another: 1, expected: -1 },
+      { one: 1, another: 1, expected: 0 },
+      { one: 1, another: 2, expected: -1 },
+      { one: 1, another: '1', expected: 0 },
+      { one: 2, another: '10', expected: -1 },
+    ].forEach(tc => {
+      expect(compareResults({ ...input, code: tc.one }, { ...input, code: tc.another })).toEqual(tc.expected);
+    });
+  });
+});