Application of message traits (intentionally) replacing existing attributes

[c-pius]

Describe the bug
According to specification, message traits are applied using json-merge-patch protocol.

A list of traits to apply to the message object. Traits MUST be merged into the message object using the JSON Merge Patch algorithm in the same order they are defined here. The resulting object MUST be a valid Message Object.

The implication of this is that properties defined in a trait will "override" those defined in the message itself. I am not sure if this is actually a bug (it is just how json-merge-patch works) but I personally think this is a bit unfortunate.

A concrete scenario where I run into troubles is that I have a trait that is defining some standard message headers where some of them are required. Then I have several messages that include some specific header attributes per message where also some of them are required. Now the problem is, that when the trait is applied, the required attributes from the message are replaced by the ones from the trait (see the examples below for more clarity).

Of course I could also just remove the required array from the trait and add them into the message, but then the trait is not really self-contained anymore... Therefore I wanted to check if this behavior of traits is really the intended one? Or if there are any other mitigations I could take?

To Reproduce
Steps to reproduce the behavior:

1. Take the sample document below
2. Parse it with parser-js
3. Check the resulting message

{
  // ...
  "components": {
    "messages": {
      "channelMessage": {
        "headers": {
          // ...
          "required": [ // ["req-header"] has been replaced by the trait
            "t-req-header"
          ]
        }
      }
    }
    // ...
  }
}

Expected behavior
A clear and concise description of what you expected to happen.

{
  // ...
  "components": {
    "messages": {
      "channelMessage": {
        "headers": {
          // ...
          "required": [
            "req-header",
            "t-req-header"
          ]
        }
      }
    }
    // ...
  }
}

Sample document

asyncapi: 2.0.0
info:
  title: My API
  version: '1.0.0'

channels:
  mychannel:
    publish:
      message:
        $ref: '#/components/messages/channelMessage'

components:
  messages:
    channelMessage:
      traits:
        - $ref: '#/components/messageTraits/some-trait'
      headers:
        type: object
        properties:
          req-header:
            type: integer
          non-req-header:
            type: integer
        required:
          - req-header
  schemas:
    testSchema:
      type: object
      properties:
        name:
          type: string
  messageTraits:
    some-trait:
      headers:
        type: object
        properties:
          t-req-header:
            type: string
          t-non-req-header:
            type: string
        required:
          - t-req-header

Screenshots
If applicable, add screenshots to help explain your problem.

Additional context
Add any other context about the problem here.

[Fannon]

Thanks for formulating this @c-pius .
I would also consider this a bug, because it (1) is counter to what people will likely expect. And (2) the way it works as of now will place several restrictions on what you can safely do with inheritance / information reuse as c-pius pointed out.

I've also brought this problem up in slack a while ago and discussed it with @derberg .
I'm not sure the link still works, as this message is out of the workspace history for me: https://asyncapi.slack.com/archives/C34F2JV0U/p1588916689469300

In your spec there is the following statement:
"Traits MUST be merged into the message object using the JSON Merge Patch algorithm in the same order they are defined here."
If i interpret this correctly, the trait would overwrite what's already defined in the message itself?
This is where I ran into problems because I assumed this works more like OOP inheritance or mixins where you can have defaults and overwrite them in the concrete place.
Is the behavior that the trait will override attributes on the object where it is applied desired? Or did I oversee something?

[fmvilas]

Yeah, that's a bit unfortunate but the reason we did that is that we had to choose between JSON Merge Patch or JSON Patch, which will suit this use case perfectly but its UX sucks for this purpose.

If we choose to merge arrays, we can end up having duplicate keys. And more importantly, there's no standard algorithm like the ones mentioned above.

This is not really a bug but it's true the UX is not great. How do you think we can solve this? I'm wondering if maybe headers should not be overridable by traits and maybe we can provide another mechanism to extend them. Anyway, happy to hear some proposals.

[Fannon]

Hi @fmvilas,

actually I do like that you've chosen to use a standard for this. The problem is the order of merging beeing done, which is opposite to what you might expect. Effectively, the parent overwrites the properties of the child (the trait overwrites the properties of the object where it is applied to) and not vice versa.

The merging of arrays is indeed complicated. I ran into this issue as well and usually end up with one of the following solutions:

• Just accept that an array is always overwritten (like a primitive type). This is very simple and I think what some merging libraris do by default.
• Define clear rules how array merging is done. E.g. arrays are merged by appending them in the sequence of merge steps and duplicates are removed. Please note that this makes it impossible to remove array items.
• Devise additional keywords / vocabulary that controls how array merging is done. Then the user can define himself what the rules of array merging are. In one project I used the following "directives" (some of them could also be combined): '@overwrite', '@prepend', '@append', '@unique', '@sorted', '@unsorted'

Regarding a solution: I'm afrait that the specification can't change this behavior without introducing a breaking change and adjusting the various implementations (parser, renderers, etc.).

I'd propose to clearly state this behavior (and it's consequences) in the specification and rethink how we want to do information merging / inheritance in the next major version of this specification.

In our internal project we decided to just state a clear warning that makes clear that traits do not behave as you'd expect and give some rules when they can be used and when they shouldn't.
One fallback possibility is also to not use traits at all and if you do need it: Apply merging / inheritance with a preprocessor of you own choosing (e.g. YAML has some built-in rudimentary merging).

Best regards,
Simon

[fmvilas]

I'd propose to clearly state this behavior (and it's consequences) in the specification and rethink how we want to do information merging / inheritance in the next major version of this specification.

Would any of you mind opening a PR to add it to the spec?

Regarding a solution: I'm afrait that the specification can't change this behavior without introducing a breaking change and adjusting the various implementations (parser, renderers, etc.).

Don't worry about breaking changes. Go ahead and create a proposal on how to solve it. We're currently changing the way to contribute to the spec, in case you're interested: #511.

[Fannon]

@fmvilas yes, I can create a PR to clarify this (just need to find a bit of time for this).

Here is a very quick draft, but I didn't have time to have a look at the contribution guidelines yet. But I'll catch up on that.

PR: #517

Regardings the proposal how to solve it, this would take a bit more thought and time.

[Fannon]

I didn't find time to propose the new behavior via PR, but some first ideas and sketches (still hacky and just a basis for further discussion and refinement):

• After some thoughts, I'm not sure it is a good idea to use the JSON Merge Patch standard for trait inheritance. The reason is that this standard is about how an existing document is patched (and therefore partially overwritten) by a patch. When we talk about traits, we would expect the opposite behavior. The trait is inherited, but will never overwrite the initial object. Therefore the semantics / original purpose of the JSON Merge patch standard is different from the traits semantics.
• Therefore we could just state how the desired inheritance behavior is and provide a simple reference implementation or rely on existing deep merge implementations like lodash (_.merge()).
• The simplest rule of merging could be: The merge function will overwrite primitive types, deep merge objects (recursively) and overwrite (not merge!) arrays. The merge should not mutate its parameters. Merge objects that come later in the parameter list will overwrite those before.
  • If we want / need a more flexible merge logic, we could add some rules how arrays can be merged. For example: 1. append new items, 2. deduplicate items. If an array gets merged with null it is removed. (therefore we still can overwrite an array by having an intermediary merge with null)
• Now we need to define the order in which the merging happens. For traits, this would be: merge(trait1, trait2, trait3, ..., targetObject)

Here is some JavaScript Snippet (See JSFiddle):

const targetObject = {
	// I don't want this to be overwritten when applying traits
  type: 'my message type',
  messageObject: true,
}

const trait1 = {
  source: 'inherited source from trait 1',
  type: 'type from trait1 that I dont want to inherit',
  trait1: true,
  someObject: {
  	trait1: true,
  },
  someArray: ['trait1']
}

const trait2 = {
	source: 'inherited source from trait 2',
  trait2: true,
  someObject: {
  	trait2: true,
  },
  someArray: ['trait2']
}

const currentBehavior = _.merge({}, targetObject, trait1, trait2);

console.log('Current trait inheritance', currentBehavior)

const proposedBehavior = _.merge({}, trait1, trait2, targetObject);
console.log('Proposed trait inheritance', proposedBehavior)

// The _.merge function will overwrite primitive types, deep merge objects (recursively) and overwrite (not merge!) arrays.

[c-pius]

To me this proposal looks like it goes in to the direction of what I would intuitively expected from traits. As of the current porposal, is this not just the same as with json-merge-patch but with a different order (as I understood it it also deep merges objects and replaces arrays)?

Traits MUST be merged into the message object using the JSON Merge Patch algorithm in the same order they are defined here.

would change to something like

Starting from an empty object, traits MUST be merged into the object in the same order they are defined here and lastly the operation object is merged. For the merging, the JSON Merge Patch algorithm is used

However, for the given problem with the required array, this would still not actually solve the problem (unless we define how arrays are merged instead of replaced). Still an improvement I guess.

[Fannon]

Yes, I also interpreted the JSON Merge patch behavior that arrays get replaced and not merged there as well. This would correspond to the simpler merge algorithm I stated above.

See https://tools.ietf.org/html/rfc7386#section-3

But I agree, for JSON Schema required properties, this would be annoying. However for other arrays it might be excactly what you want. Therefore I think we need to either replace arrays all the time or give the end user control on merging OR replacing them.

[c-pius]

Yes, I also interpreted the JSON Merge patch behavior that arrays get replaced and not merged there as well. This would correspond to the simpler merge algorithm I stated above.

yes I exactly. The point I wanted to make is that the suggested algorithm (without merging arrays) is just the JSON Merge Patched applied in a different order than as of now

[jonaslagoni]

I agree with everything here actually, @Fannon your example is plenty enough to work on the general "fix" for the parser 👍 Well explained.

I think it makes sense to alter the definition of traits in the specification 👍

Related function in JS Parser: https://github.com/asyncapi/parser-js/blob/8dc1d6cfb49a6a87732b9bdd996d4092b5e111fe/lib/parser.js#L226

