search

ThreeTen / threeten

This project was the home of code used to develop a modern date and time library for JDK8. Development has moved to OpenJDK and a separate backport project, threetenbp.
http://threeten.github.io/
191 stars 37 forks source link

Better error messages for parsing #342

Closed jodastephen closed 10 years ago

jodastephen commented 10 years ago

Parsing originally had good error messages but some of that has been lost.

When calling methods like LocalDate.parse(), the method should print the toString of the object being parsed to ensure that the internal data is visible to the user.

The toString of Parsed also needs enhancing to avoid printing null if one of date or time is null.

jodastephen commented 10 years ago 
# HG changeset patch
# User scolebourne
# Date 1380233967 25200
# Node ID 671161b3333ed8650adf9f881d27b0e8fbc734b2
# Parent  9d3bef2027e04459d27262c11ac7a17c47a12621
Enhance error messages for parsing

See #342

diff -r 9d3bef2027e0 -r 671161b3333e src/share/classes/java/time/DayOfWeek.java
--- a/src/share/classes/java/time/DayOfWeek.java    Thu Sep 26 09:22:34 2013 -0700
+++ b/src/share/classes/java/time/DayOfWeek.java    Thu Sep 26 15:19:27 2013 -0700
@@ -187,7 +187,12 @@
         if (temporal instanceof DayOfWeek) {
             return (DayOfWeek) temporal;
         }
-        return of(temporal.get(DAY_OF_WEEK));
+        try {
+            return of(temporal.get(DAY_OF_WEEK));
+        } catch (DateTimeException ex) {
+            throw new DateTimeException("Unable to obtain DayOfWeek from TemporalAccessor: " +
+                    temporal + " of type " + temporal.getClass().getName(), ex);
+        }
     }

     //-----------------------------------------------------------------------
diff -r 9d3bef2027e0 -r 671161b3333e src/share/classes/java/time/Instant.java
--- a/src/share/classes/java/time/Instant.java  Thu Sep 26 09:22:34 2013 -0700
+++ b/src/share/classes/java/time/Instant.java  Thu Sep 26 15:19:27 2013 -0700
@@ -366,9 +366,14 @@
             return (Instant) temporal;
         }
         Objects.requireNonNull(temporal, "temporal");
-        long instantSecs = temporal.getLong(INSTANT_SECONDS);
-        int nanoOfSecond = temporal.get(NANO_OF_SECOND);
-        return Instant.ofEpochSecond(instantSecs, nanoOfSecond);
+        try {
+            long instantSecs = temporal.getLong(INSTANT_SECONDS);
+            int nanoOfSecond = temporal.get(NANO_OF_SECOND);
+            return Instant.ofEpochSecond(instantSecs, nanoOfSecond);
+        } catch (DateTimeException ex) {
+            throw new DateTimeException("Unable to obtain Instant from TemporalAccessor: " +
+                    temporal + " of type " + temporal.getClass().getName());
+        }
     }

     //-----------------------------------------------------------------------
diff -r 9d3bef2027e0 -r 671161b3333e src/share/classes/java/time/LocalDate.java
--- a/src/share/classes/java/time/LocalDate.java    Thu Sep 26 09:22:34 2013 -0700
+++ b/src/share/classes/java/time/LocalDate.java    Thu Sep 26 15:19:27 2013 -0700
@@ -356,7 +356,8 @@
         Objects.requireNonNull(temporal, "temporal");
         LocalDate date = temporal.query(TemporalQuery.localDate());
         if (date == null) {
-            throw new DateTimeException("Unable to obtain LocalDate from TemporalAccessor: " + temporal.getClass());
+            throw new DateTimeException("Unable to obtain LocalDate from TemporalAccessor: " +
+                    temporal + " of type " + temporal.getClass().getName());
         }
         return date;
     }
diff -r 9d3bef2027e0 -r 671161b3333e src/share/classes/java/time/LocalDateTime.java
--- a/src/share/classes/java/time/LocalDateTime.java    Thu Sep 26 09:22:34 2013 -0700
+++ b/src/share/classes/java/time/LocalDateTime.java    Thu Sep 26 15:19:27 2013 -0700
@@ -449,7 +449,8 @@
             LocalTime time = LocalTime.from(temporal);
             return new LocalDateTime(date, time);
         } catch (DateTimeException ex) {
-            throw new DateTimeException("Unable to obtain LocalDateTime from TemporalAccessor: " + temporal.getClass(), ex);
+            throw new DateTimeException("Unable to obtain LocalDateTime from TemporalAccessor: " +
+                    temporal + " of type " + temporal.getClass().getName(), ex);
         }
     }

diff -r 9d3bef2027e0 -r 671161b3333e src/share/classes/java/time/LocalTime.java
--- a/src/share/classes/java/time/LocalTime.java    Thu Sep 26 09:22:34 2013 -0700
+++ b/src/share/classes/java/time/LocalTime.java    Thu Sep 26 15:19:27 2013 -0700
@@ -397,7 +397,8 @@
         Objects.requireNonNull(temporal, "temporal");
         LocalTime time = temporal.query(TemporalQuery.localTime());
         if (time == null) {
-            throw new DateTimeException("Unable to obtain LocalTime from TemporalAccessor: " + temporal.getClass());
+            throw new DateTimeException("Unable to obtain LocalTime from TemporalAccessor: " +
+                    temporal + " of type " + temporal.getClass().getName());
         }
         return time;
     }
diff -r 9d3bef2027e0 -r 671161b3333e src/share/classes/java/time/Month.java
--- a/src/share/classes/java/time/Month.java    Thu Sep 26 09:22:34 2013 -0700
+++ b/src/share/classes/java/time/Month.java    Thu Sep 26 15:19:27 2013 -0700
@@ -217,7 +217,8 @@
             }
             return of(temporal.get(MONTH_OF_YEAR));
         } catch (DateTimeException ex) {
-            throw new DateTimeException("Unable to obtain Month from TemporalAccessor: " + temporal.getClass(), ex);
+            throw new DateTimeException("Unable to obtain Month from TemporalAccessor: " +
+                    temporal + " of type " + temporal.getClass().getName(), ex);
         }
     }

diff -r 9d3bef2027e0 -r 671161b3333e src/share/classes/java/time/MonthDay.java
--- a/src/share/classes/java/time/MonthDay.java Thu Sep 26 09:22:34 2013 -0700
+++ b/src/share/classes/java/time/MonthDay.java Thu Sep 26 15:19:27 2013 -0700
@@ -266,7 +266,8 @@
             }
             return of(temporal.get(MONTH_OF_YEAR), temporal.get(DAY_OF_MONTH));
         } catch (DateTimeException ex) {
-            throw new DateTimeException("Unable to obtain MonthDay from TemporalAccessor: " + temporal.getClass(), ex);
+            throw new DateTimeException("Unable to obtain MonthDay from TemporalAccessor: " +
+                    temporal + " of type " + temporal.getClass().getName(), ex);
         }
     }

diff -r 9d3bef2027e0 -r 671161b3333e src/share/classes/java/time/OffsetDateTime.java
--- a/src/share/classes/java/time/OffsetDateTime.java   Thu Sep 26 09:22:34 2013 -0700
+++ b/src/share/classes/java/time/OffsetDateTime.java   Thu Sep 26 15:19:27 2013 -0700
@@ -347,8 +347,8 @@
         if (temporal instanceof OffsetDateTime) {
             return (OffsetDateTime) temporal;
         }
-        ZoneOffset offset = ZoneOffset.from(temporal);
         try {
+            ZoneOffset offset = ZoneOffset.from(temporal);
             try {
                 LocalDateTime ldt = LocalDateTime.from(temporal);
                 return OffsetDateTime.of(ldt, offset);
@@ -357,7 +357,8 @@
                 return OffsetDateTime.ofInstant(instant, offset);
             }
         } catch (DateTimeException ex) {
-            throw new DateTimeException("Unable to obtain OffsetDateTime from TemporalAccessor: " + temporal.getClass(), ex);
+            throw new DateTimeException("Unable to obtain OffsetDateTime from TemporalAccessor: " +
+                    temporal + " of type " + temporal.getClass().getName(), ex);
         }
     }

diff -r 9d3bef2027e0 -r 671161b3333e src/share/classes/java/time/OffsetTime.java
--- a/src/share/classes/java/time/OffsetTime.java   Thu Sep 26 09:22:34 2013 -0700
+++ b/src/share/classes/java/time/OffsetTime.java   Thu Sep 26 15:19:27 2013 -0700
@@ -284,7 +284,8 @@
             ZoneOffset offset = ZoneOffset.from(temporal);
             return new OffsetTime(time, offset);
         } catch (DateTimeException ex) {
-            throw new DateTimeException("Unable to obtain OffsetTime from TemporalAccessor: " + temporal.getClass(), ex);
+            throw new DateTimeException("Unable to obtain OffsetTime from TemporalAccessor: " +
+                    temporal + " of type " + temporal.getClass().getName(), ex);
         }
     }

diff -r 9d3bef2027e0 -r 671161b3333e src/share/classes/java/time/Year.java
--- a/src/share/classes/java/time/Year.java Thu Sep 26 09:22:34 2013 -0700
+++ b/src/share/classes/java/time/Year.java Thu Sep 26 15:19:27 2013 -0700
@@ -249,7 +249,8 @@
             }
             return of(temporal.get(YEAR));
         } catch (DateTimeException ex) {
-            throw new DateTimeException("Unable to obtain Year from TemporalAccessor: " + temporal.getClass(), ex);
+            throw new DateTimeException("Unable to obtain Year from TemporalAccessor: " +
+                    temporal + " of type " + temporal.getClass().getName(), ex);
         }
     }

diff -r 9d3bef2027e0 -r 671161b3333e src/share/classes/java/time/YearMonth.java
--- a/src/share/classes/java/time/YearMonth.java    Thu Sep 26 09:22:34 2013 -0700
+++ b/src/share/classes/java/time/YearMonth.java    Thu Sep 26 15:19:27 2013 -0700
@@ -252,7 +252,8 @@
             }
             return of(temporal.get(YEAR), temporal.get(MONTH_OF_YEAR));
         } catch (DateTimeException ex) {
-            throw new DateTimeException("Unable to obtain YearMonth from TemporalAccessor: " + temporal.getClass(), ex);
+            throw new DateTimeException("Unable to obtain YearMonth from TemporalAccessor: " +
+                    temporal + " of type " + temporal.getClass().getName(), ex);
         }
     }

diff -r 9d3bef2027e0 -r 671161b3333e src/share/classes/java/time/ZoneId.java
--- a/src/share/classes/java/time/ZoneId.java   Thu Sep 26 09:22:34 2013 -0700
+++ b/src/share/classes/java/time/ZoneId.java   Thu Sep 26 15:19:27 2013 -0700
@@ -503,7 +503,8 @@
     public static ZoneId from(TemporalAccessor temporal) {
         ZoneId obj = temporal.query(TemporalQuery.zone());
         if (obj == null) {
-            throw new DateTimeException("Unable to obtain ZoneId from TemporalAccessor: " + temporal.getClass());
+            throw new DateTimeException("Unable to obtain ZoneId from TemporalAccessor: " +
+                    temporal + " of type " + temporal.getClass().getName());
         }
         return obj;
     }
diff -r 9d3bef2027e0 -r 671161b3333e src/share/classes/java/time/ZoneOffset.java
--- a/src/share/classes/java/time/ZoneOffset.java   Thu Sep 26 09:22:34 2013 -0700
+++ b/src/share/classes/java/time/ZoneOffset.java   Thu Sep 26 15:19:27 2013 -0700
@@ -336,7 +336,8 @@
         Objects.requireNonNull(temporal, "temporal");
         ZoneOffset offset = temporal.query(TemporalQuery.offset());
         if (offset == null) {
-            throw new DateTimeException("Unable to obtain ZoneOffset from TemporalAccessor: " + temporal.getClass());
+            throw new DateTimeException("Unable to obtain ZoneOffset from TemporalAccessor: " +
+                    temporal + " of type " + temporal.getClass().getName());
         }
         return offset;
     }
diff -r 9d3bef2027e0 -r 671161b3333e src/share/classes/java/time/ZonedDateTime.java
--- a/src/share/classes/java/time/ZonedDateTime.java    Thu Sep 26 09:22:34 2013 -0700
+++ b/src/share/classes/java/time/ZonedDateTime.java    Thu Sep 26 15:19:27 2013 -0700
@@ -553,7 +553,8 @@
                 return of(ldt, zone);
             }
         } catch (DateTimeException ex) {
-            throw new DateTimeException("Unable to create ZonedDateTime from TemporalAccessor: " + temporal.getClass(), ex);
+            throw new DateTimeException("Unable to obtain ZonedDateTime from TemporalAccessor: " +
+                    temporal + " of type " + temporal.getClass().getName(), ex);
         }
     }

diff -r 9d3bef2027e0 -r 671161b3333e src/share/classes/java/time/format/Parsed.java
--- a/src/share/classes/java/time/format/Parsed.java    Thu Sep 26 09:22:34 2013 -0700
+++ b/src/share/classes/java/time/format/Parsed.java    Thu Sep 26 15:19:27 2013 -0700
@@ -588,11 +588,23 @@
     //-----------------------------------------------------------------------
     @Override
     public String toString() {
-        String str = fieldValues.toString() + "," + chrono + "," + zone;
+        StringBuilder buf = new StringBuilder(64);
+        buf.append(fieldValues).append(',').append(chrono);
+        if (zone != null) {
+            buf.append(',').append(zone);
+        }
         if (date != null || time != null) {
-            str += " resolved to " + date + "," + time;
+            buf.append(" resolved to ");
+            if (date != null) {
+                buf.append(date);
+                if (time != null) {
+                    buf.append('T').append(time);
+                }
+            } else {
+                buf.append(time);
+            }
         }
-        return str;
+        return buf.toString();
     }

 }
diff -r 9d3bef2027e0 -r 671161b3333e test/java/time/test/java/time/format/TestDateTimeFormatter.java
--- a/test/java/time/test/java/time/format/TestDateTimeFormatter.java   Thu Sep 26 09:22:34 2013 -0700
+++ b/test/java/time/test/java/time/format/TestDateTimeFormatter.java   Thu Sep 26 15:19:27 2013 -0700
@@ -61,12 +61,32 @@

 import static java.time.temporal.ChronoField.DAY_OF_MONTH;
 import static org.testng.Assert.assertSame;
+import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.fail;

-import java.time.format.DecimalStyle;
+import java.time.DateTimeException;
+import java.time.DayOfWeek;
+import java.time.Instant;
+import java.time.LocalDate;
+import java.time.LocalDateTime;
+import java.time.LocalTime;
+import java.time.Month;
+import java.time.MonthDay;
+import java.time.OffsetDateTime;
+import java.time.OffsetTime;
+import java.time.Year;
+import java.time.YearMonth;
+import java.time.ZoneId;
+import java.time.ZoneOffset;
+import java.time.ZonedDateTime;
+import java.time.chrono.ThaiBuddhistChronology;
 import java.time.format.DateTimeFormatter;
 import java.time.format.DateTimeFormatterBuilder;
+import java.time.format.DecimalStyle;
 import java.time.format.SignStyle;
+import java.time.temporal.TemporalAccessor;
 import java.util.Locale;
+import java.util.function.Function;

 import org.testng.annotations.Test;

@@ -87,4 +107,93 @@
         assertSame(test, base);
     }

+    @Test
+    public void test_parse_errorMessage() throws Exception {
+        assertGoodErrorDate(DayOfWeek::from, "DayOfWeek");
+        assertGoodErrorDate(Month::from, "Month");
+        assertGoodErrorDate(YearMonth::from, "YearMonth");
+        assertGoodErrorDate(MonthDay::from, "MonthDay");
+        assertGoodErrorDate(LocalDate::from, "LocalDate");
+        assertGoodErrorDate(LocalTime::from, "LocalTime");
+        assertGoodErrorDate(LocalDateTime::from, "LocalDateTime");
+        assertGoodErrorDate(OffsetTime::from, "OffsetTime");
+        assertGoodErrorDate(OffsetDateTime::from, "OffsetDateTime");
+        assertGoodErrorDate(ZonedDateTime::from, "ZonedDateTime");
+        assertGoodErrorDate(Instant::from, "Instant");
+        assertGoodErrorDate(ZoneOffset::from, "ZoneOffset");
+        assertGoodErrorDate(ZoneId::from, "ZoneId");
+        assertGoodErrorDate(ThaiBuddhistChronology.INSTANCE::date, "");
+
+        assertGoodErrorTime(DayOfWeek::from, "DayOfWeek");
+        assertGoodErrorTime(Month::from, "Month");
+        assertGoodErrorTime(Year::from, "Year");
+        assertGoodErrorTime(YearMonth::from, "YearMonth");
+        assertGoodErrorTime(MonthDay::from, "MonthDay");
+        assertGoodErrorTime(LocalDate::from, "LocalDate");
+        assertGoodErrorTime(LocalTime::from, "LocalTime");
+        assertGoodErrorTime(LocalDateTime::from, "LocalDateTime");
+        assertGoodErrorTime(OffsetTime::from, "OffsetTime");
+        assertGoodErrorTime(OffsetDateTime::from, "OffsetDateTime");
+        assertGoodErrorTime(ZonedDateTime::from, "ZonedDateTime");
+        assertGoodErrorTime(Instant::from, "Instant");
+        assertGoodErrorTime(ZoneOffset::from, "ZoneOffset");
+        assertGoodErrorTime(ZoneId::from, "ZoneId");
+        assertGoodErrorTime(ThaiBuddhistChronology.INSTANCE::date, "");
+    }
+
+    private void assertGoodErrorDate(Function<TemporalAccessor, Object> function, String expectedText) {
+        DateTimeFormatter f = DateTimeFormatter.ofPattern("yyyy-mm-dd");
+        TemporalAccessor temporal = f.parse("2010-06-30");
+        try {
+            function.apply(temporal);
+            fail("Should have failed");
+        } catch (DateTimeException ex) {
+            String msg = ex.getMessage();
+            assertTrue(msg.contains(expectedText), msg);
+            assertTrue(msg.contains("Year"), msg);
+            assertTrue(msg.contains("MinuteOfHour"), msg);
+            assertTrue(msg.contains("DayOfMonth"), msg);
+        }
+    }
+
+    private void assertGoodErrorTime(Function<TemporalAccessor, Object> function, String expectedText) {
+        DateTimeFormatter f = DateTimeFormatter.ofPattern("HH:MM:ss");
+        TemporalAccessor temporal = f.parse("11:30:56");
+        try {
+            function.apply(temporal);
+            fail("Should have failed");
+        } catch (DateTimeException ex) {
+            String msg = ex.getMessage();
+            assertTrue(msg.contains(expectedText), msg);
+            assertTrue(msg.contains("HourOfDay"), msg);
+            assertTrue(msg.contains("MonthOfYear"), msg);
+            assertTrue(msg.contains("SecondOfMinute"), msg);
+        }
+    }
+
+    @Test
+    public void test_parsed_toString_resolvedTime() {
+        DateTimeFormatter f = DateTimeFormatter.ofPattern("HH:mm:ss");
+        TemporalAccessor temporal = f.parse("11:30:56");
+        String msg = temporal.toString();
+        assertTrue(msg.contains("11:30:56"), msg);
+    }
+
+    @Test
+    public void test_parsed_toString_resolvedDate() {
+        DateTimeFormatter f = DateTimeFormatter.ofPattern("yyyy-MM-dd");
+        TemporalAccessor temporal = f.parse("2010-06-30");
+        String msg = temporal.toString();
+        assertTrue(msg.contains("2010-06-30"), msg);
+    }
+
+    @Test
+    public void test_parsed_toString_resolvedDateTime() {
+        DateTimeFormatter f = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss");
+        TemporalAccessor temporal = f.parse("2010-06-30 11:30:56");
+        String msg = temporal.toString();
+        assertTrue(msg.contains("2010-06-30"), msg);
+        assertTrue(msg.contains("11:30:56"), msg);
+    }
+
 }
RogerRiggs commented 10 years ago

The exception message improvements are useful but would not provide specific details that would help the developer. In the original issue (https://bugs.openjdk.java.net/browse/JDK-8025507) the resolver was not able to find enough information to create a time. Can the resolver produce a more useful exception method that can be added as a cause on the exception created by parse.

jodastephen commented 10 years ago

Previously, the error contained no information about the parsed data. Now it contains "cannot extract from" "Year", "MinuteOfHour" and "DayOfMonth" or similar. Thats definitely a step forward.

The resolver cannot throw an exception itself, and any kind of stored exception would essentially be trapping specific use cases , which feels like a kludge.

We did have better errors 2 years ago, but they were a trade off loss in reducing the API size and mechanism of merging.

RogerRiggs commented 10 years ago

good enough then.

RogerRiggs commented 10 years ago

JBS issue https://bugs.openjdk.java.net/browse/JDK-8025718 Better error messages for parsing

jodastephen commented 10 years ago

http://hg.openjdk.java.net/jdk8/tl/jdk/rev/09e2a73aa1dc