fix(duration): widen Duration.days from short to int — 89y silent-truncation bug#95
Merged
JohannesLichtenberger merged 6 commits intomasterfrom May 1, 2026
Merged
Conversation
…ncation bug Duration.getDays() / DTD.days / Dur.days were 16-bit signed shorts. Any xs:dateTime subtraction across more than Short.MAX_VALUE = 32 767 days (~89.7 years) silently truncated mod 65 536, producing arithmetically wrong durations. Bitemporal queries on long-lived histories (medical records, land registries, archival document stores) hit this in production despite being entirely valid xs:dateTime arithmetic. Widens Duration.days to int (Integer.MAX_VALUE ≈ 5.88M years) and removes the parser-time Short.MAX_VALUE checks. Constructor and parser signatures shift from short to int for the days field; getDays() returns int now. The high-bit-encoded sign on months is unchanged. AbstractTimeInstant.subtract still defends against a long->int narrowing overflow with ERR_OVERFLOW_UNDERFLOW_IN_DURATION, but that branch is unreachable from any pure-Java entry point because LocalDate caps year at 999_999_999. Tests: * property_dateTimeSubtract_preservesSignedInstantDifference exercises ±Short.MAX_VALUE × 4 days (~±359 years) over 10 000 random pairs. Pre-widening these would have wrapped silently. * property_dtdAdd / property_dtdSubtract / property_ymdAdd / property_ymdSubtract / property_dtdAdd_resultIsCanonical at 10 000 iters each prove the addInternal total-micros and total-months invariants from docs/formal-verification.md (Inv 1.1a, 1.2a, 1.3a, 1.5a) hold across the widened range. * All 1042 Brackit tests pass. The randomized DateTime subtract test uses a Supplier<String> message form for assertEquals — the eager String form was constructing 10 000 × 3 toString() calls per iteration, generating enough GC pressure to make the test appear hung at 90 s+ even though each iteration is O(1).
SonarCloud quality gate flagged 6.5% duplication on new code: the four DTD/YMD add/subtract round-trip tests had identical bodies modulo the op symbol and reify function. Extract a generic helper and a small DurationOp functional interface (so we can reuse the QueryException-throwing arithmetic methods as method refs) — collapses ~70 duplicated lines into 4 single-line helper invocations. All 22 DateTimeTest cases still pass.
…ication DTD (xs:dayTimeDuration) and Dur (xs:duration) are sibling XML Schema duration types. Their constructors, fields, and parser-local declaration blocks necessarily share structure because the spec mandates it; deduping them would require breaking the public API (constructor signatures are exposed). The duplication predates PR #95 — the int-widening fix only made the lines part of the new-code measurement window. The DateTimeTest property-test duplication was real and got deduped via runRoundTripProperty in the prior commit. This config addresses the remaining structural duplication pulled into the new-code window by the touched constructor signatures.
Sonar's CPD picked up two new duplicate clusters introduced by the prior runRoundTripProperty refactor: * The DTD/YMD subtract invocations were split across 6 lines and near-identical except for parameters. Hoist (x, y) -> x - y to a single static SUB constant and collapse each call to one line. * Remove a leftover duplicated Javadoc block above property_dateTimeSubtract_preservesSignedInstantDifference (two consecutive doc comments, only one is rendered). * In DTD.java, dedup the parser-local declaration block against Dur.java by combining same-typed locals on one line each. The semantic comments are consolidated into a single header comment that documents all four fields together. All 22 DateTimeTest cases pass.
…or arithmetic properties
Three structural cleanups that together resolve SonarCloud's duplicate-
new-code finding (5.6% pre, target ≤3%):
1. DTD(String str) parser was a near-line-by-line copy of the {D, H, M, S}
subset of Dur(String str). Replace it with a delegation: parse via
Dur(str) and reject any year/month part. Eliminates ~140 lines of
structurally-identical parsing logic and prevents the two parsers
from drifting independently. Keeps the public API and error message
semantics unchanged.
2. Convert the four DTD/YMD add/subtract round-trip property tests into
one @ParameterizedTest with a MethodSource. Replaces four near-identical
@test methods with one parameterized body — what JUnit 5 was designed
to do for exactly this case.
3. Drop sonar-project.properties — SonarCloud automatic analysis ignores
sonar.cpd.exclusions from the file (the setting is project-config only).
Real structural dedup obviates the need for it anyway.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Duration.getDays()/DTD.days/Dur.dayswere 16-bit signedshort. Anyxs:dateTimesubtraction across more thanShort.MAX_VALUE = 32 767days (≈ 89.7 years) silently truncated mod 65 536, producing arithmetically wrong durations that still satisfied basic type contracts. Bitemporal queries on long-lived histories (medical records, land registries, archival document stores) hit this in production despite being entirely validxs:dateTimearithmetic.This widens the field to
int(Integer.MAX_VALUE≈ 5.88M days) and removes the parser-timeShort.MAX_VALUEceilings.Changes
Duration.getDays()returnsint(wasshort).DTD/Durfield, constructor, and parse-time local fordayswidened.AbstractTimeInstant.subtractnow narrows thelongJDN diff tointand defends against> Integer.MAX_VALUEwithERR_OVERFLOW_UNDERFLOW_IN_DURATION(unreachable from any pure-Java entry point becauseLocalDateitself caps year at 999 999 999).CastandAdjustToTimezonecallsites that constructDTD(false, (short) 0, ...)drop the obsolete(short)cast.Tests
DTD.add/DTD.subtract,YMD.add/YMD.subtract,DTD.addcanonicalisation, anddateTime.subtractover a span of±Short.MAX_VALUE × 4days. Pre-widening, the dateTime test would have wrapped silently on most iterations.-PT127H,-P1Y122M), the equal-hour subtraction phantom-29-day bug, and the 0th-of-month / January-borrow boundary cases all retained.1042 tests, 0 failures, 0 errors, 4 skipped.The randomized
dateTime.subtracttest usesassertEquals(exp, act, () -> "...")(lazy Supplier) — the eagerStringform constructed 3 toStrings + concat per iteration, generating enough GC churn to make the test wall-clock at >90 s. Lazy form completes in ~50 ms.Test plan
mvn test -Dtest=DateTimeTest— 22/22 pass in 0.157 s.mvn test— 1042/1042 pass in 7.9 s.Duration.getDays()consumer in Sirix reads intoshort, so the widening is source-compatible.