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

ChannelItem.$ref field deprecation + proposal how to replace it with simpler and more flexible solution #607

Closed
Tracked by #520
char0n opened this issue Aug 4, 2021 · 27 comments

Comments

@char0n
Copy link
Collaborator

char0n commented Aug 4, 2021

Hi all,

I was wondering if specification would be open to proposal of deprecating ChannelItem.$ref field and instead introduce a more flexible solution based on following spec changes:

Introduce new fixed field called channelItems to Component Object

Field Name Type Description
channelItems Map[string, Channel Item Object | Reference Object] An object to hold reusable Channel Item Objects.

Change type of Channels Object

The type of Channels Object patterned field would change from Channel Item Object to Channel Item Object | Reference Object

Deprecating the Channel Item Object $ref field.


This proposal makes specification more flexible (introducing reusable Channel Item Objects) and simpler as there will be only one mechanism for referencing other objects (Reference Object) instead of two (Reference Object, Channel Item Object).

Similar course of action is already in progress in OAS: OAI/OpenAPI-Specification#2635

@char0n char0n added the 💡 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md) label Aug 4, 2021
@fmvilas
Copy link
Member

fmvilas commented Aug 5, 2021

That's a good catch 👍 I'll put this on hold while we see how to solve #520 because most probably the Channels object will be reworked too. I'm listing this issue there so we don't forget. Thanks!

@github-actions
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 Jan 14, 2022
@derberg derberg removed the stale label Jan 14, 2022
@derberg
Copy link
Member

derberg commented Jan 14, 2022

@char0n if you want to push this one further into the spec in 3.0, next week we start meetings related to 3.0 where all folks can sync the async work 😄

Just check this calendar https://calendar.google.com/calendar/u/0/[email protected] or join https://groups.google.com/u/1/g/asyncapi-users to get invites

@char0n
Copy link
Collaborator Author

char0n commented Jan 17, 2022

@derberg sure, I'd like to be the champion of this RFC.

As far as I understand next SIG is at 25.11.2021, at 9:00 CET right? I'll be there.

@char0n
Copy link
Collaborator Author

char0n commented Jan 19, 2022

@derberg I have an overlapping meeting in my work today. Will there be another chance to challenge this issue for 3.0 release?

@derberg
Copy link
Member

derberg commented Jan 19, 2022

@char0n 3.0 meetings will be regular, and happen every 2 weeks, so no worries

@char0n
Copy link
Collaborator Author

char0n commented Jan 19, 2022

Proposed course of action:

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]

@jonaslagoni
Copy link
Member

@char0n do you champion this for the 3.0 release? 🙂 Or can we consider this issue as needs champion? 🤔

@char0n
Copy link
Collaborator Author

char0n commented Jan 19, 2022

@jonaslagoni sure I can champion this. I've only champion one other RFC here before: #609 and there wan't really much of a championing there ;]

In #607 (comment) @fmvilas mentioned that his putting this on hold due to the #520. That's the only reason I haven't produced an RFC PRs yet. I can issue PRs if it's warranted at this stage.

@jonaslagoni
Copy link
Member

jonaslagoni commented Jan 20, 2022

Hmmmm, taking a closer look at the issue, is this not already solved by #665 🧐

cc @fmvilas @smoya

@fmvilas
Copy link
Member

fmvilas commented Jan 20, 2022

Yeah, I think so. Can you confirm it's the same, @char0n?

@char0n
Copy link
Collaborator Author

char0n commented Jan 21, 2022

Hey guys,

Unfortunately #665 introduces ambiguity (which I was trying to avoid with this RFC) into the specification and couple of issues with naming and types.

Naming issues

Channel vs Channel Item

https://github.com/asyncapi/spec/pull/665/files#diff-34bca0c91901dffce093114c7d4cac16477410fbca7cabf3a1b888d7dfc3eb4dR1437 - new field Components.channels is created. Would be probably more consistent to name this field Components.channelItems.

Incorrect type

The type is defined as Map[string, Channel Object | Reference Object] but should be Map[string, Channel Item Object | Reference Object] as there is no Channel Object in the specification.

Ambiguity

Now this is the most important thing. The ambiguity is now in Components.channels field. As this field defined it's type as Map[string, Channel Item Object | Reference Object].

Given following AsyncAPI definition fragment:

components:
 channels:
   $ref: #/pointer

there is no way to determine if the author of the definition intended to define a Channel Item or Reference Object as both of these objects can have just one single field $ref. This means author intention is lost and semantic parsing is basically just a guess now. Both specifications objects have different behavior of merging/omitting additional properties so it does matters which object is recognized. Creating proper schema validation for this is very problematic as well.

PS: these changes are still not released, we can intercept and provide remediation

@jonaslagoni
Copy link
Member

jonaslagoni commented Jan 21, 2022

cc @dalelane just to make you aware of the issue.

@smoya
Copy link
Member

smoya commented Jan 21, 2022

The type is defined as Map[string, Channel Object | Reference Object] but should be Map[string, Channel Item Object | Reference Object] as there is no Channel Object in the specification.

I do see this is a typo and should be fixed indeed.

However, I don't fully understand why we would change channels components field to channelItems without changing it at the root level of the doc as well, since they right now share the same type.

@char0n
Copy link
Collaborator Author

char0n commented Jan 21, 2022

However, I don't fully understand why we would change channels components field to channelItems without changing it at the root level of the doc as well, since they right now share the same type.

