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

fix: deprecate $ref property in Channel Item Object #696

Merged

Conversation

char0n
Copy link
Collaborator

@char0n char0n commented Jan 21, 2022

Fixes #607


Summary:

This PR builds on top #665 which introduced Components.channels field without need to revert it, is aligned with release strategy described below (ref: #607 (comment)) and eliminates the ambiguity between Channe Item Object and Reference Object $ref field.

Release strategy

AsyncAPI 2.x

The aim here is to limit complexity and bring more flexibility. Resolution rules around Channel Item Object.$ref field are not so clear: #612

  1. Deprecate the use of $ref fixed field in Channel Item Object
  2. Introduce new fixed field called channels in Components Object with type of Map[string, Channel Item Object]

AsyncAPI 3.x.

The aim here is to have single referencing mechanism within AsyncAPI 3.x spec (reducing complexity) - Reference Object

  1. Remove $ref from Channel Item Object fixed fields
  2. Change Channel Object patterned field type to Reference Object | Channel Item Object
  3. Components Object.channels field type will change to Map[string, Channel Item Object | Reference Object]

@@ -565,7 +565,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*.
<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.
Copy link
Member

@smoya smoya Jan 21, 2022

Choose a reason for hiding this comment

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

I think we should give a better explanation on the reason for deprecating. Like mentioning we deprecate it in favor of using Reference Object from the Channels Object instead of Channel Item Object. WDYT? cc @fmvilas @jonaslagoni

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we say we deprecate in favor of Reference Object it may indicate that future versions will use Reference Object, but later versions of the spec might handle this quite differently. Best way to probably handle this is to have wording self-contained withing the version of the spec this RFC targets - and the sentence I put there was the only one I could come up with. I'm really open to suggestions here ;]

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 fine if others see it make sense 👍

@smoya
Copy link
Member

smoya commented Jan 21, 2022

@char0n I think this should be a fix: rather than docs: since we are fixing the spec and semantic-release would need to know.

@char0n char0n force-pushed the char0n/rfc-channel-item-refs branch from a88f5e2 to 5c06b72 Compare January 21, 2022 12:23
@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
No Duplication information No Duplication information

@char0n char0n changed the title docs(spec): add RFC for safe Channel Item referencing fix(spec): add RFC for unambiguous Channel Item referencing Jan 21, 2022
@char0n
Copy link
Collaborator Author

char0n commented Jan 21, 2022

@smoya I've force-pushed to change the commit msg type

@smoya
Copy link
Member

smoya commented Jan 21, 2022

@smoya I've force-pushed to change the commit msg type

FYI, for next time you just only need to change the PR title instead of the commit, because the squashed merged commit uses the PR title as commit message.

@smoya
Copy link
Member

smoya commented Jan 21, 2022

@char0n Additionally, would you like to provide some words regarding this change for the v2.3.0 release notes @dalelane is handling in asyncapi/website#512? 🙏

@char0n
Copy link
Collaborator Author

char0n commented Jan 21, 2022

@smoya yes that would be a great medium where to describe the deprecation plan of the $ref field. Added a PR comment there with suggested addition: https://github.com/asyncapi/website/pull/512/files#r789842794

@dalelane
Copy link
Collaborator

dalelane commented Jan 23, 2022

@char0n @smoya Should we also update the JSON schema at https://github.com/asyncapi/spec-json-schemas/pull/142/files#diff-c42eed206b44fe4fa90f1635ff156f9804ae2450b901771b9c4d6db5188d640cR498-R500 to mark $ref as deprecated?

e.g.

    "channelItem": {
       ...
       "properties": {
         "$ref": {
           "$ref": "#/definitions/ReferenceObject",
           "deprecated": true
         },

Update: Never mind - ignore me. "deprecated" was added in JSON schema draft-08 and we're using draft-07 - so we can't add that.

@dalelane dalelane mentioned this pull request Jan 23, 2022
39 tasks
@dalelane
Copy link
Collaborator

@smoya Do you think anything else is needed to be able to approve and merge this?

Copy link
Member

@smoya smoya left a comment

Choose a reason for hiding this comment

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

LGTM (not an owner anyway)

@smoya
Copy link
Member

smoya commented Jan 24, 2022

@smoya Do you think anything else is needed to be able to approve and merge this?

OK on my side! Also a +1 from an Owner is already provided 👍

@derberg derberg changed the title fix(spec): add RFC for unambiguous Channel Item referencing fix: deprecate $ref property in Channel Item Object Jan 24, 2022
@derberg
Copy link
Member

derberg commented Jan 24, 2022

this one was pretty interesting and education read 😄

@char0n please have a look at the change I did in PR title, I think most important is to highlight deprecation in the release notes

also can you please extend description of this issue so reader of release notes, that will end up in this PR doesn't have to go through a long discussion from the issue but have a summary here in PR description?

@char0n
Copy link
Collaborator Author

char0n commented Jan 25, 2022

@char0n please have a look at the change I did in PR title, I think most important is to highlight deprecation in the release notes

Yeah, change in PR title looks good. @dalelane did a great job in verbalizing the proper deprecation message in https://github.com/asyncapi/website/pull/512/files#diff-a43f6e629535a43413950aefe222b7d7e591e50515314573ff440e91b8aaed8aR66

also can you please extend description of this issue so reader of release notes, that will end up in this PR doesn't have to go through a long discussion from the issue but have a summary here in PR description?

I'll do my best ;]

@derberg
Copy link
Member

derberg commented Jan 25, 2022

/rtm

@asyncapi-bot asyncapi-bot merged commit e234ecf into asyncapi:2022-01-release Jan 25, 2022
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 2.3.0-2022-01-release.3 🎉

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.

6 participants