asyncapi / jasyncapi

/jay-sync-api/ is a Java code-first tool for AsyncAPI specification
Apache License 2.0
67 stars 23 forks source link

fix: properly support multiple types in asyncapi 2.6.0 #153

Closed ctasada closed 4 months ago

ctasada commented 1 year ago

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) https://github.com/asyncapi/jasyncapi/issues/126

sam0r040 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@ctasada hi!

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

ctasada commented 1 year ago

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

sam0r040 commented 1 year ago

@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 commented 12 months ago

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:

Goal:

Possible solution:


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 commented 11 months ago

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

github-actions[bot] commented 7 months ago

This pull request has been automatically marked as stale because it has not had recent activity :sleeping:

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