feat: new traits merge mechanism #907

magicmatatjahu · 2023-02-16T10:57:35Z

title: "new traits merge mechanism"

Related issue(s):
#505

Note
That PR is based on the #532 Thanks @Fannon for awesome work!

That PR introduces the new traits merge mechanism. In simple words, in the current approach (in 2.x.x) traits overwrite values defined on the main object. New approach treat main object with higher priority, so Message Object like:

messageId: userSignup
description: A longer description.
payload:
  $ref: '#/components/schemas/userSignupPayload'
traits:
  - name: UserSignup
    title: User signup
    summary: Action to sign a user up.
    description: Description from trait.
  - tags:
      - name: user
      - name: signup
      - name: register

after trait merging should be defined as:

messageId: userSignup
name: UserSignup
title: User signup
summary: Action to sign a user up.
description: A longer description. # it's still description from "main" object
payload:
  $ref: '#/components/schemas/userSignupPayload'
tags:
  - name: user
  - name: signup
  - name: register

That PR doesn't fix problems described in the PR #532, it only change the "main" trait merge mechanism, not JSON Merge Patch mechanism.

sonarqubecloud · 2023-02-16T10:57:59Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

fmvilas

Like it but got some suggestions.

spec/asyncapi.md

jonaslagoni · 2023-03-02T11:11:19Z

@magicmatatjahu am I to understand you champion this for version 3?

magicmatatjahu · 2023-03-22T14:20:28Z

Yeah, I can champion this, but please give me a time to apply changes :)

smoya · 2023-03-27T09:09:41Z

Yeah, I can champion this, but please give me a time to apply changes :)

Would you be able to provide an ETA? Please don't take it as me chasing but just want to know if it's gonna be a week, a month, a quarter, etc, so we can decide if it makes sense to move forward, or look for an alternative, etc.

Thanks!

smoya · 2023-03-29T12:19:00Z

Concerning my previous comment, @magicmatatjahu do you think you will have time to work on it this week or the next one?
We are actually late on releasing parser v2, and im concerned about the spec v3 release date being compromised (June).
I know you are no longer working full-time on AsyncAPI, so I would say is completely fine if you like to drop this so any other member can pick it up.
It obviously will be amazing if you author it, but again, if you can't make it, which is normal btw, someone else could take it.

Thanks!

magicmatatjahu · 2023-03-29T13:28:16Z

@smoya It should be easy to add in parser :)

Hopefully I will do it in this weekend :)

github-advanced-security · 2023-04-02T12:26:59Z

You have successfully added a new SonarCloud configuration ``. As part of the setup process, we have scanned this repository and found no existing alerts. In the future, you will see all code scanning alerts on the repository Security tab.

Co-authored-by: Fran Méndez <[email protected]>

sonarqubecloud · 2023-04-02T12:27:49Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

magicmatatjahu · 2023-04-02T13:40:23Z

@smoya PR in ParserJS asyncapi/parser-js#744 I also added review suggestions. It can be merged.

smoya · 2023-04-11T10:05:11Z

spec/asyncapi.md

@@ -2627,6 +2627,31 @@ Message Payload Property | `$message.payload#/messageId` | Correlation ID is set

 Runtime expressions preserve the type of the referenced value.

+### <a name="traitsMergeMechanism"></a>Traits Merge Mechanism
+
+Traits MUST be merged with the main object using the [JSON Merge Patch](https://tools.ietf.org/html/rfc7386) algorithm in the same order they are defined. A property on a trait MUST NOT override the same property on the main object.


I would be more explicit on the exception over JSON Merge Patch mechanism on not overriding properties on the target. Like mentioning it is literally an exception

Additionally, I will suggest using wording from JSON Merge Patch RFC, for example calling "target" to what you called "main object"

Target is good, but you need to keep in mind that the whole problem that JSON Merge patch addresses and describes is an inverse situation than merging traits ;)

So how to call it?

Yeah, target sounds better to me too.

jonaslagoni · 2023-05-10T17:27:40Z

@magicmatatjahu do you need anything specific to push this forward?

spec/asyncapi.md

fmvilas · 2023-05-30T08:50:38Z

spec/asyncapi.md

@@ -2627,6 +2627,31 @@ Message Payload Property | `$message.payload#/messageId` | Correlation ID is set

 Runtime expressions preserve the type of the referenced value.

+### <a name="traitsMergeMechanism"></a>Traits Merge Mechanism
+
+Traits MUST be merged with the main object using the [JSON Merge Patch](https://tools.ietf.org/html/rfc7386) algorithm in the same order they are defined. A property on a trait MUST NOT override the same property on the main object.


Yeah, target sounds better to me too.

magicmatatjahu · 2023-06-13T12:04:25Z

@jonaslagoni as discussed in DM, go forward with this PR :)

Co-authored-by: Fran Méndez <[email protected]>

fmvilas

🚀

jonaslagoni · 2023-06-20T13:50:27Z

@Fannon, @smoya, @derberg, @dalelane, @char0n ping ping

spec/asyncapi.md

smoya

LGTM! 🚀🌔

Fannon · 2023-06-21T18:29:35Z

Thanks for bringing this in, @magicmatatjahu !
I like that this proposal here is very simple in just avoiding the main problem by stating that the target object should never be overwritten.

Fannon · 2023-06-21T18:32:28Z

spec/asyncapi.md

@@ -2625,6 +2625,32 @@ Message Payload Property | `$message.payload#/messageId` | Correlation ID is set

 Runtime expressions preserve the type of the referenced value.

+### <a name="traitsMergeMechanism"></a>Traits Merge Mechanism
+
+Traits MUST be merged with the target object using the [JSON Merge Patch](https://tools.ietf.org/html/rfc7386) algorithm in the same order they are defined. A property on a trait MUST NOT override the same property on the target object.


When implementing this, I think we need to be a bit careful here. The JSON Merge Patch is meant to overwrite the target by design. It's not just making sure that properties are not overridden, also the "null" semantics of JSON Merge Patch is then not applicable, I think.

But I like that this solution is very concise and you avoided defining your own merge algorithm.

Imho, it would be very nice to point to a reference implementation or code snippet how this is to be implemented correctly. As a Dev, I would feel more confident looking at a code example to check my understanding. I like the example, but a simple example cannot cover all the interesting corner cases :)

I created a followup issue for your suggestion @Fannon asyncapi/website#1879

cc @jonaslagoni @magicmatatjahu @derberg

sonarqubecloud · 2023-06-30T05:44:42Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

smoya · 2023-06-30T05:52:35Z

@fmvilas @Fannon a last review round so we can merge? 🙏

fmvilas

LGTM 👍

smoya · 2023-07-13T21:10:04Z

/rtm

asyncapi-bot · 2023-09-12T07:35:07Z

🎉 This PR is included in version 3.0.0-next-major-spec.13 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

magicmatatjahu requested review from fmvilas, derberg, dalelane, smoya, char0n and asyncapi-bot-eve as code owners February 16, 2023 10:57

fmvilas requested changes Feb 16, 2023

View reviewed changes

spec/asyncapi.md Outdated Show resolved Hide resolved

spec/asyncapi.md Outdated Show resolved Hide resolved

spec/asyncapi.md Outdated Show resolved Hide resolved

spec/asyncapi.md Outdated Show resolved Hide resolved

spec/asyncapi.md Outdated Show resolved Hide resolved

jonaslagoni mentioned this pull request Mar 1, 2023

Work on 3.0 release #691

Closed

smoya mentioned this pull request Mar 22, 2023

Application of message traits (intentionally) replacing existing attributes #505

Closed

feat: new traits merge mechanism

fdc93d3

magicmatatjahu force-pushed the next-major/new-trait-bechaviour branch from 2ff014b to fdc93d3 Compare April 2, 2023 12:26

Apply suggestions from code review

53ede05

Co-authored-by: Fran Méndez <[email protected]>

magicmatatjahu mentioned this pull request Apr 2, 2023

chore: new trait behaviour in v3 spec asyncapi/parser-js#744

Merged

magicmatatjahu requested a review from fmvilas April 3, 2023 08:28

smoya reviewed Apr 11, 2023

View reviewed changes

fmvilas requested changes May 30, 2023

View reviewed changes

This was referenced Jun 5, 2023

AsyncAPI spec v3 support in parser-js asyncapi/parser-js#780

Closed

AsyncAPI spec v3 support in JSON Schema files asyncapi/spec-json-schemas#400

Closed

switched to target

2121454

Update spec/asyncapi.md

51ed5db

Co-authored-by: Fran Méndez <[email protected]>

fmvilas previously approved these changes Jun 13, 2023

View reviewed changes

Merge branch 'next-major-spec' into next-major/new-trait-bechaviour

5b17dca

jonaslagoni reviewed Jun 20, 2023

View reviewed changes

spec/asyncapi.md Outdated Show resolved Hide resolved

Update spec/asyncapi.md

5a2ee1e

jonaslagoni dismissed fmvilas’s stale review via 5a2ee1e June 20, 2023 13:51

smoya requested review from Fannon and smoya June 21, 2023 13:22

smoya approved these changes Jun 21, 2023

View reviewed changes

Fannon approved these changes Jun 21, 2023

View reviewed changes

Fannon reviewed Jun 21, 2023

View reviewed changes

Merge branch 'next-major-spec' into next-major/new-trait-bechaviour

