fix(outbox): gate PG OutboxAutoConfiguration on backend=PG (resolves Phase 0a B2) #9
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/outbox-pg-backend-gate"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Resolves the Phase 0a B2 deferred item (now load-bearing for Wave A.2 Phase 1)
The PG
OutboxAutoConfigurationactivated onim2be.outbox.enabled=truealone. A Redis-only consumer (identity-service:im2be.outbox.backend=REDIS) that setsenabled=truetherefore ALSO triggered the PG graph — which wires JPA + a ShedLockJdbcTemplateLockProviderneeding aDataSource+ spring-jdbc it doesn't have. Condition evaluation crashes at context load:This is exactly the B2 item Phase 0a deferred "to when the Redis module lands" — it just bit identity-service's Phase 1a wiring.
Fix
Gate the whole PG autoconfig:
backenddefaults toPG→ the 7 PG services (enabled=true, no explicit backend) are unchanged.backend=REDIS→ the entire PG graph is skipped;RedisOutboxAutoConfiguration(Phase 0b) supplies theOutboxBackendbean instead.Tests
New
OutboxBackendSelectionTest(4, ApplicationContextRunner): PG-active for enabled+default-backend AND enabled+backend=PG (mock DataSource/Kafka/repo so the active graph wires); skipped forbackend=REDISandenabled=false. Catches an expression typo that would otherwise silently disable all 7 PG services.Verification
mvn -pl outbox-publisher -am clean verify→ BUILD SUCCESS, 0[WARNING], 100 unit (+4) + 4 IT green.Note: the Forgejo
mvn installCI check is red on this repo for all commits (pre-existing runner-toolchain bug, PR-PLATFORM-CI-1). Verified locally.The PG OutboxAutoConfiguration activated on im2be.outbox.enabled=true alone, so a Redis-only consumer (identity-service: im2be.outbox.backend=REDIS) that sets enabled=true ALSO triggered the PG graph — which wires JPA + a ShedLock JdbcTemplateLockProvider needing a DataSource + spring-jdbc the consumer does not have. Condition evaluation then crashes at context load ("Error processing condition on OutboxAutoConfiguration.outboxLockProvider"). This is the Phase 0a B2 deferred item, now load-bearing because the Phase 0b Redis backend + its first consumer (identity Phase 1a) exist. Fix: gate the whole PG autoconfig with @ConditionalOnExpression("'${im2be.outbox.enabled:false}' == 'true' and '${im2be.outbox.backend:PG}' == 'PG'"). The backend default is PG, so the 7 PG services (enabled=true, no explicit backend) are UNCHANGED; a Redis consumer (backend=REDIS) skips the entire PG graph, and RedisOutboxAutoConfiguration (Phase 0b) supplies the OutboxBackend bean instead. Class Javadoc documents the gate + why a Redis-only classpath cannot load the PG graph. New OutboxBackendSelectionTest (4 cases, ApplicationContextRunner): PG-active for enabled+default-backend AND enabled+backend=PG (supplying mock DataSource/Kafka/ repo so the active graph wires); skipped for backend=REDIS and for enabled=false. Catches an expression typo that would silently disable all 7 PG services. Verification: - mvn -pl outbox-publisher -am clean verify -> BUILD SUCCESS, 0 [WARNING] - 100 unit (+4 new) + 4 IT green; the 7-PG-service path unchanged (default PG).Show previous round
hib-pr-reviewer review — PR #9 (affinity-intelligence-rework/im2be-platform-libs)
Round 1 — head
db7788dbcee7, basemain, triggeropenedTL;DR: CONDITIONAL_APPROVE — kept 1 agreed minor finding (SpEL case-sensitivity regression) and 2 verified unique info findings (missing no-DataSource test scenario; misleading Javadoc); no blocking issues.
Summary
Reconciliation: 3 findings kept (1 agreed minor + 2 unique info, all verified)
Agreed finding kept (1): Both A and B independently flag the SpEL
==case-sensitivity regression on line 118. Reading the file confirms the exact expression'${im2be.outbox.enabled:false}' == 'true' and '${im2be.outbox.backend:PG}' == 'PG'. I also verifiedRedisOutboxAutoConfiguration.javaline 98 uses@ConditionalOnProperty(havingValue = "REDIS")— which IS case-insensitive — confirming the asymmetry A described (A incorrectly claimed it was "line 98 of that file" rather than a sibling module, but the substance is correct). Finding kept at minor severity.Unique-to-A (info, verified): A claims
pgAutoConfigSkippedWhenBackendRedisexercises the gate via a shared runner that already includes aDataSourcemock, and therefore does not cover the production crash scenario (no DataSource + backend=REDIS). Reading lines 27–33 ofOutboxBackendSelectionTest.javaconfirms.withBean(DataSource.class, () -> mock(DataSource.class))is part of the class-levelrunnerused by all four tests including the REDIS case. The missing-DataSource scenario is the exact original failure mode the gate was designed to prevent. Finding kept at info severity.Unique-to-B (info, verified): B flags that the class-level Javadoc (lines 19–22) and the per-test comment (lines 58–61) both assert "classpath has no DataSource / spring-jdbc" yet the shared runner provides one. Reading the test file confirms the Javadoc text at line 19 ("whose classpath has no DataSource / spring-jdbc") is factually false given the runner definition. This is a distinct, actionable documentation fix separate from A's test-gap concern. Finding kept at info severity.
No findings dropped. Total reconciled: 3 (1 minor, 2 info).
Blast Radius
The diff modifies the class-level activation condition of
OutboxAutoConfiguration, which is the PG outbox auto-configuration consumed by 7 services. A misconfigured condition (e.g. the case-sensitivity regression) would silently deactivate the entire PG outbox graph in all 7 consumers with no error at startup. The Redis module and its consumers are unaffected by this PR directly, but the asymmetry between the two gates now creates a consistency risk for future maintainers.BLAST_SCORE: 5/10
CI status (head
db7788dbcee7)Overall: ✗ failure
2 checks: 2 pending
Findings (3)
[MINOR] SpEL
==is case-sensitive — silent regression from@ConditionalOnProperty'sequalsIgnoreCasefor bothenabledandbackendoutbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxAutoConfiguration.java:118
Line 117–118:
SpEL
==delegates toString.equals()— strict case comparison. The replaced@ConditionalOnProperty(havingValue = "true")usedequalsIgnoreCaseinternally, soenabled=TRUEorenabled=True(both legal in YAML and common in environment-variable injection) activated the prior config. Under the new expression those values silently evaluate tofalseand the entire PG graph is skipped with no diagnostic.The
backendcomparison carries a separate risk: Spring Boot's relaxed binding normalisespg→PGwhen binding toOutboxProperties.Backendat runtime, but that normalisation does not apply inside@ConditionalOnExpressionplaceholder resolution. A YAML author writingim2be.outbox.backend: pg(lower-case, idiomatic YAML) gets'pg' == 'PG'→false, the PG graph is skipped, no Redis graph exists, and the context loads with noOutboxBackendbean and no meaningful error.Asymmetry confirmed:
RedisOutboxAutoConfiguration(redis-outbox-backend module, line 98) still uses@ConditionalOnProperty(prefix = "im2be.outbox", name = "backend", havingValue = "REDIS")— the case-insensitive path — so the two halves of the gate are now inconsistent.Fix — use
.equalsIgnoreCase()in SpEL:Alternatively, keep the battle-tested
@ConditionalOnPropertyfor the boolean and add a second property condition for backend:(Two
@ConditionalOnPropertyannotations are ANDed by Spring.)[INFO]
pgAutoConfigSkippedWhenBackendRedisdoes not exercise the original crash path — shared runner provides aDataSourcemockoutbox-publisher/src/test/java/com/aim2be/platform/outbox/OutboxBackendSelectionTest.java:57
The class-level runner (lines 27–33) registers
.withBean(DataSource.class, () -> mock(DataSource.class))and is reused by all four tests, includingpgAutoConfigSkippedWhenBackendRedisat line 57. The bug that prompted this PR was a context crash onJdbcTemplateLockProvidercondition evaluation when no DataSource is present (identity-service, Redis-only classpath). The existing test proves the gate fires and suppressesOutboxBackend/OutboxPublisherbeans, but does not prove the gate fires when DataSource is absent from the context — which is the production scenario.Consider adding a dedicated test without the
DataSourcebean to guard against regression of the original crash independently of the mock infrastructure:[INFO] Class Javadoc (and per-test comment) falsely claim no
DataSourceis present for the REDIS test caseoutbox-publisher/src/test/java/com/aim2be/platform/outbox/OutboxBackendSelectionTest.java:19
Lines 19–22 of the class Javadoc read:
Lines 58–61 of
pgAutoConfigSkippedWhenBackendRedissimilarly say "the absentJdbcTemplateLockProvider".Both claims are contradicted by the shared
runnerdefinition (lines 27–33), which registers.withBean(DataSource.class, () -> mock(DataSource.class))for all four tests including the REDIS case. The test is not simulating a no-DataSource classpath; it is verifying that the gate's CLOSED decision preventsOutboxBackendandOutboxPublisherbeans from appearing (a valid and correct assertion). The comments should be updated to reflect what the test actually exercises, for example:backend=REDISis set, regardless of whether aDataSourceis present"Note: the actual crash-regression scenario (no DataSource + backend=REDIS) is the subject of the separate finding at line 57.
Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 1 • 3 findings (1m/2i) • 2026-05-28T13:24:05.739Z → 2026-05-28T13:26:22.379Z • posted-as: pr-reviewer-bot
R1 verdict CONDITIONAL_APPROVE, findings (kept=3: 1 MINOR / 2 INFO): (1) MINOR — SpEL '==' is case-SENSITIVE, a regression from the replaced @ConditionalOnProperty(havingValue) which matches case-INSENSITIVELY. enabled=TRUE / backend: pg (idiomatic YAML + env-var casing) would have silently skipped the PG graph with no diagnostic; also inconsistent with the Redis module's case-insensitive @ConditionalOnProperty(havingValue=REDIS). (2) INFO — OutboxBackendSelectionTest's REDIS case used the shared runner that provides a mock DataSource, so it never exercised the ACTUAL crash path (no DataSource present — identity's Redis-only classpath). (3) INFO — the class Javadoc + per-test comment claimed "no DataSource" while the shared runner supplies one (contradiction). Fix: - (1) gate now uses .equalsIgnoreCase('true') / .equalsIgnoreCase('PG') so enabled=TRUE and backend=pg still match (parity with @ConditionalOnProperty + Spring relaxed binding). Class Javadoc documents the case-insensitivity + why a SpEL == would regress. - (2) new pgAutoConfigSkippedWhenBackendRedisNoDataSourceDoesNotCrash — a FRESH runner with NO DataSource bean (mirrors identity's Redis-only classpath), asserting the context loads cleanly (the gate prevents the original JdbcTemplateLockProvider crash). This is the real regression guard. - (1-guard) new gateIsCaseInsensitiveForEnabledAndBackend (enabled=TRUE + backend=pg → PG graph active) + gateSkipsForLowercaseRedisBackend (backend=redis → skipped) — would fail under a SpEL == regression. - (3) class Javadoc + per-test comment corrected: the shared runner supplies mocks to prove bean-gating + active-graph wiring; the dedicated no-DataSource test proves crash-prevention. Verification: - mvn -pl outbox-publisher -am clean verify -> BUILD SUCCESS, 0 [WARNING] - 103 unit (+3) + 4 IT green.Show previous round
hib-pr-reviewer review — PR #9 (affinity-intelligence-rework/im2be-platform-libs)
Round 2 — head
7bdf98e64173, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — all three round-1 findings confirmed closed by both reviewers; one unique-to-B minor finding (SpEL injection via ${…} interpolation at line 123) verified against source and kept.
Summary
Arbitration — Round 2
Round-1 findings (R1-F1 SpEL case sensitivity, R1-F2 crash-path coverage, R1-F3 false Javadoc): Both reviewers agree all three are closed. No arbitration needed.
Unique-to-B finding (SpEL injection via
${…}interpolation): Verified by reading lines 122-124 ofOutboxAutoConfiguration.java. The code exactly matches what Reviewer B described:Spring resolves
${im2be.outbox.backend:PG}to the raw property string before handing the concatenated expression to the SpEL parser. A value containing a single-quote (e.g.PG') produces a broken string literal; a crafted value can escape the literal and execute arbitrary SpEL. The injection surface is genuine. Severity minor is correct — these are developer/ops-controlled properties in practice, so exploitation requires compromising the config pipeline, but the antipattern is real and the fix is low-effort. Finding kept.Blast Radius
The diff touches two files: one auto-configuration class and one new test class. The
@ConditionalOnExpressionchange gates a single auto-configuration bean graph and affects only services that setim2be.outbox.backend; the seven existing PG services are unaffected by default. Blast radius is contained to the outbox-publisher module.BLAST_SCORE: 3/10
Risk Indicators
@ConditionalOnExpressionCI status (head
7bdf98e64173)Overall: ✗ failure
2 checks: 2 pending
Findings (1)
[MINOR] SpEL injection surface:
${...}property values embedded as string literals in@ConditionalOnExpressionoutbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxAutoConfiguration.java:123
Lines 123-124 embed property values directly into the SpEL expression string via
${…}interpolation:Spring resolves
${im2be.outbox.backend:PG}to the raw property value and concatenates it into the expression string before SpEL parsing. A value containing a single-quote breaks the SpEL parser; a crafted value such asPG'.concat(T(java.lang.Runtime).getRuntime().exec('...')).'can escape the string literal and execute arbitrary SpEL at context load time. In typical deployments these properties are developer/ops-controlled, so exploitation requires compromising the config pipeline (external config server or env-var injection), but the pattern is a recognised Spring security antipattern and the fix is a one-liner.Recommended fix — resolve property values at runtime via
environment.getProperty()so they are never embedded in the expression string:environmentis theConfigurableEnvironmentbound in the@ConditionalOnExpressionevaluation context byConditionEvaluator. Property values are fetched asStringobjects at runtime — never spliced into the expression string — so the injection surface is eliminated. Relaxed binding is preserved because Spring Boot'sConfigurationPropertySourcesare already registered on thatEnvironment. No test changes required; the existing six-test suite covers both open/closed semantics and case-insensitivity in both directions.Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 2 • 1 finding (1m) • 2026-05-28T13:33:03.187Z → 2026-05-28T13:33:51.993Z • posted-as: pr-reviewer-bot
R2 verdict CONDITIONAL_APPROVE; all 3 R1 findings confirmed closed. 1 new MINOR (kept): (1) MINOR — SpEL injection surface: the @ConditionalOnExpression embedded ${im2be.outbox.backend} / ${im2be.outbox.enabled} raw property VALUES into the expression string via ${…} interpolation. A crafted value (single-quote + T(java.lang.Runtime)…) escapes the string literal and executes arbitrary SpEL at context load — the recognised SpEL-injection antipattern. Internal config makes it low-risk, but it is a real surface (rule 1). Fix: replaced @ConditionalOnExpression with a custom @Conditional (OnPgOutboxBackendCondition) that reads the Environment directly off the ConditionContext in Java + compares with equalsIgnoreCase. No SpEL, no ${} interpolation → no injection surface; still case-insensitive (reviewer R1). NB (rule 63 — verified the suggestion before applying): the reviewer's suggested `environment.getProperty(...)` SpEL form does NOT work in @ConditionalOnExpression — at condition-eval time the BeanExpressionContext exposes neither an `environment` property (EL1008E) nor an `@environment` bean (EL1058E, NoSuchBeanDefinition). The custom Condition is the robust path: ConditionContext .getEnvironment() is always available. The condition's Javadoc records both the injection rationale and the failed-SpEL-forms for the next maintainer. Verification: - mvn -pl outbox-publisher -am clean verify -> BUILD SUCCESS, 0 [WARNING] - 103 unit + 4 IT green; OutboxBackendSelectionTest (7) unchanged + passing against the custom Condition (PG-active default/PG, skipped REDIS + disabled + no-DataSource crash-path, case-insensitive enabled=TRUE/backend=pg).hib-pr-reviewer review — PR #9 (affinity-intelligence-rework/im2be-platform-libs)
Round 3 — head
ac825147eb59, basemain, triggersynchronizeTL;DR: NO_NEW_FINDINGS — No new findings this round.
Summary
[quorum-converged] A=0 = B=0. ## Round 3 Review
All four prior-round findings are resolved:
==case-sensitivity atOutboxAutoConfiguration.java:118OnPgOutboxBackendConditionusesequalsIgnoreCasefor bothenabledandbackendpgAutoConfigSkippedWhenBackendRedisdoesn't exercise the crash path (shared runner has aDataSource)pgAutoConfigSkippedWhenBackendRedisNoDataSourceDoesNotCrashtest at line 82 uses a completely fresh runner with noDataSource,KafkaTemplate, orOutboxRecordRepositoryDataSourcewas presentDataSource${…}interpolation in@ConditionalOnExpression@ConditionalOnExpressionreplaced entirely with@Conditional(OnPgOutboxBackendCondition.class); condition readsEnvironmentdirectly in Java with no SpEL involvedImplementation quality:
OnPgOutboxBackendConditionis correct and minimal.ConditionContext.getEnvironment()is guaranteed non-null by Spring's contract; the default-value overloads ensurenull-safety; the class is intentionally package-private (Spring processes@Conditionalvia reflection so visibility is irrelevant). The@AutoConfigureAfterentries are pre-existing and Spring handles gracefully-absent ordering targets.Test coverage: Seven complementary tests cover the open path (default, explicit PG, mixed case), the closed path (REDIS with and without
DataSource, lowercase redis, disabled), and the exact crash scenario that motivated the PR. No actionable new findings.CI status (head
ac825147eb59)Overall: ✗ failure
2 checks: 2 pending
Findings
No new findings this round.
Quorum converged on empty findings (A + B both returned 0).
Verdict
NO_NEW_FINDINGS
hib-pr-reviewer • round 3 • 0 findings • 2026-05-28T13:40:04.359Z → 2026-05-28T13:42:16.004Z • posted-as: pr-reviewer-bot • [bookkeeping fallback]