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

Feature/replace componentscan #386

Merged
merged 9 commits into from
Oct 13, 2023

Conversation

tvahrst
Copy link
Contributor

@tvahrst tvahrst commented Oct 2, 2023

This PR resolves #291 . The core module and all plugin modules have their own local, isolated autoconfigurations. The autoconfigurations define the necessary beans explicitly via @bean annotations, component-scan over all spring-wolf modules was removed.

Some implementation details:

  • I structed the configuration classes: One 'root' autoconfiguration and special configurations (for the scanner/processor beans and for the web-beans) which are imported into the autoconfiguration.

  • two core classes used @Autowired for dependency injection (AbstractClassLevelListenerScanner and AbstractMethodLevelListenerScanner). I replaced @Autowired with Constructor injection (like all other Springbean classes)

  • ConfigurationProperties Beans do not need to be spring-configurations, so I removed the @configuration annotation from these bean-classes.

  • @conditional* and @order Annotations had to be moved from springbean classes into the configuration classes, because these annotations must be located where beans are defined.

  • The @service / @component annotations in the springbean classes aren't necessary anymore, but I leaved them as kind of 'documentation' that these classes are springbean classes.

  • Some classes used Optional<> for dependencies, i.e. Optional<Environment>, Optional<SpringwolfConfigProperties>, Optional<List<...>>. I removed these Optionals:
    ** Optional<Environment> is not necessary, because the Environment is always present
    ** Optional<SpringwolfConfigProperties> is now explicitly defined the spring configuration and not optional anymore
    ** Optional<List<...>> is not necessary, because spring injects always a non-null Collection

  • One test (DefaultAsyncApiDocketServiceTest#testNoConfigurationShouldThrowException) became obsolet because SpringwolfConfigProperties aren't optional anymore.

  • Some tests had to be modified due to the change from component-scanning to explicit bean configuration

  • currently the packages in the plugin modules share the same namespace, there are no plugin specific packages to differentiate for example kafka and amqp scanners. For now, I put the spring configurations in all plugin modules into the package 'io.github.stavshamir.springwolf.asyncapi.{pluginname}'

@netlify
Copy link

netlify bot commented Oct 2, 2023

Deploy Preview for springwolf-ui canceled.

Name Link
🔨 Latest commit 26d082b
🔍 Latest deploy log https://app.netlify.com/sites/springwolf-ui/deploys/6528d9a42cd02800087e7182

@tvahrst
Copy link
Contributor Author

tvahrst commented Oct 2, 2023

TBD: springboot internal autoconfigurations as well as external springboot 'starter' libs typically define their beans with @ConditionalOnMissingBean. This allows application code to 'override' specific beans with their own (customized) implementations. Should we add this possibilty to all/some springwolf beans?

@timonback
Copy link
Member

Thank you for this huge improvement @tvahrst and good to see you again.

We most likely review the PR this Friday and am already excited to dive more in depth and learn from it!

The use of conditionalOnMissingBean sounds good. If it is a larger change, maybe a second PR is good for it (can be on top of this one)?

@tvahrst
Copy link
Contributor Author

tvahrst commented Oct 2, 2023

well, the PR looks larger than it effectivly is ;-) I basically put all springbeans as @bean Definitions into configuration classes...

Extending the bean definitions with @ConditionalOnMissingBean is indeed an optional step, we can add this feature with an additional PR.

@timonback
Copy link
Member

Thank you @tvahrst and @harare for this huge improvement of springwolf and transforming it more into a library!

We like the changes very much and indeed, the change looked bigger than it actually is :)

We applied your suggestion of cleaning up the @Component and @Service annotation to reduce confusion in the future (https://github.com/LVM-IT/springwolf-core/pull/2). (When you activate the 'Maintainers are allowed to edit this pull request' option on the PR, we can update your branch directly)

We do have one question regarding the @Order annotation. For some classes, the annotation was moved to the AutoConfiguration class - which makes sense. Shall the @Order annotation of the scanners also be moved to the AutoConfiguration classes?

Let's put the @ConditionalOnMissingBean in the next PR.

From our point of view, this is ready to be merged and this can be marked as Ready for review.

@timonback timonback mentioned this pull request Oct 6, 2023
@tvahrst tvahrst marked this pull request as ready for review October 7, 2023 12:18
@tvahrst
Copy link
Contributor Author

tvahrst commented Oct 7, 2023

We do have one question regarding the @order annotation. For some classes, the annotation was moved to the AutoConfiguration class - which makes sense. Shall the @order annotation of the scanners also be moved to the AutoConfiguration classes?

Yes, I think, this is a good idea. If we were using the (old) spring interface Ordered in our scanner classes, we had to put the order information into the class. With the @Ordered annotation, we are free to place the annotation into the spring class or into the configuration class. Moving the annotation to the configuration classes will concentrate the spring DI concerns at one place...

Should we move the annotations with this PR or with an seperate one?

@timonback
Copy link
Member

timonback commented Oct 7, 2023

Great, let's move the remaining Ordered annotations to the Configuration classes as well.

For me, it is consistent to include it in this PR as well - in essence the existing annotation is refactored/moved.

After updating the PR, we can merge it :)

@tvahrst
Copy link
Contributor Author

tvahrst commented Oct 13, 2023

Ok! I moved the remaining @Order annotations from the spring classes into the spring configurations.

@timonback timonback merged commit c23bcf0 into springwolf:master Oct 13, 2023
@timonback
Copy link
Member

The change has been merged and will be part of the next release.

If you want to try and verify it in your application, use the current SNAPSHOT build as described in our README.md

Thank you for the report/contribution!

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.

replace componentscan based context configuration with explicit configuration file and @Beans declarations
3 participants