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: properly support multiple types in asyncapi 2.6.0 #153

Closed
wants to merge 3 commits into from

Conversation

ctasada
Copy link

@ctasada ctasada commented Jun 22, 2023

Description

Some of the AsyncAPI fields support more than an object type. Previously this problem was
"solved" accepting any Object but this approach required castings or directly losing
the generic validations in any consumer code.

Here we propose a different approach. All the multiple type fields use a generic wildcard
(?) providing flexibility when passing the field values. At the same time we add validation,
so if any of those fields receive a value of an unexpected class, an IllegalArgumentException
is thrown informing about the error.

Note this PR is WIP to discuss the approach before making it more generic.

Also, 2 minor fixes related with Maven best practices. Those can be deleted or moved to their own PR.

Related issue(s)
#126

@sam0r040
Copy link

@timonback and me also noticed the issue last week and discussed possible solutions. I think using the wildcard Operator is a possible fix, but we would like to suggest another option which does not use reflection: Cant we use interface to control the types that can be added to the bindings? Something like MessageBindingValues (probably needs a better name) which is then implemented by MessageBinding and Reference. Reference would then implement a lot of those interfaces but I think that would be okay and also reflect the spec where you can use a Reference object in many but not all places. What do you think?

@ctasada
Copy link
Author

ctasada commented Jun 23, 2023

@sam0r040 I will take a look to the interface approach. I kind of like the MessageBindingValues naming pattern, and yes, Reference may end implementing too many interfaces, but it removes a lot confusion about which class can be used where.

@ctasada ctasada force-pushed the ctasada/asyncapi-2.6.0 branch from 5f49700 to abd23b4 Compare June 26, 2023 10:35
Carlos Tasada added 3 commits June 26, 2023 13:10
Files related with the IDE are usually ignored from VCS. The reason is that those files are usually strongly coupled with local development environments and difficult to share across users.
The Maven Release lifecycle indicates that between releases, the `<version>`
value should be prefixed with `-SNAPSHOT`. This allows to properly identify
what's the next version release and provide an easy way to deploy development versions for local testing.
Some of the AsyncAPI fields support more than an object type. Previously this problem was
"solved" accepting any `Object` but this approach required castings or directly losing
the generic validations in any consumer code.

Here we propose to define an interface for each field type. For example `MessageBindingValue`
interface can be implemented both by the `Message` and the `Reference` models.

Note this PR is WIP to discuss the approach before making it more generic.
@ctasada ctasada force-pushed the ctasada/asyncapi-2.6.0 branch from abd23b4 to b3d6fb7 Compare June 26, 2023 11:10
@ctasada
Copy link
Author

ctasada commented Jun 26, 2023

@sam0r040 @timonback Code is refactored to use the proposed interface approach. The resulting code looks quite good, since doesn't require any magic and any undesired field type will fail during compilation.

I am open to better names, since I'm not really attached to MessageBindingValue Maybe something like MessageBindingValueType or MessageBindingObjectType would be more descriptive

@Pakisan what do you think?

@Pakisan
Copy link
Member

Pakisan commented Jun 27, 2023

@ctasada hi!

Will check it till end of week. Thanks for your PR

@ctasada
Copy link
Author

ctasada commented Aug 20, 2023

@Pakisan have you had the chance to take a look?

@sam0r040
Copy link

@Pakisan Can you have look? Springwolf is depending on jasyncapi and we are kind of waiting for the updates. If you need help maintaining, please reach out to us :)

@Pakisan
Copy link
Member

Pakisan commented Sep 17, 2023

Hey, folks, @ctasada @sam0r040, sorry for late review

When I viewed this PR for the first time, I thought, it was ok, to use common interface, to prevent wrong values in runtime, but now I'm not sure

Possible Problem:

  • proposed asyncapi-core/src/main/java/com/asyncapi/v2/MessageTraitValue.java, won't work correctly for MessageTrait, because it has variants for two versions - 2.0.0 and 2.6.0

Goal:

  • prevent undefined behavior when user crafts its specification from multiple versions

Possible solution:

  • each version provides its own interfaces, which will be implemented by common parts of the specification, like Reference or MessageBinding

For me, this issue is about how to handle changes like this in the future - when bindings can hold references too, for example

It's actual as never because of new major release. I don't want to bring same problems and limitations to new specification

@ctasada
Copy link
Author

ctasada commented Oct 3, 2023

@Pakisan I have been looking into your proposal

The main problem we are trying to solve is that the current implementation allows for Object in many places. While that's OK for a deserialization process (parse asyncapi.yaml), it's not good enough for a builder API, where we need to be really protective about the types that we accept.

My understanding is that your main concern is regarding how future proof is this approach.

Lets imagine that, in the future, the ComponentsObject.MessageTrait cannot accept a Reference object anymore. Then we will need to remove the MessageTraitValue interface from the Reference implementation list. That will happen in it's own version v.2.X.Y so everything should stay correct. Those interfaces are intended to define behaviours.

In your comment you mention "prevent undefined behavior when user crafts its specification from multiple versions" I'm not sure to understand what would be this scenario.

If you prefer we can also schedule a face-to-face meeting and discuss the alternatives.

@ctasada ctasada mentioned this pull request Oct 21, 2023
Copy link

github-actions bot commented Feb 1, 2024

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

@github-actions github-actions bot added the stale label Feb 1, 2024
@ctasada ctasada closed this Apr 20, 2024
@ctasada ctasada deleted the ctasada/asyncapi-2.6.0 branch April 20, 2024 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants