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

Ruleset: AsyncAPI #974

Merged
merged 1 commit into from
Apr 18, 2020
Merged

Ruleset: AsyncAPI #974

merged 1 commit into from
Apr 18, 2020

Conversation

nulltoken
Copy link
Contributor

@nulltoken nulltoken commented Feb 19, 2020

Fixes #965
Fixes #789

Checklist

  • Tests added / updated
  • Docs added / updated
  • Log issue to add a rule to ensure all variables used in server url are defined
  • Log issue to add a rule to ensure all defined server variables are used
  • Log issue to add a rule to ensure all parameters used in channel path are defined
  • Log issue to add a rule to ensure all defined channel parameters are used
  • Log issue to add a rule to identify orphaned components (messageTraits, ...)
  • Migrate asyncApi2PayloadValidation to a custom function of the ruleset

Does this PR introduce a breaking change?

  • Yes
  • No

If indicated yes above, please describe the breaking change(s).

Remove this quote before creating the PR.

Screenshots

If applicable, add screenshots or gifs to help demonstrate the changes. If not applicable, remove this screenshots section before creating the PR.

Additional context

Add any other context about the pull request here. Remove this section if there is no additional context.

scripts/generate-assets.js Outdated Show resolved Hide resolved
@nulltoken
Copy link
Contributor Author

@philsturgeon Few questions:

  • In the original document, some rules were commented out. I didn't consider them. Should I try and re-integrate them or were they left out on purpose?
  • Some rules are identical between oas and aas. Should this stay as is or should we try to somehow reuse a common set of shared rules between the two rulesets?

@P0lip @philsturgeon Next steps:

  • I believe I've now got the overall skeleton in place (@P0lip Would you have some time for a first review pass?). I'm now going to create tests for each of those new rules (at least one passing and one failing per rule).
  • Then some documentation for the new ruleset (@philsturgeon I might need your help on that)

@philsturgeon
Copy link
Contributor

@nulltoken the commented out ones were things I didn't delete or make work. I deleted some things which were definitely not worth trying to port over, and left in some things I hadn't researched. If those rules are relevant lets get them working, if they are not we can delete them.

should we try to somehow reuse a common set of shared rules between the two rulesets?

A little bit of duplication is ok for now, I'd rather get it working than spend a bunch of time on DRYing it prematurely.

rollup.config.js Outdated Show resolved Hide resolved
@philsturgeon
Copy link
Contributor

@nulltoken the commented out ones were things I didn't delete or make work. I deleted some things which were definitely not worth trying to port over, and left in some things I hadn't researched. If those rules are relevant lets get them working, if they are not we can delete them.

should we try to somehow reuse a common set of shared rules between the two rulesets?

A little bit of duplication is ok for now, I'd rather get it working than spend a bunch of time on DRYing it prematurely.

@nulltoken
Copy link
Contributor Author

nulltoken commented Mar 21, 2020

Dear Diary,

At last! Back in the world of CI green. Pretty relieved!

Suffered a lot in Karma land and weird execution of jest BeforeAll() test hooks. Had to switch over to BeforeEach(). A bit of a waste as we keep on rebuilding something which is supposed to be immutable, but, at least... it works.

@nulltoken nulltoken requested a review from P0lip April 17, 2020 13:24
docs/reference/asyncapi-rules.md Outdated Show resolved Hide resolved
docs/reference/asyncapi-rules.md Outdated Show resolved Hide resolved

### asyncapi2-channel-no-query-nor-fragment

Query parameters and fragments shouldn't be used in channel names. Instead, use bindings to define them.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get some examples?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@philsturgeon I'm afraid I won't be able to come up with a good/bad example that would make sense in an AsyncAPI context.

@fmvilas Could you please help me here? What's the known bad practices regarding query and fragments and how should they be converted to bindings?

docs/reference/asyncapi-rules.md Outdated Show resolved Hide resolved
docs/reference/asyncapi-rules.md Outdated Show resolved Hide resolved
docs/reference/asyncapi-rules.md Show resolved Hide resolved
docs/reference/asyncapi-rules.md Outdated Show resolved Hide resolved
src/rulesets/asyncapi/index.json Show resolved Hide resolved
src/rulesets/asyncapi/index.json Outdated Show resolved Hide resolved
src/rulesets/asyncapi/index.json Show resolved Hide resolved
Comment on lines 223 to 229
Spectral currently only supports payload validation against the default AsyncAPI schema.

