refactor: 각 어댑터터별 리펙토링 진행
This commit is contained in:
DongHyeonka
@@ -0,0 +1,165 @@
|
||||
# Task 6 — the five reviews' P0 findings, reconciled
|
||||
|
||||
The remediation design this wave executed never referenced a review id (`grep -c` over it: zero).
|
||||
It took one theme from the five reviews — five adapters ship and do not run — and Waves 0–6 executed
|
||||
that theme. So "how much of the reviews is reflected" had never been measured. This is the first
|
||||
measurement, and then the work that followed it.
|
||||
|
||||
Three read-only audits ran in parallel, one per review family, each instructed to treat a passing
|
||||
test as evidence only when it exercises the real composition. Their headline claims were then
|
||||
re-checked by hand before being acted on; one was wrong and is recorded as such below.
|
||||
|
||||
## Where the P0 set stood when measured
|
||||
|
||||
| Review | P0 | CLOSED | PARTIAL | OPEN |
|
||||
| --- | ---: | ---: | ---: | ---: |
|
||||
| jpa | 6 | 5 | 1 | 0 |
|
||||
| messaging | 5 | 2 | 3 | 0 |
|
||||
| graphql | 2 | 1 | 1 | 0 |
|
||||
| notification | 12 | 1 | 11 | 0 |
|
||||
| mongodb | 9 | 0 | 9 | 0 |
|
||||
| **total** | **34** | **9** | **25** | **0** |
|
||||
|
||||
Nothing was OPEN: every P0 had been worked. What the audits found instead, in three independent
|
||||
voices, was the same shape — **the implementation is substantially real and the gate under it is
|
||||
thin**. Six notification adapter classes were referenced by zero tests. Both Mongo executors were
|
||||
referenced by zero tests. Every messaging real-broker test skips silently without Docker. And the
|
||||
Compose matrix, the strongest evidence this repository produces, runs in no workflow.
|
||||
|
||||
## What was fixed, and how each was proven
|
||||
|
||||
### NTF-004 / JPA-004 — the completion write was not fenced
|
||||
|
||||
Two audits reached this independently from different reviews, which is why it was taken first.
|
||||
|
||||
The claim is fenced (a single `FOR UPDATE SKIP LOCKED` CTE that bumps `lease_fence`) and the renew
|
||||
is fenced. The *completion* was `findById → mutate → saveAndFlush` with no owner or fence predicate.
|
||||
A worker whose lease expired during the provider call — the one stretch the platform deliberately
|
||||
spends outside a transaction — came back and wrote its outcome over the row a new holder had already
|
||||
claimed. The `@Version` column does not stop that: it detects a concurrent edit, not a superseded
|
||||
writer, and the late worker's read is recent enough to win.
|
||||
|
||||
Fixed with conditional statements in the same idiom as the renew, `saveHeldBy`/`transitionHeldBy` on
|
||||
the port returning empty when the lease is gone, and all four branches of `applyNextAction` moved
|
||||
onto them. Losing the lease is not an error: the new holder owns the job and will record its own
|
||||
outcome.
|
||||
|
||||
`@Modifying(clearAutomatically = true)` on both, because a native update bypasses the persistence
|
||||
context and the immediate re-read would otherwise be served the values it just replaced — a trap one
|
||||
of the audits had found elsewhere in this same platform.
|
||||
|
||||
**Proof:** two real-PostgreSQL cases in the JPA contract lane. A superseded holder's completion
|
||||
matches zero rows and the live holder's state is untouched; the current holder's completion writes.
|
||||
The second exists because without it the first is satisfied by a statement that matches nothing ever.
|
||||
|
||||
### NTF-012 — the SSRF guard had no callers
|
||||
|
||||
`requireExternallyRoutable` refuses the cloud metadata service, RFC 1918, link-local, IPv6 local,
|
||||
userinfo disguise and multi-answer DNS. It had an eight-case test suite, all green, and **one
|
||||
occurrence in the repository: its own definition.** The two sites it was written for —
|
||||
`WebhookSubscription` and `SesProviderProperties` — still called `requireSecureOrLoopback`, which
|
||||
reads the scheme and nothing else.
|
||||
|
||||
Both now call it. The new test goes through the constructors rather than the helper, because testing
|
||||
the helper again is exactly what failed to catch this.
|
||||
|
||||
What is not closed: `allowLoopback` is true, so a user-supplied target naming `localhost` still
|
||||
passes. Closing it means the allowance becomes a decision the caller states, and the caller does not
|
||||
exist — WEBHOOK has no `ProviderRuntimeAssembler`, so a webhook profile refuses to boot and nothing
|
||||
in production constructs the record. Writing the policy now would mean choosing its default with no
|
||||
caller to check it against. Recorded in the code at the call site.
|
||||
|
||||
### NTF-001 — the documented callback switch broke startup
|
||||
|
||||
`CallbackRequestFactory` is a constructor argument of the MVC controller, the WebFlux handler and the
|
||||
WebFlux configuration, and was produced by no production code — the only instantiation was in a test.
|
||||
So `APP_NOTIFICATION_PLATFORM_CALLBACKS_ENABLED=true`, a key in the env registry and in the
|
||||
configuration reference, did not enable callbacks; it failed the boot on an unsatisfied dependency.
|
||||
|
||||
The beans now exist, conditioned on the same switch. The missing `trusted-proxies` setting came with
|
||||
them, defaulting to empty — with no entry, forwarded headers are not believed, because honouring
|
||||
them unconditionally lets any caller choose the URL its own signature is checked against. Registered
|
||||
in `application.yml`, the env registry, `.env.example` and the configuration reference; both
|
||||
notification gates pass.
|
||||
|
||||
### MNG-007 — a health view Actuator could not read, and a reactive half nothing built
|
||||
|
||||
`MongoPlatformHealthIndicator` computed topology mismatch, secondary availability and a bounded
|
||||
detail map, and implemented neither `HealthIndicator` nor `HealthContributor`. The adapter carries no
|
||||
Actuator dependency and should not: the delivery platform had already established the shape, where
|
||||
the adapter computes the facts and the composition root maps them onto `Health`. Done the same way.
|
||||
|
||||
Separately, every reactive class in the leaf — executor, consistency binder, session factory, cursor
|
||||
guard — was declared by no configuration. They shipped and no configuration could construct them.
|
||||
Now wired in a nested configuration conditioned on a `ReactiveMongoTemplate` *bean*, not just the
|
||||
class: the class is on the compile classpath unconditionally, so a class condition alone would try to
|
||||
build the reactive path in a servlet-only deployment and fail a startup for a capability nobody asked
|
||||
for. Both the positive and the negative case are asserted.
|
||||
|
||||
### MNG-005 / MNG-023 — a deadline that only produced a report
|
||||
|
||||
The blocking executor compared elapsed time to the declared timeout *after* the callback returned,
|
||||
and said so in its own comment: a Java callback cannot be interrupted mid driver call. That is an
|
||||
overrun report, not a deadline.
|
||||
|
||||
The scoped API is the narrowed surface where the number can actually be sent, so every method taking
|
||||
a `Query` or an `Aggregation` now carries it as `maxTimeMS`, which the server enforces. `insert` has
|
||||
no query to attach it to. `Duration.ZERO` is refused, because zero means "no limit" to the server and
|
||||
accepting it would turn a misconfiguration into an unbounded operation.
|
||||
|
||||
The raw escape hatches (`rawOperations()`, `executeInternal(...)`) are genuinely used inside the
|
||||
platform by the geospatial, atomic and bulk operations, and their callers live in sibling packages,
|
||||
so package-private cannot express the rule. It is enforced as a boundary from the composition root —
|
||||
the only place that sees both the platform and everything consuming it. The rule was falsified by
|
||||
widening its scope until it fired, and a second case asserts the platform still uses them, so the
|
||||
rule cannot pass by the hatches having been deleted.
|
||||
|
||||
### MNG-006 — a guard that compared a declaration against nothing
|
||||
|
||||
`LocalDateTimeMappingGuard` was constructed `withoutConverters()` and then asked to validate the
|
||||
manifest. It could only ever reject `LOCAL_DATE_TIME_WITH_REGISTERED_CONVERTER`: a deployment that
|
||||
*had* registered the named converter was rejected exactly as loudly as one that had not, so the check
|
||||
that exists to distinguish those two cases could not tell them apart. It now reads the converters the
|
||||
deployment actually registered, and a test asserts the same manifest passes with the converter and
|
||||
fails without it.
|
||||
|
||||
### MNG-008 — a promotion gate that required five of the six categories it declares
|
||||
|
||||
`MongoAdvancedPromotionEvidence.REQUIRED` listed six; `MongoAdvancedPromotionGate.verify()` required
|
||||
five. `migration` was missing, so a promotion could pass with no migration evidence at all.
|
||||
|
||||
### GQL-002 — the batch policy applied to nothing
|
||||
|
||||
`GraphQlBatchLoaderRegistrar` carried the chunking, the budget and the request scope, was unit
|
||||
tested, and was declared by no configuration — the only file mentioning it was itself. A field
|
||||
resolving through `@BatchMapping` or a `DataLoader` met none of it. The chain
|
||||
(`BatchPolicyRegistry → DataLoaderFactory → BatchLoaderRegistrar`) is now assembled by the platform.
|
||||
The batch ceiling reuses `maximumPageSize` rather than adding a setting: both answer how many rows
|
||||
one downstream call may ask for, and a batch limit above the page limit would let one request fan out
|
||||
past the bound it already accepted.
|
||||
|
||||
## One thing an audit got wrong, and one fix that was reverted
|
||||
|
||||
The mongodb audit reported that the UUID axis is "declaration-only" and mapped onto the driver
|
||||
nowhere. It is mapped — `MongoClientSettingsFactory` calls `.uuidRepresentation(...)`. Verified
|
||||
before acting.
|
||||
|
||||
Acting on the adjacent concern — the manifest and the profile describing the same fact independently
|
||||
— a startup check was written to refuse a disagreement between them. Writing its test showed the
|
||||
disagreement cannot occur: `MongoUuidRepresentation` has two values, only `STANDARD` is writable, and
|
||||
an existing check already refuses the other. **A guard for a state that cannot arise is the same
|
||||
"declaration nothing checks" this wave has been removing**, so it was reverted rather than kept with
|
||||
an unfalsifiable test.
|
||||
|
||||
## What remains, and why
|
||||
|
||||
- **NTF-012 loopback residue** — belongs with the change set that gives WEBHOOK an assembler.
|
||||
- **MSG-015** — the application-owned port and anti-corruption bridge between `application-core` and
|
||||
the messaging platform. The review names it and specifies the target shape; it is a runtime wiring
|
||||
change, and the review itself says the physical work is a separate change set.
|
||||
- **MSG-003/004/005 residue** — on code no deployment can execute: the platform's outbox and inbox
|
||||
migrations are applied only by tests, the production Kafka transport is publish-only, and Rabbit
|
||||
declares no transport bean.
|
||||
- **P1 and P2 — 100 findings, never audited.** They were not in this remediation's scope and their
|
||||
state is unknown. Saying so is the honest position; the P0 audit took three parallel agents and
|
||||
the P1/P2 set is three times larger.
|
||||
Reference in New Issue
Block a user