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

feat: new traits merge mechanism #907

Conversation

magicmatatjahu
Copy link
Member

@magicmatatjahu magicmatatjahu commented Feb 16, 2023


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.

@sonarcloud
Copy link

sonarcloud bot commented Feb 16, 2023

Kudos, SonarCloud Quality Gate passed!    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

Copy link
Member

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

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

Like it but got some suggestions.

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 jonaslagoni mentioned this pull request Mar 1, 2023
@jonaslagoni
Copy link
Member

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

@magicmatatjahu
Copy link
Member Author

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

@smoya
Copy link
Member

smoya commented Mar 27, 2023

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
Copy link
Member

smoya commented Mar 29, 2023

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
Copy link
Member Author

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

Hopefully I will do it in this weekend :)

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

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.

@sonarcloud
Copy link

sonarcloud bot commented Apr 2, 2023

Kudos, SonarCloud Quality Gate passed!    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

@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Apr 2, 2023

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

spec/asyncapi.md Outdated
@@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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 ;)

Copy link
Member Author

@magicmatatjahu magicmatatjahu Apr 17, 2023

Choose a reason for hiding this comment

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

So how to call it?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, target sounds better to me too.

Copy link
Member

Choose a reason for hiding this comment

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

Solved

@jonaslagoni
Copy link
Member

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

spec/asyncapi.md Show resolved Hide resolved
spec/asyncapi.md Outdated
@@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, target sounds better to me too.

@magicmatatjahu
Copy link
Member Author

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

Co-authored-by: Fran Méndez <[email protected]>
fmvilas
fmvilas previously approved these changes Jun 13, 2023
Copy link
Member

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

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

🚀

@jonaslagoni
Copy link
Member

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

spec/asyncapi.md Outdated Show resolved Hide resolved
Copy link
Member

@smoya smoya left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀🌔

@Fannon
Copy link

Fannon commented Jun 21, 2023

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.

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

@Fannon Fannon Jun 21, 2023

Choose a reason for hiding this comment

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

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 :)

Copy link
Member

@smoya smoya Jun 30, 2023

Choose a reason for hiding this comment

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

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

cc @jonaslagoni @magicmatatjahu @derberg

@sonarcloud
Copy link

sonarcloud bot commented Jun 30, 2023

Kudos, SonarCloud Quality Gate passed!    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

@smoya
Copy link
Member

smoya commented Jun 30, 2023

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

Copy link
Member

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@smoya
Copy link
Member

smoya commented Jul 13, 2023

/rtm

@asyncapi-bot
Copy link
Contributor

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

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants