adamsaghy commented on code in PR #6187:
URL: https://github.com/apache/fineract/pull/6187#discussion_r4302657556
##########
fineract-core/src/main/java/org/apache/fineract/infrastructure/core/domain/JdbcSupport.java:
##########
@@ -65,6 +66,18 @@ public static LocalTime getLocalTime(final ResultSet rs,
final String columnName
return localTime;
}
+ public static OffsetTime getOffsetTime(final ResultSet rs, final String
columnName) throws SQLException {
+ try {
+ return rs.getObject(columnName, OffsetTime.class);
+ } catch (SQLException exception) {
Review Comment:
@AnvayKharb Thanks for testing without the customizer. You're right that
mapping `OffsetTime` directly doesn't work, and I looked into why. The
customizer only partly fixes it, so I'd like to suggest a simpler approach.
**Why `OffsetTime` breaks when `OffsetDateTime` doesn't**
EclipseLink 5.0.2 handles the two types differently in
`DatabasePlatform.setParameterValueInDatabaseCall`:
- `OffsetDateTime` → `Timestamp.from(odt.toInstant())`: converted to the
correct instant and stored as UTC.
- `OffsetTime` → `Timestamp.valueOf(LocalDateTime.of(epoch,
ot.toLocalTime()))`: the offset is dropped **without** converting the time.
On read (`DatabaseAccessor`), the server JVM's current offset is attached.
That's why `02:00:00+05:30` came back as `02:00:00Z`.
The customizer fixes saving, but:
- **PostgreSQL:** with the field type set to `Object`, EclipseLink reads the
column with `ResultSet.getObject()`. The PostgreSQL driver returns a
`java.sql.Time` for `timetz`, so the entity gets `20:30Z` and the offset is
lost. `copyTransaction()` then saves `20:30+00` next to the original date for
reversals (undo, interest posting) and overdraft replacement transactions.
- **MySQL/MariaDB:** a `TIME` column can't hold an offset, so the UTC clock
time is stored. Neither driver can return the original offset: Connector/J
converts to the session timezone, and the MariaDB driver doesn't support
`OffsetTime` at all.
- **Converting to UTC is itself wrong here,** because `transactionDate` is a
separate column in the client's own calendar. `27 July` + `02:00+05:30` stored
as `27 July` + `20:30Z` points to a moment a day too late. `OffsetDateTime` is
safe because its date moves with it; `OffsetTime` next to a separate
`LocalDate` isn't.
Seems i was wrong here and we might want to use LocalTime (just as your 1st
approach):
**Proposed approach: store the clock time and the offset separately**
```xml
<addColumn tableName="m_savings_account_transaction">
<column name="transaction_time" type="TIME(6)"/>
<column name="transaction_time_offset" type="INT"/>
</addColumn>
<!-- same for m_account_transfer_transaction -->
```
- `transaction_time` is the clock time exactly as the client sent it, mapped
as a `LocalTime`.
- `transaction_time_offset` is `ZoneOffset.getTotalSeconds()`: `+05:30` →
`19800`, `-04:00` → `-14400`, `Z` → `0`.
The offset is stored in seconds because `ZoneOffset` uses seconds itself
(`getTotalSeconds()` / `ofTotalSeconds()`). Every offset `ISO_OFFSET_TIME`
accepts, including half- and quarter-hour ones, can be stored exactly.
Entity:
```java
@Column(name = "transaction_time")
private LocalTime transactionTime;
@Column(name = "transaction_time_offset")
private Integer transactionTimeOffset;
// set both or neither, in one place
this.transactionTime = time == null ? null : time.toLocalTime();
this.transactionTimeOffset = time == null ? null :
time.getOffset().getTotalSeconds();
public OffsetTime getTransactionTime() {
return transactionTime == null ? null
: OffsetTime.of(transactionTime,
ZoneOffset.ofTotalSeconds(transactionTimeOffset));
}
```
For reads, use `JdbcSupport.getLocalTime(...)` and
`JdbcSupport.getInteger(...)`, then `OffsetTime.of(...)`.
**What this gives us**
- **No API change:** `transactionTime` and `transferTime` still accept and
return ISO offset times such as `14:30:00+05:30`.
- **The same value on PostgreSQL, MySQL and MariaDB:** the integration test
can drop the `COMPOSE_FILE` switch and expect the original value on every
database.
- **The exact moment can be rebuilt:**
`transactionDate.atTime(getTransactionTime())`. For example, `27 July 2026` +
`02:00` + `19800` → `2026-07-26T20:30Z`.
- **Copies keep the value:** reversals and replacement transactions just
copy a `LocalTime` and an `Integer`.
- **Less code:** we can remove `OffsetTimePersistenceCustomizer` (and its
PostgreSQL driver `PGobject` dependency), the error-message fallback in
`JdbcSupport.getOffsetTime`, and the per-database branches.
**Also please**
- Use `TIME(6)`; without a precision, MySQL creates `TIME(0)` and drops
fractional seconds.
- Add both columns to the scheduler path: the insert in
`SavingsSchedularInterestPoster.batchQueryForTransactionInsertion` and
`SavingsAccountTransactionData.copyTransaction`. Otherwise replacement
transactions written by the interest-posting job lose the time.
- Add integration test cases for undo (the reversal transaction should keep
the original time) and for a time whose UTC conversion would cross midnight.
--
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]
