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

Possible bug with example - adeo-kafka-request-reply-asyncapi #1031

Closed
Pakisan opened this issue Feb 12, 2024 · 18 comments
Closed

Possible bug with example - adeo-kafka-request-reply-asyncapi #1031

Pakisan opened this issue Feb 12, 2024 · 18 comments

Comments

@Pakisan
Copy link
Member

Pakisan commented Feb 12, 2024

Hi!

I'm not sure, but looks like this example is broken - https://github.com/asyncapi/spec/blob/master/examples/adeo-kafka-request-reply-asyncapi.yml

I checked It in Studio - no warnings, but when I tried to parse this example using JAsyncAPI I enfaced with errors

I found two strange things:

  1. extensions
bindings:
      kafka:
        x-key.subject.name.strategy:
          type: string
          description: >
            We use the RecordNameStrategy to infer the messages schema. Use
            `key.subject.name.strategy=io.confluent.kafka.serializers.subject.RecordNameStrategy`
            in your consumer configuration.
        x-value.subject.name.strategy:
          type: string
          description: >
            We use the RecordNameStrategy to infer the messages schema. Use
            `value.subject.name.strategy=io.confluent.kafka.serializers.subject.RecordNameStrategy`
            in your consumer configuration.

This property name x-key.subject.name.strategy must not be interpreted as valid because of our regex ^x-[\w\d\-\_]+$ or not?

  1. Schema structure
CorrelationId:
      type: string
      format: uuid
      description: >-
        A unique Correlation ID defined from the `REQUEST_ID` or the
        `MESSAGE_ID` provided in the Costing Request.
      example: 1fa6ef40-8f47-40a8-8cf6-f8607d0066ef

I may be wrong, but draft-07 doesn't have such field, only examples or not?

@Pakisan
Copy link
Member Author

Pakisan commented Feb 12, 2024

@smoya
Copy link
Member

smoya commented Feb 14, 2024

Regarding point 1, I see the regex we show in our documentation doesn't correspond with the one enforced in our JSON Schema documents, which indeed allows for . characters. So I completely understand then why Studio is not complaining, but I don't get why JAsyncAPI doesn't, tbh.
So in summary, we should either fix the docs or fix the regex. A decision should be taken.

Regarding point 2, yes you are right: there is no example field but examples.

@Pakisan
Copy link
Member Author

Pakisan commented Feb 14, 2024

Regarding point 1, I see the regex we show in our documentation doesn't correspond with the one enforced in our JSON Schema documents, which indeed allows for . characters. So I completely understand then why Studio is not complaining, but I don't get why JAsyncAPI doesn't, tbh.
So in summary, we should either fix the docs or fix the regex. A decision should be taken.

Yes, you are right. We need to discuss and decide which pattern to use.

That's why I requested clarification. Because are using description from out website and not digging into our JSON Schemas, to write specs, and that's is a reason why I receive issues, with parsing errors, from users

Regarding point 2, yes you are right: there is no example field but examples.

I'll create MR with fix for adeo example, tonight

@Pakisan
Copy link
Member Author

Pakisan commented Feb 14, 2024

Regarding point 1, I see the regex we show in our documentation doesn't correspond with the one enforced in our JSON Schema documents, which indeed allows for . characters. So I completely understand then why Studio is not complaining, but I don't get why JAsyncAPI doesn't, tbh. So in summary, we should either fix the docs or fix the regex. A decision should be taken.

@smoya @fmvilas @derberg I think that docs must be fixed, because we already released 3.0.0 with this regex - ^x-[\\w\\d\\.\\x2d_]+$

I'll crate MR tonight

@smoya
Copy link
Member

smoya commented Feb 14, 2024

Regarding point 1, I see the regex we show in our documentation doesn't correspond with the one enforced in our JSON Schema documents, which indeed allows for . characters. So I completely understand then why Studio is not complaining, but I don't get why JAsyncAPI doesn't, tbh. So in summary, we should either fix the docs or fix the regex. A decision should be taken.

@smoya @fmvilas @derberg I think that docs must be fixed, because we already released 3.0.0 with this regex - ^x-[\\w\\d\\.\\x2d_]+$

I'll crate MR tonight

I agree, unless allowing dots in the extension name has an edge case I can't see.

@derberg
Copy link
Member

derberg commented Feb 14, 2024

Topic: Extensions with dots

yeah dots in extensions should be allowed, no question about it. For me it is just docs bug as the pattern described here do not match the description.

Topic: example keyword

  • JSON Schema Draft 7 do not have example but also the schema do not block you from using extra keywords. I mean if you put there my-custom-keyword it should also be ok
  • I think people use example as they got used to it through OpenAPI 3.0 when it had it included in OpenAPI Schema as additional supported keyword (we have some extra fields in AsyncAPI Schema)

So it works with AsyncAPI because:
- pass validation because what I wrote in 1st bullet point
- docs rendering also shows the example specified in schema because we use openapi-sampler for very long, to generate examples, and it of course understands example

Might make sense to maybe finally formalize it in the spec and add to the list in here. Not sure 🤔

@smoya
Copy link
Member

smoya commented Feb 14, 2024

  • but also the schema do not block you from using extra keywords. I mean if you put there my-custom-keyword it should also be ok

I don't think so since aditionalProperties is set to false.

EDIT: I realize you specified you were talking about the example keyword. 👍 you are completely right.

