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: add a constructor that accepts AaiMessage to ChangeHeadersRefCommand_Aai20 to fix change of message headers ref in apicurio #803

Merged

Conversation

ben-lc
Copy link

@ben-lc ben-lc commented Aug 14, 2024

Hi,
I'm new to apicurio so I will appreciate any advice regarding this PR if I did something wrong.

This PR is related to Apicurio/apicurio-studio#2737

I tried to use ChangePropertyCommand as suggested by deprecated doc in ChangeHeadersRefCommand_Aai20,
but since we have to create headers node on the message node if headers is null, it's not possible to use ChangePropertyCommand cause it only works with simple types.
I ended up updating ChangeHeadersRefCommand_Aai20 and bringing same modifications that were done to ChangePayloadRefCommand_Aai20 to support a constructor with AaiMessage.

…mand_Aai20 to avoid "invalid overload error" in apicurio-studio (see studio issue #2737)
@EricWittmann
Copy link
Member

Hi @ben-lc I think this looks good, but it's a nice opportunity to add a test for the use-case you're trying to fix. It should be very easy to do. Have a look at the test framework for commands here:

https://github.com/Apicurio/apicurio-data-models/blob/1.x/src/test/resources/fixtures/cmd/tests.json

You just need to follow the pattern in that file (and obviously the files it points to). :)

wdyt?

@ben-lc
Copy link
Author

ben-lc commented Aug 19, 2024

Hi @EricWittmann, thanks for your feedback, I've added the test for this use case.

@EricWittmann EricWittmann merged commit fabeb4d into Apicurio:1.x Aug 20, 2024
@EricWittmann
Copy link
Member

👍👍

@EricWittmann
Copy link
Member

Merged!

This pull request was closed.
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