feat: rework error filtering to preserve sibling additionalProperties errors #1433

mkistler · 2020-12-30T16:14:35Z

This PR makes some minor modifications to the filtering logic of errors returned by AJV so that multiple additionalProperties errors within the same object are preserved and presented to the user.

Checklist

Tests added / updated
Docs added / updated - NA

Does this PR introduce a breaking change?

Yes
No

Additional context

The original motivation for this fix is this issue opened against our validator, which (as of recently) incorporates Spectral. My first thought was to add a custom rule to catch this case, but then realized that it should really be caught by the oas3-schema validation. But further investigation showed that oas3-schema discarded errors after the first one encountered, and also was imprecise in the location of the error -- pointing to "responses" -- the parent object -- rather than directly to the offending property.

Figuring out how to fix this was a challenge! There are so many layers of filtering done on errors from AJV. There's the filtering done in prepareResults, then later filtering done in better-ajv-errors, and then filtering done over all errors with computeFingerprint. I made my change at the lowest level (prepareResults) because a) something had to be done there to prevent these from being filtered out there, and b) a small change there ensures that these errors are preserved through all the additional levels of filtering.

I also fixed the way validators are cached to respect the allErrors argument -- a bug in its own right I believe.

There are numerous small changes to the tests -- some are obvious and others less so. I'll try to comment on these below to explain why I think the change is acceptable.

src/__tests__/linter.test.ts

mkistler · 2020-12-30T16:24:34Z

src/__tests__/linter.test.ts

@@ -727,7 +728,7 @@ responses:: !!foo
      }),
      expect.objectContaining({
        code: 'oas3-schema',
-        message: 'Property `type` is not expected to be here.',
+        message: '`header-1` property should have required property `schema`.',


Same situation here as above.

P0lip · 2020-12-30T16:24:55Z

Thanks a lot for submitting the PR.
The change looks good at first glance! It may turn out that the change is worth porting over to @stoplight/better-ajv-errors, but we shall see. Let me know when you think the PR is good for a review from our side.
better-ajv-errors has a dedicated error type for additionalProperty -> https://github.com/stoplightio/better-ajv-errors/blob/master/src/validation-errors/additional-prop.js.

There are so many layers of filtering done on errors from AJV.

Yeah. We in fact have some heavy filtering of errors reported by ajv in case of oas2/oas3-schemas rules.
It's because we pass these huge oas2/oas3 schemas that tend to produce dozens of errors at times.

I also fixed the way validators are cached to respect the allErrors argument -- a bug in its own right I believe.

Yup, right. It's an issue. Good spot. Thanks

src/__tests__/spectral.jest.test.ts

src/cli/services/__tests__/linter.test.ts

philsturgeon · 2020-12-31T10:08:58Z

@mkistler thank you so much, this is excellent!

@P0lip I'll leave it to you to figure out how to get this in, ported to AJV or merged directly, all up to you.

mkistler · 2021-01-07T13:19:28Z

@P0lip I've rebased my code onto the latest develop. Please let me know if there are any other changes you think are needed.

P0lip · 2021-01-08T00:47:42Z

@mkistler could you resolve conflicts in tests? I'll take a look as soon as that's done.

P0lip · 2021-01-12T23:40:09Z

Hey! Just letting you know the PR is on my radar. I'm going review it on Wednesday.

P0lip

One minor comment + a single merge conflict to solve.
Otherwise, good to merge!
Thanks a lot for the contribution.

src/rulesets/oas/functions/oasDocumentSchema.ts

…errors

mkistler · 2021-01-14T02:12:00Z

I've merged your suggestion and addressed the merge conflict.

P0lip

Thank you!

P0lip · 2021-01-14T03:59:19Z

ah crap, we need to update 2 test harness test cases.
Could you be so kind and do that as well?

yarn build.binary
yarn test.harness # this run tests that verify spectral binary

It's totally fine if you're busy. In such case, I can create a git patch you could apply and push.
Just LMK. You've already done a lot, so I wouldn't like to add more stuff onto your plate.

P0lip · 2021-01-14T04:07:11Z

I went ahead and created a patch.

From 6f830363bdb3223c2274ba7a419a733d8879c2f6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jakub=20Ro=C5=BCek?= <[email protected]>
Date: Thu, 14 Jan 2021 05:06:39 +0100
Subject: [PATCH] test: update harness scenarios

---
 test-harness/scenarios/oas3-schema.scenario              | 9 +++++----
 .../scenarios/results-format-junit.oas3.scenario         | 2 +-
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/test-harness/scenarios/oas3-schema.scenario b/test-harness/scenarios/oas3-schema.scenario
index 3880b064..097cd353 100644
--- a/test-harness/scenarios/oas3-schema.scenario
+++ b/test-harness/scenarios/oas3-schema.scenario
@@ -57,13 +57,14 @@ rules:
 OpenAPI 3.x detected
 
 {document}
-  5:11  error  oas3-schema  Property `foo` is not expected to be here.                     info.contact
- 12:11  error  oas3-schema  Property `type` is not expected to be here.                    paths./user.get.parameters[0]
+  6:10  error  oas3-schema  Property `foo` is not expected to be here.                     info.contact.foo
+ 12:17  error  oas3-schema  Property `type` is not expected to be here.                    paths./user.get.parameters[0].type
  23:18  error  oas3-schema  `type` property type should be string.                         paths./user.get.parameters[1].schema.type
- 24:11  error  oas3-schema  Property `type` is not expected to be here.                    paths./user.get.parameters[2]
+ 24:11  error  oas3-schema  `2` property should have required property `schema`.           paths./user.get.parameters[2]
+ 26:17  error  oas3-schema  Property `type` is not expected to be here.                    paths./user.get.parameters[2].type
  37:28  error  oas3-schema  `user_id` property type should be object.                      paths./user.get.responses[200].content.application/json.schema.properties.user_id
  41:28  error  oas3-schema  `properties` property type should be object.                   paths./user.get.responses[200].content.application/yaml.schema.properties
  43:23  error  oas3-schema  `description` property type should be string.                  paths./user.get.responses[400].description
  46:17  error  oas3-schema  `responses` property should not have fewer than 1 properties.  paths./address.get.responses
 
-✖ 8 problems (8 errors, 0 warnings, 0 infos, 0 hints)
+✖ 9 problems (9 errors, 0 warnings, 0 infos, 0 hints)
diff --git a/test-harness/scenarios/results-format-junit.oas3.scenario b/test-harness/scenarios/results-format-junit.oas3.scenario
index e0d3bf78..c34028d9 100644
--- a/test-harness/scenarios/results-format-junit.oas3.scenario
+++ b/test-harness/scenarios/results-format-junit.oas3.scenario
@@ -14,6 +14,6 @@ OpenAPI 3.x detected
 <?xml version="1.0" encoding="utf-8"?>
 <testsuites>
 <testsuite package="org.spectral" time="0" tests="1" errors="0" failures="1" name="{document}">
-<testcase time="0" name="org.spectral.oas3-schema" classname="{document|no-ext}"><failure message="Property `foo` is not expected to be here."><![CDATA[line 2, col 6, Property `foo` is not expected to be here. (oas3-schema) at path #/info]]></failure></testcase>
+<testcase time="0" name="org.spectral.oas3-schema" classname="{document|no-ext}"><failure message="Property `foo` is not expected to be here."><![CDATA[line 5, col 7, Property `foo` is not expected to be here. (oas3-schema) at path #/info/foo]]></failure></testcase>
 </testsuite>
 </testsuites>
-- 
2.30.0

mkistler · 2021-01-14T13:29:21Z

Thanks for the patch. I have applied, retested (all pass), and pushed to my branch.

…s-errors

mkistler · 2021-02-14T19:18:53Z

@P0lip What is the plan for this PR making it into an official release?

P0lip · 2021-02-19T10:34:50Z

Hey @mkistler
It's going to be released in the next major version, that is 6.0.
Unfortunately, I don't know when exactly this is going to happen, but hopefully in Q1/Q2.
Is it an urgent change for you or something that can wait a bit?

… errors (#1433) * fix: rework error filtering to preserve sibling additionalProperties errors * test: update harness scenarios Co-authored-by: Jakub Rożek <[email protected]>

mkistler commented Dec 30, 2020

View reviewed changes

src/__tests__/linter.test.ts Outdated Show resolved Hide resolved

mkistler commented Dec 30, 2020

View reviewed changes

src/__tests__/spectral.jest.test.ts Show resolved Hide resolved

mkistler commented Dec 30, 2020

View reviewed changes

src/cli/services/__tests__/linter.test.ts Outdated Show resolved Hide resolved

mkistler commented Dec 30, 2020

View reviewed changes

src/cli/services/__tests__/linter.test.ts Show resolved Hide resolved

philsturgeon previously approved these changes Dec 31, 2020

View reviewed changes

mkistler dismissed philsturgeon’s stale review via 98f0271 January 7, 2021 13:11

mkistler force-pushed the mdk/preserve-sibling-additional-properties-errors branch from a6d4981 to 98f0271 Compare January 7, 2021 13:11

mkistler force-pushed the mdk/preserve-sibling-additional-properties-errors branch 2 times, most recently from 5227d28 to 9df817b Compare January 8, 2021 04:32

mkistler requested a review from P0lip January 11, 2021 21:16

P0lip changed the title ~~Rework error filtering to preserve sibling additionalProperties errors~~ feat: rework error filtering to preserve sibling additionalProperties errors Jan 13, 2021

P0lip assigned mkistler Jan 13, 2021

P0lip added the enhancement New feature or request label Jan 13, 2021

P0lip reviewed Jan 13, 2021

View reviewed changes

src/rulesets/oas/functions/oasDocumentSchema.ts Outdated Show resolved Hide resolved

fix: rework error filtering to preserve sibling additionalProperties …

fe1f7c3

…errors

mkistler force-pushed the mdk/preserve-sibling-additional-properties-errors branch from 9f956e6 to fe1f7c3 Compare January 14, 2021 02:08

mkistler requested a review from P0lip January 14, 2021 02:12

P0lip previously approved these changes Jan 14, 2021

View reviewed changes

test: update harness scenarios

2f41119

mkistler dismissed P0lip’s stale review via 2f41119 January 14, 2021 13:28

Merge branch 'develop' into mdk/preserve-sibling-additional-propertie…

6b90146

…s-errors

P0lip enabled auto-merge (squash) January 14, 2021 21:39

P0lip approved these changes Jan 14, 2021

View reviewed changes

P0lip disabled auto-merge January 14, 2021 22:13

P0lip merged commit baf3883 into stoplightio:develop Jan 14, 2021

mkistler deleted the mdk/preserve-sibling-additional-properties-errors branch January 16, 2021 13:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: rework error filtering to preserve sibling additionalProperties errors #1433

feat: rework error filtering to preserve sibling additionalProperties errors #1433

mkistler commented Dec 30, 2020

mkistler Dec 30, 2020

P0lip commented Dec 30, 2020 •

edited

Loading

philsturgeon commented Dec 31, 2020

mkistler commented Jan 7, 2021

P0lip commented Jan 8, 2021

P0lip commented Jan 12, 2021

P0lip left a comment

mkistler commented Jan 14, 2021

P0lip left a comment

P0lip commented Jan 14, 2021

P0lip commented Jan 14, 2021 •

edited

Loading

mkistler commented Jan 14, 2021

mkistler commented Feb 14, 2021

P0lip commented Feb 19, 2021

feat: rework error filtering to preserve sibling additionalProperties errors #1433

feat: rework error filtering to preserve sibling additionalProperties errors #1433

Conversation

mkistler commented Dec 30, 2020

mkistler Dec 30, 2020

Choose a reason for hiding this comment

P0lip commented Dec 30, 2020 • edited Loading

philsturgeon commented Dec 31, 2020

mkistler commented Jan 7, 2021

P0lip commented Jan 8, 2021

P0lip commented Jan 12, 2021

P0lip left a comment

Choose a reason for hiding this comment

mkistler commented Jan 14, 2021

P0lip left a comment

Choose a reason for hiding this comment

P0lip commented Jan 14, 2021

P0lip commented Jan 14, 2021 • edited Loading

mkistler commented Jan 14, 2021

mkistler commented Feb 14, 2021

P0lip commented Feb 19, 2021

P0lip commented Dec 30, 2020 •

edited

Loading

P0lip commented Jan 14, 2021 •

edited

Loading