e38d925

This was referenced Jun 30, 2023

Expand documentation for the traits merge mechanism in v3 asyncapi/website#1878

Closed

[📑 Docs]: expand documentation for the new traits merge mechanism introduced in v3 asyncapi/website#1879

Closed

smoya requested review from Fannon and fmvilas June 30, 2023 05:52

fmvilas approved these changes Jul 7, 2023

View reviewed changes

asyncapi-bot added the ready-to-merge label Jul 13, 2023

asyncapi-bot merged commit e26b9a5 into asyncapi:next-major-spec Jul 13, 2023

This was referenced Jul 20, 2023

Update JS parser to support new traits behavior asyncapi/parser-js#810

Closed

Update JS parser to support new multi-format schemas asyncapi/parser-js#811

Closed

smoya mentioned this pull request Aug 11, 2023

fix: redefine the trait inheritance behavior #532

Closed

derberg mentioned this pull request Sep 1, 2023

docs: adding document for Reuse with traits page asyncapi/website#2036

Closed

asyncapi-bot added the released on @next-major-spec label Sep 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: new traits merge mechanism #907

feat: new traits merge mechanism #907

magicmatatjahu commented Feb 16, 2023 •

edited

Loading

sonarqubecloud bot commented Feb 16, 2023

fmvilas left a comment

jonaslagoni commented Mar 2, 2023

magicmatatjahu commented Mar 22, 2023

smoya commented Mar 27, 2023

smoya commented Mar 29, 2023

magicmatatjahu commented Mar 29, 2023

github-advanced-security bot commented Apr 2, 2023

sonarqubecloud bot commented Apr 2, 2023

magicmatatjahu commented Apr 2, 2023 •

edited

Loading

smoya Apr 11, 2023

smoya Apr 15, 2023

Fannon Apr 15, 2023

magicmatatjahu Apr 17, 2023 •

edited

Loading

fmvilas May 30, 2023

jonaslagoni Jun 13, 2023

jonaslagoni commented May 10, 2023

fmvilas May 30, 2023

magicmatatjahu commented Jun 13, 2023

fmvilas left a comment

jonaslagoni commented Jun 20, 2023

smoya left a comment

Fannon commented Jun 21, 2023

Fannon Jun 21, 2023 •

edited

Loading

smoya Jun 30, 2023 •

edited

Loading

sonarqubecloud bot commented Jun 30, 2023

smoya commented Jun 30, 2023

fmvilas left a comment

smoya commented Jul 13, 2023

asyncapi-bot commented Sep 12, 2023

feat: new traits merge mechanism #907

feat: new traits merge mechanism #907

Conversation

magicmatatjahu commented Feb 16, 2023 • edited Loading

title: "new traits merge mechanism"

Related issue(s): #505

sonarqubecloud bot commented Feb 16, 2023

fmvilas left a comment

Choose a reason for hiding this comment

jonaslagoni commented Mar 2, 2023

magicmatatjahu commented Mar 22, 2023

smoya commented Mar 27, 2023

smoya commented Mar 29, 2023

magicmatatjahu commented Mar 29, 2023

github-advanced-security bot commented Apr 2, 2023

sonarqubecloud bot commented Apr 2, 2023

magicmatatjahu commented Apr 2, 2023 • edited Loading

smoya Apr 11, 2023

Choose a reason for hiding this comment

smoya Apr 15, 2023

Choose a reason for hiding this comment

Fannon Apr 15, 2023

Choose a reason for hiding this comment

magicmatatjahu Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

fmvilas May 30, 2023

Choose a reason for hiding this comment

jonaslagoni Jun 13, 2023

Choose a reason for hiding this comment

jonaslagoni commented May 10, 2023

fmvilas May 30, 2023

Choose a reason for hiding this comment

magicmatatjahu commented Jun 13, 2023

fmvilas left a comment

Choose a reason for hiding this comment

jonaslagoni commented Jun 20, 2023

smoya left a comment

Choose a reason for hiding this comment

Fannon commented Jun 21, 2023

Fannon Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

smoya Jun 30, 2023 • edited Loading

Choose a reason for hiding this comment

sonarqubecloud bot commented Jun 30, 2023

smoya commented Jun 30, 2023

fmvilas left a comment

Choose a reason for hiding this comment

smoya commented Jul 13, 2023

asyncapi-bot commented Sep 12, 2023

magicmatatjahu commented Feb 16, 2023 •

edited

Loading

Related issue(s):
#505

magicmatatjahu commented Apr 2, 2023 •

edited

Loading

magicmatatjahu Apr 17, 2023 •

edited

Loading

Fannon Jun 21, 2023 •

edited

Loading

smoya Jun 30, 2023 •

edited

Loading