This rule will notify you when a payload is decorated with a `schemaFormat` property. Spectral won't be able to validate it and potential issues in this payload will go undetected by the `asyncapi-payload-*` rules.

Copy link
Contributor

@philsturgeon philsturgeon Apr 17, 2020

Choose a reason for hiding this comment

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

Let's go with something like....

AsyncAPI can support various schemaFormat values, with the default being one of the following:

application/vnd.aai.asyncapi;version=2.0.0
application/vnd.aai.asyncapi+json;version=2.0.0
application/vnd.aai.asyncapi+yaml;version=2.0.0

At this point no other schemaFormat's are supported by Spectral, so if you use them this rule will emit an info message and skip validating the payload.

Other formats such as OpenAPI Schema Object, JSON Schema Draft 07 and Avro will be added in various upcoming versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@philsturgeon Awesome! I've slightly reworded your proposal (as we don't support setting the schemaFormat at all).

Copy link
Contributor

@philsturgeon philsturgeon left a comment

Choose a reason for hiding this comment

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

@nulltoken with those few changes done this is all thumbs from me! When you and @P0lip are happy merge away and let's get a beta out for people to play with! 🥳

import { IFunction, IFunctionContext } from '../types';
import { IFunctionResult } from '../types/function';

import * as asyncApi2Schema from '../rulesets/asyncapi/schemas/schema.asyncapi2.json';
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be provided in a ruleset as a function option?

@nulltoken nulltoken force-pushed the ntk/asyncapi branch 2 times, most recently from 40cb464 to ab947a0 Compare April 18, 2020 10:34
@nulltoken nulltoken requested review from philsturgeon and P0lip April 18, 2020 10:47
@nulltoken
Copy link
Contributor Author

@nulltoken with those few changes done this is all thumbs from me! When you and @P0lip are happy merge away and let's get a beta out for people to play with! 🥳

@philsturgeon Thanks a lot for the feedback.

@P0lip What's your thinking Sir?


import { IFunction, IFunctionResult } from '../types';
import { getLintTargets } from '../utils';
import { schema } from './schema';
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to import it.

return validator;
};

export const asyncApi2PayloadValidation: IFunction<IPayloadValidationOptions> = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export const asyncApi2PayloadValidation: IFunction<IPayloadValidationOptions> = (
export function asyncApi2PayloadValidation = (
this: IFunctionContext,
// ..rest of args

paths,
otherValues,
) => {
const ajvValidationFn = buildAsyncApi2SchemaObjectValidator(schema);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const ajvValidationFn = buildAsyncApi2SchemaObjectValidator(schema);
const ajvValidationFn = buildAsyncApi2SchemaObjectValidator(this.functions.schema);

const results: IFunctionResult[] = [];

for (const relevantItem of relevantItems) {
const path = rootPath.slice(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

How about?

Suggested change
const path = rootPath.slice(0);
const path = [...rootPath, ...relevantItem.path]

const path = rootPath.slice(0);
Array.prototype.push.apply(path, relevantItem.path);

const result = schema(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const result = schema(
const result = this.functions.schema(

) => {
const ajvValidationFn = buildAsyncApi2SchemaObjectValidator(schema);

const relevantItems = getLintTargets(targetVal, opts.field);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for the clarity, we've discussed this one on slack - this should not be needed, since runner already handles it for us.

@P0lip
Copy link
Contributor

P0lip commented Apr 18, 2020

Dang, hit the button too late. 😆

P0lip
P0lip previously approved these changes Apr 18, 2020
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.

One last comment.
Looks good from my point of view.

// happens
}
return opts;
return { result: parse(opts.result) };
Copy link
Contributor

Choose a reason for hiding this comment

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

If I am not mistaken, we should return original options.
I suggest not touch it unless it's needed?

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

Successfully merging this pull request may close these issues.

format and ruleset: AsyncAPI JSON Schema Validation struggles with circular references
5 participants