fix: redefine the trait inheritance behavior

[Fannon]

---

title: Redefine and explicitly formalize the trait inheritance behavior

---

Related issue(s): #505

Related PR(s): #517 (will be deprecated by this PR)

Code Example: https://jsfiddle.net/FannonF/yLsonr57/latest/

---

The basic issue is that the current defined trait merging behavior would overwrite the target object with the traits.
This might not what you would expect from trait inheritance and potentially limits the usefulness of traits, as they can't be used for inheritance use-cases.

There are three options that are proposed:

• A) Do not change the current trait behavior and just describe it and it's limitations (fix: clarify that traits overwrite the objects they are applied to #517)
  • PRO: No breaking change to the spec. Just clarifications.
  • CON: The current trait behavior makes inheritance use-cases impossible. It can be used for composition, but needs to be carefully handled so nothing is accidentally overwritten without the creator realizing it.
• B) Change the trait behavior of the spec by changing the order of the merging / patching
  • Proposal: Make the target of where the trait is applied to the "most specific" and not the "least specific", so it is never overwritten.
  • PRO: Now we support inheritance like use-cases. We still can do composition.
  • CON: We break the use-case where we explicitly wan't to overwrite the target object.
  • If we implement this, I see two ways how to go about it
    • B1) Based on JSON merge patch (as we do already)
      • Pro: Make use of an existing standard, including existing libraries
      • Con: The goal and semantics of a merge patch are different than what we want to do with trait inheritance. We are re-using a standard for a different purposes than originally intended.
    • B2) Based on a generic object deep merge algorithm that we clearly define.
      • Behaves similar to JSON merge patch, but likely without the null behavior
      • I'd highly recommend to not merge arrays. Just overwriting is is the simplest solution and keeps the spec simple. While for some use-cases this might be a bit inconvenient, it gives the user full control.
      • Pro: No "dependency" to another standards or libraries. We can describe the algorithm rather shortly (like RAML) and provide a reference implementation (see my code examples).
    • Con: This needs to be custom implemented and there might be no existing implementation for your ecosystem.
• C) Consider not supporting such traits / inheritance in a future AsyncAPI version (drop feature)
  • If the goal is to keep the spec itself simple and also the consumer side, we might event want to drop more complicated features
  • As an alternative, we move such functionality to a pre-processor stage, which the AsyncAPI providers are responsible for themselves. There they can use template languages, traits, inheritance as they like if they implement it on their own to their own expectations. In the simplest case, you can write the spec in YAML, use features like anchors and merge keys, parse and serialize it to a "simple", denormalized YAML / JSON file and be done with it.

In case we go for an option in the "B range", we have to decide:

