springwolf-core icon indicating copy to clipboard operation
springwolf-core copied to clipboard

(feat): Decoupled Binding annotations

Open ctasada opened this issue 1 year ago • 6 comments

Decouple the binding annotations.

The annotations to document the bindings were only available via the different springwolf-plugins, and were forcing to import multiple extra dependencies.

Those annotations are now published in it's own module without extra dependencies, so they can be use for documentation goals. If you want to produce/consume events, you still need to use the adequate springwolf-plugin

  • springwolf-plugins:springwolf-sqs-binding
    • @SqsAsyncOperationBinding
    • @SqsAsyncQueueBinding
  • Pending
    • @AmqpAsyncOperationBinding
    • @JmsAsyncOperationBinding
    • @KafkaAsyncOperationBinding
    • @SnsAsyncOperationBinding
    • @SnsAsyncOperationBindingIdentifier

ctasada avatar Mar 02 '24 11:03 ctasada

Deploy Preview for springwolf-ui canceled.

Name Link
Latest commit 843a203e4d44e8bc74954f776c9e3cc681655cf9
Latest deploy log https://app.netlify.com/sites/springwolf-ui/deploys/65f1b2ddb88944000841cfb5

netlify[bot] avatar Mar 02 '24 11:03 netlify[bot]

Hi @ctasada,

great idea, however the timing is unfortunate as we just released 1.0.0, which allowed us to easily discuss and including breaking changes.


We discussed more in detail and focused on the argument of the dependencies that you mention as well.

We believe that the current Springwolf structure using the plugin mechanism is very powerful. A plugin currently adds 3 aspects:

  1. The detection of the spring listener annotation (only additional dependency, i.e. spring-kafka)
  2. The publishing endpoint (i.e. the spring-web dependency that is present in core as well)
  3. The binding (focus of this PR)

Moving the bindings into the core, will result in many spring beans that are being created and not used in core. More importantly, it might result in a bloated core as more and more protocol specific functionality is added/moved. The plugins are a nice way, to keep those out of core.


We thought about an alternative: New maven artifacts in the form of i.e. <plugin>-binding

The plugins do have a direct dependency on these artifacts, so existing users do not need to add another artifact. The new artifact is for the "power users", who want a pure core plus binding functionality.

To avoid breaking changes, the Spring aliasFor annotation marker can be used in case the annotation is moved to a different package - again to avoid breaking changes.

Side note: We can add/move the generic-bindings add-on as well.


Is your main motivation the dependencies only, or is there something else as well?

timonback avatar Mar 08 '24 13:03 timonback

Hey @timonback

My main motivation for this change is the injection of extra dependencies. For example, I have a service using SNS, but it's directly using the AWS client and I don't want to inject extra dependencies just for documentation.

I understand the concerns and, in fact, the creation of a <plugin>-binding would work.

With this approach in mind, I can refactor this MR:

  • Add the new plugin-binding artifacts
  • Move the annotations to the new bindings, keeping the packaging

Would that work? Alternatively we can duplicate the annotations and mark the current ones as deprecated.

ctasada avatar Mar 08 '24 16:03 ctasada

Sounds great, thanks for the clarification @ctasada.

We do have an own package namespace for each maven artifact, so the new annotation would contain the package name <plugin>-binding and the existing annotation would reference the new one.

Feel free to go ahead - either with one plugin at first (and we review) or all together.

timonback avatar Mar 08 '24 17:03 timonback

@timonback I refactored the whole MR and migrated only the SQS bindings.

Is this the approach you had in mind?

ctasada avatar Mar 09 '24 15:03 ctasada

@timonback @sam0r040 I did a new refactor.

Now the code is duplicated and the binding artifacts are in its own pipeline, so we have 100% decoupling from plugins.

None of the existing code is using the new binding so we can iterate on the approach before having any real impact.

I decided not to go with deprecation because it's really noisy, but I will fix it later

ctasada avatar Mar 10 '24 17:03 ctasada