[Fannon]

After some fiddling around, I realized that the _.merge() function does also not give the result that I would have expected by default. The issue is that it tries to merge arrays by default and does not even do this by concatenating them, but by iterating the array items and merging each with the source array item of the same index.

This behavior is not what I would have expected at first and is likely not desirable for trait inheritance. As the currently defined JSON Merge Path algorithm overwrites arrays, I'd like to keep this behavior.

Here is my current WIP idea on how to define the trait inheritance: #532

See tiny-merge-patch implementation: https://github.com/QuentinRoy/tiny-merge-patch/blob/master/esm/index.js
For examples, see my JSFiddle: https://jsfiddle.net/FannonF/yLsonr57/27/

[github-actions]

This issue 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 issue, add a comment with detailed explanation.
Thank you for your contributions ❤️

[Fannon]

Here is another example for an inheritance use-case (based on @c-pius example) that we can discuss on the AsyncAPI meeting today:

asyncapi: 2.1.0
info:
  title: Animals API
  version: 1.0.0
channels:
  birdchannel:
    publish:
      message:
        $ref: '#/components/messages/birdMessage'
components:
  messages:
    birdMessage:
      traits:
        - $ref: '#/components/messageTraits/animalTrait'
      headers:
        type: object
        description: 'A bird!'
        properties:
          type:
            type: string
            enum: ['Bird']
          chirp:
            type: string
        required:
          - name
          - type
          - chirp
  schemas:
    testSchema:
      type: object
      properties:
        name:
          type: string
  messageTraits:
    animalTrait:
      headers:
        type: object
        description: 'An (abstract) animal'
        properties:
          name:
            type: string
          type:
            type: string
            enum: ['Bird', 'Dog']
        required:
          - name
          - type

[Fannon]

And to add a real use-case and not a contrived one:

If you use AsyncAPI for describing CloudEvents, there will be some standardized header messages for each event. It would be really convenient to create one trait that describes all the CloudEvent typical headers. However, if you do this with a trait with the current behavior, you can never overwrite anything that you inherited, e.g. to make it more specific.

Especially problematic is the required array in JSON schema. If you define it in the trait, it makes it impossible to add / overwrite the required properties at the message level in case you added some additional required property.

[fmvilas]

Yeah, the CloudEvents example is a good one. We're making some great progress with the publish/subscribe issue that will be fixed in version 3.0.0. I think we should aim for changing this behavior for v3 but would not like to then find out we're breaking other more common use cases. I don't think so but I'm being fear-driven here I guess 😄

[Fannon]

No, I totally get you @fmvilas. For a spec it's really important to think this through properly and avoid making rash decisions.

[github-actions]

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

[jonaslagoni]

@Fannon as you already are in progress of championing this, do you see this as needing to be considered for 3.0?

[Fannon]

@jonaslagoni : Yes, moving this to a 3.0 release makes sense as we have the chance to rethink this without worrying too much about incompatibility.

Right now, it's somewhat undefined behavior. The proposals we discussed and preferred would change the behavior (which would be a breaking change).

[jonaslagoni]

@Fannon do you mind if I add you to the list of champions for 3.0? Joining the meetings would give you a good oppotunity to give updates and get feedback with no preasure 🙂

[Fannon]

Yes, let's try it :)

[github-actions]

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

[magicmatatjahu]

At what stage do we have this proposal? I have now encountered this problem myself, and it surprises me that traits have a higher priority than the main object itself 😆 I understand that we want to fix the behaviour of traits in v3. @jonaslagoni @Fannon

[Fannon]

@magicmatatjahu : I'm not that actively involved in this anymore, sorry for that. My feeling was that we lacked a bit a consensus on how to move this topic forward. Changing the existing behavior would be a breaking change, so it needs to be done with AsyncAPI 3.

But if we redo this, we have multiple options, I tried to outline my own proposals here: #532 (comment)

[magicmatatjahu]

@Fannon Thanks a lot! You did so much and we are grateful! I also think B2 would be a better option, but if it introduces problems then B1 is good too - the way of apply the traits themselves is more important than the details like the way to apply the arrays but we could improve that too :)

[smoya]

@magicmatatjahu You created the PR on the spec side so I understand you wanted to champion this. The PR has some feedback pending to be addressed.
Also, we would need changes to be reflected inparser-js.

Are you still willing to champion this for v3?

[smoya]

Are you still willing to champion this for v3?

Yes it does. #907 (comment)

[fmvilas]

This one is just missing the JS parser implementation. Anyone volunteering?

[github-actions]

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

[Fannon]

Thanks for bringing this in @fmvilas and @smoya ! And congrats to the AsyncAPI v3 release yesterday!

[smoya]

Thanks for bringing this in @fmvilas and @smoya ! And congrats to the AsyncAPI v3 release yesterday!

Thanks goes to you for your help and follow-up in the issue.
Also pinging other folks involved @magicmatatjahu @jonaslagoni