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

Channel Item Object $ref field resolution rules #612

Closed
char0n opened this issue Aug 24, 2021 · 14 comments · Fixed by #842
Closed

Channel Item Object $ref field resolution rules #612

char0n opened this issue Aug 24, 2021 · 14 comments · Fixed by #842

Comments

@char0n
Copy link
Collaborator

char0n commented Aug 24, 2021

Description of the $ref field explicitly says:

Allows for an external definition of this channel item. The referenced structure MUST be in the format of a Channel Item Object. If there are conflicts between the referenced definition and this Channel Item's definition, the behavior is undefined.

As a tooling author I have couple of questions here:

Allowing only external definition

Why are we allowing only external definition and not an internal as well? By changing wording to Allows for a referenced definition of this channel item. we open a possibility to reference other Channel Items defined in the same document. This constraint (external definition) can be easily bypassed by defining a $ref field as URL+JSON Pointer fragment, where URL has the same baseURL as the root document.

Rules of resolving

Rules of resolving are missing entirely. It is not clear how to do the actual resolution. I can only assume what the rules might be by reading one of the paragraph in Reference Object section:

For this specification, reference resolution is done as defined by the JSON Reference specification and not by the JSON Schema specification.

So if resolution for Reference Object is done using JSON Reference spec, I'd assume the same for Channel Item Object. This assumption can be wrong thought. In order to remove un-clarity I propose to mention following sentence in Channel Item Object.$ref field description: Resolution is done as defined by the JSON Reference specification.

I can open PR with proposed changes if/when we agree on them.

@char0n char0n added the ❔ Question A question about the spec or processes label Aug 24, 2021
@bali182
Copy link

bali182 commented Aug 29, 2021

This is a very good question, I was wondering about the $ref field too. OpenAPI has the same issue with Operation Objects. Could we either

  • Remove the $ref field from Channel Item Object, this way forcing all Channel Item Objects to be defined in the same document
  • Make it union type (either a Channel Item Object or a Reference Object) and make it behave like all other referenceable pieces of the schema?

I think either would be better than the mixture of both

@char0n
Copy link
Collaborator Author

char0n commented Aug 29, 2021

This is a very good question, I was wondering about the $ref field too. OpenAPI has the same issue with Operation Objects. Could we either

I guess you mean Path Item Object in OpenAPI. I am already driving effort to eliminate it in v3.2: OAI/OpenAPI-Specification#2635

  • Remove the $ref field from Channel Item Object, this way forcing all Channel Item Objects to be defined in the same document
  • Make it union type (either a Channel Item Object or a Reference Object) and make it behave like all other referenceable pieces of the schema?

I think either would be better than the mixture of both

For this I’ve already submitted a proposal to remove $ref field and replace it with Reference Object: #607

This particular issue exclusively deals with rules of resolution for Channel Item Object with $ref field

@char0n
Copy link
Collaborator Author

char0n commented Dec 10, 2021

Just pinging here. Clarification from the spec authors can help move this forward. After the clarification and further discussion I can issue a PR to amend the specification.

@smoya
Copy link
Member

smoya commented Dec 31, 2021

If we finally move forward with asyncapi/parser-js#401, the idea will be that Parser-API will define all the bundling and ref/dereferencing logic Parsers should follow. Meaning the spec documentation will link to it for tool developers. See asyncapi/parser-api#30

Btw, your feedback is more than welcome in the parser-js issue!

@char0n
Copy link
Collaborator Author

char0n commented Jan 17, 2022

@smoya I've read the intention driven proposals and the article. IMHO it's completely separate from this issue. It should be clear from specification how to create the implementation, which is not at this point - implementation is not the source of truth, the specification is. What I'm proposing is it to remove the unclarity by making the specification better.

@smoya
Copy link
Member

smoya commented Jan 17, 2022

@smoya I've read the intention driven proposals and the article. IMHO it's completely separate from this issue. It should be clear from specification how to create the implementation, which is not at this point - implementation is not the source of truth, the specification is. What I'm proposing is it to remove the unclarity by making the specification better.

Yes, I totally agree with your point. Let me clarify that the idea around https://github.com/asyncapi/parser-api is not only about defining such intent-driven API (what the post talks about) for parsers but also defining the cross-cutting concerns of them, such as (but not limited to) bundling.
However, I agree specification should contain, at least, a statement on how bundling is expected to work on an AsyncAPI document.

I would suggest we extend the scope to improve the whole documentation and not only the channels part.

BTW, I have been lately discussing how the bundling could be improved in future versions at #649 (comment), just in case you are interested to join forces as well.

@char0n
Copy link
Collaborator Author

char0n commented Jan 17, 2022

However, I agree specification should contain, at least, a statement on how bundling is expected to work on an AsyncAPI document.
I would suggest we extend the scope to improve the whole documentation and not only the channels part.

The problem is actually only with Channel Item object. It's the only object within the spec which allows referencing data apart from Reference Object. Currently it says it allows only external definition which given other sections of the spec means - definition contained in external document and not this one. That doesn't necessarily mean that the URL for $ref field is defined as absolute, it can also be defined as relative to the baseURI of the document. And that's basically what this issue is about - defining rules of resolution for this single field and currently in my opinion it's not defined and it's up for implementor intrepretation.

It's also worth mentioning that there is RFC1 about removing $ref field from Channel Item Object - #607

BTW, I have been lately discussing how the bundling could be improved in future versions at #649 (comment), just in case you are interested to join forces as well.

I'll take a look, thanks!

@char0n
Copy link
Collaborator Author

char0n commented May 4, 2022

This is now limited to version to specification version >= 2.0.0 <= 3.0.0. The reason is #699, which removes the $ref field from Channel Item Object.

@github-actions
Copy link

github-actions bot commented Sep 2, 2022

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 stale and removed stale labels Sep 2, 2022
@char0n
Copy link
Collaborator Author

char0n commented Sep 6, 2022

PR attached to this issue is: #779

@char0n char0n mentioned this issue Sep 6, 2022
61 tasks
@char0n
Copy link
Collaborator Author

char0n commented Sep 7, 2022

Closing as the #779 has been merged to master as editorial change.

@asyncapi-bot
Copy link
Contributor

🎉 This issue has been resolved in version 3.0.0-next-major-spec.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@asyncapi-bot
Copy link
Contributor

🎉 This issue has been resolved in version 2.5.0-next-spec.5 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@derberg
Copy link
Member

derberg commented Jan 31, 2023

Recent comments about the release from the bot were added by mistake. More details in #899

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

Successfully merging a pull request may close this issue.

5 participants