• Change the current behavior of the trait inheritance ?
  • This would either be a breaking change to the bug or if we stretch it a "bugfix" if we consider the current behavior as unintended (which I think we don't do after discussions with @fmvilas )
• Keep the current trait implementation and add a new feature besides of it (as a new property?)
  • This would further increase the complexity but also the flexibility of the spec
  • This is a breaking change for consumers and tools, because they now need to support the new inheritance feature or else they would interpret the AsyncAPI docs wrong / incompletely. For AsyncAPI providers, it's a compatible change (the documents do not need to be migrated)

For a detailed description and discussion of the problem, see #505 and its comments.

Related Material / Inspiration

• RAML Traits:
  • Specification: https://github.com/raml-org/raml-spec/blob/master/versions/raml-10/raml-10.md/#algorithm-of-merging-traits-and-methods
  • They have defined and described their custom trait (merge) algorithm as part of the spec
  • It looks like a RAML trait does not overwrite the target, though

Open Questions / Problems

• Do we consider changing the trait merge behavior or do we want to keep it as it is?
• Do we consider changing the merge behavior as a breaking change? How do we go about introducing it to the spec?
• Should we stick with the JSON Merge Patch merge behavior?
  • The semantics and the use-case of JSON Merge Patch is quite different from the trait merge use-case. This may lead to confusions (and it already has)
  • JSON Merge Patch has a special behavior on merging null values, which may not be useful / expected for trait inheritance

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[fmvilas]

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?

[Fannon]

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/

[Fannon]

@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.

[jonaslagoni]

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.
...

[jonaslagoni]

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 🙂

[Fannon]

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

[jonaslagoni]

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

[fmvilas]

Does this PR require a minor or a major semver version increment?

Don't worry about versioning in the PR. We handle the release process separately.

Should we stick with the JSON Merge Patch merge behavior?

I think we should. Inventing new algorithms will make it harder for tooling implementers. I agree that maybe we can reverse the order in which the merge is applied though. My two cents. Would love to know what others think.

[github-actions]

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 ❤️

[derberg]

Proposal shared with the wider community:

• LinkedIn
• Twitter
• Slack (need to join first- {invite link](https://www.asyncapi.com/slack-invite))

[jessemenning]

I am intrigued by the proposal, and if we move forward with a more reference based spec these rules will become increasingly important. I will admit I'm not smart enough to fully understand the implications though, would it be possible for @Fannon or others to present at a SIG, or another meeting?

[Fannon]

@jmenning-solace : Yes, this would benefit much from some explaining. I'd be happy to do that.
When is the next SIG meeting or a good occasion for it?

[derberg]

@Fannon next meeting is next week -> asyncapi/community#51

[github-actions]

This pull request 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 pull request, add a comment with detailed explanation.

There can be many reasons why some specific pull request 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 pull request forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

[jonaslagoni]

@Fannon you have listed a bunch of options and ways to solve this, however, as a reader, it is hard to keep track of all the pros and cons and understand the ripple effect (through tooling, etc).

I have a hard time figuring out what this PR is trying to fix, and I guess it comes down to which direction we should take it.

Since you have the knowledge, my suggestion would be that you pick the direction you would like to see AsyncAPI adopt, regardless of breaking change or not. See it as a perfect world, which option would then be best suited?

That way you can focus the PR on one suggestion that highlights the changes it requires, as well as letting others know why you chose it and why not one of the other options.

You can also do it in multiple stages, so you start by Do not change the current trait behavior and just describe it and it's limitations, and then work towards major version change where you suggest the new behavior you would like to see 🙂

I think this issue is definitely something that should be addressed, so thanks for taking the time!

[Fannon]

Hi @jonaslagoni,

yes, I see that this PR is hard to understand in its current state.
The problem is, we already discussed where we wanted to go and I wasn't sure if we already had agreement.
In this PR I described the compromise solution that I understood that AsyncAPI found best: Keep using the JSON merge path algorithm, change the order of merging and clearly define the previously undefined aspects.

Only in the code-example I have a second option (not using JSON merge patch, but an own definition of merge algorithm + implementatoin).

There is also one PR that only documents the status quo: #517

Best,
Simon

[Fannon]

So I've thought about this a bit more in another context.
My last state was that we didn't reach consensus and a decision on how to approach this.

In this PR I've tried to show a compromise solution, chaning the order of inheritance but keeping the JSON Merge Patch behavior. My personal opinion on this has changed even more in the direction that we should not use JSON Merge Patch for defining the merge behavior:

• Like already stated, JSON merge patch is written and built for the purpose of patching (e.g. overwriting) the target objects. This has already led to some confusions
• If we want to have more trait like or inheritance like behavior, we want to do basically the opposite. Of course, we can inverse the direction how the algorithm is applied, but this requires some inversions in your thinking :)
• The JSON Merge patch has the feature of using a special null semantics, used for deleting properties in the target. If we keep this behavior and just apply it on the merged object instead of the target, we would do something that is not common in inheritance concepts. It even violates a well established principle, the Liskov Substitution Principle.

To sum it up: My proposal would be to define our own merge algorithm (It's not that much anyway) in written form and additionally as a reference code example, inkcl. unit tests with example data.
We still need to make up our mind how we interpret and explain the term trait. I understand that you can use traits to implement composition, but to me those are different things. Compositions are not using inheritance at all, they just add multiple aspects to one object and live side by side with each other. What we currently have with traits seems to be more like multi-parent inheritance / an object merge algorithm.

[github-actions]

This pull request 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 pull request, add a comment with detailed explanation.

There can be many reasons why some specific pull request 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 pull request forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

[patrickp-at-work]

Hello @fmvilas,
in #600, @clemensv had asked for a definition of traits. You had closed #600 as duplicate of this task which makes good sense to me. But I checked the PR and it doesn't really define traits or explain their use case(s). Can we still add that? As per my understanding, this issue has not changed the general idea of traits.

[patrickp-at-work]

I think we even already have a nice definition which was filed within #108:

Traits are pieces of information that will be merged into the referencing object.

We could extend this definition to:

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.

[fmvilas]

Yeah, as long as the PR is open we still have time 😄 In any case, I'm not the champion of this issue but @Fannon is.

[patrickp-at-work]

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.

[Fannon]

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

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[Fannon]

So how do we want to proceed with this?

Right now, this PR / issue is in a complicated state, because to me it is not really clear which direction we want to take and how this would be decided.

We've discussed a few options, here are the ones that I find most sensible / realistic.
Fore more detailed description, see the PR description:

• Option C) Remove trait feature from AsyncAPI 3 as it introduces too much complexity
• Option B) Keep trait feature, but change how it is applied and describe it
  • Here we need to decide between the two B variants. My personal favorite would be B2, but this PR is following the B1 approach as this seemed to be the preference when we discussed it a while ago.

I can also imagine that with the restructuring of AsyncAPI 3, a few circumstances have changed and new insights have been gained. Maybe that also changes our perspective here ;)

[github-actions]

This pull request 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 pull request, add a comment with detailed explanation.

There can be many reasons why some specific pull request 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 pull request forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

[smoya]

Closing in favour of #907. Thanks @Fannon for the work you did here and that inspired the new traits merge mechanism 🤜✨🤛