From a100ce8321912db85f4c9c029d8bccf3e432d90b Mon Sep 17 00:00:00 2001 From: Dmitry Khalanskiy <52952525+dkhalanskyjb@users.noreply.github.com> Date: Mon, 13 May 2024 13:49:25 +0200 Subject: [PATCH] Fix `Instant.parse` succeeding even when seconds are omitted on the JVM and JS (#370) Fixes #369 --- core/common/src/format/DateTimeComponents.kt | 2 +- core/common/test/InstantTest.kt | 1 + core/commonJs/src/Instant.kt | 34 ++++------------ core/js/test/JsConverterTest.kt | 4 +- core/jvm/src/Instant.kt | 42 +++++++------------- 5 files changed, 25 insertions(+), 58 deletions(-) diff --git a/core/common/src/format/DateTimeComponents.kt b/core/common/src/format/DateTimeComponents.kt index a3fb80945..5d5fd2b1f 100644 --- a/core/common/src/format/DateTimeComponents.kt +++ b/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. diff --git a/core/common/test/InstantTest.kt b/core/common/test/InstantTest.kt index 5310c89e0..6fd784bf5 100644 --- a/core/common/test/InstantTest.kt +++ b/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: diff --git a/core/commonJs/src/Instant.kt b/core/commonJs/src/Instant.kt index cfbd3da2f..af7cc868a 100644 --- a/core/commonJs/src/Instant.kt +++ b/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): 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): 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(), diff --git a/core/js/test/JsConverterTest.kt b/core/js/test/JsConverterTest.kt index fa01de3d4..de5eeb6f3 100644 --- a/core/js/test/JsConverterTest.kt +++ b/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()) } } diff --git a/core/jvm/src/Instant.kt b/core/jvm/src/Instant.kt index 24dfcbe91..ab3474647 100644 --- a/core/jvm/src/Instant.kt +++ b/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): 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): 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) {