@Pakisan
Copy link
Member Author

Pakisan commented Feb 14, 2024

I understood root of this problems. Here is fresh example - asyncapi/jasyncapi#160

Might make sense to maybe finally formalize it in the spec and add to the list in here. Not sure 🤔

I'm sure that we must update status of OAS extensions or at least discuss them to decrease number of questions

tldr; Looks like JAsyncAPI is the most strict implementation of our specification 😅

@smoya
Copy link
Member

smoya commented Feb 16, 2024

Might make sense to maybe finally formalize it in the spec and add to the list in here. Not sure 🤔

Even though we could see this as a UX improvement, I wouldn't add it as we already have examples which it also supports an array so you can provide several examples. But just my opinion.

@Pakisan
Copy link
Member Author

Pakisan commented Feb 16, 2024

Might make sense to maybe finally formalize it in the spec and add to the list in here. Not sure 🤔

Even though we could see this as a UX improvement, I wouldn't add it as we already have examples which it also supports an array so you can provide several examples. But just my opinion.

Yeah, you are right, that we already have examples. For me, main reason that studio or other validator are not throwing errors when folks are working with non valid properties in their schemas.

That's why I receive questions in DM or MR with proposals to change discriminator property or add example/nullable.

It's already happen, so we must clarify this situation and decide what to do with properties from OAS schema

upd:

de facto: users can use them
de jure: strict specification implementation must interpret them as specification error

@smoya
Copy link
Member

smoya commented Feb 19, 2024

so we must clarify this situation and decide what to do with properties from OAS schema

I think we are clear "enough" about that in https://www.asyncapi.com/docs/reference/specification/v3.0.0#schemaObject. The fact people is used to OAS and thinks not having those in the AsyncAPI Schema Object is a bug might be because they don't read the docs (not hard feelings, just clarifying btw).

However, it might make sense to discuss if any of those extra fields (added in the OAS Schema) could also make sense into AsyncAPI Schema object. IMHO, taking the example keyword, it doesn't because of redundancy. But what about the rest?
What is the amount of requests for adding those fields we receive? Can we list them? This could definitely help to understand those use cases.

I guess this could go into a new issue anyway.

@Pakisan
Copy link
Member Author

Pakisan commented Feb 19, 2024

so we must clarify this situation and decide what to do with properties from OAS schema

I think we are clear "enough" about that in https://www.asyncapi.com/docs/reference/specification/v3.0.0#schemaObject. The fact people is used to OAS and thinks not having those in the AsyncAPI Schema Object is a bug might be because they don't read the docs (not hard feelings, just clarifying btw).

However, it might make sense to discuss if any of those extra fields (added in the OAS Schema) could also make sense into AsyncAPI Schema object. IMHO, taking the example keyword, it doesn't because of redundancy. But what about the rest?

What is the amount of requests for adding those fields we receive? Can we list them? This could definitely help to understand those use cases.

I guess this could go into a new issue anyway.

Yep, I'm agree with you. Will close this issue as resolved, after MR with fix for regex will be merged

After that I'll create new issue to refresh discussion about field from OAS schema and will provide related issues from jasyncapi and modelina

@smoya
Copy link
Member

smoya commented Feb 19, 2024

Yep, I'm agree with you. Will close this issue as resolved, after MR with fix for regex will be merged

merged now btw! 🎉 🚀

@derberg
Copy link
Member

derberg commented Feb 22, 2024

Strange but after adding #1034 the adeo example fails validation. We definitely need to push #957 for next bounty:

Diagnostic err: "0" property type must be string in path ["components","schemas","RequesterCode","examples","0"] starting L261 C10, ending L261 C11
Diagnostic err: "0" property type must be string in path ["components","schemas","BuCode","examples","0"] starting L292 C10, ending L292 C11

yeah, don't even get me started on the quality of errors 😄

anyway, the issue is related to examples - if I manually remove s - example is valid again 🤔

@derberg derberg reopened this Feb 22, 2024
@Pakisan
Copy link
Member Author

Pakisan commented Feb 22, 2024

Strange but after adding #1034 the adeo example fails validation. We definitely need to push #957 for next bounty:

Diagnostic err: "0" property type must be string in path ["components","schemas","RequesterCode","examples","0"] starting L261 C10, ending L261 C11
Diagnostic err: "0" property type must be string in path ["components","schemas","BuCode","examples","0"] starting L292 C10, ending L292 C11

yeah, don't even get me started on the quality of errors 😄

anyway, the issue is related to examples - if I manually remove s - example is valid again 🤔

What a twist

@jonaslagoni
Copy link
Member

Reason for the failed validation is RequesterCode and BuCode have example values as integer, NOT string.

i.e. should be the following:

    RequesterCode:
      type: string
      description: >-
        The Costing requester code (generally the BU Code). The requester code
        is useful to get the dedicated context (tenant).
      examples:
        - "1"
    BuCode:
      type: string
      description: The Business Unit code for which data are applicable.
      examples:
        - "1"

It's a YAML syntax where lone numbers without characters are interpreted as integer not string by default as you see in the other examples.

However... The error codes SUCKS and does not explain it in studio 👎

@Pakisan
Copy link
Member Author

Pakisan commented Feb 28, 2024

@derberg @jonaslagoni can I close issue?

Copy link

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label Jun 28, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Oct 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants