adamsaghy commented on PR #6231:
URL: https://github.com/apache/fineract/pull/6231#issuecomment-5240795377
Considering the importance and complexity of the PR, AI was also used to
review the behaviour.
## Issues
### 1. Hook ordering puts the idempotency check *after* the audit insert →
duplicate rows, then hard failure on the 3rd call
Orders in the single `CommandHookBefore` list (Spring sorts collection
injection by `@Order`):
| order | hook |
| ----- | ---- |
| 10–12 | servlet-headers / timestamp / username (`fineract-command`) |
| 20 | `AuditCommandHookBefore` → `store.store(ctx)` (INSERT,
`UNDER_PROCESSING`) |
| 30 | `IdempotencyKeyHeaderCommandHook` → sets `command.idempotencyKey` |
| 31 | `IdempotencyCheckCommandHook` → lookup |
Trace of requests with key `K`:
- **Request 1** — audit-pre inserts row1 with `idempotency_key = NULL` (the
key isn't read
until order 30). Handler runs. `AuditCommandHookAfter` updates row1
(`commandId` was set by
`store`) → row1 now has `K`.
- **Request 2** — audit-pre inserts **row2** with `idempotency_key = NULL`.
Order 31 finds
row1's response → `INVALID` → handler skipped. `AuditCommandHookAfter`
still runs → updates
row2 → **row2 also gets `K`**.
- **Request 3** — audit-pre inserts row3, then `getResponseByKey(K)` →
`findOneByIdempotencyKey` is an `Optional`-returning derived query hitting
2 rows →
`IncorrectResultSizeDataAccessException` out of `hookManager.before(...)`
→ error hook →
**500 to the caller**.
`m_command_idempotency_key_index` is **not** unique
(`fineract-command-jdbc/src/main/resources/db/changelog/tenant/module/command/parts/0002_command_fix_structure.xml`),
so nothing prevents this at the DB level.
**We might want to consider use `idempotency key + request` combo for
identifying duplicate requests.**
Two independent fixes are needed:
- Resolve the idempotency key in the earliest before-hooks (≤ 12, alongside
the other
servlet-header extraction) and run the check *before* audit-pre, so a
replay never inserts a
row at all.
- Suppress the audit-after store on the replay path (or make `store` a no-op
when the context
is a replay).
Note this is also a **regression** relative to `develop`:
`fineract-command/src/main/java/org/apache/fineract/command/hook/ServletHeadersCommandHook.java`
previously set the key at order 10, so the audit INSERT carried it.
### 2. No atomic key claim → the idempotency guarantee doesn't hold under
concurrency
`IdempotencyCheckCommandHook` is read-then-execute with no unique constraint
and no claim
insert. Two simultaneous requests with the same key both see `null` and both
execute the
handler — which for a financial command is the failure mode idempotency
exists to prevent.
This needs a unique index on `m_command.idempotency_key` plus an
insert-and-claim (catch the
constraint violation → replay or 409), and a Liquibase changeset that
de-duplicates existing
rows before adding the constraint.
**We should eagerly store incoming command + idempotency and fail-fast if
such entries already exists: replay till result is gathered / timeout.**
### 3. In-flight and failed commands are not detected
`getResponseByKey` returns `null` both when no record exists *and* when a
record exists with a
null `response` (i.e. `UNDER_PROCESSING` or `ERROR`). So a client retry
while the first call is
still running re-executes the handler.
The store already exposes `getStateByKey` — the hook should branch on state:
- `PROCESSED` → replay
- `UNDER_PROCESSING` → 409 / retry-after
- `ERROR` → decide explicitly whether a retry is allowed
### 4. `INVALID` is the wrong state for a replay
The code comment acknowledges this. Because `AuditCommandHookAfter` now
persists
`ctx.getState()` instead of a hard-coded `PROCESSED`, a *successful* replay
is recorded as
`INVALID` in the audit trail, conflating "request failed validation" with
"already processed".
It also means any current or future hook that branches on `INVALID` will
treat a replay as a
validation failure. A dedicated state (or a non-persisted `skipExecution`
flag on the context)
is worth the extra enum value here.
### 5. No request-equivalence check on the key
Idempotency keys are client-supplied, and nothing verifies that the incoming
request matches
the one originally stored under that key. A client reusing a key across two
different endpoints
gets the *previous, unrelated* response back. Worse, the replayed object is
deserialized from
the stored `@class` attribute and assigned to `ctx.setResponse(...)`, so the
caller can get a
`ClassCastException` on a type mismatch instead of an error.
Standard behaviour is to reject key reuse with a different payload (422/409)
— at minimum store
and compare a request fingerprint.
### 6. Idempotency silently no-ops on the async and disruptor dispatchers
`hookManager.before(ctx)` runs on a pool/disruptor thread in both, and
`ServletHeaderUtil.getHeader` reads `RequestContextHolder`, which is a
non-inheritable
thread-local. So `command.getIdempotencyKey()` is always null there and the
check hook does
nothing. The header extraction belongs in the API/filter layer (before
`dispatch`), not in a
before-hook.
### 7. Silent-disable configuration model
All the `@ConditionalOnProperty` guards lack `matchIfMissing`, so a typo in
`fineract.command.idempotency.hook-check-pre` disables idempotency with no
log line and no
startup failure. Same for the renamed audit properties:
`fineract.command.hooks.audit-pre` →
`fineract.command.audit.hook-pre` silently turns off audit persistence for
anyone who set the
property directly rather than via `FINERACT_COMMAND_PROCESSORS_AUDIT_PRE`.
For guarantees of this kind, please fail fast or at least `log.warn` when
the module is on the
classpath but its hooks are off, and call the rename out in the release
notes.
## Minor
- `IdempotencyCheckCommandHook` injects `IdempotencyCommandProperties` but
never uses it — dead
field.
- The check hook needs `@ConditionalOnBean(CommandStore.class)`; with
`hook-check-pre=true` and
no store bean (e.g. `fineract.command.jdbc.enabled=false`) the context
fails to start.
- `getHeader(properties.getKeyHeaderName(), true)` changes behaviour: the
key is now also
accepted as a **query parameter** (previously `false`). Deliberate?
Idempotency keys in query
strings end up in access logs. Also `getParameter(name.toLowerCase())` is
case-sensitive for
parameters, so only a lowercase `idempotency-key` param would ever match.
- `ServletHeaderUtil` is `@UtilityClass` *and* `public final` with explicit
`public static`
members — the Lombok annotation makes all three redundant, and the rest of
the module uses
the explicit private-constructor idiom (`CommandConstants`).
- `fineract-command-idempotency/build.gradle:59` — copy-paste: the Eclipse
output dir points at
`fineract-command/bin/main` instead of
`fineract-command-idempotency/bin/main`.
- `mapstruct` / `mapstruct-processor`, `resilience4j-spring-boot3` and
`jmh-core` are declared
but unused by the module.
- `AutoConfiguration.imports` is missing a trailing newline.
- `CommandProperties.hooks` (`Map<String, Boolean>`) is now effectively dead
— nothing reads the
map, only the raw property paths. Since audit moved to typed properties,
the three core hooks
should follow for consistency (three naming schemes now coexist:
`COMMAND_HOOK_ORDER_*`,
`COMMAND_AUDIT_HOOK_*`, `COMMAND_IDEMPOTENCY_HOOK_*`).
- `fineract-doc/src/docs/en/chapters/architecture/idempotency.adoc` still
documents only the
legacy `fineract.idempotency-key-header-name` mechanism; the two now
coexist and that's worth
a paragraph.
- **Drive-by, pre-existing but in a file you touch:**
`DisruptorCommandDispatcher.CommandEvent.future` is initialised once in
the event factory
(`CommandEvent::new`) and never reset, so once the ring buffer wraps,
`dispatch` hands back an
already-completed future from a previous command. Worth to revisit this...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
