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: clarify that traits overwrite the objects they are applied to #517

Closed
wants to merge 2 commits into from

Conversation

Fannon
Copy link

@Fannon Fannon commented Mar 16, 2021

Description
This PR will add some clarification (basically a warning) that a traits properties will overwrite the properties of the object it is applied to. This is potentially counter-intuitive. For more explanations, please see #505 and this comment: #505 (comment)

Related issue(s)
See also #505

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 and the instructions about a basic recommended setup 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.

@Fannon Fannon changed the title Clarify that traits overwrite the objects they are applied to fix: Clarify that traits overwrite the objects they are applied to Mar 16, 2021
@Fannon Fannon changed the title fix: Clarify that traits overwrite the objects they are applied to fix: clarify that traits overwrite the objects they are applied to Mar 16, 2021
@sonarcloud
Copy link

sonarcloud bot commented Mar 16, 2021

Kudos, SonarCloud 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

@@ -657,7 +657,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 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 [Operation Object](#operationObject). **WARNING**: Please be aware that due to the defined order of merge patching, a trait will actually *overwrite* the properties of the object they are applied to.
Copy link
Author

Choose a reason for hiding this comment

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

This description might get a bit too long and is partially duplicated with the Message Trait Object description.

One option would be to introduce a dedicated section in the spec where both can point to. This section could then also carry more information and explain the traits in more detail.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it would make sense to clarify what #505 estates: as the JSON Merge Patch mentions, arrays are fully replaced as a whole value (by the ones in the trait).

Copy link
Author

Choose a reason for hiding this comment

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

Yes, this should also be clarified probably.

If we need further clarification, having a dedicated section where we can refer to would make all the more sense.

Btw. I've created an alternative PR which has such a section, but also redefines the behavior: #532

@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 60 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation.
Thank you for your contributions ❤️

@github-actions github-actions bot added the stale label Jun 29, 2021
@smoya smoya removed the stale label Jun 29, 2021
@Fannon
Copy link
Author

Fannon commented Jun 29, 2021

@smoya : This PR could indeed be deprecated in favor of #532
But this is up to you, which approach you think makes more sense.

@smoya smoya closed this Aug 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants