adamsaghy commented on PR #6069:
URL: https://github.com/apache/fineract/pull/6069#issuecomment-5191069482
Note: Considering the nature and size of this PR, AI was also used to review
and highlight areas that requires further refinement / revision.
# Code Review — PR #6069: FINERACT-2621 Upgrade dependencies to Spring Boot
4.1
The import churn is mechanical and low-risk:
- `org.springframework.batch.core.*` repackaging (`JobExecution` →
`job.JobExecution`, `StepExecution` → `step.StepExecution`, `item.*` →
`infrastructure.item.*`, `repeat.*` → `infrastructure.repeat.*`)
- `org.springframework.lang.{NonNull,Nullable}` →
`org.jspecify.annotations.*`
- `spring-boot-starter-web` → `spring-boot-starter-webmvc`, `starter-batch`
→ `starter-batch-jdbc`, `resilience4j-spring-boot3` → `-boot4`
- Boot 4 module splits (`spring-boot-jpa`, `spring-boot-liquibase`,
`spring-boot-gson`, `starter-tomcat-runtime`)
Risk concentrates in two areas: **Spring Batch 5 → 6** and the **Jackson 2 →
3 split**.
## Summary table
| # | Severity | Area | Finding |
|---|---|---|---|
| 1 | **High** | Batch 6 | Execution-context serializer format break, no
data migration |
| 2 | **High** | Batch 6 / COB | Chunk atomicity now conditional on
thread-pool size |
| 3 | **High** | Config | CloudWatch metrics permanently disabled |
| 4 | Medium | Batch 6 | Step-listener comment is wrong; fix causes double
invocation |
| 5 | Medium | Batch 6 | `jobOperator.run(...)` is deprecated for removal |
| 6 | Medium | Batch 6 | `getNextJobParameters` hand-rolled and duplicated |
| 7 | Medium | Batch 6 / COB | Queue capacity silently overridden;
connection-pool pressure |
| 8 | Medium | COB | Reviewer's `AsyncTaskExecutor` fix not applied to
Working Capital config |
| 9 | Medium | Jersey | Write path changed more than claimed (generic type +
stream close) |
| 10 | Medium | Jackson 3 | Unknown-property rejection silently relaxed on a
public API |
| 11 | Medium | Jackson | Dual-Jackson runtime has no guard rail |
| 12 | Medium | Batch 6 | `StepSynchronizationManager` ref-counting far more
contended |
| 13 | Low | Batch 6 | `StuckJobExecutorServiceImpl` NPEs on unknown
execution id |
| 14 | Low | EclipseLink 5 | Fragile unconditional `OffsetDateTime` cast on
JPQL projection |
| 15 | Low | Jackson 3 | `asLong(0)` bakes loan id `0` into COB routing |
| 16 | Low | Docker | `ReservedCodeCacheSize=128m` lowers the JVM default |
| 17 | Low | CI | `POLLING_EVENT_WAIT_TIMEOUT_IN_MS` addition is a no-op |
| 18 | Low | Process | e2e properties still modified despite "moved to
#6175" |
| 19 | Low | Integration tests | `keystoreType JKS` silently dropped with
tomcat11x bump |
| 20 | Low | Tests | Class-wide `Strictness.LENIENT` added |
| 21 | Low | Process | Unrelated churn remains in the PR |
---
# High severity
## 1. Execution-context serializer format break, with no migration path
**File:**
`fineract-provider/src/main/java/org/apache/fineract/infrastructure/jobs/ScheduledJobRunnerConfig.java`
`Jackson2ExecutionContextStringSerializer` is replaced by
`JacksonExecutionContextStringSerializer`. These produce **mutually
incompatible JSON**:
| Aspect | Batch 5 (`Jackson2...`) | This PR (`Jackson...`) |
|---|---|---|
| Typing scope | `DefaultTyping.NON_FINAL` | `OBJECT_AND_NON_CONCRETE` |
| Type inclusion | `As.PROPERTY` → `"@class": "..."` | `As.WRAPPER_ARRAY` →
`["java.util.HashSet", …]` |
| Unknown properties | `FAIL_ON_UNKNOWN_PROPERTIES = true` | Jackson 3
default: `false` |
| Unsafe base types | `BLOCK_UNSAFE_POLYMORPHIC_BASE_TYPES = true` | not set
|
| Modules | `JobParametersModule`, `JavaTimeModule` | java.time is built
into Jackson 3 core (OK) |
The one-argument `activateDefaultTyping(ptv)` used in the new code is a
convenience overload that resolves to `OBJECT_AND_NON_CONCRETE` +
`WRAPPER_ARRAY`:
```java
// tools/jackson/databind/cfg/MapperBuilder.java:1710
public B activateDefaultTyping(PolymorphicTypeValidator subtypeValidator) {
return activateDefaultTyping(subtypeValidator,
DefaultTyping.OBJECT_AND_NON_CONCRETE);
}
// :1727
public B activateDefaultTyping(PolymorphicTypeValidator subtypeValidator,
DefaultTyping dti) {
return activateDefaultTyping(subtypeValidator, dti,
JsonTypeInfo.As.WRAPPER_ARRAY);
}
```
:warning: **RISK** :warning:
Consequently every existing row in `BATCH_JOB_EXECUTION_CONTEXT` and
`BATCH_STEP_EXECUTION_CONTEXT` becomes undeserializable after the upgrade.
### Why this bites immediately
The new `getNextJobParameters()` calls
`jobRepository.getLastJobExecution(lastInstance)`, and Batch 6 eagerly hydrates
contexts on that path:
```java
// SimpleJobRepository:171 (extends SimpleJobExplorer)
public JobExecution getLastJobExecution(JobInstance jobInstance) {
JobExecution lastJobExecution =
jobExecutionDao.getLastJobExecution(jobInstance);
if (lastJobExecution != null) {
fillJobExecutionDependencies(lastJobExecution);
for (StepExecution stepExecution :
lastJobExecution.getStepExecutions()) {
fillStepExecutionDependencies(stepExecution);
}
}
return lastJobExecution;
}
// SimpleJobExplorer
protected void fillJobExecutionDependencies(JobExecution jobExecution) {
...
jobExecution.setExecutionContext(ecDao.getExecutionContext(jobExecution)); //
← deserializes
}
```
**The first post-upgrade run of every scheduled job that has execution
history will attempt to deserialize Batch-5-format context.**
`StuckJobExecutorServiceImpl.resumeStuckJob` → `jobOperator.restart(...)` hits
the same path.
### Recommendation
Pick one and document it in the PR / upgrade notes:
1. A Liquibase changeset that truncates `BATCH_JOB_EXECUTION_CONTEXT` and
`BATCH_STEP_EXECUTION_CONTEXT` (acceptable only if no in-flight jobs are
expected across the upgrade), **or**
2. A serializer that detects and reads both the `@class` property form and
the wrapper-array form.
Silent failure of all scheduled jobs on first startup after upgrade is a bad
outcome for downstream deployments.
### Secondary concern — security posture
The allow-list was widened from Batch 5's explicit `trustedClassNames` to
the whole `org.apache.fineract.` prefix. This is genuinely required
(`Set<BusinessStepNameAndOrder>` and `COBParameter` are stored in partition
contexts), but the two hardening flags Batch 5 set —
`FAIL_ON_UNKNOWN_PROPERTIES` and `BLOCK_UNSAFE_POLYMORPHIC_BASE_TYPES` — are
silently gone.
---
## 2. COB chunk atomicity is now conditional on thread-pool size
**Files:**
-
`fineract-cob/src/main/java/org/apache/fineract/cob/COBBusinessStepServiceImpl.java:59`
-
`fineract-provider/src/main/java/org/apache/fineract/cob/loan/LoanCOBWorkerConfiguration.java:102`
The `@Transactional` added to `COBBusinessStepServiceImpl.run(...)` — and
the explanatory comment above it — is **accurate about the mechanism**.
Confirmed in `ChunkOrientedStep`:
```java
// ChunkOrientedStep:368 — the chunk transaction is bound to the step thread
protected void doExecute(StepExecution stepExecution) throws Exception {
while (this.chunkTracker.get().moreItems() &&
!interrupted(stepExecution)) {
this.transactionTemplate.executeWithoutResult(transactionStatus -> {
...
processNextChunk(transactionStatus, contribution, stepExecution);
...
});
}
}
// ChunkOrientedStep:447 — item processing is fanned out with NO transaction
Future<O> itemProcessingFuture = this.taskExecutor.submit(() -> {
try {
StepSynchronizationManager.register(stepExecution); // step
context only
return processItem(item, contribution);
} finally {
StepSynchronizationManager.close();
}
});
```
`@Transactional` resolves to the `@Primary` `jpaTransactionManager`
(`JpaTransactionConfig:39-40`), which is the same bean the step receives via
the unqualified `@Autowired PlatformTransactionManager` in
`LoanCOBWorkerConfiguration:62`. So the comment's claim that sequential mode
joins the chunk transaction holds.
### The consequences go beyond what the comment states
The step is configured fault-tolerant:
```java
.faultTolerant()
.retry(Exception.class)
.retryLimit(propertyService.getRetryLimit(LoanCOBConstant.JOB_NAME))
.skip(Exception.class)
.skipLimit(propertyService.getChunkSize(LoanCOBConstant.JOB_NAME) + 1)
```
And `doProcess` wraps the processor in a retry template:
```java
// ChunkOrientedStep:721
private @Nullable O doProcess(I item) throws Exception {
if (this.faultTolerant) {
Retryable<O> retryableProcess = new Retryable<>() {
public O execute() throws Throwable {
...
return itemProcessor.process(item);
}
};
return this.retryTemplate.execute(retryableProcess);
}
return this.itemProcessor.process(item);
}
```
:warning: **RISK** :warning:
Therefore, in **concurrent** mode:
- **Retries re-apply committed side effects.** Each retry attempt is a
separately committing transaction. A retried item re-runs the whole COB
business-step chain — accruals, charges, external events — on top of
already-committed effects.
- **Chunk rollback leaves orphaned process writes.** If `writeChunk` fails
and the chunk transaction rolls back, the per-item process writes are already
committed while the writer's lock release and `lastClosedBusinessDate` update
are not. The loan is reprocessed on the next run → double accrual.
- **Write-skip scan mode.** `ChunkOrientedStep.scan()` skips the write for
an item whose process-time writes are committed.
COB correctness now depends on `LOAN_COB_THREAD_POOL_MAX_POOL_SIZE` — which
defaults to `5`, i.e. **concurrent by default**.
FINERACT-2684 is referenced in the comment, but the PR ships the divergence
rather than gating it.
### Recommendation
Treat as a merge blocker independent of the rest of the PR. Options:
1. Force sequential COB (`maxPoolSize = 1`) until FINERACT-2684 is resolved,
restoring Batch 5 atomicity at the cost of throughput.
2. Move the business-step writes into the chunk transaction by keeping
processing on the step thread and parallelising at the *partition* level only.
3. Make COB business steps idempotent and accept per-item commits as the new
contract — but this needs explicit design sign-off, not a comment.
### Related (pre-existing in Batch 6, worth noting)
`processItem(item, contribution)` is called concurrently from N pool threads
against a **single shared `StepContribution`**, whose counters are plain `int`
fields (`incrementFilterCount`, `incrementProcessSkipCount`). Skip and filter
counts will be lossy. This is a Spring Batch 6 issue rather than a Fineract
one, but it affects reported job metrics.
---
## 3. CloudWatch metrics are now permanently off
**File:**
`fineract-provider/src/main/resources/application.properties:427-433`
```properties
management.cloudwatch.metrics.export.enabled=${FINERACT_MANAGEMENT_CLOUDWATCH_ENABLED:false}
management.cloudwatch.metrics.export.namespace=${FINERACT_MANAGEMENT_CLOUDWATCH_NAMESPACE:fineract}
management.cloudwatch.metrics.export.step=${FINERACT_MANAGEMENT_CLOUDWATCH_STEP:1m}
spring.autoconfigure.exclude=${FINERACT_AUTOCONFIGURE_EXCLUDE:io.awspring.cloud.autoconfigure.core.AwsAutoConfiguration,\
io.awspring.cloud.autoconfigure.core.CredentialsProviderAutoConfiguration,\
io.awspring.cloud.autoconfigure.metrics.CloudWatchExportAutoConfiguration}
```
`spring.autoconfigure.exclude` is **unconditional**. Adding
`CloudWatchExportAutoConfiguration` to it means the three renamed properties
directly above can never take effect — setting
`FINERACT_MANAGEMENT_CLOUDWATCH_ENABLED=true` now does nothing.
:warning: **RISK** :warning:
Either the exclusion is wrong, or the properties are dead and should be
removed. As written it is a silent functional regression for any deployment
using CloudWatch metrics.
---
# Medium severity
## 4. The step-listener comment is factually wrong, and the fix it justifies
causes double invocation
**Files:**
-
`fineract-provider/src/main/java/org/apache/fineract/cob/loan/LoanCOBWorkerConfiguration.java:111-114`
-
`fineract-working-capital-loan/.../WorkingCapitalLoanCOBWorkerConfiguration.java:98-101`
-
`fineract-provider/src/main/java/org/apache/fineract/cob/loan/LoanInlineCOBConfig.java:93-96`
-
`fineract-provider/src/main/java/org/apache/fineract/cob/loan/WorkingCapitalLoanInlineCOBConfig.java:99-103`
-
`fineract-provider/src/main/java/org/apache/fineract/cob/loan/AbstractLoanItemReader.java:39`
-
`fineract-cob/src/main/java/org/apache/fineract/cob/processor/AbstractItemProcessor.java:42`
The comment repeated in four configs states:
> Batch 6's chunk-oriented step no longer scans item components for
step-listener annotations, so register the reader and processor as
StepExecutionListeners explicitly
This does not match the source. `ChunkOrientedStepBuilder.build()` **does**
scan them:
```java
// ChunkOrientedStepBuilder:400 build()
addAsStreamAndListener(this.reader);
addAsStreamAndListener(this.writer);
if (this.processor != null) {
chunkOrientedStep.setItemProcessor(this.processor);
addAsStreamAndListener(this.processor);
}
// ChunkOrientedStepBuilder:483
private void addAsStreamAndListener(Object itemHandler) {
if (itemHandler instanceof ItemStream itemStream) {
this.streams.add(itemStream);
}
// Register as listener if implements the interface
if (itemHandler instanceof StepListener listener) {
this.stepListeners.add(listener);
// ← branch A
}
// Register as listener if annotated methods are present
if (StepListenerFactoryBean.isListener(itemHandler)) {
StepListener listener =
StepListenerFactoryBean.getListener(itemHandler);
this.stepListeners.add(listener);
// ← branch B
}
}
```
And `isListener` returns `true` as soon as the object implements the
interface — it does **not** require annotations:
```java
// AbstractListenerFactoryBean:isListener
public static boolean isListener(Object target, Class<?> listenerType,
ListenerMetaData[] metaDataValues) {
if (target == null) return false;
if (listenerType.isInstance(target)) return true; // ←
short-circuits here
...
}
```
`StepListenerMetaData` includes `BEFORE_STEP` and `AFTER_STEP` (`:56-57`),
so the branch-B proxy routes `beforeStep`/`afterStep` to the annotated methods.
### Two consequences
**(a) The added `listener(...)` calls are dead code.** `stepListeners` is a
`LinkedHashSet<StepListener>` (`ChunkOrientedStepBuilder:94`), and `build()`
already adds the same instance. Registering it again is a no-op.
**(b) `beforeStep`/`afterStep` now fire twice per step.** This PR added
`implements StepExecutionListener` to both item components **while keeping**
the `@BeforeStep`/`@AfterStep` annotations:
```diff
-public abstract class AbstractLoanItemReader<T extends
AbstractPersistableCustom<Long>> implements ItemReader<T> {
+public abstract class AbstractLoanItemReader<T extends
AbstractPersistableCustom<Long>> implements ItemReader<T>,
StepExecutionListener {
```
Both branch A and branch B therefore fire, adding two distinct objects (the
instance, and the annotation proxy) to the set. Batch 5 registered only the
proxy:
```java
// Batch 5 — SimpleStepBuilder.registerAsStreamsAndListeners
if (StepListenerFactoryBean.isListener(itemHandler)) {
StepListener listener =
StepListenerFactoryBean.getListener(itemHandler); // proxy only
if (listener instanceof StepExecutionListener) {
listener((StepExecutionListener) listener); }
...
}
```
:warning: **RISK** :warning:
So the double invocation is **new in this PR**.
### Practical impact
`LoanItemReader.beforeStep` calls
`BeforeStepLockingItemReaderHelper.filterRemainingData(stepExecution)`, which
issues two SELECTs
(`retrieveAllNonClosedLoansByLastClosedBusinessDateAndMinAndMaxLoanId` +
`findLockIdsByLoanIdInAndLockOwner`). Running twice per partition means:
- Two extra queries per partition per step
- `remainingData` is rebuilt from a **second, possibly different** lock
snapshot
Read-only, so not corrupting — but wasteful and non-deterministic.
`AbstractItemProcessor.beforeStep`/`afterStep` are idempotent, so no impact
there beyond overhead.
### Recommendation
Pick one mechanism, not both:
- Keep `implements StepExecutionListener` and **remove** the
`@BeforeStep`/`@AfterStep` annotations, **and**
- Delete all four blocks of `stepBuilder.listener(reader)` /
`listener(processor)` calls, **and**
- Delete the incorrect comment.
---
## 5. `jobOperator.run(...)` is deprecated for removal
**File:**
`fineract-provider/src/main/java/org/apache/fineract/infrastructure/jobs/service/JobStarter.java:106-111`
```java
// Batch 6 JobOperator.start() silently discards explicit parameters for
jobs that define an
// incrementer and launches the next instance instead; the incrementer is
already applied in
// getNextJobParameters(), so use the parameter-respecting launch inherited
from JobLauncher
@SuppressWarnings("removal")
JobExecution result = jobOperator.run(job, jobParameters);
```
The reasoning is correct. Confirmed:
```java
// JobLauncher.java:41 — the entire interface is deprecated
@Deprecated(since = "6.0", forRemoval = true)
public interface JobLauncher { ... }
// JobOperator.java:44
public interface JobOperator extends JobLauncher { ... }
// SimpleJobOperator.start
public JobExecution start(Job job, JobParameters jobParameters) throws ... {
if (job.getJobParametersIncrementer() != null) {
logger.warn("... Additional parameters will be ignored.");
return startNextInstance(job);
}
return run(job, jobParameters); // ← delegates to run() when no
incrementer
}
```
:warning: **RISK** :warning:
Note the last line: `start()` itself delegates to `run()` **when there is no
incrementer**.
### Recommendation
The forward-compatible fix is to **drop the `JobParametersIncrementer` from
the Fineract job definitions** and supply uniqueness via an explicit parameter
Fineract controls (a run-id or timestamp). Then `start(job, params)` works, no
suppression is needed, and the code survives the next major. Worth doing now
rather than deferring to a breaking upgrade.
Same suppression pattern appears at
`InlineCommonLockableCOBExecutorService.java:143`.
---
## 6. `getNextJobParameters` hand-rolled and duplicated
**Files:**
-
`fineract-provider/src/main/java/org/apache/fineract/infrastructure/jobs/service/JobStarter.java:124-129`
-
`fineract-provider/src/main/java/org/apache/fineract/cob/service/InlineCommonLockableCOBExecutorService.java:214-219`
Two byte-identical private copies:
```java
private JobParameters getNextJobParameters(Job job) {
JobInstance lastInstance =
jobRepository.getLastJobInstance(job.getName());
JobExecution lastExecution = lastInstance == null ? null :
jobRepository.getLastJobExecution(lastInstance);
JobParameters parameters = lastExecution == null ? new JobParameters() :
lastExecution.getJobParameters();
return job.getJobParametersIncrementer() == null ? parameters :
job.getJobParametersIncrementer().getNext(parameters);
}
```
:warning: **RISK** :warning:
Issues:
1. **Duplication** — should be one shared helper.
2. **Lost assertion.** Batch's `JobParametersBuilder.getNextJobParameters`
asserts the incrementer is non-null. This version silently returns the
*previous execution's parameters verbatim* when the incrementer is null, which
surfaces later as a confusing `JobInstanceAlreadyCompleteException` instead of
a clear configuration error.
3. **Eager context deserialization.** `getLastJobExecution` hydrates the
full job execution *and every step execution context* (see finding 1) purely to
read job parameters — which live in `BATCH_JOB_EXECUTION_PARAMS` as plain
columns. Consider `jobExecutionDao`-level access or a targeted query.
---
## 7. Task-executor queue capacity silently overridden; connection-pool
pressure
**Files:**
-
`fineract-provider/src/main/java/org/apache/fineract/cob/loan/LoanCOBWorkerConfiguration.java:133`
-
`fineract-working-capital-loan/.../WorkingCapitalLoanCOBWorkerConfiguration.java:125`
```diff
-taskExecutor.setQueueCapacity(propertyService.getThreadPoolQueueCapacity(JobName.LOAN_COB.name()));
+taskExecutor.setQueueCapacity(Math.max(propertyService.getThreadPoolQueueCapacity(JobName.LOAN_COB.name()),
+ propertyService.getChunkSize(JobName.LOAN_COB.name())));
```
The change is **necessary**: `processChunkConcurrently` submits up to
`chunkSize` tasks before blocking on `future.get()`. Defaults are `chunk-size =
100` and `thread-pool-queue-capacity = 20`, so without this you would get
`RejectedExecutionException` → `TaskRejectedException` at task 26.
:warning: **RISK** :warning:
But there are three problems:
1. **It overrides an operator-configured value with no log line.** An
operator setting `LOAN_COB_THREAD_POOL_QUEUE_CAPACITY=20` silently gets 100.
2. **It can pin parallelism to `corePoolSize`.** `ThreadPoolExecutor` only
grows past `corePoolSize` once the queue is *full*. Defaults are `core = max =
5` so this is fine today, but any deployment with `core=5 / max=20` now has
effective parallelism of 5 and `maxPoolSize` becomes inert.
3. **Connection-pool sizing needs review.** Per partition you now hold the
chunk transaction on the step thread **plus** up to `maxPoolSize` independent
per-item transactions (finding 2). Roughly double the Batch 5 demand,
multiplied by concurrent partitions.
### Recommendation
Prefer `CallerRunsPolicy` (or bound submission to the available pool) over
silently enlarging the queue, and log when the configured capacity is
insufficient. Re-check HikariCP `maximumPoolSize` against `partitions × (1 +
maxPoolSize)`.
### Minor
In the Working Capital config the queue capacity reads
`WORKING_CAPITAL_JOB_NAME` while the chunk size reads
`JobName.LOAN_COB.name()`. It is at least consistent with the actual chunk size
used in the step builder, but reads like a copy-paste and deserves a comment or
a fix.
---
## 8. Reviewer's `AsyncTaskExecutor` fix not applied to the Working Capital
config
**File:**
`fineract-working-capital-loan/src/main/java/org/apache/fineract/cob/workingcapitalloan/WorkingCapitalLoanCOBWorkerConfiguration.java:103-105`
The review comment on `LoanCOBWorkerConfiguration` was addressed there:
```java
if (cobTaskExecutor() instanceof AsyncTaskExecutor asyncTaskExecutor) {
stepBuilder.taskExecutor(asyncTaskExecutor);
}
```
But the sibling config still has the exact pattern that was flagged:
```java
if (propertyService.getThreadPoolMaxPoolSize(WORKING_CAPITAL_JOB_NAME) > 1) {
stepBuilder.taskExecutor((AsyncTaskExecutor)
workingCapitalCobTaskExecutor());
}
```
:warning: **RISK** :warning:
`workingCapitalCobTaskExecutor()` returns `SyncTaskExecutor` when
`maxPoolSize == 1`, and `SyncTaskExecutor` implements only `TaskExecutor`. The
cast is safe only because the enclosing condition duplicates the branch inside
the bean method. Should those ever diverge, `ClassCastException`. Apply the
same `instanceof` pattern.
---
## 9. Jersey write path changed more than claimed
**File:**
`fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/jersey/JerseyJacksonObjectArgumentHandler.java`
```diff
-converter.write(t, genericType, MediaType.APPLICATION_JSON, new
SimpleHttpOutputMessage(entityStream, headers));
+objectMapper.writeValue(entityStream, t);
```
The PR response states the wire format is unchanged because the same
`ObjectMapper` bean is used. That is true of the mapper configuration, but two
behavioural differences remain:
:warning: **RISK** :warning:
### (a) Declared generic type vs runtime type
`AbstractJackson2HttpMessageConverter.writeInternal` serialises against the
**declared generic type** via `objectMapper.writerFor(javaType)`.
`objectMapper.writeValue(out, t)` serialises against the **runtime type** of
`t`. For proxied, subclassed, or narrowed returns this can emit additional
fields that were previously suppressed.
### (b) Stream closing
`writeValue(OutputStream, Object)` routes through
`_writeValueAndClose(...)`, and `JsonGenerator.Feature.AUTO_CLOSE_TARGET` is
enabled by default — so the generator **closes** the underlying stream. The
converter only called `generator.flush()`.
Closing Jersey's `CommittingOutputStream` mid-chain can break
`WriterInterceptor` chains (for example the GZip interceptor).
### (c) Read path mirror
`objectMapper.readValue(entityStream, ...)` likewise closes the source
(`AUTO_CLOSE_SOURCE` default). The anonymous `HttpInputMessage` handed to
`HttpMessageNotReadableException` then exposes an already-consumed stream, so
any error handler that tries to read the body gets nothing.
### Recommendation
Use `objectMapper.writerFor(javaType).writeValue(...)` to preserve
generic-type serialisation, and construct the generator with
`AUTO_CLOSE_TARGET` disabled (or use `writeValue(JsonGenerator, Object)` and
flush without closing). Add contract tests on (i) a generic-typed resource
method and (ii) the GZip response path.
---
## 10. Jackson 3 relaxes unknown-property rejection on a public API
**File:**
`fineract-loan/src/main/java/org/apache/fineract/portfolio/interestpauses/data/InterestPauseRequestDto.java:47,55`
```diff
-return new ObjectMapper().writeValueAsString(this);
+return new JsonMapper().writeValueAsString(this);
...
-return new ObjectMapper().readValue(json, InterestPauseRequestDto.class);
+return new JsonMapper().readValue(json, InterestPauseRequestDto.class);
```
Defaults differ between the two versions:
```java
// tools/jackson/databind/DeserializationFeature.java:150
FAIL_ON_UNKNOWN_PROPERTIES(false),
```
Jackson 2's default was `true`. `fromJson` is the batch-API entry point for
four command strategies:
- `CreateLoanInterestPauseByExternalIdCommandStrategy:62`
- `UpdateLoanInterestPauseByExternalIdCommandStrategy:63`
- `CreateLoanInterestPauseByLoanIdCommandStrategy`
- (and the corresponding update-by-loan-id strategy)
So those endpoints now **silently ignore unrecognised fields** where they
previously failed with `IllegalArgumentException("Error deserializing request
from JSON")`.
Intentional or not, this is an undocumented public API behaviour change.
Either restore strictness explicitly or note the relaxation in the API docs.
All four fields are `String`, so there is no `java.time` format concern
here. (Jackson 3 has java.time built into databind core via
`JavaTimeInitializer`, so the missing `JavaTimeModule` registration is not an
issue.)
:warning: **RISK** :warning:
Separately: constructing a new mapper per call is a pre-existing performance
smell, unchanged by this PR but worth a follow-up.
---
## 11. Dual-Jackson runtime has no guard rail
**File:** `fineract-provider/dependencies.gradle:145-146`
```groovy
// Spring Boot 4 defaults to Jackson 3; re-enable Jackson 2 so the Jersey
REST layer (Jackson-2 island) keeps working
implementation 'org.springframework.boot:spring-boot-jackson2'
```
Current split in `src/main`:
| Namespace | Files |
|---|---|
| `tools.jackson` (Jackson 3) | 23 |
| `com.fasterxml.jackson` (Jackson 2) | 46 |
The shared `com.fasterxml.jackson.annotation` package masks most of the
hazard, but the *databind* annotations are version-specific:
- Jackson 2: `com.fasterxml.jackson.databind.annotation.{JsonSerialize,
JsonDeserialize, JsonNaming, JsonPOJOBuilder}`
- Jackson 3: `tools.jackson.databind.annotation.{...}`
**Jackson 3 ignores Jackson 2's databind annotations entirely.** A DTO
carrying `@JsonSerialize(using = …)` loses its custom serializer, silently, if
it ever crosses onto a Jackson 3 path.
Currently exposed (all on the Jersey side, so safe today):
- `fineract-provider/.../template/domain/Template.java`
- `fineract-provider/.../template/domain/TemplateEntity.java`
- `fineract-provider/.../template/domain/TemplateType.java`
-
`fineract-provider/.../infrastructure/configuration/data/ExternalServicesPropertiesData.java`
- `fineract-provider/.../interoperation/data/InteropRequestData.java`
:warning: **RISK** :warning:
Related: `lombok.config` now carries both versions, so every `@Jacksonized`
type emits both annotation sets:
```properties
lombok.jacksonized.jacksonVersion += 2
lombok.jacksonized.jacksonVersion += 3
```
### Recommendation
Add an ArchUnit rule (or Checkstyle import restriction) pinning the two
islands — e.g. no `tools.jackson` in packages reachable from Jersey resources,
and no `com.fasterxml.jackson.databind` in the Spring MVC / batch /
external-event paths. Without a guard rail the next crossover fails silently.
---
## 12. `StepSynchronizationManager` ref-counting is far more contended
**Files:**
-
`fineract-core/src/main/java/org/springframework/batch/core/scope/context/StepSynchronizationManager.java:52`
-
`fineract-loan/src/main/java/org/apache/fineract/cob/loan/ContextAwareTaskDecorator.java:44`
The `Enhancer.isEnhanced` short-circuit added to the shadowed
`StepSynchronizationManager` is correct and necessary — without it,
re-registering on a pool thread would create a second proxy and miss the
existing context.
:warning: **RISK** :warning:
But registration is now **triple-nested per item**:
1. `ContextAwareTaskDecorator` (added in this PR) —
`StepSynchronizationManager.register(stepContext.getStepExecution())`
2. `ChunkOrientedStep:447` — the submitted lambda registers again
3. `ChunkOrientedStep:726` — `doProcess`'s `Retryable` registers a third time
`SynchronizationManagerSupport` keys a shared map on the `StepExecution` and
guards mutation with global monitors:
```java
private final Map<E, AtomicInteger> counts = new ConcurrentHashMap<>();
private final Map<E, C> contexts = new ConcurrentHashMap<>();
public C register(E execution) {
getCurrent().push(execution);
synchronized (contexts) { context = contexts.computeIfAbsent(execution,
this::createNewContext); }
increment();
return context;
}
private void decrement() {
E current = getCurrent().pop();
if (current != null) {
int remaining = counts.get(current).decrementAndGet(); // ← NPE if
already removed
if (remaining <= 0) {
synchronized (contexts) { contexts.remove(current);
counts.remove(current); }
}
}
}
```
With `chunkSize = 100` across many partitions this means heavy contention on
`synchronized(contexts)` / `synchronized(counts)`, and much wider exposure to
the
[spring-batch#4774](https://github.com/spring-projects/spring-batch/issues/4774)
race that this shadow class exists to work around — including the
`counts.get(current)` NPE when another thread has already zeroed the entry, and
context destruction while a thread still holds it.
### Additional issues in `ContextAwareTaskDecorator`
- **The registration is redundant.** `ChunkOrientedStep` already registers
on the pool thread (step 2 above). Recommend dropping it.
- **Unbalanced `close()` on init failure.** `register` happens *inside* the
try, after `ThreadLocalContextUtil.init(context)`:
```java
return () -> {
try {
ThreadLocalContextUtil.init(context); // ← if this throws…
if (stepContext != null) {
StepSynchronizationManager.register(stepContext.getStepExecution());
}
runnable.run();
} finally {
if (stepContext != null) {
StepSynchronizationManager.close(); // ← …this still
runs
}
ThreadLocalContextUtil.reset();
}
};
```
A leftover registration from a previous task on the same pooled thread
would be decremented.
### Verified as correct — no action needed
Two things I checked that turn out to be fine:
**(a) `TaskDecorator` does apply to `submit(Callable)`.**
`ChunkOrientedStep` uses `taskExecutor.submit(Callable)`, and historically
Spring's `ThreadPoolTaskExecutor.submit` bypassed the decorator. In Spring
Framework 7 the decoration happens inside an anonymous
`ThreadPoolExecutor.execute(Runnable)` override:
```java
// ThreadPoolTaskExecutor:284 (inside initializeExecutor)
public void execute(Runnable command) {
Runnable decorated = command;
if (taskDecorator != null) {
decorated = taskDecorator.decorate(command);
if (decorated != command) { decoratedTaskMap.put(decorated,
command); }
}
super.execute(decorated);
}
```
`AbstractExecutorService.submit(Callable)` wraps into a `FutureTask` and
calls `execute(...)`, so the override fires. **Tenant context does reach COB
worker threads.**
**(b) The new `jobRegistry()` bean is genuinely required and
self-populates.**
```java
// BatchRegistrar:232-235 — only wires jobRegistry if a bean of that name
exists
String jobRegistryRef = batchAnnotation.jobRegistryRef();
if (registry.containsBeanDefinition(jobRegistryRef)) {
beanDefinitionBuilder.addPropertyReference("jobRegistry",
jobRegistryRef);
}
// MapJobRegistry:46,64 — self-populates from all Job beans
public class MapJobRegistry implements JobRegistry,
SmartInitializingSingleton, ApplicationContextAware {
public void afterSingletonsInstantiated() {
Map<String, Job> jobBeans =
this.applicationContext.getBeansOfType(Job.class);
for (Job job : jobBeans.values()) { register(job); }
}
}
```
So `jobRegistry.getJob(name)` works, and the
`null`-instead-of-`NoSuchJobException` handling in `JobRegisterServiceImpl` /
`InlineCommonLockableCOBExecutorService` is correct (`JobRegistry.getJob` is
`@Nullable`).
⚠️ One caveat: `register` throws `DuplicateJobException` →
`IllegalStateException`, so **context startup now fails hard if any two `Job`
beans share a `getName()`**. Worth keeping in mind for custom modules.
**(c) The shadowed classes' API surface matches Batch 6.** Both
`StepSynchronizationManager` and `JobSynchronizationManager` expose exactly
`getContext()`, `register(...)`, `close()`, `release()` in both the Fineract
shadow and the real Batch 6 class — no `NoSuchMethodError` risk from Batch 6
internals calling the shadowed versions.
---
# Low severity / process
## 13. `StuckJobExecutorServiceImpl` NPEs on unknown execution id
**File:**
`fineract-provider/src/main/java/org/apache/fineract/infrastructure/jobs/service/StuckJobExecutorServiceImpl.java:59,90`
```diff
-jobOperator.restart(stuckJobId);
+jobOperator.restart(jobRepository.getJobExecution(stuckJobId));
```
`getJobExecution(id)` returns `null` for an unknown id, so `restart(null)`
throws NPE instead of the old, clear `NoSuchJobExecutionException`. Add a null
check with an explicit error.
## 14. Fragile unconditional cast on a JPQL projection
**File:**
`fineract-accounting/src/main/java/org/apache/fineract/accounting/glaccount/jobs/updatetrialbalancedetails/UpdateTrialBalanceDetailsTasklet.java:83`
```diff
-tb.setTransactionDate((LocalDate) row[4]);
+tb.setTransactionDate(((OffsetDateTime) row[4]).toLocalDate());
```
Two observations:
- The cast is unconditional on an `Object[]` JPQL projection, and the
underlying column type differs by dialect — `DATETIME` on MySQL vs `TIMESTAMP
WITH TIME ZONE` on PostgreSQL
(`0025_add_audit_entries_to_journal_entry.xml:28,34`). The author verified
PostgreSQL only. Prefer a defensive conversion helper that handles
`OffsetDateTime` / `Timestamp` / `LocalDateTime`.
- `.toLocalDate()` takes the date **in the stored offset (UTC)** rather than
going through `DateUtils` / the tenant zone. This is likely equivalent to what
EclipseLink 4 returned, so probably not a regression — but it hard-codes
UTC-date semantics at a new call site.
No NPE risk: `created_on_utc` carries a `NOT NULL` constraint (`:61,68`).
:warning: **RISK** :warning:
Let's double check we still store UTC datetime and fetch as UTC same value
on postgres and mysql too!
## 15. `asLong(0)` bakes loan id `0` into COB routing
**Files:**
-
`fineract-provider/.../infrastructure/jobs/filter/LoanCOBFilterHelperImpl.java:198`
-
`fineract-provider/.../infrastructure/jobs/filter/ProgressiveLoanModelCheckerHelper.java:136`
-
`fineract-provider/.../infrastructure/jobs/filter/WorkingCapitalLoanCOBFilterHelperImpl.java:173`
```diff
-return jsonNode.get("loanId").asLong();
+return jsonNode.get("loanId").asLong(0);
```
Faithfully preserves the Jackson 2 behaviour (Jackson 3's no-arg `asLong()`
throws `JsonNodeException` on non-coercible values), and the author's diagnosis
of the `BatchApiTest` failure is credible. But the review point stands: an
unresolved chained-batch reference like `"loanId": "$.loanId"` becomes loan id
`0` and enters COB lock/routing logic. Harmless today (no rows match), but it
masks a real gap. Preserving behaviour is defensible for an upgrade PR — raise
the proper fix (resolve or skip the placeholder) as a follow-up ticket.
Also note the incidental widening flagged in the PR thread:
`ALLOW_UNESCAPED_CONTROL_CHARS` moved from a `LoanCOBFilterHelperImpl`-only
instance field to the shared `COBFilterApiMatcher` base, so it now applies to
all three matchers. Acknowledged by the author; worth a line in the commit
message.
## 16. `ReservedCodeCacheSize=128m` lowers the JVM default
**File:** `config/docker/env/fineract-common.env:60`
```diff
-JAVA_TOOL_OPTIONS="-Xmx1G ... -XX:TieredStopAtLevel=1
-XX:+UseContainerSupport ..."
+JAVA_TOOL_OPTIONS="-Xmx1G ... -XX:TieredStopAtLevel=1
-XX:ReservedCodeCacheSize=128m -XX:+UseContainerSupport ..."
```
The JVM default is 240 MB; this **lowers** it. If the intent was to fit the
1 GB container, be aware the failure mode is `CodeCache is full. Compiler has
been disabled` — a silent and severe throughput cliff rather than a clean OOM.
Please state the reasoning in the PR, and confirm it was measured under a full
COB run.
## 17. CI env addition is a no-op
**File:** `.github/workflows/build-e2e-tests.yml:37`
```yaml
POLLING_EVENT_WAIT_TIMEOUT_IN_MS: 15000
```
This equals the existing default
(`fineract-test-application.properties:40`), and now that `delay-in-ms` /
`interval-in-ms` have been decoupled from this variable (finding 18) it no
longer influences them either. Dead configuration — remove it, or set the value
that was actually intended.
## 18. e2e properties still modified despite "moved to #6175"
**File:**
`fineract-e2e-tests-core/src/test/resources/fineract-test-application.properties`
The PR thread says this file was reverted and moved to #6175, but it is
still modified on the branch:
```diff
-fineract-test.event.delay-in-ms=${POLLING_EVENT_WAIT_TIMEOUT_IN_MS:100}
-fineract-test.event.interval-in-ms=${POLLING_EVENT_WAIT_TIMEOUT_IN_MS:100}
+fineract-test.event.delay-in-ms=${POLLING_EVENT_DELAY_IN_MS:100}
+fineract-test.event.interval-in-ms=${POLLING_EVENT_INTERVAL_IN_MS:100}
-fineract-test.client-read-timeout=${CLIENT_READ_TIMEOUT:60}
+fineract-test.client-read-timeout=${CLIENT_READ_TIMEOUT:120}
```
The `CLIENT_READ_TIMEOUT: 60 → 120` doubling was **not** part of the
reverted set described in the thread and has not been discussed. Doubling an
e2e read timeout in a dependency-upgrade PR usually indicates a latency
regression worth understanding rather than absorbing.
For contrast, these reverts **did** land locally (now import-only):
`BusinessDateWritePlatformServiceImpl`, `IncreaseBusinessDateBy1DayTasklet`,
`IncreaseCobDateBy1DayTasklet` and their tests, plus the e2e event files.
## 19. `keystoreType JKS` silently dropped with the tomcat11x bump
**File:** `integration-tests/build.gradle:87`
```diff
-containerId "tomcat10x"
+containerId "tomcat11x"
...
-property 'cargo.tomcat.connector.keystoreType', 'JKS'
```
The keystore is still `fineract-provider/src/main/resources/keystore.jks`.
If Tomcat 11 / Cargo now defaults to PKCS12, this is a latent HTTPS
misconfiguration that happens to work only because of a fallback. Please state
why it was removed.
## 20. Class-wide `Strictness.LENIENT` added
**File:**
`fineract-provider/src/test/java/org/apache/fineract/portfolio/loanaccount/jobs/generateloanlossprovisioning/GenerateLoanlossProvisioningTaskletTest.java`
```java
@MockitoSettings(strictness = Strictness.LENIENT)
```
Class-wide leniency suppresses unused-stub detection for every test in the
class. The sibling change used per-stub `lenient()` — prefer that here too, and
note which stub actually needed it.
## 21. Unrelated churn remains
Consistent with the maintainer's request to split the PR, these are still
bundled and unrelated to the upgrade:
- `TransactionBoundCacheManager`: private method rename `resetCaches` →
`clearAllCaches` (cosmetic)
- `SavingsAccountTransactionsSearchServiceImpl:142`:
`@SuppressFBWarnings("NP_BOOLEAN_RETURN_NULL")` — the author's JSpecify
explanation is correct (`org.springframework.lang.Nullable` carried a JSR-305
`@CheckForNull` nickname that SpotBugs understood;
`org.jspecify.annotations.Nullable` does not), but the tri-state `Boolean`
deserves its own cleanup ticket
- `mockito-inline` removal across ~8 `dependencies.gradle` files (correct —
it is a no-op on Mockito 5 — but orthogonal)
- `application-test.properties`: added
`fineract.job.retainedEarning-chunk-size`
- `CobPartitioningTest`:
`RestAssured.enableLoggingOfRequestAndResponseIfValidationFails()`
---
# Build and dependency notes
## Justified, but worth tracking
**`build.gradle:98` — buildscript `spring-core` pin.** The author's
verification is sound: the Boot 4 Gradle plugin puts spring-core 7 on the
shared buildscript classpath, and `license-maven-plugin:3.0` calls a
`PropertyPlaceholderHelper(String, String, String, boolean)` constructor that
Spring Framework 7 removed.
```groovy
configurations.classpath {
resolutionStrategy { force 'org.springframework:spring-core:6.2.18' }
}
```
Buildscript-only, no runtime effect. Add a TODO referencing the hierynomus
license plugin issue so it gets removed rather than ossifying.
**`fineract-client/build.gradle:274-281` — JUnit BOM pinned to 5.14.4.**
Reasonable: the generated SDK targets Java 8 and JUnit 6 requires JVM 17+.
Results in two JUnit versions in one build, which is acceptable across separate
modules but should be commented as deliberate (it is).
**`build.gradle:719` — `jcl-over-slf4j` added.** Correct: Spring Framework 7
removed `spring-jcl`, which had been providing `org.apache.commons.logging`,
and `commons-logging` is excluded globally.
**`build.gradle:428-432` — `lombok.config` as a compile task input.** A
genuine correctness fix for incremental/cached builds, and newly load-bearing
given finding 11.
## BOM changes
| Dependency | From | To |
|---|---|---|
| `spring-boot-dependencies` | 3.5.15 | 4.1.0 |
| `spring-boot` Gradle plugin | 3.5.15 | 4.1.0 |
| `eclipselink` / `org.eclipse.persistence.jpa` | 4.0.9 | 5.0.1 |
| `junit-bom` | 5.14.4 | 6.0.3 |
| `jersey-bom` | 3.1.11 | 4.0.2 |
| `springdoc-openapi-starter-webmvc-ui` | 2.8.17 | 3.0.3 |
| `resilience4j-spring-boot3` | — | `resilience4j-spring-boot4` 2.4.0
(pinned; the BOM does not manage the boot4 variant) |
| `jackson-bom` | 2.22.1 | 2.22.1 (retained alongside Jackson 3 from the
Boot 4 BOM) |
Now delegated to the Boot 4 BOM (explicit pins removed): `spring-core`,
`spring-security-core`, `spring-restdocs`, `thymeleaf` (+ `thymeleaf-spring7`),
Tomcat embed (10.1 → 11).
Netty stays pinned on the 4.1 line for MockServer compatibility — unchanged,
still correct.
---
# Recommendation: split the PR
The maintainer's request to split is well-founded, and the review surface
supports a concrete three-way split.
### Part 1 — Mechanical only
Batch package moves, `org.springframework.lang` → JSpecify, `starter-web` →
`starter-webmvc`, Boot 4 module splits, `mockito-inline` removal, BOM bumps
that do not change behaviour. Large but reviewable by inspection.
### Part 2 — Jackson 2 → 3 island
`tools.jackson` migration, `spring-boot-jackson2`, the Jersey handler
rewrite, `lombok.config`. Needs contract tests on the REST wire format.
Findings: **9, 10, 11**
### Part 3 — Spring Batch 6 semantics
`ChunkOrientedStep` transaction boundaries, `@Transactional` on
`COBBusinessStepService`, listener registration, `JobOperator` / `JobRegistry`
/ `getNextJobParameters`, task-executor queue sizing, and the execution-context
serializer plus its data migration.
Findings: **1, 2, 4, 5, 6, 7, 8, 12**
This part needs load testing against a partitioned COB run and is where I
would hold the line. Parts 1 and 2 are mergeable with normal review.
---
# Blocking items
Before merge, regardless of how the PR is split:
1. **Finding 1** — decide and implement the execution-context migration
strategy.
2. **Finding 2** — resolve or explicitly gate the COB atomicity divergence
(FINERACT-2684).
3. **Finding 3** — fix the CloudWatch exclusion/property contradiction.
4. **Finding 4** — remove the double listener registration and the incorrect
comment.
5. **Finding 8** — apply the `AsyncTaskExecutor` fix to the Working Capital
config.
6. **Finding 18** — confirm the actual branch contents against the PR
thread, and explain the `CLIENT_READ_TIMEOUT` doubling.
--
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]
