feat: create managed dependent resources through Spring - #320
Conversation
Managed dependent resources are now instantiated via Spring's AutowireCapableBeanFactory instead of reflection, so they can receive Spring-managed dependencies (including via constructor injection) while still going through the standard Java Operator SDK configuration step. A user-supplied DependentResourceFactory bean still takes precedence over this default. Related to operator-framework/java-operator-sdk#2166
|
Thank you @hej090224 I'm a bit busy with something else ATM, but will take a look ASAP |
There was a problem hiding this comment.
Pull request overview
This PR updates the Spring Boot starter to create Java Operator SDK managed dependent resources via Spring’s AutowireCapableBeanFactory (enabling Spring-managed injection, including constructor injection), while keeping JOSDK’s dependent-resource configuration behavior and allowing users to override the factory via a custom DependentResourceFactory bean.
Changes:
- Introduces
SpringDependentResourceFactoryand wires it intoOperatorAutoConfigurationbehind@ConditionalOnMissingBean. - Adds Spring Boot test coverage for constructor injection, workflow registration, default factory behavior, and factory override behavior.
- Documents the new default dependent-resource instantiation behavior in the README.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| starter/src/main/java/io/javaoperatorsdk/operator/springboot/starter/SpringDependentResourceFactory.java | Adds a Spring-backed DependentResourceFactory implementation. |
| starter/src/main/java/io/javaoperatorsdk/operator/springboot/starter/OperatorAutoConfiguration.java | Registers the default factory bean and applies it to the operator configuration. |
| starter/src/test/java/io/javaoperatorsdk/operator/springboot/starter/SpringManagedDependentResourceIntegrationTest.java | Integration test ensuring Spring-managed dependent resources receive injected beans and are registered in workflows. |
| starter/src/test/java/io/javaoperatorsdk/operator/springboot/starter/SpringManagedDependentResource.java | Test dependent resource using constructor injection to validate Spring creation. |
| starter/src/test/java/io/javaoperatorsdk/operator/springboot/starter/SpringManagedDependentReconciler.java | Test reconciler defining a workflow with both Spring-created and no-arg dependent resources. |
| starter/src/test/java/io/javaoperatorsdk/operator/springboot/starter/NoArgConstructorDependentResource.java | Test dependent resource validating the no-arg-constructor path still works. |
| starter/src/test/java/io/javaoperatorsdk/operator/springboot/starter/GreetingService.java | Test service interface used for constructor-injection verification. |
| starter/src/test/java/io/javaoperatorsdk/operator/springboot/starter/DependentResourceFactoryConfigurationTest.java | Verifies default factory creation and user override precedence in a lightweight context. |
| starter/src/test/java/io/javaoperatorsdk/operator/springboot/starter/AutoConfigurationTest.java | Adds assertion that the operator configuration uses SpringDependentResourceFactory by default. |
| README.md | Documents the new default dependent-resource instantiation behavior and override mechanism. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public class SpringManagedDependentReconciler implements Reconciler<TestResource> { | ||
|
|
||
| @Override | ||
| public UpdateControl<TestResource> reconcile(TestResource testResource, Context context) { |
|
Hi @hej090224 Some findings that are partially also reported by copilot: Nice approach overall — 1.
|
- SpringDependentResourceFactory#associatedResourceType now destroys the throwaway instance it creates via destroyBean(), instead of leaking it. Falling back to the reflection-based default was not an option: it requires a no-arg constructor, which defeats the purpose of constructor-injected dependent resources. - Add javaoperatorsdk.dependent-resources.spring-managed property (default true) so existing users can opt out of the new default behavior and keep JOSDK's no-arg-constructor instantiation. - Fix @order collision between the two ConfigurationServiceOverrider beans, and switch the dependent-resource-factory overrider to an ObjectProvider gated on @ConditionalOnBean, so it degrades gracefully instead of failing to wire when the factory bean is absent or ambiguous. - Use Context<TestResource> instead of a raw Context in the test reconciler. - Reset the LAST_CREATED_INSTANCE static test hook after the class runs, and document why it reliably captures the workflow instance rather than the (now-destroyed) type-discovery throwaway. - Document the Spring bean lifecycle differences and the new opt-out property in the README. - Add unit tests for the factory's create/destroy behavior and for the new property.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe starter now creates dependent resources through Spring by default. It adds configuration properties, custom-factory precedence, lifecycle handling, resource-type discovery cleanup, and integration tests for injection and workflow registration. ChangesSpring-managed dependent resources
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant SpringBootContext
participant OperatorAutoConfiguration
participant SpringDependentResourceFactory
participant SpringManagedDependentReconciler
SpringBootContext->>OperatorAutoConfiguration: create configured factory
OperatorAutoConfiguration->>SpringDependentResourceFactory: inject factory into operator configuration
SpringManagedDependentReconciler->>SpringDependentResourceFactory: create dependent resource
SpringDependentResourceFactory->>SpringBootContext: request Spring-managed instance
SpringBootContext-->>SpringDependentResourceFactory: return injected instance
SpringDependentResourceFactory-->>SpringManagedDependentReconciler: return configured dependent resource
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thanks for the detailed review, @csviri! Pushed a fix addressing all of it: 1. Instead, 2. 3. Behavior change for existing users — added Smaller items — all applied:
Full test suite (incl. new tests) passes locally, spotless clean. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@starter/src/main/java/io/javaoperatorsdk/operator/springboot/starter/OperatorAutoConfiguration.java`:
- Around line 185-191: Update dependentResourceFactoryConfigServiceOverrider to
use ObjectProvider.ifUnique instead of ifAvailable, so the overrider is applied
only when a single DependentResourceFactory bean can be selected and startup
does not fail when multiple non-primary factories exist.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 20f4e83f-da34-490b-9a72-ca55dc9e976f
📒 Files selected for processing (12)
README.mdstarter/src/main/java/io/javaoperatorsdk/operator/springboot/starter/OperatorAutoConfiguration.javastarter/src/main/java/io/javaoperatorsdk/operator/springboot/starter/OperatorConfigurationProperties.javastarter/src/main/java/io/javaoperatorsdk/operator/springboot/starter/SpringDependentResourceFactory.javastarter/src/test/java/io/javaoperatorsdk/operator/springboot/starter/AutoConfigurationTest.javastarter/src/test/java/io/javaoperatorsdk/operator/springboot/starter/DependentResourceFactoryConfigurationTest.javastarter/src/test/java/io/javaoperatorsdk/operator/springboot/starter/GreetingService.javastarter/src/test/java/io/javaoperatorsdk/operator/springboot/starter/NoArgConstructorDependentResource.javastarter/src/test/java/io/javaoperatorsdk/operator/springboot/starter/SpringDependentResourceFactoryTest.javastarter/src/test/java/io/javaoperatorsdk/operator/springboot/starter/SpringManagedDependentReconciler.javastarter/src/test/java/io/javaoperatorsdk/operator/springboot/starter/SpringManagedDependentResource.javastarter/src/test/java/io/javaoperatorsdk/operator/springboot/starter/SpringManagedDependentResourceIntegrationTest.java
ifAvailable() still throws NoUniqueBeanDefinitionException when multiple non-primary DependentResourceFactory beans are present, same as a direct injection would - it doesn't actually make the two-user-supplied-factories case degrade gracefully. ifUnique() does: it simply skips applying the override, leaving JOSDK's own default factory in place. Adds a test that registers two ambiguous custom factories and asserts applying the overrider doesn't throw and doesn't call withDependentResourceFactory. Addresses coderabbitai's review comment on the previous fix.
|
Addressed CodeRabbit's follow-up finding from the last review:
Good catch — Added a test ( Copilot's two comments from the first round (leak in |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (2)
starter/src/main/java/io/javaoperatorsdk/operator/springboot/starter/SpringDependentResourceFactory.java:47
- This method uses the raw DependentResource type, which will generate unchecked/raw-type warnings. Using DependentResource here avoids those warnings while keeping behavior the same.
final DependentResource instance =
(DependentResource) beanFactory.createBean(spec.getDependentResourceClass());
starter/src/main/java/io/javaoperatorsdk/operator/springboot/starter/SpringDependentResourceFactory.java:36
- If dependent-resource configuration (configure/configureWith) throws, the newly created Spring-managed instance is currently leaked (it has already gone through bean initialization). Consider destroying the instance before rethrowing so failures don’t leave partially-initialized dependent resources hanging around.
This issue also appears on line 46 of the same file.
public DependentResource createFrom(DependentResourceSpec spec,
ControllerConfiguration<?> controllerConfiguration) {
final DependentResource instance =
(DependentResource) beanFactory.createBean(spec.getDependentResourceClass());
configure(instance, spec, controllerConfiguration);
csviri
left a comment
There was a problem hiding this comment.
LGTM, thank you @hej090224 !!
Summary
AutowireCapableBeanFactoryinstead of reflection, so they can receive Spring-managed dependencies (including via constructor injection)configureWith) for dependent resources that implementConfiguredDependentResourceDependentResourceFactorybean (@ConditionalOnMissingBean)Testing
./mvnw -pl starter -am test -Dtest=SpringManagedDependentResourceIntegrationTest,DependentResourceFactoryConfigurationTest,AutoConfigurationTest./mvnw test(all modules)./mvnw verify(all modules)./mvnw spotless:apply/./mvnw spotless:checkRelated to operator-framework/java-operator-sdk#2166
Summary by CodeRabbit
New Features
Documentation
Tests