spring-projects / spring-batch-extensions

Spring Batch Extensions
242 stars 258 forks source link

IsoFormattingDateDataFormatter does not work #121

Closed szmeti closed 3 months ago

szmeti commented 1 year ago

IsoFormattingDateDataFormatter reads the value as a LocalDateTime and then attempts to format it as an ISO_OFFSET_DATE_TIME. A LocalDateTime does not have zone offset information, therefore this always fails with an exception:

java.time.temporal.UnsupportedTemporalTypeException: Unsupported field: OffsetSeconds

A simple test to test this:

@Test
void dateTest() throws Exception {
  LocalDateTime now = LocalDateTime.now();
  now.format(DateTimeFormatter.ISO_OFFSET_DATE_TIME);
}

Am I missing something?

dgray16 commented 1 year ago

Dude, this is hardly related to Spring Batch. I believe JDK repository is what you need: https://github.com/openjdk/jdk

szmeti commented 1 year ago

Ok, let me be more explicit then. There is an IsoFormattingDateDataFormatter class in the project: https://github.com/spring-projects/spring-batch-extensions/blob/main/spring-batch-excel/src/main/java/org/springframework/batch/extensions/excel/IsoFormattingDateDataFormatter.java

This class does not use the Java APIs correctly. When it converts a date cell to an ISO_OFFSET_DATE_TIME, it does the following:

if (cellType == CellType.NUMERIC && DateUtil.isCellDateFormatted(cell, cfEvaluator)) {
  LocalDateTime value = cell.getLocalDateTimeCellValue();
  return (value != null) ? value.format(DateTimeFormatter.ISO_OFFSET_DATE_TIME) : "";
}

As I wrote previously, a LocalDateTime can never be formatted with DateTimeFormatter.ISO_OFFSET_DATE_TIME because a LocalDateTime does not have zone offset information. My code sample was just a simple demonstration of how you can verify the issue.

Not even sure what the intention is here since Excel itself does not deal with time zones, dates are always local. So if this feature in Spring Batch is supposed to convert an Excel date to an ISO offset format, it should first convert the LocalDateTime to a ZonedDateTime by picking a time zone. That time zone should probably be configurable as well since you cannot just pick an arbitrary time zone, it really depends on the contents of the actual Excel file. If it is not configurable, then the documentation should clearly state which time zone Spring Batch is going to use.

mdeinum commented 1 year ago

This was an oversight on my part and this is what you get for not writing a proper unit test for the class in question.

Just to clarify this isn't part of Spring Batch but rather the Spring Batch Extensions project, the latter is community driven and isn't part of Spring Batch itself.

See #100 and #98 for why this class exists and what the intent of it is.

ddoaj97 commented 9 months ago

I also ran into this issue and was wondering whether there are any plans on fixing it?

Seems to me like the solution is simply to replace these 2 lines with the following code:

LocalDateTime value = cell.getLocalDateTimeCellValue();
return (value != null) ? value.format(DateTimeFormatter.ISO_LOCAL_DATE_TIME) : "";

This would be sufficient to fix it as the code exclusively uses LocalDateTime. Excel does not support time zones, so there is no point in using a formatter like ISO_OFFSET_DATE_TIME that expects a timezone and only works with ZonedDateTimes. ISO_LOCAL_DATE_TIME uses the exact same date-time format just without the timezone information.

In my opinion this behaviour should have been the default from the start as the current default mapping does not give any indication about the formatting or locale the user set on the cell making it impossible to interpret the date string correctly in Java.