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

docs: channel object / $ref note. #826

Merged
merged 8 commits into from
Oct 18, 2023
Merged

Conversation

ponelat
Copy link
Contributor

@ponelat ponelat commented Aug 22, 2022


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 ChannelItem.$ref field deprecation + proposal how to replace it with simpler and more flexible solution #607 (comment)

cc @char0n @smoya

Change note about `$ref` fixed field, to prevent scaring folks off from using `$ref` under `channels`.
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide 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.

spec/asyncapi.md Outdated
@@ -569,7 +569,7 @@ Describes the operations available on a single channel.

Field Name | Type | Description
---|:---:|---
<a name="channelItemObjectRef"></a>$ref | `string` | Allows for an external definition of this channel item. The referenced structure MUST be in the format of a [Channel Item Object](#channelItemObject). If there are conflicts between the referenced definition and this Channel Item's definition, the behavior is *undefined*. <br/><br/>**Deprecated:** Usage of the `$ref` property has been deprecated.
<a name="channelItemObjectRef"></a>$ref | `string` | Allows for an external definition of this channel item. The referenced structure MUST be in the format of a [Channel Item Object](#channelItemObject). If there are conflicts between the referenced definition and this Channel Item's definition, the behavior is *undefined*. <br/><br/>**Technical Note:** Going forward, `$ref` will no longer form part of these _fixed fields_, but will continue to function in the same role, as part of [Reference Object](#referenceObject).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @ponelat,

Thanks for the proposal, here are couple of notes I have to this change:

Removal of deprecation message

If we remove the deprecation message, it's going to look like a cosmetic change, but it's not. We're actually removing a fixed field from the Channel Item Object, which will change the way, how the eventual transcluded referenced value shape will look like (merging). I think we can make the deprecation message more precise by doing following change to clarify the deprecation scope:

**Deprecated:** Usage of the `$ref` property has been deprecated when accompanied with one or more fixed fields from Channel Item Object`.

We did the same change in OpenAPI specification some time ago to Path Item Object - https://github.com/OAI/OpenAPI-Specification/pull/2656/files

Self containment

The wording of the 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 - OAI/OpenAPI-Specification#2656 (comment)

Different behavior

but will continue to function in the same role, as part of [Reference Object](#referenceObject) - this might not be true. Reference Object does't allow additional fields, which means, no merging with referenced value is performed. ChannelItem.$ref does allow additional fields, which gets merged with the referenced value. If there is a conflict, the behavior is undefined, but most implementations do merge using right shallow merge algorithm.

Copy link
Contributor Author

@ponelat ponelat Aug 22, 2022

Choose a reason for hiding this comment

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

Hey @char0n ,

Removal of deprecation message
Understood, although without highlighting how it can be used outside of fixed fields, then it reads as "Don't use $ref here".

Perhaps a tweak on your suggestion:

Deprecated: Using $ref with sibling fixed fields has been deprecated. Usage without any siblings is still encouraged!
The motivation behind changing the wording: highlighting that a specific usage of $ref is deprecated.

Self containment and Different behavior. Understood and agreed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The tweak sounds great, and is aligned with #699

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the change.

Copy link
Member

Choose a reason for hiding this comment

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

I have to disagree with you. IMHO, the sentence Using $ref with sibling fixed fields has been deprecated. Usage without any siblings is still encouraged! is very difficult to understand. In particular, the word siblings and referring to fixed fields sounds confusing to me.

WDYT about this version?

Usage of the $ref property has been deprecated when accompanied by any of the other properties of a Channel Item Object.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @smoya,

Usage of the $ref property has been deprecated when accompanied by any of the other properties of a Channel Item Object.

When using https://grammarly.com/ to analyze the sentence, there are two suggestions to it:

  1. Passive voice - This sentence appears to be written in the passive voice. Consider writing in the active voice.
  2. Wordy sentence - Too many non-content words may indicate wordiness. Consider rewriting to avoid some of these words: of, the, has, been, when, by, any, a

IMHO @ponelat intention was to make explicit that if you use $ref without any other properties nothing will change in the future.

I propose another suggestion that passes grammarly and is less technical (possibly less confusing as well):

Using $ref property has been deprecated when accompanied by other properties.

This doesn't suffer from passive voice and wordy sentence issues and it aligned with your suggestion. properties is already used within the spec to refer to fixed or pattern fields. For me personally it also implicitly communicates that using $ref with no additional properties is not deprecated and will continue to functional equivalently, but IMHO it's worth considering if we want to communicate explicitly as @ponelat did in his suggestion as a second sentence.

Copy link
Member

Choose a reason for hiding this comment

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

I'm happy with:

Using $ref property has been deprecated when accompanied by other properties.

Let's see what others say.

cc @alequetzalli @derberg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, but used field instead of property, to be consistent with the column name Field name and the section Fixed Fields.

char0n
char0n previously approved these changes Aug 23, 2022
@fmvilas fmvilas changed the title ✏️ Update docs: Channel Object / $ref note. docs: channel object / $ref note. Aug 25, 2022
fmvilas
fmvilas previously approved these changes Aug 25, 2022
Copy link
Member

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

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

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

@smoya
Copy link
Member

smoya commented Aug 25, 2022

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 #826 (comment)

@fmvilas
Copy link
Member

fmvilas commented Aug 25, 2022

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

dalelane
dalelane previously approved these changes Aug 25, 2022
@ponelat ponelat dismissed stale reviews from dalelane, fmvilas, and char0n via 4d113c2 September 5, 2022 13:29
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 5, 2022

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

@char0n char0n mentioned this pull request Sep 6, 2022
61 tasks
@char0n
Copy link
Collaborator

char0n commented Sep 6, 2022

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

dalelane
dalelane previously approved these changes Sep 6, 2022
@derberg
Copy link
Member

derberg commented Sep 7, 2022

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

@char0n
Copy link
Collaborator

char0n commented Sep 7, 2022

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: #607 and OAI/OpenAPI-Specification#2656 (comment) (just imagine that Channel Item Object === Path Item Object)

@char0n
Copy link
Collaborator

char0n commented Sep 9, 2022

@derberg did my explanation helped a bit?

@derberg
Copy link
Member

derberg commented Sep 21, 2022

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

??

@char0n
Copy link
Collaborator

char0n commented Sep 22, 2022

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 #826 (comment) (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) - OAI/OpenAPI-Specification#2656 (comment)

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.

@derberg
Copy link
Member

derberg commented Sep 26, 2022

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?

@char0n
Copy link
Collaborator

char0n commented Sep 27, 2022

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
Copy link
Collaborator

char0n commented Feb 5, 2023

@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.

@github-actions github-actions bot removed the stale label Feb 6, 2023
@derberg
Copy link
Member

derberg commented Feb 6, 2023

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

@char0n
Copy link
Collaborator

char0n commented Feb 8, 2023

@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.

@ponelat
Copy link
Contributor Author

ponelat commented Feb 9, 2023

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

spec/asyncapi.md Outdated Show resolved Hide resolved
@github-actions
Copy link

This pull request 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 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 ❤️

@github-actions github-actions bot added the stale label Jun 10, 2023
@github-actions github-actions bot removed the stale label Jun 30, 2023
@sonarqubecloud
Copy link

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

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

@char0n please approve and merge. I took the liberty to update PR and solve merge conflicts to not bother and ping @ponelat

@jonaslagoni
Copy link
Member

@derberg seems you need to resolve some more conflicts 🙂

@derberg derberg requested a review from char0n September 28, 2023 06:58
@sonarqubecloud
Copy link

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

@derberg
Copy link
Member

derberg commented Sep 28, 2023

@jonaslagoni thanks

@char0n please have a final look and approve

@derberg
Copy link
Member

derberg commented Oct 9, 2023

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

Copy link
Collaborator

@char0n char0n left a comment

Choose a reason for hiding this comment

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

Looks good. Good to go!

@char0n
Copy link
Collaborator

char0n commented Oct 18, 2023

/ready-to-merge

@asyncapi-bot asyncapi-bot merged commit d4c99e4 into asyncapi:master Oct 18, 2023
9 checks passed
@asyncapi-bot
Copy link
Contributor

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

The release is available on GitHub release

Your semantic-release bot 📦🚀

@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 3.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants