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.

Sign up for GitHub

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: redefine the trait inheritance behavior #532

Closed
wants to merge 8 commits into from
Closed

fix: redefine the trait inheritance behavior #532

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9118ce9
Merge pull request #1 from asyncapi/master
Fannon Apr 29, 2021
ef3975a
Add section on trait inheritance and refer to it from trait objects
Fannon Apr 29, 2021
07ba172
First draft how to formalize the trait inheritance rules
Fannon Apr 29, 2021
89322f8
Explicitly define behavior for undefined and null values
Fannon Apr 30, 2021
f305df8
Define what happens with undefined and null
Fannon Apr 30, 2021
06db043
Updated description of trait inheritance merge behavior
Fannon Apr 30, 2021
c747072
Excplicitly state that must be dereferenced before merging
Fannon May 1, 2021
6f57e01
Added trait explanation
Fannon Aug 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions spec/asyncapi.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,25 @@ This is applicable for `$ref` fields in the specification as follows from the [J

By convention, the AsyncAPI Specification (A2S) file is named `asyncapi.json` or `asyncapi.yaml`.

Copy link

Choose a reason for hiding this comment

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

The spec should define "traits" before explaining their behavior. My suggestion from the PR conversation:

### <a name="traits"></a>Traits

Traits are pieces of information that will be merged into the referencing object. Using traits reduces the amount of repetitive code within an AsyncAPI document.

Copy link
Author

Choose a reason for hiding this comment

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

I've added this now to the PR just above the ### Trait Inheritance Section

### <a name="traits"></a>Traits

Traits are pieces of information that will be merged into the referencing object. Using traits reduces the amount of repetitive information within an AsyncAPI document.

### <a name="traitInheritance"></a>Trait Inheritance

When traits ([Operation Trait Object](#operationTraitObject) and [Message Trait Object](#messageTraitObject)) are defined, they MUST be merged into the target object according to the following rules:

* Traits are merged with a recursive merge algorithm, as defined in the [JSON Merge Patch](https://tools.ietf.org/html/rfc7386) RFC. To summarize the main merge rules as a merge from source object into a target object:
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 confused now. If this is as defined in JSON Merge Patch, why do we have custom rules below? I'm also having a hard time figuring out how this would work. Maybe we could have some examples along with the explanation?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I'm also unsure about that.

So the RFC already states the merge rules, but it also covers many other aspects that are not relevant to this trait inheritance at all. If you get the rules right, you could just state a few rules here. I still thinks its valuable to say that this is RFC-7386 conformant, so you can reuse existing libraries.

In the linked issue, I've referenced some examples and code. I wasn't sure if this should be part of the specification itself? But having some examples and code is always helful if you can link to it.

See https://jsfiddle.net/FannonF/yLsonr57/27/

Copy link
Author

Choose a reason for hiding this comment

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

@fmvilas Btw. some of those rules are still necessary because they are not part of the JSON Merge Patch RFC.

The following rules are specific to AsyncAPI itself and have been missing (undefined) previously:

  • Before applying merges, JSON Schema $ref MUST be dereferenced
  • The Message Object and the Operation Object that the trait is applied to is the most specific and is therefore never overwritten, only extended.

The following rule is AsyncAPI specific but was already there before I think:

  • The order of the traits defines their specificity. Subsequent traits in the trait array are more specific.

Copy link
Member

@jonaslagoni jonaslagoni Dec 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe we should just explicitly state the extension to the RFC? 🤔 That way we ensure there is less interpretation involved.

Something similar to:

The traits ([Operation Trait Object](#operationTraitObject) and [Message Trait Object](#messageTraitObject)) are defined using [JSON Merge Patch](https://tools.ietf.org/html/rfc7386). In extension to these merge rules, the following applies:
- Any references in traits, must be resolved before applying them. 
- The Message Object and the Operation Object that the trait is applied to is the most specific and is therefore never overwritten, only extended.
- The order of the traits defines their specificity. Subsequent traits in the trait array are more specific.
...

Copy link
Member

Choose a reason for hiding this comment

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

For your coding examples, do you mind writing a comment in this issue: asyncapi/parser-api#36

This way we can include your examples as documentation and explanation of how to achieve the correct behavior in tooling 🙂

Copy link
Author

Choose a reason for hiding this comment

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

Hi Jonas,

sure, I can do that! However, we were not completely agreeing on what the future approach should be.
This code example was my take on how I would do it ;)

Best,
Simon

Copy link
Member

Choose a reason for hiding this comment

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

Ahh yea, my bad, I saw this PR as an attempt to clarify the current behavior. Gonna follow up in #557

* Objects are merged recursively (if the source and the target property are both objects).
* If the value of the source object property is `null`, the target object property gets removed.
* In all other cases (including arrays and `undefined`) the source object property overwrites the target object property.
* The sequence of merging is the following
* Before applying merges, JSON Schema `$ref` MUST be dereferenced
* The [Message Object](#messageObject) and the [Operation Object](#operationObject) that the trait is applied to is the most specific and is therefore never overwritten, only extended.
* The order of the traits defines their specificity. Subsequent traits in the trait array are more specific.
* The resulting merge order is the following, with the most specific object to the right:
* `trait1`, `trait2`, `trait3`, ..., `message/operation object`

### <a name="schema"></a>Schema

#### <a name="A2SObject"></a>AsyncAPI Object
Expand Down Expand Up @@ -661,7 +680,7 @@ Field Name | Type | Description
<a name="operationObjectTags"></a>tags | [Tags Object](#tagsObject) | A list of tags for API documentation control. Tags can be used for logical grouping of operations.
<a name="operationObjectExternalDocs"></a>externalDocs | [External Documentation Object](#externalDocumentationObject) | Additional external documentation for this operation.
<a name="operationObjectBindings"></a>bindings | [Operation Bindings Object](#operationBindingsObject) \| [Reference Object](#referenceObject) | A map where the keys describe the name of the protocol and the values describe protocol-specific definitions for the operation.
<a name="operationObjectTraits"></a>traits | [[Operation Trait Object](#operationTraitObject) &#124; [Reference Object](#referenceObject) ] | A list of traits to apply to the operation object. Traits MUST be merged into the operation object using the [JSON Merge Patch](https://tools.ietf.org/html/rfc7386) algorithm in the same order they are defined here.
<a name="operationObjectTraits"></a>traits | [[Operation Trait Object](#operationTraitObject) &#124; [Reference Object](#referenceObject) ] | A list of traits to apply to the operation object. Traits MUST be merged into the operation object as defined in the [trait inheritance section](#traitInheritance). The resulting object MUST be a valid [Operation Object](#operationObject).
<a name="operationObjectMessage"></a>message | [[Message Object](#messageObject) &#124; [Reference Object](#referenceObject)] | A definition of the message that will be published or received on this channel. `oneOf` is allowed here to specify multiple messages, however, **a message MUST be valid only against one of the referenced message objects.**

This object can be extended with [Specification Extensions](#specificationExtensions).
Expand Down Expand Up @@ -1014,7 +1033,7 @@ Field Name | Type | Description
<a name="messageObjectExternalDocs"></a>externalDocs | [External Documentation Object](#externalDocumentationObject) | Additional external documentation for this message.
<a name="messageObjectBindings"></a>bindings | [Message Bindings Object](#messageBindingsObject) \| [Reference Object](#referenceObject) | A map where the keys describe the name of the protocol and the values describe protocol-specific definitions for the message.
<a name="messageObjectExamples"></a>examples | [Map[`string`, `any`]] | An array of key/value pairs where keys MUST be either **headers** and/or **payload**. Values MUST contain examples that validate against the [headers](#messageObjectHeaders) or [payload](#messageObjectPayload) fields, respectively.
<a name="messageObjectTraits"></a>traits | [[Message Trait Object](#messageTraitObject) &#124; [Reference Object](#referenceObject)] | A list of traits to apply to the message object. Traits MUST be merged into the message object using the [JSON Merge Patch](https://tools.ietf.org/html/rfc7386) algorithm in the same order they are defined here. The resulting object MUST be a valid [Message Object](#messageObject).
<a name="messageObjectTraits"></a>traits | [[Message Trait Object](#messageTraitObject) &#124; [Reference Object](#referenceObject)] | A list of traits to apply to the message object. Traits MUST be merged into the message object as defined in the [trait inheritance section](#traitInheritance). The resulting object MUST be a valid [Message Object](#messageObject).

This object can be extended with [Specification Extensions](#specificationExtensions).

Expand Down