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

feat(rulesets): validate oas3 runtime expressions #1608

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

barrett-schonefeld
Copy link

Checklist

  • Tests added / updated
  • Docs added / updated

Does this PR introduce a breaking change?

  • Yes
  • No

Additional context

This PR validates links.parameters expressions against the oas3 specification for runtime expressions.

Existing Test Failures

  • In the tests that create a Spectral object and call loadRuleset, the new rule is not being loaded from the index.json file. I'm likely missing something obvious, but I would appreciate suggestions on what may be happening.
  • In the acceptance test I added that creates a Spectral object, configures it, and validates against a spec, no errors are returned. The problem is either a result of the problem directly above or I'm not correctly configuring the Spectral object before calling run. I didn't want to spend too much time trying to resolve the configuration in case this problem is the result of the above-mentioned problem.

targetVal,
null,
{ given: ['paths', '/path', 'get', 'responses', '200', 'links', 'link', 'parameters', 'param'] },
{ given: null, original: null, documentInventory: new DocumentInventory(doc, {} as any), rule: {} as any }, // TODO
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably don't need to bother about this todo comment.
As a part of the V6 release, a custom function will receive some lifting here and there.

@P0lip
Copy link
Contributor

P0lip commented May 13, 2021

Hey again!
Thanks for the PR.
I didn't have much time to go through it thoroughly, but I'll try to review it soon.

Copy link
Contributor

@P0lip P0lip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good in general! Thanks!

@mnaumanali94 mind swinging by the message we use here?
I feel like some of them are a tiny bit off, but curious to hear your thoughts.

src/rulesets/oas/index.json Outdated Show resolved Hide resolved
];
}

function validateJsonPointer(jsonPointer: string): void | IFunctionResult[] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually pretty cool util that's worth porting to our @stoplight/json package.
We have a set of functions that deal with various json pointers scenarios, and this would be a perfect fit.

Do you mind if I port it over there, so we can use it here and elsewhere in our ecosystem?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely, please feel free to do that!

I apologize for not following up sooner. This PR just slipped my mind.

Comment on lines 4 to 5
export const runtimeExpression: IFunction = function (exp): void | IFunctionResult[] {
// oas3 spec allows for type Any, so only validate when exp is a string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A default export is needed too.

src/rulesets/oas/functions/runtimeExpression.ts Outdated Show resolved Hide resolved
src/rulesets/oas/functions/runtimeExpression.ts Outdated Show resolved Hide resolved
Co-authored-by: Jakub Rożek <jakub@rozek.tech>
Co-authored-by: Jakub Rożek <jakub@rozek.tech>
Co-authored-by: Jakub Rożek <jakub@rozek.tech>
@barrett-schonefeld
Copy link
Author

barrett-schonefeld commented Sep 2, 2021

Hey, folks!

I'm sorry I missed the follow-ups on May 31, and I let this PR fall behind a bit.

Is this still something to include in Spectral or as mentioned above, port to @stoplight/json?

Please let me know if this is something on which to move forward.

@P0lip P0lip force-pushed the develop branch 3 times, most recently from 02ec0d4 to 84faec8 Compare June 9, 2023 19:43
@dpopp07
Copy link
Contributor

dpopp07 commented Aug 9, 2023

@P0lip what's the status of this PR? I think this would be a valuable change to include in Spectral. I'd be happy to help out here if any additional effort is needed

@P0lip
Copy link
Contributor

P0lip commented Aug 21, 2023

@dpopp07 I'll try to revisit it this week! I'll let you know in case I need any help :)

@P0lip P0lip changed the title feat: validate oas3 runtime expressions feat(rulesets): validate oas3 runtime expressions Aug 24, 2023
@P0lip
Copy link
Contributor

P0lip commented Aug 24, 2023

@mnaumanali94 whatcha think?

@P0lip P0lip force-pushed the develop branch 3 times, most recently from 9e92f34 to 6d09915 Compare September 20, 2023 18:42
@dpopp07
Copy link
Contributor

dpopp07 commented Jan 24, 2024

@P0lip following up on this again - any updates or plans to complete it? Again, happy to help bring it across the finish line if need be.

Also, if you decide against merging it at all and plan to close it let me know so I can look at incorporating the changes to my own ruleset. Thanks!

@dpopp07
Copy link
Contributor

dpopp07 commented Apr 3, 2024

@P0lip @mnaumanali94 Any updates? At this point, it's almost a 3 year old PR 🙂 If there aren't plans to complete this work in the near-ish future, we'll plan to bring it in to our own ruleset and y'all can close this out (the PR was originally opened out of a request to include this logic in our ruleset - it fell off our priorities but now I'd like to close it out).

@P0lip
Copy link
Contributor

P0lip commented Apr 4, 2024

It's up to @mnaumanali94 Other than the minor merge conflict, the code is ready to go and I'd love to have that rule.

@P0lip P0lip force-pushed the develop branch 3 times, most recently from dc90b7a to c22f408 Compare April 4, 2024 13:29
@dpopp07
Copy link
Contributor

dpopp07 commented Apr 9, 2024

@P0lip @mnaumanali94 sounds good, thanks. I'll give it another week or so to hear what the plan is before I look at porting it to our own ruleset.

@mnaumanali94
Copy link
Contributor

@dpopp07 We're good to merge this. Lets fix the conflict and merge away

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants