fix #915: Ignore examples with circular schemas

[m-mohr]

Fixes #915.

Checklist

• Tests added / updated
• Docs added / updated

Does this PR introduce a breaking change?

• Yes
• No

Additional context

This is a first very dump attempt at #915, see the issues for more context. This is more a workaround than a proper fix, but I don't see a better alternative that doesn't need a lot of work yet.

Better would be to send an information instead of an error when ignoring, but not sure how to do that.

The rules could be changed as follows whenever users want to ignore examples with circular schemas:

"oas3-valid-schema-example": {
      "description": "Examples must be valid against their defined schema.",
      "message": "{{error}}",
      "severity": 0,
      "formats": ["oas3"],
      "recommended": true,
      "type": "validation",
      "given": "$.components.schemas..[?(@property !== 'properties' && @.example && (@.type || @.format || @.$ref))]",
      "then": {
        "function": "schemaPath",
        "functionOptions": {
          "field": "example",
          "schemaPath": "$",
          "oasVersion": 3,
          "ignoreCircular": true
        }
      }
    }

Even better would be to simply inherit by specifying just the additional rule, but I don't know how to achieve it.

"oas3-valid-schema-example": {
  "ignoreCircular": true
}

[aerfus]

Hello,
@m-mohr do you plan to release the feature in the near future?

[m-mohr]

@aerfus It's not on me releasing this. You need to ask @P0lip and the others at stoplight.io.

To be honest, this is only a hack to stop failures in the CI. I guess a better solution would be to use #939 or fix the bug completely so that recursive schemas can be validated.

[aerfus]

@m-mohr I see, thank you for the information!

[P0lip]

@m-mohr please accept my sincere apologies for dropping a ball here. It's been a couple of hectic days for me and didn't have much time to contribute to Spectral. 😞

I'm more than happy to release a minor release once this is in, since there are a couple of others PRs we've already merged and it makes sense to get that stuff out to public.

@nulltoken mind giving a look at this PR?

[nulltoken]

@P0lip on it, Sir!

@m-mohr Few thoughts:

• Rather than short circuiting the function, I believe that your proposal of letting the user know about this shortcoming may be of great value. Taking a look at typedEnums.ts might be a good start to see how we can return custom-made messages.
• My understanding of the current version of the code is that it doesn't only focus on circular references but would actually exclude any schema that includes a $ref. Would that be correct, it might be a bit too broad (as composing schemas is a widely used feature).
• With regards to detecting circular references, it's possible that the DepGraph feature may (⚠️ Unvalidated blurry "gut feeling" area) be helpful
  Spectral internally leverage the dependency-graph library. And it happens that detecting cycles seems to already be a thing (Expose CheckCycle API jriecken/dependency-graph#25). FWIW, you can find an example of the graph thingie usage in unreferencedReusableObject.ts

Minor things that this PR would require in order to be merged:

• updating the related doco to describe the change in the schemaPath options
• some tests to demonstrate the feature (and protect it from potential regressions)

[m-mohr]

Thanks for the review @nulltoken. No need to apologize, @P0lip :-)

• Rather than short circuiting the function, I believe that your proposal of letting the user know about this shortcoming may be of great value. Taking a look at typedEnums.ts might be a good start to see how we can return custom-made messages.

The problem is that (at least in my case) I would still want the CI to fail if examples are not correct, but only for circular referenced examples that can't be validated properly I want to send an info. I can't see anything in typedEnums.ts that lets me set the severity though.

• My understanding of the current version of the code is that it doesn't only focus on circular references but would actually exclude any schema that includes a $ref. Would that be correct, it might be a bit too broad (as composing schemas is a widely used feature).

As far as I understood the schemas are dereferenced and $ref are only left in place where it couldn't resolve the ref (usually when there's a circular ref). So that shouldn't be a problem, but I'm not deeply invested in your source code so that might be a wrong assumption (although it works in my tested cases). Ultimately, the dereferencing is why the error occurs and the tool probably shouldn't dereference internal refs so that it can validate circular refs.

