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: validate message object examples with spectral schemas #782

Conversation

chrispatmore
Copy link
Contributor

validate message object examples against the schemas in the
object by parsing the schemas using spectral before validating
this proves that the examples are valid.

Related issue(s)
Contributes to: #781

Signed-off-by: Chris Patmore [email protected]

Copy link
Contributor Author

@chrispatmore chrispatmore left a comment

Choose a reason for hiding this comment

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

Couple of comments / questions for others

package.json Show resolved Hide resolved
@@ -238,6 +239,7 @@ export const v2SchemasRuleset = (parser: Parser) => {
},
},
},
'asyncapi2-message-examples-custom-format': asyncApi2MessageExamplesParserRule(parser),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the rule in but might not want to do this yet. If not, need a pointer on how to get the tests to run without being in the ruleset

@chrispatmore
Copy link
Contributor Author

There is code duplication w.r.t to the old rule. I can deal with this after review / thoughts on what's there

@chrispatmore
Copy link
Contributor Author

has duplication which I can sort out with other comments. and is out of date with base now I think, but any comments on the code are appreciated @jonaslagoni

@jonaslagoni
Copy link
Member

I dont think it changed, I just need to find some time to review it ASAP 😅

@chrispatmore
Copy link
Contributor Author

This will probably need some slight changes / merge conflict resolutions from #786

@jonaslagoni
Copy link
Member

@chrispatmore fixed the conflicts, gonna review this once I get back from the gym 🙂

Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

@chrispatmore would it make sense to add the rule ONLY for when schemaFormat has been defined? i.e. that is the difference between the two spectral rules and they can both live together without having to change that down the road?

Cause that would remove all duplicated code parts and tests and simplify the approach by a mile

@chrispatmore
Copy link
Contributor Author

Let me take a look. Forcing me to play around with json path expressions again 😉

Chris Patmore added 3 commits June 15, 2023 13:33
validate message object examples against the schemas in the
object by parsing the schemas using spectral before validation
this proves that the examples are valid

Signed-off-by: Chris Patmore <[email protected]>
Signed-off-by: Chris Patmore <[email protected]>
switch the new rule to only run of the schemaFormat is
set in the message. Also adjust the tests to better align
with this separation.

Signed-off-by: Chris Patmore <[email protected]>
@chrispatmore chrispatmore force-pushed the validate-examples-against-non-default-schemas branch from b5a47e4 to 26eda0c Compare June 15, 2023 13:07
@chrispatmore
Copy link
Contributor Author

chrispatmore commented Jun 15, 2023

@jonaslagoni This block still duplicated across the two rules:

function getMessageExamples(message: v2.MessageObject): Array<{ path: JsonPath; value: v2.MessageExampleObject }> {
  if (!Array.isArray(message.examples)) {
    return [];
  }
  return (
    message.examples.map((example, index) => {
      return {
        path: ['examples', index],
        value: example,
      };
    }) ?? []
  );
}

function validate(
  value: unknown,
  path: JsonPath,
  type: 'payload' | 'headers',
  schema: unknown,
  ctx: RulesetFunctionContext,
): ReturnType<typeof schemaFn> {
  return schemaFn(
    value,
    {
      allErrors: true,
      schema: schema as JSONSchema7,
    },
    {
      ...ctx,
      path: [...ctx.path, ...path, type],
    },
  );
}

Not really sure how best to de-depe. Could export from the first one so new once can use, but they're so minor I'm not sure it's worth it

@chrispatmore
Copy link
Contributor Author

That code smell is false positive, if I remove the as any it won't build

Signed-off-by: Chris Patmore <[email protected]>
@chrispatmore
Copy link
Contributor Author

chrispatmore commented Jun 21, 2023

How did removing code increase duplication...

Edit: ah now it's finding the duplicated scenarios in the tests. I think these duplicate usages are valid, because they highlight the difference in behaviour between the two rules

@chrispatmore
Copy link
Contributor Author

@jonaslagoni thoughts on duplication in the tests?

Comment on lines 348 to 355
headers: {
type: 'record',
name: 'TestHeader',
fields: [
{name: 'someKey', type: 'string'},
{name: 'someOtherKey', type: 'string'}
]
},
Copy link
Member

Choose a reason for hiding this comment

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

What was this change for? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I added this when the same tests were being used for both rules, to ensure no change in behaviour, less relevant now, can remove

@@ -378,19 +390,32 @@ testRule('asyncapi2-message-examples', [
{name: 'speed', type: 'string'}
]
},
headers: {
Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure headers cannot be avro 🤔 At least not in v2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I added the headers to the original tests (schemaFormat not set case) so that It added confidence that header behaviour was maintained across both versions of the rule (before we decided to split the rules)
I can revert this.

Though the new info that headers cannot be v2 means I can simplify the new v2 rule!

@jonaslagoni
Copy link
Member

Sorry @chrispatmore I am context-switching like crazy at the moment, might be forgetting stuff...

Do correct me if I say something completely off the chart 😄

remove logic expecting headers to be avro
remove redundant test pieces

Signed-off-by: Chris Patmore <[email protected]>
@chrispatmore
Copy link
Contributor Author

No worries @jonaslagoni not in any particular rush. Have adapted to your comments.

The headers cant be avro was interesting, because my rule validating the headers examples against an avro header schema was perfectly happy with it!

Have removed it regardless as, as you say it's not allowed in the spec atm

I expect we will still have quite high duplication in the test spect

@chrispatmore
Copy link
Contributor Author

Hey @jonaslagoni , I am back from a little break, what are we thinking here?

@sonarcloud
Copy link

sonarcloud bot commented Aug 1, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

👍

@jonaslagoni
Copy link
Member

/rtm

@jonaslagoni
Copy link
Member

Thanks for the patience @chrispatmore, sorry it took so long 🙏

@asyncapi-bot asyncapi-bot merged commit 298b289 into asyncapi:master Aug 3, 2023
11 checks passed
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 2.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@chrispatmore chrispatmore deleted the validate-examples-against-non-default-schemas branch August 3, 2023 08:13
@chrispatmore
Copy link
Contributor Author

Thanks @jonaslagoni no worries !

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

Successfully merging this pull request may close these issues.

3 participants