New dedupe in 5.0.0 prevents multiple rule violations to be reported from the same source range

[disposedtrolley]

Describe the bug
We've recently upgraded to Spectral 5.0.0 and have noticed that multiple rule violations for the same range in the code are no longer reported. We have some rules which target a broad block of the OpenAPI JSON (i.e. a map of objects), and we delegate our custom rule function to iterate over the map and return an array of rule violations. All of these violations would point to the same source file, column, and line number.

Looking through the PR history, I see that a fingerprinting function has been added which attempts to eliminate duplicate rule violations being reported. The function only seems to use the code, path, range, and source attributes of the rule to compute a fingerprint, not the error message itself. This means that if we return an array of rule violations from custom functions, only the first element will be rendered to the user.

To Reproduce

1. Given an OpenAPI file with multiple paths
2. Write a custom rule which targets $.paths
3. Attach the rule to a custom function which iterates over paths and returns an array of rule violations
4. Observe that only the first rule violation in the returned array is reported to the user

Expected behavior
All rule violations should be reported.

Environment (remove any that are not applicable):

• Library version: 5.0.0
• OS: macOS 10.15.2

Additional context
I know that targeting a broad block of JSON for rules isn't optimal, but there may be some cases where we need more complex logic than what the JSONPath syntax can offer to run our rules. I think it might be beneficial to:

• modify the dedupe function to account for the error message when computing the fingerprint.
• update the custom rules section of the docs to strongly recommend providing JSONPath expressions which target individual fields only

P.S. thanks for all the hard work that went into 5.0.0! We're seeing much better performance now.

/cc @nogates @davidlopezre @magnetikonline @adoragoh just something to watch out for when writing Spectral rules :)

[nulltoken]

modify the dedupe function to account for the error message when computing the fingerprint.

@P0lip A proposal for this already exists in #913 and is being discussed with @marbemac (cf. https://github.com/stoplightio/spectral/pull/913/files#r368241693)

[nulltoken]

@disposedtrolley Could you please provide us with a simple repro case that would mimic your issue?

[disposedtrolley]

Here you go.

If you extract the zip and run spectral lint openapi.yaml -r rules.yaml in the extracted directory, you should see that only one of the two rule violations returned by custom-func in the this-rule-does-not-work rule is actually displayed.

spectral_issue.zip

[nulltoken]

@disposedtrolley Sorry the for the delay.

Thanks for the repro.

module.exports = targetVal => {
	return [{message: "problem 1"}, {message: "problem 2"}]
}

Although this might actually win the prize for the "minimal repro case" poster child contest 😉 , it doesn't provide a lot to work with. I'm especially missing the context of what you're trying to achieve from a functional standpoint.

You're completely right that the fingerprinting strips out the second problem. It bears the same code, and is located at the same range.

I've hit a similar issue while working on #913 and eventually found out that you can refine the path (thus generating a different range) while reporting from the function (cf. https://github.com/stoplightio/spectral/blob/develop/src/functions/typedEnum.ts#L34-L39).

The function is given a first path pointing at the root of the object to consider and eventually returns more precise locations pointing at inner parts of the considered object. That made sense with regards to what this typedEnum function was processing (examining an enum and identifying offending values of that enum).

Wouldn't that help you solve your issue, could you please provide a bit more data with regards to your own context?

[disposedtrolley]

I'm especially missing the context of what you're trying to achieve from a functional standpoint.

Haha sorry! Let me try to elaborate.

One of the rules we have ensures that a specific extension property is either present in the root, or in all path definitions. The custom function we have targets the entire OAS ($) as it needs context around what exists in various parts of the OAS. The function may return more than one error message depending on where the extension property was found and not found.

[nulltoken]

@disposedtrolley Thanks for the explanation.

I believe on way to cope with your requirements, considering the current fingerprinting behavior would be to alter your function. One proposal could be:

• Scan all paths definitions. If no override exists in those, nor at the root level, raise an error without specifying an inner path
• If it's only specified at the root, then all is good.
• If it's specified in all path definitions and nothing is set at the root level, then all is good as well.
• If it's specified in some path definitions and nothing is set at the root level, then raise an error for each path definition which hasn't been decorated, specifying the inner path of each path definition.

At his stage, it's specified at the root and, at least, in one path definition.

I'd go with reporting the root being specified where it shouldn't be (without specifying an inner path). This approach will require two passes of run and fixes to get the spec cleaned up (the first one to get the root dropped, the second one to clean all the paths without any decoration)

Of course, there are certainly other ways to implement that 😉

Although the current fingerprinting implementation requires a bit more work, in the end it compels us to write more precise error reporting, eventually providing the user with a better experience with proper locations for each issues.

[disposedtrolley]

Thanks for the suggestions @nulltoken!

We’re definitely looking at changing how we process rules and realise that it’s an edge case (even for us). I’m leaning towards the two pass solution.

Have the docs been updated to reflect these changes? Maybe a warning that targeting a broad block of the OAS isn’t such a good idea anymore? Otherwise happy to get this one closed out!

[nulltoken]

Have the docs been updated to reflect these changes? Maybe a warning that targeting a broad block of the OAS isn’t such a good idea anymore? Otherwise happy to get this one closed out!

@disposedtrolley I've just opened #1035 to try and document this. Feedback woud be welcome 😄

[disposedtrolley]

@nulltoken looks great! Thanks for keeping the docs fresh :D