@smoya right, sorry I take this point back; your argument makes sense and channels is consistent with what's already in top level AsyncAPI Object.

@smoya
Copy link
Member

smoya commented Jan 21, 2022

@smoya right, sorry I take this point back; your argument makes sense and channels is consistent with what's already in top level AsyncAPI Object.

Cool! btw, I issued a PR for the documentation changes regarding Channel Object to Channel Item Object: #695

@char0n
Copy link
Collaborator Author

char0n commented Jan 21, 2022

since they right now share the same type.

I would disagree with this observation - Channels Object pattern field and Components.channels don't share the same type.

Map[string, Channel Item Object] vs Map[string, Channel Item Object | Reference Object]

If we use Map[string, Channel Item Object | Reference Object] for Channels Object pattern field, the ambiguity will be introduced there as well.

Looking at this branch: https://github.com/asyncapi/spec/blob/2022-01-release/spec/asyncapi.md

@smoya
Copy link
Member

smoya commented Jan 21, 2022

I would disagree with this observation - Channels Object pattern field and Components.channels don't share the same type.

Technically speaking, checking the JSON Schema:

Channels at root level:
https://github.com/asyncapi/spec-json-schemas/blob/daa269da38c68274c8a033fe2aa6fabcf063ff7d/schemas/2.3.0.json#L246-L255

Channels at components level:
https://github.com/asyncapi/spec-json-schemas/blob/daa269da38c68274c8a033fe2aa6fabcf063ff7d/schemas/2.3.0.json#L273-L275

Meaning channels at components level are referencing to the same object as far as I can see.

So I'm still confused and I can't see the issue. Sorry @char0n but its on my side 😓 so I would need you to explain it again if you don't mind.

@char0n
Copy link
Collaborator Author

char0n commented Jan 21, 2022

So I'm still confused and I can't see the issue. Sorry @char0n but its on my side sweat so I would need you to explain it again if you don't mind.

Sure, np. I'll try on more examples. Let's limit our discussion to types mentioned above, not the ambiguity issue. I'm looking at this https://github.com/asyncapi/spec/blob/2022-01-release/spec/asyncapi.md spec for my observations.

Components Object

Specification defines it like this:

image

This clearly says that the type for Components.channels is Map[string, Channel Object | Reference Object] which will be fixed in #695 to Map[string, Channel Item Object | Reference Object].

This tells us that the value of every key in this object can be Channel Item Object or Reference Object.

Channels Object

image

This clearly says that the type for pattern field is Map[{channel}, Channel Object] which can be translated to Map[string, Channel Object]

This tells us that the value of every key in this object can be exclusively Channel Item Object.

@smoya
Copy link
Member

smoya commented Jan 21, 2022

This tells us that the value of every key in this object can be exclusively Channel Item Object.

I think I got it. You mean we somehow need to unify that to a consistent definition. Either change the channels object at root level doc to mention that the type can be either Channel Item Object | Reference Object, right?

At the end, I see this is a change on documentation but not in the JSON Schema, right?

@char0n
Copy link
Collaborator Author

char0n commented Jan 21, 2022

I think I got it. You mean we somehow need to unify that to a consistent definition. Either change the channels object at root level doc to mention that the type can be either Channel Item Object | Reference Object, right?

Yes, the inconsistency I see is in the fact that you cannot use Reference Object in the Channels Object, but you can in Components.channels.

At the end, I see this is a change on documentation but not in the JSON Schema, right?

Yes, the documentation in MARKDOWN format is the source of truth not the JSON Schema as far as I understand in this specification.

But circling back to ambiguity problem defined in #607 (comment), having union type of Channel Item Object | Reference Object creates problem of ambiguity. You're introducing something that has been identified in OpenAPI specification as problematic and has been dealt with - OAI/OpenAPI-Specification#2635

My proposed sequence of steps to get rid of this problem is here: #607 (comment). Some of the steps you already did in #665, but the changes needs to be introduced in proper sequence to avoid the ambiguity.

@jonaslagoni
Copy link
Member

jonaslagoni commented Jan 21, 2022

Right... What a mess 😆

I honestly think we should just follow your suggestion @char0n #607 (comment) to the letter. And simply accepting introducing this change makes it a little bit weird, but we don't break any interpretation of it.

And then in the major version, we fix this, as you wrote in the comment. + Renaming it makes sense 👍

Or directly target 3.0 and revert #665 🤷

@char0n
Copy link
Collaborator Author

char0n commented Jan 21, 2022

Here is RFC PR #696 that builds on top #665 without need to revert it, is aligned with #607 (comment) and eliminates the ambiguity.

@char0n
Copy link
Collaborator Author

char0n commented Jan 25, 2022

This issue can be succeed by #699 (that I've just created) to limit the context of the next steps for 3.x version of the spec.

@asyncapi-bot
Copy link
Contributor

🎉 This issue has been resolved in version 2.3.0-2022-01-release.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@char0n
Copy link
Collaborator Author

char0n commented Feb 7, 2022

I'm not sure if I already asked this, but should I close my own issues or will @asyncapi-bot close it automatically in some point in time? Thanks!

@fmvilas
Copy link
Member

fmvilas commented Feb 14, 2022

Usually, Github will close it automatically if you include Fixes #nnn or Solves #nnn (and more) in your PR description: https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword.

Otherwise, you have to close it manually :)

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

No branches or pull requests

6 participants