Redocly / redocly-cli

⚒️ Redocly CLI makes OpenAPI easy. Lint/validate to any standard, generate beautiful docs, and more.
https://redocly.com/docs/cli/
MIT License
934 stars 146 forks source link

There is no way to enforce existence of some node (not property) with configurable rules #1253

Open RomanHotsiy opened 1 year ago

RomanHotsiy commented 1 year ago

Is your feature request related to a problem? Please describe.

There is no way to enforce existence of some node (not a property) with configurable rules.

For example, if we want to ensure that the license info exists we can set the following rule on the parent object:

rule/license-exists:
  subject:
    type: Info
    property: license
  assertions:
    defined: true

But what if we want to ensure that Operation has at least one Parameter with in: header?

There is no way to achieve it right now with the current syntax. The following rule won't work:

rule/header-param-exists:
  where:
    - subject:
        type: Operation
      assertions:
        defined: true
    - subject:
        type: Parameter
        property: in
      assertions:
        const: header
  subject:
    type: Parameter
  assertions:
    defined: true

Because we never hit the context (e.g. if there are no parameters at all) the defined assertion won't be even executed.

We can't change the behavior of the defined assertion as it will make the opposite case impossible: check something if only it is defined (e.g. check that all Parameters have schema defined but do not fail if there are no parameters at all).

We need some solution for this.

Describe the solution you'd like

I have two main ideas: new assertion and new subject modifier.

Idea A (new assertion):

This assertion enforces existence of the node in the parent context. It can be also used without context, then it will enforce existence globally. It may a bit confusing for users to understand the difference from the defined assertion.

Some ideas for assertion names:

See example below:

rule.yaml ```yaml rule/header-param-exists: where: - subject: type: Operation assertions: defined: true - subject: type: Parameter property: in assertions: const: header subject: type: Parameter assertions: definedAndExists: true ```

Idea B (new subject modifier):

This may look the same but it's different because it can modify all the other assertions so they are not enforcing all the matching objects to comply but enforcing the context to have at least one compliant instance. So we can avoid extra context.

Some ideas for subject modifier names:

I lean towards the first or second because the other ones do not imply that other instances can be not matching (not sure if this is clear).

See the example below:

Rule yaml ```yaml rule/header-param-exists: where: - subject: type: Operation assertions: defined: true subject: type: Parameter property: in mode: exists # other mode: every (default), exists assertions: defined: true const: header # we can now combine assertion here as it won't enforce all the parameters to be `header` ```

Any other ideas anyone?

Describe alternatives you've considered

Do not support it in configurable rules, it can be always handled by custom rules.

Additional context

Related issue: https://github.com/Redocly/redocly-cli/issues/1254

tatomyr commented 1 year ago

I'm against the subject modifier as this introduces a new concept. The new exists assertion sounds a lot better.

RomanHotsiy commented 1 year ago

I'm against the subject modifier as this introduces a new concept.

We already have subject modifiers though. E.g. filterInParentKeys (maybe we call them differently in the code though, we don't name them in the docs though).

tatomyr commented 1 year ago

Then, having even more modifiers will be even more confusing. How many different combinations will it produce? It makes users do too much logical gymnastics. If we want to keep the configurable rules simple, it's better to extend assertions, I believe. On the other hand, I see that those modifiers allow users to write the rules more concisely, so it's a bit easier to read. But when writing, you have to go through 2 different lists: assertions and modifiers, and then calculate their combinations.

TBH, sometimes I ask myself if there was a way to avoid those filterInParentKeys just by using assertions themselves 🙂

lornajane commented 1 year ago

I'm not in favour of adding this. The configurable rules are already quite complicated and I don't think adding more functionality is valuable here when we're not sure how well users are using these features. I think a custom plugin is an easier way (where you have to do your own implementation) than understanding what we've done here. We could publish more example plugins to help users if we think they would struggle to implement this way.

jeremyfiel commented 1 year ago

is it possible to assert defined on the property: parameters in the where clause?

If they are valid in more than one place, you would need two separate rules. Although, the output may be messy.

  rule/header-param-exists:
    subject:
      type: Parameter
      property: in
    where:
      - subject:
          type: Operation
          property: parameters
        assertions:
          defined: true
    assertions:
      const: header

not sure why the output looks like nonsense, but the rule is evaluated.

rule/header-param-exists failed because the Parameter in didn't meet the assertions: query should be equal header

repro

openapi: 3.0.3
info:
  title: blah
  version: 1.1.0
paths:
  /test/api:
    get:
      parameters: 
        - name: jeremy
          in: query
          schema:
            type: string
      responses:
        "200":
          content:
            application/json:
              schema:
                $ref: "../../../common/shared/confirm-message-schema_v03.json"
components:
  requestBodies:
    jeremy:
      required: true
      content:
        application/json:
          schema:
            $ref: "./common/shared/-schema_v01.json"
  schemas:
    jeremy:
      type: object
      properties:
        test:
          $ref: "jeremy-test"

image

I think this may not be what you're asking though.. because if parameters doesn't exist at all, nothing is evaluated.