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

Pakisan commented 9 months ago

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:

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?

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 commented 9 months ago

@dalelane @smoya @char0n @GreenRover

smoya commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

I understood root of this problems. Here is fresh example - https://github.com/asyncapi/jasyncapi/issues/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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

Strange but after adding https://github.com/asyncapi/spec/pull/1034 the adeo example fails validation. We definitely need to push https://github.com/asyncapi/spec/issues/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 🤔

Pakisan commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

@derberg @jonaslagoni can I close issue?

github-actions[bot] commented 5 months ago

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

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 :heart:

asyncapi / spec