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

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

Open RomanHotsiy opened this issue 11 months ago • 5 comments

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:

  • A1: exists
  • A2: definedAndExists

See example below:

rule.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:

  • mode: exists - the mode can be default (default behavior) or exists - enforces that at least 1 matching node exists.
  • mode: presence
  • exists: true
  • mustExist: true
  • atLeastOne: true
  • minimumMatches: 1 - not sure if this can be implemented but should be possible

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
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

RomanHotsiy avatar Sep 11 '23 07:09 RomanHotsiy

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

tatomyr avatar Sep 11 '23 10:09 tatomyr

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).

RomanHotsiy avatar Sep 11 '23 12:09 RomanHotsiy

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 🙂

tatomyr avatar Sep 11 '23 12:09 tatomyr

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.

lornajane avatar Sep 11 '23 14:09 lornajane

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.

[34mrule/header-param-exists[39m failed because the [34mParameter[39m [34min[39m 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.

jeremyfiel avatar Oct 06 '23 19:10 jeremyfiel