Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rule: remove when #585

Closed
P0lip opened this issue Sep 23, 2019 · 7 comments · Fixed by #614
Closed

Rule: remove when #585

P0lip opened this issue Sep 23, 2019 · 7 comments · Fixed by #614
Assignees
Labels
breaking Pull requests that introduce a breaking change

Comments

@P0lip
Copy link
Contributor

P0lip commented Sep 23, 2019

Chore summary
Remove when member from IRule interface.

  when?: {
    // the `path.to.prop` to field, or special `@key` value to target keys for matched `given` object
    // EXAMPLE: if the target object is an oas object and given = `$..responses[*]`, then `@key` would be the response code (200, 400, etc)
    field: string;

    // a regex pattern
    pattern?: string;
  };

It's used only by parameter-description rule and does not really seem to be needed, as a jsonpath expression could be used instead.
Moreover, we don't really have that property documented anywhere, therefore the impact is likely to be low.

@P0lip
Copy link
Contributor Author

P0lip commented Sep 23, 2019

@philsturgeon @marbemac do you have anything against?
Obviously, we don't need to remove it right now, but this would be a good candidate for 5.0.

@philsturgeon
Copy link
Contributor

Sure!

@P0lip P0lip self-assigned this Sep 28, 2019
@P0lip P0lip mentioned this issue Sep 30, 2019
4 tasks
@philsturgeon philsturgeon added the breaking Pull requests that introduce a breaking change label Oct 1, 2019
@Amachua
Copy link
Contributor

Amachua commented Oct 2, 2019

Hello, I would like to get your opinion/inputs on a rule I want to create (and on which I thought the when field could have been used) (even if I'm not sure how to do it properly for the moment).

When I've a request with a path parameter such as those in the attach file, then I would like to have an error that tells me that I've forget the 404 response for the requests:

  • GET /other_resources/{other_resource_id} on line 33
  • PATCH /other_resources/{other_resource_id} on line 39
  • DELETE /other_resources/{other_resource_id} on line 48
  • PUT /other_resources/{other_resource_id}/sub_resources on line 49

For the moment, I don't know how this can be performed and I imagine that the when field could have been used for such purpose. (if you do know how to do it properly, I'll be happy to have your opinion on this).

404_responses.txt

@Amachua
Copy link
Contributor

Amachua commented Oct 4, 2019

I've continued my research on this point and I think that I can find a work around using JSONPath expression. Only thing is... I don't think the tool you're using to parse a JSON can do what I have in mind.

Before going to the point, I'd like to share with you the behavior I'm working in case this could also help in the building of the Spectral project.

Ensure that the path parameters follow a pattern

The first rule I've created was to ensure that all the path parameters do follow a specific naming convention (often used with resource_id). This latter was written this way:

  path-parameter-names-snake-case:
    type: style
    severity: error
    recommended: true
    description: A parameter in the path should be written in snake_case and ends with _id
    message: "The path parameter must be snake_cased and finish with _id: {{error}}"
    given: $..parameters[?(@.in === 'path')]
    then:
      field: name
      function: pattern
      functionOptions:
        match: ^[a-z0-9]+((_[a-z0-9]+)+)?_id$

404 response for requests with path parameters

Then I try to define a second rule that assess if a request with a path parameter do have a 404 response. For this latter, I'd to use the pattern that I've defined in the previous steps and I found that it could be done with a rule such as:

  missing-404-response:
    type: validation
    severity: error
    recommended: true
    description: Any operation must define the 404 response.
    message: "The request must define a 404 response."
    given: "$.paths[?(@property =~ /_id}/ )][?(@property === 'get' || @property === 'post' || @property === 'delete' || @property === 'options' || @property === 'patch' )].responses"
    then:
      field: '404'
      function: truthy

The only thing is... the operator =~ is not defined in the jsonpath-plus but is in this other library https://github.com/json-path/JsonPath.

@philsturgeon @P0lip I don't know what are the impacts of this change in your code but I do you think that it would be a good idea to give this opportunity to your community. Do you share this point of view? :)

@Amachua
Copy link
Contributor

Amachua commented Oct 6, 2019

Finally got something working with a this rule:

  missing-404-response:
    type: validation
    severity: error
    recommended: true
    description: Any operation must define the 404 response.
    message: "The request must define a 404 response."
    given: "$.paths[?(@property.includes('_id}'))][?(@property === 'get' || @property === 'post' || @property === 'delete' || @property === 'options' || @property === 'patch' )].responses"
    then:
      field: '404'
      function: truthy

I didn't know that we could directly use javascript functions in a JsonPath and I think that it could be great that you set in your documentation that such function can directly be use :)

@P0lip
Copy link
Contributor Author

P0lip commented Oct 10, 2019

@Amachua,
First of all, my apologies for dropping a ball and not responding for so long.

I didn't know that we could directly use javascript functions in a JsonPath and I think that it could be great that you set in your documentation that such function can directly be use :)

Yeah, you can use filter expressions, which are evaluated during the runtime.
If that wasn't an option, we wouldn't remove when.
I hope you figured it out.

I think that it could be great that you set in your documentation that such function can directly be use :)

Mind opening a PR and sharing what you achieved in the docs? :)
Thank you.

@philsturgeon
Copy link
Contributor

@P0lip what is the plan here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Pull requests that introduce a breaking change
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants