spec icon indicating copy to clipboard operation
spec copied to clipboard

docs: channel object / $ref note.

Open ponelat opened this issue 3 years ago • 8 comments


title: ✏️ Update docs: Channel Object / $ref note.

This is to prevent confusion around whether folks can/should make use of $refs under #/channels.

Hopefully it is

  1. True
  2. Does not discourage use of $ref in $.channels.*.$ref
  3. Conforms to https://github.com/asyncapi/spec/issues/607#issuecomment-1016605546

cc @char0n @smoya

ponelat avatar Aug 22 '22 10:08 ponelat

LGTM. @ponelat I changed the title to make it conformant to the rules.

Not sure you saw (I think its marked as resolved?) we were discussing about the wording here https://github.com/asyncapi/spec/pull/826#discussion_r954667501

smoya avatar Aug 25 '22 09:08 smoya

No, didn't see. Thanks for pointing out.

fmvilas avatar Aug 25 '22 09:08 fmvilas

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 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

sonarqubecloud[bot] avatar Sep 05 '22 13:09 sonarqubecloud[bot]

Hi @derberg, would you mind looking in this PR? Thanks!

char0n avatar Sep 06 '22 11:09 char0n

Hey, sorry but I'm not a native speaker, and Using the $ref field has been deprecated when accompanied by other fields is totally confusing for me. For me, deprecated means deprecated and when accompanied by other fields is confusing. What other fields? Users should not use $ref at all as it is deprecated and in 3.0 goes away -> https://github.com/asyncapi/spec/pull/777

derberg avatar Sep 07 '22 14:09 derberg

Hey, sorry but I'm not a native speaker, and Using the $ref field has been deprecated when accompanied by other fields is totally confusing for me. For me, deprecated means deprecated and when accompanied by other fields is confusing. What other fields? Users should not use $ref at all as it is deprecated and in 3.0 goes away -> #777

@derberg

Deprecated: Usage of the $ref property has been deprecated. vs Deprecated: Using the $ref field has been deprecated when accompanied by other fields.

We're removing the $ref field in Channel Item Object and replacing it with union type of Channel Item Object | Reference Object. That means that all the shapes where union type Channel Item Object | Reference Object is expected and the shape will contain $ref field will always be semantically recognized as Reference Object and not as Channel Item Object. Reference Object ignores all additional properties, instead of merging them with referenced structure. Hence the more precise deprecation message.

Saying it also in different way:

All the Channel Item Object shapes with single $ref field will automatically become Reference Objects in future version of the spec and will have exactly the same behavior of resolution.

All the Channel Item Object shapes with $ref field and additional fields will automatically become Reference Objects in future version of the spec and and all the additional fields will be ignored instead of merging them with referenced structure.

More info in: https://github.com/asyncapi/spec/issues/607 and https://github.com/OAI/OpenAPI-Specification/pull/2656#issuecomment-885539956 (just imagine that Channel Item Object === Path Item Object)

char0n avatar Sep 07 '22 16:09 char0n

@derberg did my explanation helped a bit?

char0n avatar Sep 09 '22 16:09 char0n

thanks a lot for the explanation 🙏🏼

How about:

`Channel Item Object` with $ref field and additional properties will automatically become Reference Objects in future versions of the spec, and all the additional properties will be ??? instead of merging with referenced structure.

how about this way?

The most important part is ??? as you wrote ignore. This is a problem. For example we use https://github.com/APIDevTools/json-schema-ref-parser in the parser and it basically overrides what comes from a referenced object with what is in the original object. In other words, what is in the original object is preserved, not overwritten or ignored.

JSON reference says:

A JSON Reference is a JSON object, which contains a member named
   "$ref", which has a JSON string value.  Example:

   { "$ref": "http://example.com/example.json#/foo/bar" }

   If a JSON value does not have these characteristics, then it SHOULD
   NOT be interpreted as a JSON Reference.

my question is:

  • shouldn't we make sure that our json schemas are written in a way that if you have properties other than $ref, then it is invalid ref object,
  • wouldn't be better to just say that $ref used with other properties will fail validation

??

derberg avatar Sep 21 '22 13:09 derberg

Channel Item Object with $ref field and additional properties will automatically become Reference Objects in future versions of the spec, and all the additional properties will be ??? instead of merging with referenced structure.

As I pointed in https://github.com/asyncapi/spec/pull/826#discussion_r951337047 (and other issues and comments) - the wording of the suggested change is IMHO not self-contained. The wording inside one version of the spec should probably not refer to the future version of the spec (as this version of the spec can be the last one) - https://github.com/OAI/OpenAPI-Specification/pull/2656#issuecomment-885539956

For example we use https://github.com/APIDevTools/json-schema-ref-parser in the parser and it basically overrides what comes from a referenced object with what is in the original object. In other words, what is in the original object is preserved, not overwritten or ignored.

