Fix Instant.parse succeeding even when seconds are omitted on the JVM and JS (#370)

Fixes #369

Author: dkhalanskyjb

core/common/src/format/DateTimeComponents.kt

@@ -67,7 +67,7 @@ public class DateTimeComponents internal constructor(internal val contents: Date
          * ISO 8601 extended format for dates and times with UTC offset.
          *
          * For specifying the time zone offset, the format uses the [UtcOffset.Formats.ISO] format, except that during
-         * parsing, specifying the minutes of the offset is optional.
+         * parsing, specifying the minutes of the offset is optional (so offsets like `+03` are also allowed).
          *
          * This format differs from [LocalTime.Formats.ISO] in its time part in that
          * specifying the seconds is *not* optional.

core/common/test/InstantTest.kt

@@ -86,6 +86,7 @@ class InstantTest {
 
         assertInvalidFormat { Instant.parse("1970-01-01T23:59:60Z")}
         assertInvalidFormat { Instant.parse("1970-01-01T24:00:00Z")}
+        assertInvalidFormat { Instant.parse("1970-01-01T23:59Z")}
         assertInvalidFormat { Instant.parse("x") }
         assertInvalidFormat { Instant.parse("12020-12-31T23:59:59.000000000Z") }
         // this string represents an Instant that is currently larger than Instant.MAX any of the implementations:

core/commonJs/src/Instant.kt

@@ -7,7 +7,6 @@ package kotlinx.datetime
 
 import kotlinx.datetime.format.*
 import kotlinx.datetime.internal.JSJoda.Instant as jtInstant
-import kotlinx.datetime.internal.JSJoda.OffsetDateTime as jtOffsetDateTime
 import kotlinx.datetime.internal.JSJoda.Duration as jtDuration
 import kotlinx.datetime.internal.JSJoda.Clock as jtClock
 import kotlinx.datetime.internal.JSJoda.ChronoUnit as jtChronoUnit
@@ -74,36 +73,17 @@ public actual class Instant internal constructor(internal val value: jtInstant)
             if (epochMilliseconds > 0) MAX else MIN
         }
 
-        public actual fun parse(input: CharSequence, format: DateTimeFormat<DateTimeComponents>): Instant =
-            if (format === DateTimeComponents.Formats.ISO_DATE_TIME_OFFSET) {
-                try {
-                    Instant(jsTry { jtOffsetDateTime.parse(fixOffsetRepresentation(input.toString())) }.toInstant())
-                } catch (e: Throwable) {
-                    if (e.isJodaDateTimeParseException()) throw DateTimeFormatException(e)
-                    throw e
-                }
-            } else {
-                try {
-                    format.parse(input).toInstantUsingOffset()
-                } catch (e: IllegalArgumentException) {
-                    throw DateTimeFormatException("Failed to parse an instant from '$input'", e)
-                }
-            }
+        // TODO: implement a custom parser to 1) help DCE get rid of the formatting machinery 2) move Instant to stdlib
+        public actual fun parse(input: CharSequence, format: DateTimeFormat<DateTimeComponents>): Instant = try {
+            // This format is not supported properly by Joda-Time, so we can't delegate to it.
+            format.parse(input).toInstantUsingOffset()
+        } catch (e: IllegalArgumentException) {
+            throw DateTimeFormatException("Failed to parse an instant from '$input'", e)
+        }
 
         @Deprecated("This overload is only kept for binary compatibility", level = DeprecationLevel.HIDDEN)
         public fun parse(isoString: String): Instant = parse(input = isoString)
 
-        /** A workaround for the string representations of Instant that have an offset of the form
-         * "+XX" not being recognized by [jtOffsetDateTime.parse], while "+XX:XX" work fine. */
-        private fun fixOffsetRepresentation(isoString: String): String {
-            val time = isoString.indexOf('T', ignoreCase = true)
-            if (time == -1) return isoString // the string is malformed
-            val offset = isoString.indexOfLast { c -> c == '+' || c == '-' }
-            if (offset < time) return isoString // the offset is 'Z' and not +/- something else
-            val separator = isoString.indexOf(':', offset) // if there is a ':' in the offset, no changes needed
-            return if (separator != -1) isoString else "$isoString:00"
-        }
-
         public actual fun fromEpochSeconds(epochSeconds: Long, nanosecondAdjustment: Long): Instant = try {
             /* Performing normalization here because otherwise this fails:
                assertEquals((Long.MAX_VALUE % 1_000_000_000).toInt(),

core/js/test/JsConverterTest.kt

@@ -12,7 +12,7 @@ import kotlin.test.*
 class JsConverterTest {
     @Test
     fun toJSDateTest() {
-        val releaseInstant = "2016-02-15T00:00Z".toInstant()
+        val releaseInstant = Instant.parse("2016-02-15T00:00:00Z")
         val releaseDate = releaseInstant.toJSDate()
         assertEquals(2016, releaseDate.getUTCFullYear())
         assertEquals(1, releaseDate.getUTCMonth())
@@ -23,7 +23,7 @@ class JsConverterTest {
     fun toInstantTest() {
         val kotlinReleaseEpochMilliseconds = 1455494400000
         val releaseDate = Date(milliseconds = kotlinReleaseEpochMilliseconds)
-        val releaseInstant = "2016-02-15T00:00Z".toInstant()
+        val releaseInstant = Instant.parse("2016-02-15T00:00:00Z")
         assertEquals(releaseInstant, releaseDate.toKotlinInstant())
     }
 }

core/jvm/src/Instant.kt

@@ -12,13 +12,11 @@ import kotlinx.datetime.internal.*
 import kotlinx.datetime.serializers.InstantIso8601Serializer
 import kotlinx.serialization.Serializable
 import java.time.DateTimeException
-import java.time.format.DateTimeParseException
-import java.time.temporal.ChronoUnit
+import java.time.temporal.*
 import kotlin.time.*
 import kotlin.time.Duration.Companion.nanoseconds
 import kotlin.time.Duration.Companion.seconds
 import java.time.Instant as jtInstant
-import java.time.OffsetDateTime as jtOffsetDateTime
 import java.time.Clock as jtClock
 
 @Serializable(with = InstantIso8601Serializer::class)
@@ -67,35 +65,23 @@ public actual class Instant internal constructor(internal val value: jtInstant)
         public actual fun fromEpochMilliseconds(epochMilliseconds: Long): Instant =
                 Instant(jtInstant.ofEpochMilli(epochMilliseconds))
 
-        public actual fun parse(input: CharSequence, format: DateTimeFormat<DateTimeComponents>): Instant =
-            if (format === DateTimeComponents.Formats.ISO_DATE_TIME_OFFSET) {
-                try {
-                    Instant(jtOffsetDateTime.parse(fixOffsetRepresentation(input)).toInstant())
-                } catch (e: DateTimeParseException) {
-                    throw DateTimeFormatException(e)
-                }
-            } else {
-                try {
-                    format.parse(input).toInstantUsingOffset()
-                } catch (e: IllegalArgumentException) {
-                    throw DateTimeFormatException("Failed to parse an instant from '$input'", e)
-                }
-            }
+        // TODO: implement a custom parser to 1) help DCE get rid of the formatting machinery 2) move Instant to stdlib
+        public actual fun parse(input: CharSequence, format: DateTimeFormat<DateTimeComponents>): Instant = try {
+            /**
+             * Can't use built-in Java Time's handling of `Instant.parse` because it supports 24:00:00 and
+             * 23:59:60, and also doesn't support non-`Z` UTC offsets on older JDKs.
+             * Can't use custom Java Time's formats because Java 8 doesn't support the UTC offset format with
+             * optional minutes and seconds and `:` between them:
+             * https://docs.oracle.com/javase/8/docs/api/java/time/format/DateTimeFormatterBuilder.html#appendOffset-java.lang.String-java.lang.String-
+             */
+            format.parse(input).toInstantUsingOffset()
+        } catch (e: IllegalArgumentException) {
+            throw DateTimeFormatException("Failed to parse an instant from '$input'", e)
+        }
 
         @Deprecated("This overload is only kept for binary compatibility", level = DeprecationLevel.HIDDEN)
         public fun parse(isoString: String): Instant = parse(input = isoString)
 
-        /** A workaround for a quirk of the JDKs older than 11 where the string representations of Instant that have an
-         * offset of the form "+XX" are not recognized by [jtOffsetDateTime.parse], while "+XX:XX" work fine. */
-        private fun fixOffsetRepresentation(isoString: CharSequence): CharSequence {
-            val time = isoString.indexOf('T', ignoreCase = true)
-            if (time == -1) return isoString // the string is malformed
-            val offset = isoString.indexOfLast { c -> c == '+' || c == '-' }
-            if (offset < time) return isoString // the offset is 'Z' and not +/- something else
-            val separator = isoString.indexOf(':', offset) // if there is a ':' in the offset, no changes needed
-            return if (separator != -1) isoString else "$isoString:00"
-        }
-
         public actual fun fromEpochSeconds(epochSeconds: Long, nanosecondAdjustment: Long): Instant = try {
             Instant(jtInstant.ofEpochSecond(epochSeconds, nanosecondAdjustment))
         } catch (e: Exception) {