Skip to content

fix(starter): harden auto-configuration and fluent findBySpec robustness - #188

Closed
shihyuho wants to merge 1 commit into
jakartafrom
fix/178-starter-autoconfig-robustness
Closed

fix(starter): harden auto-configuration and fluent findBySpec robustness#188
shihyuho wants to merge 1 commit into
jakartafrom
fix/178-starter-autoconfig-robustness

Conversation

@shihyuho

Copy link
Copy Markdown
Member

Summary

Hardens the starter's auto-configuration lifecycle and the fluent query API (WP6):

  • COR-13 — the fluent findBySpec(Object, Function) now tolerates empty/null criteria like every sibling *BySpec method, substituting an unrestricted specification when mapping yields null instead of letting Spring Data's findBy throw IllegalArgumentException("Specification must not be null").
  • COR-12 — the JpaRepositoryFactoryBeanPostProcessor @Bean is now static and injects ObjectProvider<RepositoryFactoryCustomizer> (resolved lazily inside postProcessBeforeInitialization), so registering this BeanPostProcessor no longer force-instantiates customizer beans or the enclosing configuration class during the BPP registration phase.
  • MAINT-02SpecMapperAutoConfiguration is now @AutoConfiguration(after = JpaRepositoriesAutoConfiguration.class), making the nested @ConditionalOnBean(JpaRepositoryFactoryBean.class) gate ordering contractual instead of dependent on alphabetical FQCN sorting.

MAINT-05 (API parity) is intentionally out of scope per the agent brief.

Closes #178

Acceptance criteria

  • Fluent findBySpec(Object, Function) with empty criteria (all fields null) returns the fluent result over all rows rather than throwing; null criteria behaves identically to the other *BySpec methods.
  • New test findByEmptySpecAndQuery mirrors findByEmptySpec for the fluent variant (FetchableFluentQuery::all with empty criteria) and asserts all rows are returned.
  • The @Bean producing JpaRepositoryFactoryBeanPostProcessor no longer eagerly resolves a List<RepositoryFactoryCustomizer>; customizers are supplied lazily via ObjectProvider resolved inside postProcessBeforeInitialization, and the method is static.
  • JpaRepositoryFactoryBeanPostProcessor still applies every registered RepositoryFactoryCustomizer to each JpaRepositoryFactoryBean it post-processes (via customizers.orderedStream()).
  • SpecMapperAutoConfiguration is declared as a proper auto-configuration ordered after JpaRepositoriesAutoConfiguration.
  • The existing starter test suite continues to pass unchanged.

Testing

mvn test across all modules — BUILD SUCCESS. mapper (96 tests) and starter (16 tests, including the new findByEmptySpecAndQuery) both green.

Warning

Higher-risk auto-configuration change. Please closely review the auto-config lifecycle/ordering: the @AutoConfiguration(after = ...) ordering, the static @Bean + lazy ObjectProvider resolution timing relative to BeanPostProcessor registration, and note that SpecMapperAutoConfiguration is shared with the deferred WP1/#173 — coordinate to avoid conflicts.

COR-13: fluent findBySpec(Object, Function) now substitutes an unrestricted
specification when mapping yields null, so empty/null criteria return all rows
(via the fluent query) instead of throwing IllegalArgumentException, matching
every sibling *BySpec method.

COR-12: the JpaRepositoryFactoryBeanPostProcessor @bean is now static and
injects ObjectProvider<RepositoryFactoryCustomizer>, resolved lazily inside
postProcessBeforeInitialization, so registering the BeanPostProcessor no longer
force-instantiates customizer beans (or the enclosing config) during BPP
registration.

MAINT-02: SpecMapperAutoConfiguration is now @autoConfiguration(after =
JpaRepositoriesAutoConfiguration.class), making the nested
@ConditionalOnBean(JpaRepositoryFactoryBean) gate ordering contractual rather
than dependent on alphabetical FQCN sorting.

Refs #178

Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>

@shihyuho shihyuho left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ABSTAIN — the commits check is red and is not attributable to this diff; human review advised

All 9 axes were reviewed and none produced a must-fix finding. The verdict is capped at abstain only because a red check may never be approved over: commits failed while pulling the wagoid/commitlint-github-action:6.2.1 image from Docker Hub — three retries, each Client.Timeout exceeded while awaiting headers — so it never reached the point of linting any commit, and this diff touches no CI configuration. Re-running that job should clear it. This is not a judgement on the code.

Non-blocking (2)

  • FYI · Architecture — starter/src/main/java/tw/com/softleader/data/jpa/spec/starter/repository/support/JpaRepositoryFactoryBeanPostProcessor.java:40 — the public constructor's parameter type changes from List<RepositoryFactoryCustomizer> to ObjectProvider<RepositoryFactoryCustomizer>, a source- and binary-breaking change to a published public type that ships under a fix: (patch) release with no deprecation window.
  • Nit · Test coverage — starter/src/main/java/tw/com/softleader/data/jpa/spec/starter/autoconfigure/SpecMapperAutoConfiguration.java:116 — the COR-12 change ships with no test asserting its actual payload (that customizer beans are no longer force-instantiated during the BeanPostProcessor registration phase), so a future revert to a non-static @Bean or an eager List parameter would reintroduce the silent proxy-loss bug with fully green CI.
Coverage & checks
Axis Status
Correctness clean — traced the COR-13 null path end to end and confirmed the assertion being avoided against SimpleJpaRepository.doFindBy in spring-data-jpa 3.5.9; confirmed toSpec returns null for both a null rootObject and all-empty criteria, so one ternary covers both
Spec conformance clean — checked all six stated acceptance criteria and the three in-scope findings (COR-13, COR-12, MAINT-02) against the sources at the head SHA
Scope clean — all four changed files trace to a named finding of the linked issue; import churn checked for unrelated drift
Convention clean — AGENTS.md's commit-hygiene rule is explicitly CI-enforced (skipped per the charter); CONTRIBUTING.md checked rule by rule; the PR title is a well-formed conventional commit
Security clean — no string-built queries (the substituted spec contributes no predicate), no secrets, no authz surface touched
Readability clean — 2 lines replace 1, flat ternary; orphaned imports removed exactly where the change orphaned them, with no unrelated pruning
Architecture 1 finding
Performance clean — orderedStream() now resolves per factory bean, but getBeanNamesForType is served from the frozen allBeanNamesByType cache, so the cost is not material
Test coverage 1 finding
  • Verifiedcontinuous-integration/jenkins/branch, continuous-integration/jenkins/pr-merge and pr-title green; commits red — Docker Hub image-pull timeout, see the verdict above (all read via gh pr checks, never run locally)
  • Out-of-band — read QueryBySpecExecutorAdapter, SpecCodec/SpecMapper and the new test at the head SHA · cross-checked SimpleJpaRepository.doFindBy's Assert.notNull(spec, ...) against the pinned spring-data-jpa 3.5.9 source · traced DefaultListableBeanFactory.getBeanNamesForType caching to dismiss the orderedStream() performance candidate

🤖 Reviewed by Claude Opus 4.8

(self-review — same account; GitHub records this as event=COMMENT with no badge, so the verdict above is advisory.)

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.

[WP6] Starter auto-configuration robustness (fluent null-spec, BPP early-init, ordering, API parity)

1 participant