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

message examples invalid against avro payload #772

Closed
chrispatmore opened this issue May 17, 2023 · 12 comments · Fixed by #778
Closed

message examples invalid against avro payload #772

chrispatmore opened this issue May 17, 2023 · 12 comments · Fixed by #778
Labels
bug Something isn't working released

Comments

@chrispatmore
Copy link
Contributor

Describe the bug

When supplying an avro payload for a message, and supplying examples that match that payload the examples cause a validation error:

schema is invalid: data/type must be equal to one of the allowed values, data/type must be array, data/type must match a schema in anyOf
  • If I comment out the examples the async api validates correctly and generates an example that looks exactly like the one supplied.
  • If I comment out the payload and leave the examples the async api validates correctly
  • If I switch the avro payload to a json payload everything validates correctly

How to Reproduce

Use this example api:

asyncapi: 2.6.0
info:
  description: Orders which have been placed on our webstore
  title: Orders
  version: 1.0.0
tags:
  - name: Webstore
  - name: Product
channels:
  Orders:
    subscribe:
      message:
        schemaFormat: "application/vnd.apache.avro;version=1.9.0"
        payload: {"type":"record","name":"WindRecordings","fields":[{"name":"direction","type":{"type":"enum","name":"directionEnum","symbols":["North","East","South","West"]}},{"name":"speed","type":"string"}]}
        examples: [
          {
            payload: {
              "direction": "North",
              "speed": "18"
            }
          }
        ]
Screenshot 2023-05-17 at 14 01 40

Expected behaviour

I would expect that the parser would validate the examples against the avro schema and my async api would be valid.

Notes

I tried to look through the code to find where the issue was coming from, and my best guess is that

const payloadSchema = serializeSchema(targetVal.payload, 'payload');
is not taking into consideration that the schema may be avro. Given that this may actually be a feature request, or I may be completely wrong

@chrispatmore chrispatmore added the bug Something isn't working label May 17, 2023
@github-actions
Copy link

Welcome to AsyncAPI. Thanks a lot for reporting your first issue. Please check out our contributors guide and the instructions about a basic recommended setup useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

@jonaslagoni
Copy link
Member

I think you are completely right @chrispatmore 👍

Wanna propose a change for it?

@chrispatmore
Copy link
Contributor Author

Will give it a shot when I get a moment

@chrispatmore
Copy link
Contributor Author

chrispatmore commented May 24, 2023

Hi, I'm going to need some help with some questions before I can complete this.

In https://github.com/asyncapi/parser-js/blob/master/src/ruleset/v2/ruleset.ts
There are some rules which I'm struggling to logically separate based on their behaviour and implementation.

  1. asyncapi2-schema-examples Which runs against examples where the schema format has not been changed (so not giving the bug above) has description Examples must be valid against their defined schema. However as far as I can tell from the implementation and tests it's really only validating examples inside Schema Object instances. and examples in Message Object instances have their own type Message Example Object .
  2. asyncapi2-message-examples Which runs against any publish or subscribe message (Message Example Object instances) has description Examples of message object should follow by "payload" and "headers" schemas. which I don't fully understand but the implementation and tests imply it's trying to validate the example against the message schema (leading to above bug as not expecting no default schemas)

My questions are:

  • Should (1) be even bothering to look at Message Example Object instances as I'm not sure it's really validating anything ?
  • Should (2) description be more clear and specific to Message Example Object instances ?
  • Do we need both (1) and (2) ?
  • Given either would need the parser passing in in order to validate against no default schemas, and that the only one that does this today is in a slightly different dir Where would you want the new equivalent "non default" version to be, where the majority of the others are. or in this schema-parser folder?

Sorry for the faff, just want to make sure I do the right thing?

@chrispatmore
Copy link
Contributor Author

Any thoughts @jonaslagoni

@jonaslagoni
Copy link
Member

Sorry @chrispatmore, must have missed the notification.

Should (1) be even bothering to look at Message Example Object instances as I'm not sure it's really validating anything ?

No, should definitely not check anything related to message examples, which I don't think it does 🤔 It only look into examples in schemas.

Should (2) description be more clear and specific to Message Example Object instances ?

If you think it needs to be clearer sure 👍

Do we need both (1) and (2) ?

Only change 2 I think.

I think the solution is in this line: https://github.com/asyncapi/parser-js/blob/481b24e8f6e2216d0ea5c3d9bb95c938f4736eef/src/ruleset/v2/ruleset.ts#LL231C33-L231C63

[?(@.schemaFormat === void 0)] basically means that this rule is only valid when the schemaFormat property is not sat i.e. defaulting to being the AsyncAPI Schema object. So by adding that check to https://github.com/asyncapi/parser-js/blob/master/src/ruleset/v2/ruleset.ts#L118-L139 we would not run into this problem.

It does however not account for when a user explicitly defines the AsyncAPI Schema object as schemaFormat, but I think it's fine, this is (or almost) never the case.

@chrispatmore
Copy link
Contributor Author

Thanks for the update. I was going to fix by getting the code to properly validate the example against the schema, rather than skipping the validation. Unless you think that's not a good idea?

@chrispatmore
Copy link
Contributor Author

chrispatmore commented May 31, 2023

Ah I think I see now why you're suggesting that fix. Because nothing today is doing any validation of an object against a schema defined in another format.
The validation there at the moment validates that a schema is valid as defined by the format. But there is nothing that can validate an object against a schema defined in another format. Though maybe I can parse it

@chrispatmore
Copy link
Contributor Author

chrispatmore commented May 31, 2023

Alright so, up for discussion I guess @jonaslagoni.
I have done and shown both work with unit tests that you can:

  1. skip out checking examples where schemaFormat is set (as suggested above, very simple and easy)
  2. Check examples against the schema in the format provided (more complex, requires dev import of avro-parser for unit tests, requires separating "default" examples rule and new asyncAPI2 rule using parse (like has been done with asyncapi2-schema* rules)

I feel that 2 is a more complete solution that is better for user, but bigger change.

@jonaslagoni
Copy link
Member

Then you would probably need to rename the rules to be schema specific 🙂

I would say it's perfectly fine to go with option 1, that provide a fix for this bug.

Then you can raise a feature request asking for examples to be validated according to their schema format 🙂

@chrispatmore
Copy link
Contributor Author

Okay will do

@asyncapi-bot
Copy link
Contributor

🎉 This issue has been resolved in version 2.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants