adamsaghy commented on PR #6187:
URL: https://github.com/apache/fineract/pull/6187#issuecomment-5760078938
@AnvayKharb Please review the below concerns:
**Blocking**
1. transactionDate + transactionTime can describe the wrong instant
(day-boundary bug)
Every write path normalizes the time to UTC —
fineract-savings/src/main/java/org/apache/fineract/portfolio/savings/domain/SavingsAccountTransaction.java,
fineract-provider/src/main/java/org/apache/fineract/portfolio/account/domain/AccountTransferTransaction.java,
fineract-savings/src/main/java/org/apache/fineract/portfolio/savings/data/SavingsAccountTransactionDTO.java,
and again in JsonParserHelper.extractOffsetTimeNamed — but transactionDate is
still taken verbatim from the client's local calendar.
POST { "transactionDate": "27 July 2026", "transactionTime":
"02:00:00+05:30" }
stored: date = 2026-07-27, time = 20:30:00Z
Combining those, as your own review comment asked for ("transaction date +
transaction time together can represent a more precise point in time"), gives
2026-07-27T20:30Z. The actual instant was 2026-07-26T20:30Z — off by a full
day. Same class of error for any time whose UTC conversion crosses midnight in
either direction.
The fix is to stop normalizing and persist the OffsetTime exactly as
supplied. timetz keeps the offset, so 2026-07-27 + 02:00:00+05:30 reconstructs
the correct instant. Normalizing is what breaks it, and it also makes the
round-trip lossy (the client can no longer see the offset they sent).
Caveat that pushes back on the design: MySQL's TIME(6) cannot store an
offset at all, so "stored as OffsetTime, fetched as OffsetTime" only holds on
PostgreSQL. On MySQL Hibernate falls back to NORMALIZE against the JVM default
zone. Fineract forces -Duser.timezone=UTC in its run configs so it works today,
but it's implicit — worth setting
hibernate.timezone.default_storage=NORMALIZE_UTC if you keep MySQL support here.
2. template=true silently drops the persisted time
SavingsAccountTransactionData.templateOnTop()
(fineract-core/src/main/java/org/apache/fineract/portfolio/savings/data/SavingsAccountTransactionData.java)
rebuilds the object via createData(...) and never copies transactionTime. So
GET /v1/savingsaccounts/{id}/transactions/{txnId}?template=true returns
transactionTime: null for a transaction that has one
(fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/api/SavingsAccountTransactionsApiResource.java).
This is a direct consequence of the withTransactionTime mutator approach — a
builder (as you already suggested) wouldn't have this hole.
3. Transaction adjustment accepts transactionTime and ignores it
SavingsAccountTransactionDataValidator.validate() now whitelists
transactionTime, and adjustSavingsTransaction
(fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/SavingsAccountWritePlatformServiceJpaRepositoryImpl.java)
calls that same validator — but builds its SavingsAccountTransactionDTO
without the time. Adjusting a transaction therefore accepts the parameter,
drops it, and returns 200. Either wire it through or reject it on that endpoint.
**Unaddressed from your earlier review**
- JdbcSupport.getOffsetTime still has all three branches (OffsetTime /
LocalTime / java.sql.Time). You asked for direct OffsetTime reading and the
author agreed to simplify it on 2026-09-07; it hasn't been done.
- @Schema annotations are still on AccountTransferData.transferTime and
SavingsAccountTransactionData.transactionTime, which you said weren't needed.
**Non-blocking, but still should be review and address**
- fields= filtering — transactionTime wasn't added to
SAVINGS_TRANSACTION_RESPONSE_DATA_PARAMETERS
(fineract-savings/src/main/java/org/apache/fineract/portfolio/savings/api/SavingsApiSetConstants.java),
so ?fields=transactionTime throws UnsupportedParameterException. Normal
responses are unaffected.
- Validation style — both validators call extractOffsetTimeNamed(...) purely
for its throwing side effect and discard the result. A malformed time throws
PlatformApiDataValidationException immediately instead of accumulating into
dataValidationErrors, so it masks every other validation error in the same
request. Every neighbouring check uses baseDataValidator.reset().parameter(...).
- Liquibase — the repo convention is one <addColumn> with a portable type
(see TIMESTAMP WITH TIME ZONE in 0136/0162/0230); this splits into a Postgres
<addColumn> plus a raw MySQL <sql> block with no rollback and no preconditions.
Only 2 of 254 part files use dbms at all.
- Deposit accounts — DepositAccountReadPlatformServiceImpl:1130 maps the
same m_savings_account_transaction rows but wasn't updated, so fixed/recurring
deposit transaction endpoints won't return transactionTime. Fine if
intentionally out of scope, but worth stating.
- Converter inconsistency — OffsetTimeJsonConverter.convertToObject does not
normalize to UTC while JsonParserHelper.extractOffsetTimeNamed does. Harmless
today (no Jackson-bound OffsetTime input exists), but a trap later.
--
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]
