redocly-cli icon indicating copy to clipboard operation
redocly-cli copied to clipboard

Support deep traversal of schema in nested visitors

Open aviskase opened this issue 3 years ago • 6 comments

Is your feature request related to a problem? Please describe. When you define a rule using nested visitor, for example, Response: { Schema: ... }, per documentaion:

It will be executed only for the first level of Schema Object.

Thus, if you want to check schema's properties and sub-properties, you have to do deep traversal manually.

Describe the solution you'd like As I understand it, the main advantage of nested visitors is scoping. For example, when you want to lint schema properties only in response or request bodies, but not in parameters. Doing deep traversal manually isn't worth it, because there might be refs, allOf/oneOf/anyOf. In comparison, when using Schema without nesting, the rule is checked against each individual property. It would be great to have the same behaviour in nested visitors too.

Describe alternatives you've considered Currently we use non-nested Schema visitor. We tried two approaches to detect that schema comes from parameters:

  • parent check: ctx.parent?.in → too naive and prone to false positives and negatives
  • pointer check: /\/parameters\/\d+(?:(?:\/)|(?:\/.+\/))schema/.test(ctx.location.absolutePointer) → doesn't work with refs

In the end, right now we simply use ignore file.

aviskase avatar Sep 04 '21 20:09 aviskase

One of the other advantages of not going deep in nested visitors is performance.

@aviskase could you please describe your use case?

RomanHotsiy avatar Sep 06 '21 06:09 RomanHotsiy

@RomanHotsiy use case is for scoping schema rules. For example: check that all string enums are following conventions:

  1. Response/Request body properties: values should beCAPITAL_CASE.
  2. Path parameters: values should be camelCase
  3. Other types of parameters: no restrictions

For (2) and (3) this visitor is working ok, because path parameters are always 1-level deep and we don't care about query params:

Parameter: {
    skip(node) { return node.in !== 'path'},
    Schema(node, ctx) { ... }
}

But I couldn't find a way to properly scope (1).

aviskase avatar Sep 06 '21 19:09 aviskase

@aviskase sorry for the radio silence 🙉.

I believe your use case is valid. But I don't have a good solution yet.

Same schema may be referenced from multiple places so combining scoping and deep traversal may lead to an combinatory explosion in our engine.

I will think on some alternative solution.

RomanHotsiy avatar Nov 01 '21 08:11 RomanHotsiy

Potentially related to #331

lornajane avatar Mar 31 '23 16:03 lornajane

It's not directly related to #331. It's more about intent.

There are 2 main use-cases here.

Evaluate "top-level" entity within some scope or evaluate it "deep".

For example, given the following Visitor structure and OpenAPI structure:

Operation
  Schema
/pet:
  get:
    requestBody:
      content:
        application/json:
          schema:
            title: Parent
            type: object
            properties:
              a:
                title: Nested
                type: string
              b:
                $ref: ...

The current implementation will evaluate only the Parent schema, while the deep one would evaluate also Nested and all the child schemas of the $ref.

Both of the options make sense for different usecases but the current one has some performance benefits.

Maybe we can implement some flag to explicitly enable deep traversal 🤔. But I wander how this should work in our assertions syntax.

RomanHotsiy avatar Apr 04 '23 07:04 RomanHotsiy

Sounds like we can add this later, after the general release.

lornajane avatar Apr 06 '23 08:04 lornajane