Repository navigation
Conversation
| if (contentProperty != null && contentProperty.getDateTimeFormatProperty() != null) { | ||
| format = contentProperty.getDateTimeFormatProperty().getFormat(); | ||
| } | ||
| WorkBookUtil.fillDataFormat(cellData, format, DateUtils.defaultDateFormat); |
There was a problem hiding this comment.
If format is empty, it is still passed instead of falling back to the default format. Could we normalise an empty format to null?
There was a problem hiding this comment.
Good catch — fixed in 41ad950: an empty format is now normalised to null before WorkBookUtil.fillDataFormat, matching SqlTimeDateConverter, so it falls back to the default.
Regression test: dateConverterFallsBackToDefaultFormatForEmptyDateTimeFormat — on the old code it fails with expected: <yyyy-MM-dd HH:mm:ss> but was: <>, passes after the change.
| @Override | ||
| public Timestamp convertToJavaData( | ||
| ReadCellData<?> cellData, ExcelContentProperty contentProperty, GlobalConfiguration globalConfiguration) { | ||
| return Timestamp.valueOf(DateUtils.getLocalDateTime( |
There was a problem hiding this comment.
If DateUtils.getLocalDateTime returns null, then it throws. Could we return null in that case for consistency with other converters?
There was a problem hiding this comment.
Right — fixed in 41ad950: DateUtils.getLocalDateTime returning null (a number that is not a valid Excel date, e.g. a negative value) no longer reaches Timestamp.valueOf(null); the converter now returns null, mirroring SqlTimeNumberConverter.
Regression test: numberConverterReturnsNullForInvalidExcelDate — red on the old code with NullPointerException: Cannot invoke "java.time.LocalDateTime.getYear()" because "dateTime" is null, green now.
|
Please resolve merge conflicts :) |
Carry-forward of apache#1019 (original implementation by Syed Ishmum Ahnaf): - Resolve the 1904 windowing flag through the shared DateUtils.isDate1904 helper, like the rest of the date converter family. Reading DateTimeFormatProperty#getUse1904windowing() directly broke on the null DEFAULT preserved by apache#1042 and threw a NullPointerException whenever @DateTimeFormat was present without an explicit use1904windowing value. - Drop the EasyExcel derived license header from the four new files; they are written from scratch, so the plain ASF header applies. - Align with current conventions: Class<?> supportJavaTypeKey, single-line javadoc, and tests moved to the mirrored converters.timestamp package with qualified assertions, the thread-local cache teardown used by the sibling suites, a loader family test and an XLSX/XLS/CSV round trip.
715a76d to
fe0f779
Compare
nkuprins
left a comment
nkuprins
left a comment
There was a problem hiding this comment.
LGTM! Thank you for collaborating with @SyedIshmumAhnaf.
This branch has not been deployed
3 participants
Purpose of the pull request
Related: #1017 (java.sql.Timestamp slice).
Adds
java.sql.Timestampconverter support, carried forward from #1019 by @SyedIshmumAhnaf, who handed the PR over — the original commits are preserved with their authorship and one additional commit brings the code up to date withmain.What's changed?
TimestampNumberConverterandTimestampStringConverter(read + write) andTimestampDateConverter(write), registered inDefaultConverterLoadernext to thejava.sql.Timefamily.DateUtils.getLocalDateTimeand writes convert throughTimestamp#toLocalDateTime(), so no implicit timezone shifts are introduced.Carry-forward adaptation
DateUtils.isDate1904helper. ReadingDateTimeFormatProperty#getUse1904windowing()directly throws aNullPointerExceptionsince [Bug] @DateTimeFormat use1904windowing DEFAULT is collapsed to false, bypassing the global 1904 fallback #1042 keeps the annotation DEFAULT asnull— any@DateTimeFormatwithout an explicituse1904windowingvalue would hit it.converters.timestamp, with qualified assertions and the same thread-local cache teardown as sibling suites, plus aloadConvertersRegistersTimestampFamilycase and an XLSX/XLS/CSV round trip.Review round (2026-10-15)
TimestampDateConverternormalises an empty format string tonullbeforeWorkBookUtil.fillDataFormat(matchingSqlTimeDateConverter) — an empty@DateTimeFormatwas previously written through as an empty cell format instead of falling back to the default.TimestampNumberConverter#convertToJavaDatareturnsnullwhen the number is not a valid Excel date (matchingSqlTimeNumberConverter) — it previously threwNullPointerExceptionviaTimestamp.valueOf(null).Notes carried over from #1019
dd MMMM yyyy HH:mm:ss), so it fails when the locale is not honored.Verification
NullPointerException: ... because getUse1904windowing() is null, passes after the change.dateConverterFallsBackToDefaultFormatForEmptyDateTimeFormatfails withexpected: <yyyy-MM-dd HH:mm:ss> but was: <>andnumberConverterReturnsNullForInvalidExcelDatefails withNullPointerException: ... "dateTime" is null; both pass after the change.fesod-common,fesod-shaded,fesod-sheet);spotless:checkandjavadoc:javadocpass.Supersedes #1019 — many thanks to @SyedIshmumAhnaf for the original implementation.
Checklist