Conversation
Prevent duplicate StepEventPublisher bean registration when multiple bridge modules (core, domain, data) are on the classpath. Without this guard, Spring would fail with BeanDefinitionOverrideException or pick one non-deterministically.
ancongui
left a comment
There was a problem hiding this comment.
Code Review: Add @ConditionalOnMissingBean guard to StepEventPublisher bean
Summary
This PR adds @ConditionalOnMissingBean(StepEventPublisher.class) to the stepEventPublisher() bean method in StepEventsAutoConfiguration. The goal is to prevent BeanDefinitionOverrideException when multiple bridge modules (core, domain, data) are on the classpath.
CI Status
✅ All checks pass (CodeQL, build)
Findings
1. Correct use of @ConditionalOnMissingBean (Severity: None — looks good)
The annotation is correctly placed on the @Bean method and uses the interface type StepEventPublisher.class. This means if any other module has already registered a StepEventPublisher bean, this one will back off — exactly the right behavior for an auto-configuration module.
2. @Primary + @ConditionalOnMissingBean combination (Severity: Low — informational)
Having both @Primary and @ConditionalOnMissingBean on the same bean is slightly unusual. @Primary only matters when multiple beans of the same type exist, but @ConditionalOnMissingBean prevents this bean from being created if another already exists. In practice this is fine because:
- If this bean IS created (no other exists),
@Primaryis harmless (it's the only one) - If this bean is NOT created,
@Primaryis irrelevant
However, you might consider removing @Primary since @ConditionalOnMissingBean makes it effectively unnecessary. This is a minor style point, not a blocker.
3. Consistency with sibling PRs (Severity: Low)
This PR uses @ConditionalOnMissingBean(StepEventPublisher.class) (class-based), while the domain and data PRs use @ConditionalOnMissingBean(type = "org.fireflyframework.transactional.saga.events.StepEventPublisher") (string-based). Both work, but the string-based version is safer if the class might not be on the classpath. Since StepEventPublisher is imported and used in this file anyway, the class-based approach is fine here.
4. Auto-configuration ordering (Severity: Medium — worth noting)
Since all three bridge modules (core, domain, data) now have @ConditionalOnMissingBean guards, whichever auto-configuration loads first wins. Ensure that @AutoConfigureBefore/@AutoConfigureAfter ordering is defined to make the resolution deterministic, or verify that the intended "primary" module always wins.
Recommendation
APPROVE — The change is correct and follows Spring Boot auto-configuration best practices. The minor points above are not blockers.
Summary
@ConditionalOnMissingBean(StepEventPublisher.class)toStepEventsAutoConfiguration.stepEventPublisher()to prevent duplicate bean registration when multiple bridge modules (core, domain, data) are on the classpath.BeanDefinitionOverrideExceptionor pick one non-deterministically when multiple@PrimaryStepEventPublisher beans exist.Test plan
fireflyframework-coreon classpathfireflyframework-coreandfireflyframework-domainon classpath