• With regards to detecting circular references, it's possible that the DepGraph feature may (⚠️ Unvalidated blurry "gut feeling" area) be helpful
  Spectral internally leverage the dependency-graph library. And it happens that detecting cycles seems to already be a thing (jriecken/dependency-graph#25). FWIW, you can find an example of the graph thingie usage in unreferencedReusableObject.ts

That's a bit too deep into things I don't know enough about. I'm also a bit limited on time and can't dig into these details at the moment.

Minor things that this PR would require in order to be merged:

I'm not even sure we need to merge it. As far as I understood PR #939, that PR could also solve the issue as I could just ignore the circular schemas with it?!

• updating the related doco to describe the change in the schemaPath options

The current solution is a bit verbose as required copying rules. Has anyone an answer for the question I raised in the PR description? Beforehand it doesn't make much sense to write docs.

[nulltoken]

@m-mohr

As far as I understood PR #939, that PR could also solve the issue as I could just ignore the circular schemas with it?!

It wouldn't fix the underlying bug identified by #915. It should help ignore the findings, though.
However, please keep in mind #939 is still in its early stages. It may take some time before it's even ready for a formal review.

[m-mohr]

It wouldn't fix the underlying bug identified by #915.

Indeed, but I don't see this being fixed anytime soon as it seems to need fundamental changes in spectral?! So maybe we need workarounds to help with getting the CIs running for now. At the moment, spectral is very much useless for us as I can't run it in CI due to this bug.

However, please keep in mind #939 is still in its early stages. It may take some time before it's even ready for a formal review.

Didn't know that. I thought it's relatively close to being ready. The issue is I'm pretty busy at the moment and can't work on this PR until mid-March or so.

[philsturgeon]

@nulltoken do you think the change we discovered in APIDevTools/json-schema-ref-parser#158 will help here?

[m-mohr]

I don't think so, but not really sure. How should switching from auto to id help?

[nulltoken]

@nulltoken do you think the change we discovered in APIDevTools/json-schema-ref-parser#158 will help here?

@philsturgeon It's possible than trying to $Refparser.bundle() the schema before compiling the result through ajv might solve the issue. However, off the top of my head, adding the bundling step in the schema may also require:

• to teach the rule engine to work with async functions as $Refparser.bundle is async. @P0lip started a WIP in https://github.com/stoplightio/spectral/tree/feat/async-fns but it's possible we'd have to adapt/extend our tests to cover this
• to cover the execution of the oas2 and oas3 schema rules in offline context (in order to ensure calling bundle() against the generated static assets doesn't trigger any http call). I've got something in Ruleset: AsyncAPI #974 that does this for asyncapi (cf.

  spectral/src/rulesets/asyncapi/__tests__/streetlights.jest.test.ts

  Lines 41 to 61 in 36379ee

  	test('can be linted in standard context', async () => {
  	const content = await readParsable(streetlights, { encoding: 'utf8' });
  	await lint(content, streetlights);
  	});
  	test('can be linted in offline context', async () => {
  	Spectral.registerStaticAssets(require('../../../../rulesets/assets/assets.json'));
  	const content = await readParsable(streetlights, { encoding: 'utf8' });
  	const readFileSpy = jest.spyOn(fs, 'readFile').mockImplementation(() => {
  	throw new Error();
  	});
  	nock.disableNetConnect();
  	await lint(content);
  	readFileSpy.mockRestore();
  	});

  )

I don't think so, but not really sure. How should switching from auto to id help?

@m-mohr I haven't studied the id vs $id thingie. Would this need to change in oas2, oas3, any-other-context, maybe allowing the rule writer to specify this option through a parameter to the schema function may help.

⚠️ Full disclosure, I haven't tested this extensively and I am far from being an AJV guru 😉

[P0lip]

Hey folks.
Is this still relevant?

[m-mohr]

I guess it's obsolete with the "except"ions from @nulltoken. So I'm closing the PR. Issue #915 is not solved though as the "except"ions are basically just a workaround around a spectral bug/limitation.

[nulltoken]

@P0lip With #939 and #1020 (which showcases how to work around specific circular schemas issues) in, I believe this is no longer relevant.

@m-mohr Thoughts?

[philsturgeon]

Seeing as we have a workaround for now, the long term solution will be handled by switching from stoplight/json-ref-resolver to APIDevTools/json-schema-ref-parser (#1054).

Our ref resolver handles many circular reference scenarios, but this one has better support out of the box. If there is still a problem after we switch then it'll need to be fixed over there.