This looks like an issue of the tooling. The specification is crystal clear that Reference Object additional properties SHALL be ignored. In OpenAPI 3.1 for example the Reference Object allows two additional fields - summary and description (other fields SHALL be ignored as well).

If a JSON value does not have these characteristics, then it SHOULD NOT be interpreted as a JSON Reference.

IMHO this is why the Reference Object in AsyncAPI spec provides clarification that additional fields SHALL be ignored and as soon as there is $ref field the characteristics of JSON Reference spec is satisfied.

shouldn't we make sure that our json schemas are written in a way that if you have properties other than $ref, then it is invalid ref object,

IMHO no. Additional fields allows to annotate Reference Object with additional non-semantic information, that tooling can use in auxiliary ways - as long as auxiliary ways are comfort-ant with the spec.

wouldn't be better to just say that $ref used with other properties will fail validation

As per my answer above.

char0n avatar Sep 22 '22 07:09 char0n

Thanks for all the explanation @char0n

I would only make a small suggestion that instead of Using the $ref field has been deprecated when accompanied by other fields. we write Using the $ref property together with other properties is deprecated since 2.3 version of the specification.. Thoughts?

derberg avatar Sep 26 '22 13:09 derberg

Using the $ref property together with other properties is deprecated since 2.3 version of the specification.

IMHO together word here can be understand in ambiguous way. Mentioning the version number, I guess I'm fine with it but what purpose does it serve? Here are variations of your suggestion after analyzing the grammar with grammarly.com.

Using the $ref property along with other properties is deprecated since the 2.3.0 version of the specification.

Using the $ref property with other properties is deprecated since the 2.3.0 version of the specification.

Used grammarly.com to analyze the wording.

I guess I still prefer Using $ref property has been deprecated when accompanied by other properties. the most.

char0n avatar Sep 27 '22 07:09 char0n

This pull request 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 pull request, add a comment with detailed explanation.

There can be many reasons why some specific pull request 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 pull request forward. Connect with us through one of many communication channels we established here.

Thank you for your patience :heart:

github-actions[bot] avatar Jan 26 '23 00:01 github-actions[bot]

@derberg I'd like to move this forward a bit. Any preference on one of the following sentences? It's basically your suggestion run via grammarly.com.

Using the $ref property along with other properties is deprecated since the 2.3.0 version of the specification.

Using the $ref property with other properties is deprecated since the 2.3.0 version of the specification.

char0n avatar Feb 05 '23 20:02 char0n

I'm ok with Using $ref property has been deprecated when accompanied by other properties..

I like Using the $ref property with other properties is deprecated since the 2.3.0 version of the specification. the most but will not argue to include info about 2.3.

It's just that the reality is that there are people that do not read spec on website, or on github specific tag. They read raw spec on master, so it would be useful to have text that specify since what version was the property deprecated. It is also common in other kind of docs to explicitly specify since what version something was deprecated, otherwise you need to play with blame.

Have a look at below, why I think people read from master 👇🏼 stats for last 2 weeks for spec repo, taken from repo Insights traffic info

Screenshot 2023-02-06 at 17 20 37

derberg avatar Feb 06 '23 16:02 derberg

@derberg I'm fine with either of your sentences. @ponelat what do you think?

They read raw spec on master

As I do ;]

It is also common in other kind of docs to explicitly specify since what version something was deprecated, otherwise you need to play with blame.

Right, I'm not arguing that. What I said was: The wording inside one version of the spec should probably not refer to the future version of the spec. So my argument is that we shouldn't refer to the future versions of the spec (to keep it self-contained) as this might be the last version of the spec released.

char0n avatar Feb 08 '23 16:02 char0n

Thanks @char0n @derberg I'm happy with the word changes, and I see adding the version as a good thing .

ponelat avatar Feb 09 '23 09:02 ponelat

This pull request 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 pull request, add a comment with detailed explanation.

There can be many reasons why some specific pull request 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 pull request forward. Connect with us through one of many communication channels we established here.

Thank you for your patience :heart:

github-actions[bot] avatar Jun 10 '23 00:06 github-actions[bot]

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 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

sonarqubecloud[bot] avatar Jul 20 '23 15:07 sonarqubecloud[bot]

@derberg seems you need to resolve some more conflicts 🙂

jonaslagoni avatar Sep 27 '23 06:09 jonaslagoni

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 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

sonarqubecloud[bot] avatar Sep 28 '23 07:09 sonarqubecloud[bot]

@jonaslagoni thanks

@char0n please have a final look and approve

derberg avatar Sep 28 '23 07:09 derberg

@char0n hey man, would be good to merge it before 3.0

derberg avatar Oct 09 '23 16:10 derberg

/ready-to-merge

char0n avatar Oct 18 '23 08:10 char0n

:tada: This PR is included in version 3.0.0-next-major-spec.18 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

asyncapi-bot avatar Dec 04 '23 22:12 asyncapi-bot

:tada: This PR is included in version 3.0.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

asyncapi-bot avatar Dec 05 '23 09:12 asyncapi-bot