From 0eff5d3b0a6287d93a42bc32cac8abf5a6f41db8 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 12 Nov 2024 18:19:00 +0000 Subject: [PATCH 1/2] dataconnect: relax LocalDateSerializer encoding and decoding, and add unit test coverage (#6451) --- .../firebase-dataconnect.gradle.kts | 1 + .../serializers/LocalDateSerializer.kt | 77 ++++- .../firebase/dataconnect/LocalDateUnitTest.kt | 274 ++++++++++++++++++ .../LocalDateSerializerUnitTest.kt | 232 +++++++++++++++ .../dataconnect/testutil/ThreeTenBpUtils.kt | 49 ++++ .../IntWithEvenNumDigitsDistribution.kt | 104 +++++++ .../property/arbitrary/ThreeTenBpArbs.kt | 51 ++++ .../testutil/testutil.gradle.kts | 2 + 8 files changed, 775 insertions(+), 15 deletions(-) create mode 100644 firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/LocalDateUnitTest.kt create mode 100644 firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/serializers/LocalDateSerializerUnitTest.kt create mode 100644 firebase-dataconnect/testutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/ThreeTenBpUtils.kt create mode 100644 firebase-dataconnect/testutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/property/arbitrary/IntWithEvenNumDigitsDistribution.kt create mode 100644 firebase-dataconnect/testutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/property/arbitrary/ThreeTenBpArbs.kt diff --git a/firebase-dataconnect/firebase-dataconnect.gradle.kts b/firebase-dataconnect/firebase-dataconnect.gradle.kts index 58c5d63164a..58e98be8b2a 100644 --- a/firebase-dataconnect/firebase-dataconnect.gradle.kts +++ b/firebase-dataconnect/firebase-dataconnect.gradle.kts @@ -123,6 +123,7 @@ dependencies { testImplementation(libs.kotlinx.datetime) testImplementation(libs.kotlinx.serialization.json) testImplementation(libs.mockk) + testImplementation(libs.testonly.three.ten.abp) testImplementation(libs.robolectric) androidTestImplementation(project(":firebase-dataconnect:androidTestutil")) diff --git a/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/serializers/LocalDateSerializer.kt b/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/serializers/LocalDateSerializer.kt index 0a7d1b8194b..0b699a1a6d3 100644 --- a/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/serializers/LocalDateSerializer.kt +++ b/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/serializers/LocalDateSerializer.kt @@ -36,30 +36,48 @@ public object LocalDateSerializer : KSerializer { PrimitiveSerialDescriptor("com.google.firebase.dataconnect.LocalDate", PrimitiveKind.STRING) override fun serialize(encoder: Encoder, value: LocalDate) { - value.run { - require(year >= 0) { "invalid value: $value (year must be non-negative)" } - require(month >= 0) { "invalid value: $value (month must be non-negative)" } - require(day >= 0) { "invalid value: $value (day must be non-negative)" } - } - val serializedDate = - "${value.year}".padStart(4, '0') + - '-' + - "${value.month}".padStart(2, '0') + - '-' + - "${value.day}".padStart(2, '0') + val serializedDate: String = serializeToString(value) encoder.encodeString(serializedDate) } override fun deserialize(decoder: Decoder): LocalDate { val decodedString = decoder.decodeString() - val matcher = Pattern.compile("^(\\d+)-(\\d+)-(\\d+)$").matcher(decodedString) + return deserializeToLocalDate(decodedString) + } + + private val decodeRegexPattern = Pattern.compile("^(-?\\d+)-(-?\\d+)-(-?\\d+)$") + + private fun deserializeToLocalDate(string: String): LocalDate { + val matcher = decodeRegexPattern.matcher(string) require(matcher.matches()) { - "date \"$decodedString\" does not match regular expression: ${matcher.pattern()}" + "date \"$string\" does not match regular expression: ${matcher.pattern()}" } fun Matcher.groupToIntIgnoringLeadingZeroes(index: Int): Int { - val groupText = group(index)!!.trimStart('0') - return if (groupText.isEmpty()) 0 else groupText.toInt() + val groupText = + group(index) + ?: throw IllegalStateException( + "internal error: group(index) should not be null " + + " (index=$index, string=$string, matcher=$this, error code hp48d53pbb)" + ) + + val isNegative = groupText.firstOrNull() == '-' + + val zeroPaddedString = + if (isNegative) { + groupText.substring(1) + } else { + groupText + } + + val intAbsString = zeroPaddedString.trimStart('0') + val intStringPrefix = if (isNegative) "-" else "" + val intString = intStringPrefix + intAbsString + if (intString.isEmpty()) { + return 0 + } + + return intString.toInt() } val year = matcher.groupToIntIgnoringLeadingZeroes(1) @@ -68,4 +86,33 @@ public object LocalDateSerializer : KSerializer { return LocalDate(year = year, month = month, day = day) } + + private fun serializeToString(localDate: LocalDate): String { + val yearStr = localDate.year.toZeroPaddedString(length = 4) + val monthStr = localDate.month.toZeroPaddedString(length = 2) + val dayStr = localDate.day.toZeroPaddedString(length = 2) + return "$yearStr-$monthStr-$dayStr" + } + + private fun Int.toZeroPaddedString(length: Int): String = buildString { + append(this@toZeroPaddedString) + + val firstChar = + firstOrNull()?.let { + if (it == '-') { + deleteCharAt(0) + it + } else { + null + } + } + + while (this.length < length) { + insert(0, '0') + } + + if (firstChar != null) { + insert(0, firstChar) + } + } } diff --git a/firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/LocalDateUnitTest.kt b/firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/LocalDateUnitTest.kt new file mode 100644 index 00000000000..65d78a2f165 --- /dev/null +++ b/firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/LocalDateUnitTest.kt @@ -0,0 +1,274 @@ +/* + * Copyright 2024 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +@file:OptIn(ExperimentalKotest::class) + +package com.google.firebase.dataconnect + +import com.google.firebase.dataconnect.testutil.property.arbitrary.threeTenBp +import com.google.firebase.dataconnect.testutil.shouldContainWithNonAbuttingText +import com.google.firebase.dataconnect.testutil.toDataConnectLocalDate +import com.google.firebase.dataconnect.testutil.toJavaTimeLocalDate +import com.google.firebase.dataconnect.testutil.toKotlinxDatetimeLocalDate +import io.kotest.assertions.assertSoftly +import io.kotest.assertions.withClue +import io.kotest.common.ExperimentalKotest +import io.kotest.matchers.shouldBe +import io.kotest.matchers.shouldNotBe +import io.kotest.matchers.string.shouldEndWith +import io.kotest.matchers.string.shouldStartWith +import io.kotest.property.Arb +import io.kotest.property.PropTestConfig +import io.kotest.property.arbitrary.int +import io.kotest.property.arbitrary.of +import io.kotest.property.assume +import io.kotest.property.checkAll +import kotlinx.coroutines.test.runTest +import kotlinx.datetime.number +import org.junit.Test + +@Suppress("ReplaceCallWithBinaryOperator") +class LocalDateUnitTest { + + @Test + fun `constructor() should set properties to corresponding arguments`() = runTest { + checkAll(propTestConfig, Arb.int(), Arb.int(), Arb.int()) { year, month, day -> + val localDate = LocalDate(year = year, month = month, day = day) + assertSoftly { + withClue("year") { localDate.year shouldBe year } + withClue("month") { localDate.month shouldBe month } + withClue("day") { localDate.day shouldBe day } + } + } + } + + @Test + fun `equals() should return true when invoked with itself`() = runTest { + checkAll(propTestConfig, Arb.int(), Arb.int(), Arb.int()) { year, month, day -> + val localDate = LocalDate(year = year, month = month, day = day) + localDate.equals(localDate) shouldBe true + } + } + + @Test + fun `equals() should return true when invoked with a distinct, but equal, instance`() = runTest { + checkAll(propTestConfig, Arb.int(), Arb.int(), Arb.int()) { year, month, day -> + val localDate1 = LocalDate(year = year, month = month, day = day) + val localDate2 = LocalDate(year = year, month = month, day = day) + localDate1.equals(localDate2) shouldBe true + } + } + + @Test + fun `equals() should return false when invoked with null`() = runTest { + checkAll(propTestConfig, Arb.int(), Arb.int(), Arb.int()) { year, month, day -> + val localDate = LocalDate(year = year, month = month, day = day) + localDate.equals(null) shouldBe false + } + } + + @Test + fun `equals() should return false when invoked with a different type`() = runTest { + val others = Arb.of("foo", 42, java.time.LocalDate.now()) + checkAll(propTestConfig, Arb.int(), Arb.int(), Arb.int(), others) { year, month, day, other -> + val localDate = LocalDate(year = year, month = month, day = day) + localDate.equals(other) shouldBe false + } + } + + @Test + fun `equals() should return false when only the year differs`() = runTest { + checkAll(propTestConfig, Arb.int(), Arb.int(), Arb.int(), Arb.int()) { year1, month, day, year2 + -> + assume(year1 != year2) + val localDate1 = LocalDate(year = year1, month = month, day = day) + val localDate2 = LocalDate(year = year2, month = month, day = day) + localDate1.equals(localDate2) shouldBe false + } + } + + @Test + fun `equals() should return false when only the month differs`() = runTest { + checkAll(propTestConfig, Arb.int(), Arb.int(), Arb.int(), Arb.int()) { year, month1, day, month2 + -> + assume(month1 != month2) + val localDate1 = LocalDate(year = year, month = month1, day = day) + val localDate2 = LocalDate(year = year, month = month2, day = day) + localDate1.equals(localDate2) shouldBe false + } + } + + @Test + fun `equals() should return false when only the day differs`() = runTest { + checkAll(propTestConfig, Arb.int(), Arb.int(), Arb.int(), Arb.int()) { year, month, day1, day2 + -> + assume(day1 != day2) + val localDate1 = LocalDate(year = year, month = month, day = day1) + val localDate2 = LocalDate(year = year, month = month, day = day2) + localDate1.equals(localDate2) shouldBe false + } + } + + @Test + fun `hashCode() should return the same value when invoked repeatedly`() = runTest { + checkAll(propTestConfig, Arb.int(), Arb.int(), Arb.int()) { year, month, day -> + val localDate = LocalDate(year = year, month = month, day = day) + val hashCode = localDate.hashCode() + repeat(5) { withClue("iteration=$it") { localDate.hashCode() shouldBe hashCode } } + } + } + + @Test + fun `hashCode() should return the same value when invoked on equal, but distinct, objects`() = + runTest { + checkAll(propTestConfig, Arb.int(), Arb.int(), Arb.int()) { year, month, day -> + val localDate1 = LocalDate(year = year, month = month, day = day) + val localDate2 = LocalDate(year = year, month = month, day = day) + localDate1.hashCode() shouldBe localDate2.hashCode() + } + } + + @Test + fun `hashCode() should return different values for different years`() = runTest { + checkAll(propTestConfig, Arb.int(), Arb.int(), Arb.int(), Arb.int()) { year1, month, day, year2 + -> + assume(year1.hashCode() != year2.hashCode()) + val localDate1 = LocalDate(year = year1, month = month, day = day) + val localDate2 = LocalDate(year = year2, month = month, day = day) + localDate1.hashCode() shouldNotBe localDate2.hashCode() + } + } + + @Test + fun `hashCode() should return different values for different months`() = runTest { + checkAll(propTestConfig, Arb.int(), Arb.int(), Arb.int(), Arb.int()) { year, month1, day, month2 + -> + assume(month1.hashCode() != month2.hashCode()) + val localDate1 = LocalDate(year = year, month = month1, day = day) + val localDate2 = LocalDate(year = year, month = month2, day = day) + localDate1.hashCode() shouldNotBe localDate2.hashCode() + } + } + + @Test + fun `hashCode() should return different values for different days`() = runTest { + checkAll(propTestConfig, Arb.int(), Arb.int(), Arb.int(), Arb.int()) { year, month, day1, day2 + -> + assume(day1.hashCode() != day2.hashCode()) + val localDate1 = LocalDate(year = year, month = month, day = day1) + val localDate2 = LocalDate(year = year, month = month, day = day2) + localDate1.hashCode() shouldNotBe localDate2.hashCode() + } + } + + @Test + fun `toString() should return a string conforming to what is expected`() = runTest { + checkAll(propTestConfig, Arb.int(), Arb.int(), Arb.int()) { year, month, day -> + val localDate = LocalDate(year = year, month = month, day = day) + val toStringResult = localDate.toString() + assertSoftly { + toStringResult shouldStartWith "LocalDate(" + toStringResult shouldEndWith ")" + toStringResult shouldContainWithNonAbuttingText "year=$year" + toStringResult shouldContainWithNonAbuttingText "month=$month" + toStringResult shouldContainWithNonAbuttingText "day=$day" + } + } + } + + @Test + fun `copy() with no arguments should return an equal, but distinct, instance`() = runTest { + checkAll(propTestConfig, Arb.int(), Arb.int(), Arb.int()) { year, month, day -> + val localDate1 = LocalDate(year = year, month = month, day = day) + val localDate2 = localDate1.copy() + localDate1 shouldBe localDate2 + } + } + + @Test + fun `copy() with all arguments should return a new instance with the given arguments`() = + runTest { + checkAll(propTestConfig, Arb.int(), Arb.int(), Arb.int(), Arb.int(), Arb.int(), Arb.int()) { + year1, + month1, + day1, + year2, + month2, + day2 -> + val localDate1 = LocalDate(year = year1, month = month1, day = day1) + val localDate2 = localDate1.copy(year = year2, month = month2, day = day2) + localDate2 shouldBe LocalDate(year = year2, month = month2, day = day2) + } + } + + @Test + fun `toJavaLocalDate() should return an equivalent java time LocalDate object`() = runTest { + checkAll(propTestConfig, Arb.threeTenBp.localDate()) { testData -> + val fdcLocalDate: LocalDate = testData.toDataConnectLocalDate() + val jLocalDate: java.time.LocalDate = fdcLocalDate.toJavaLocalDate() + assertSoftly { + withClue("year") { jLocalDate.year shouldBe testData.year } + withClue("month") { jLocalDate.month.number shouldBe testData.monthValue } + withClue("dayOfMonth") { jLocalDate.dayOfMonth shouldBe testData.dayOfMonth } + } + } + } + + @Test + fun `toKotlinxLocalDate() should return an equivalent java time LocalDate object`() = runTest { + checkAll(propTestConfig, Arb.threeTenBp.localDate()) { testData -> + val fdcLocalDate: LocalDate = testData.toDataConnectLocalDate() + val kLocalDate: kotlinx.datetime.LocalDate = fdcLocalDate.toKotlinxLocalDate() + assertSoftly { + withClue("year") { kLocalDate.year shouldBe testData.year } + withClue("month") { kLocalDate.month.number shouldBe testData.monthValue } + withClue("dayOfMonth") { kLocalDate.dayOfMonth shouldBe testData.dayOfMonth } + } + } + } + + @Test + fun `toDataConnectLocalDate() on java time LocalDate should return an equivalent LocalDate object`() = + runTest { + checkAll(propTestConfig, Arb.threeTenBp.localDate()) { testData -> + val jLocalDate: java.time.LocalDate = testData.toJavaTimeLocalDate() + val fdcLocalDate: LocalDate = jLocalDate.toDataConnectLocalDate() + assertSoftly { + withClue("year") { fdcLocalDate.year shouldBe testData.year } + withClue("month") { fdcLocalDate.month shouldBe testData.monthValue } + withClue("day") { fdcLocalDate.day shouldBe testData.dayOfMonth } + } + } + } + + @Test + fun `toDataConnectLocalDate() on kotlinx datetime LocalDate should return an equivalent LocalDate object`() = + runTest { + checkAll(propTestConfig, Arb.threeTenBp.localDate()) { testData -> + val kLocalDate: kotlinx.datetime.LocalDate = testData.toKotlinxDatetimeLocalDate() + val fdcLocalDate: LocalDate = kLocalDate.toDataConnectLocalDate() + assertSoftly { + withClue("year") { fdcLocalDate.year shouldBe testData.year } + withClue("month") { fdcLocalDate.month shouldBe testData.monthValue } + withClue("day") { fdcLocalDate.day shouldBe testData.dayOfMonth } + } + } + } + + private companion object { + val propTestConfig = PropTestConfig(iterations = 50) + } +} diff --git a/firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/serializers/LocalDateSerializerUnitTest.kt b/firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/serializers/LocalDateSerializerUnitTest.kt new file mode 100644 index 00000000000..6d94b9ca477 --- /dev/null +++ b/firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/serializers/LocalDateSerializerUnitTest.kt @@ -0,0 +1,232 @@ +/* + * Copyright 2024 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +@file:OptIn(ExperimentalKotest::class) + +package com.google.firebase.dataconnect.serializers + +import com.google.firebase.dataconnect.LocalDate +import com.google.firebase.dataconnect.testutil.property.arbitrary.intWithEvenNumDigitsDistribution +import com.google.firebase.dataconnect.util.ProtoUtil.decodeFromValue +import com.google.firebase.dataconnect.util.ProtoUtil.encodeToValue +import com.google.firebase.dataconnect.util.ProtoUtil.toValueProto +import io.kotest.assertions.throwables.shouldThrow +import io.kotest.common.ExperimentalKotest +import io.kotest.matchers.shouldBe +import io.kotest.property.Arb +import io.kotest.property.EdgeConfig +import io.kotest.property.PropTestConfig +import io.kotest.property.arbitrary.Codepoint +import io.kotest.property.arbitrary.arabic +import io.kotest.property.arbitrary.arbitrary +import io.kotest.property.arbitrary.ascii +import io.kotest.property.arbitrary.boolean +import io.kotest.property.arbitrary.booleanArray +import io.kotest.property.arbitrary.constant +import io.kotest.property.arbitrary.cyrillic +import io.kotest.property.arbitrary.egyptianHieroglyphs +import io.kotest.property.arbitrary.enum +import io.kotest.property.arbitrary.filterNot +import io.kotest.property.arbitrary.greekCoptic +import io.kotest.property.arbitrary.int +import io.kotest.property.arbitrary.katakana +import io.kotest.property.arbitrary.long +import io.kotest.property.arbitrary.merge +import io.kotest.property.arbitrary.next +import io.kotest.property.arbitrary.string +import io.kotest.property.arbitrary.triple +import io.kotest.property.arbitrary.withEdgecases +import io.kotest.property.checkAll +import io.mockk.every +import io.mockk.mockk +import kotlin.random.nextInt +import kotlinx.coroutines.test.runTest +import kotlinx.serialization.encoding.Decoder +import org.junit.Test + +class LocalDateSerializerUnitTest { + + @Test + fun `serialize() should produce the expected serialized string`() = runTest { + checkAll(propTestConfig, Arb.localDate()) { localDate -> + val value = encodeToValue(localDate, LocalDateSerializer, serializersModule = null) + value.stringValue shouldBe localDate.toYYYYMMDDWithZeroPadding() + } + } + + @Test + fun `deserialize() should produce the expected LocalDate object`() = runTest { + val numPaddingCharsArb = Arb.int(0..10) + val arb = Arb.triple(numPaddingCharsArb, numPaddingCharsArb, numPaddingCharsArb) + checkAll(propTestConfig, Arb.localDate(), arb) { localDate, paddingCharsTriple -> + val (yearPadding, monthPadding, dayPadding) = paddingCharsTriple + val value = + localDate + .toYYYYMMDDWithZeroPadding( + yearPadding = yearPadding, + monthPadding = monthPadding, + dayPadding = dayPadding + ) + .toValueProto() + + val decodedLocalDate = decodeFromValue(value, LocalDateSerializer, serializersModule = null) + decodedLocalDate shouldBe localDate + } + } + + @Test + fun `deserialize() should throw IllegalArgumentException when given unparseable strings`() = + runTest { + checkAll(propTestConfig, Arb.unparseableDate()) { encodedDate -> + val decoder: Decoder = mockk { every { decodeString() } returns encodedDate } + shouldThrow { LocalDateSerializer.deserialize(decoder) } + } + } + + private companion object { + val propTestConfig = + PropTestConfig( + iterations = 500, + edgeConfig = EdgeConfig(edgecasesGenerationProbability = 0.2) + ) + + fun LocalDate.toYYYYMMDDWithZeroPadding( + yearPadding: Int = 4, + monthPadding: Int = 2, + dayPadding: Int = 2, + ): String { + val yearString = year.toZeroPaddedString(yearPadding) + val monthString = month.toZeroPaddedString(monthPadding) + val dayString = day.toZeroPaddedString(dayPadding) + return "$yearString-$monthString-$dayString" + } + + fun Int.toZeroPaddedString(length: Int): String = buildString { + append(this@toZeroPaddedString) + val signChar = + firstOrNull()?.let { + if (it == '-') { + deleteCharAt(0) + it + } else { + null + } + } + + while (this.length < length) { + insert(0, '0') + } + + if (signChar !== null) { + insert(0, signChar) + } + } + + fun Arb.Companion.localDate( + year: Arb = intWithEvenNumDigitsDistribution(), + month: Arb = intWithEvenNumDigitsDistribution(), + day: Arb = intWithEvenNumDigitsDistribution(), + ): Arb { + return arbitrary( + edgecaseFn = { rs -> + val yearInt = if (rs.random.nextBoolean()) year.next(rs) else year.edgecase(rs)!! + val monthInt = if (rs.random.nextBoolean()) month.next(rs) else month.edgecase(rs)!! + val dayInt = if (rs.random.nextBoolean()) day.next(rs) else day.edgecase(rs)!! + LocalDate(year = yearInt, month = monthInt, day = dayInt) + }, + sampleFn = { LocalDate(year = year.bind(), month = month.bind(), day = day.bind()) } + ) + } + + private enum class UnparseableNumberReason { + EmptyString, + InvalidChars, + GreaterThanIntMax, + LessThanIntMin, + } + + private val codepoints = + Codepoint.ascii() + .merge(Codepoint.egyptianHieroglyphs()) + .merge(Codepoint.arabic()) + .merge(Codepoint.cyrillic()) + .merge(Codepoint.greekCoptic()) + .merge(Codepoint.katakana()) + + fun Arb.Companion.unparseableNumber(): Arb { + val reasonArb = enum() + val validIntArb = intWithEvenNumDigitsDistribution(0..Int.MAX_VALUE) + val validChars = listOf('0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '-').map { it.code } + val invalidString = + string(1..5, codepoints.filterNot { validChars.contains(it.value) }).withEdgecases("-") + val tooLargeValues = long(Int.MAX_VALUE.toLong() + 1L..Long.MAX_VALUE) + val tooSmallValues = long(Long.MIN_VALUE until Int.MIN_VALUE.toLong()) + return arbitrary { rs -> + when (reasonArb.bind()) { + UnparseableNumberReason.EmptyString -> "" + UnparseableNumberReason.GreaterThanIntMax -> "${tooLargeValues.bind()}" + UnparseableNumberReason.LessThanIntMin -> "${tooSmallValues.bind()}" + UnparseableNumberReason.InvalidChars -> { + val flags = Array(3) { rs.random.nextBoolean() } + if (!flags[0]) { + flags[2] = true + } + val prefix = if (flags[0]) invalidString.bind() else "" + val mid = if (flags[1]) validIntArb.bind() else "" + val suffix = if (flags[2]) invalidString.bind() else "" + "$prefix$mid$suffix" + } + } + } + } + + fun Arb.Companion.unparseableDash(): Arb { + val invalidString = string(1..5, codepoints.filterNot { it.value == '-'.code }) + return arbitrary { rs -> + val flags = Array(3) { rs.random.nextBoolean() } + if (!flags[0]) { + flags[2] = true + } + + val prefix = if (flags[0]) invalidString.bind() else "" + val mid = if (flags[1]) "-" else "" + val suffix = if (flags[2]) invalidString.bind() else "" + + "$prefix$mid$suffix" + } + } + + fun Arb.Companion.unparseableDate(): Arb { + val validNumber = intWithEvenNumDigitsDistribution(0..Int.MAX_VALUE) + val unparseableNumber = unparseableNumber() + val unparseableDash = unparseableDash() + val booleanArray = booleanArray(Arb.constant(5), Arb.boolean()) + return arbitrary(edgecases = listOf("", "-", "--", "---")) { rs -> + val invalidCharFlags = booleanArray.bind() + if (invalidCharFlags.count { it } == 0) { + invalidCharFlags[rs.random.nextInt(invalidCharFlags.indices)] = true + } + + val year = if (invalidCharFlags[0]) unparseableNumber.bind() else validNumber.bind() + val dash1 = if (invalidCharFlags[1]) unparseableDash.bind() else "-" + val month = if (invalidCharFlags[2]) unparseableNumber.bind() else validNumber.bind() + val dash2 = if (invalidCharFlags[3]) unparseableDash.bind() else "-" + val day = if (invalidCharFlags[4]) unparseableNumber.bind() else validNumber.bind() + + "$year$dash1$month$dash2$day" + } + } + } +} diff --git a/firebase-dataconnect/testutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/ThreeTenBpUtils.kt b/firebase-dataconnect/testutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/ThreeTenBpUtils.kt new file mode 100644 index 00000000000..77d83f1cf2d --- /dev/null +++ b/firebase-dataconnect/testutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/ThreeTenBpUtils.kt @@ -0,0 +1,49 @@ +/* + * Copyright 2024 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.firebase.dataconnect.testutil + +import android.annotation.SuppressLint +import org.threeten.bp.LocalDate +import org.threeten.bp.Month +import org.threeten.bp.Year + +fun Month.lengthInYear(year: Year): Int = length(year.isLeap) + +fun Month.dayRangeInYear(year: Year): IntRange = 1..lengthInYear(year) + +fun LocalDate.toDataConnectLocalDate(): com.google.firebase.dataconnect.LocalDate = + com.google.firebase.dataconnect.LocalDate( + year = year, + month = monthValue, + day = dayOfMonth, + ) + +@SuppressLint("NewApi") +fun LocalDate.toJavaTimeLocalDate(): java.time.LocalDate = + java.time.LocalDate.of( + year, + monthValue, + dayOfMonth, + ) + +@SuppressLint("NewApi") +fun LocalDate.toKotlinxDatetimeLocalDate(): kotlinx.datetime.LocalDate = + kotlinx.datetime.LocalDate( + year, + monthValue, + dayOfMonth, + ) diff --git a/firebase-dataconnect/testutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/property/arbitrary/IntWithEvenNumDigitsDistribution.kt b/firebase-dataconnect/testutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/property/arbitrary/IntWithEvenNumDigitsDistribution.kt new file mode 100644 index 00000000000..4ba810bdd03 --- /dev/null +++ b/firebase-dataconnect/testutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/property/arbitrary/IntWithEvenNumDigitsDistribution.kt @@ -0,0 +1,104 @@ +/* + * Copyright 2024 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +@file:Suppress("UnusedReceiverParameter") + +package com.google.firebase.dataconnect.testutil.property.arbitrary + +import com.google.common.primitives.Ints.min +import io.kotest.property.Arb +import io.kotest.property.arbitrary.choice +import io.kotest.property.arbitrary.int + +/** + * Returns an [Arb] identical to [Arb.Companion.int] except that the values it produces have an + * equal probability of having any given number of digits in its base-10 string representation. This + * is useful for testing int values that get zero padded when they are small. The negative sign is + * _not_ included in the "number of digits" count. + * + * @param range The range of values to produce; if `null` (the default) then use the entire range of + * integers (i.e. `Int.MIN_VALUE..Int.MAX_VALUE`). + * + * @see intWithEvenNumDigitsDistribution + */ +@JvmName("intWithEvenNumDigitsDistributionNonNullRange") +fun Arb.Companion.intWithEvenNumDigitsDistribution(range: IntRange): Arb { + require(!range.isEmpty()) { "range must not be empty: $range (error code tmvy8ysdjy)" } + val intRangesByNumDigits = mutableMapOf>() + + var first = range.first + while (first <= range.last) { + val numDigits = "$first".trimStart('-').length + val numDigitsKey = if (first >= 0) numDigits else (-numDigits) + val numDigitsRange = rangeByNumDigits[numDigitsKey] + checkNotNull(numDigitsRange) { + "internal error: rangeByNumDigits[numDigitsKey] returned null " + + "(first=$first, numDigitsKey=$numDigitsKey, rangeByNumDigits=$rangeByNumDigits, " + + "error code 3z37g9zfy8)" + } + + val last = min(range.last, numDigitsRange.last) + val curIntRangesByNumDigits = intRangesByNumDigits.getOrPut(numDigits) { mutableListOf() } + curIntRangesByNumDigits.add(first..last) + if (last == Int.MAX_VALUE) { + break + } + first = last + 1 + } + + val arbLists: List>> = + intRangesByNumDigits.values.map { intRanges -> intRanges.map { intRange -> Arb.int(intRange) } } + val arbs: List> = arbLists.map { if (it.size == 1) it.single() else Arb.choice(it) } + return Arb.choice(arbs) +} + +/** + * Returns an [Arb] identical to [Arb.Companion.int] except that the values it produces have an + * equal probability of having any given number of digits in its base-10 string representation. This + * is useful for testing int values that get zero padded when they are small. The negative sign is + * _not_ included in the "number of digits" count. + * + * @param range The range of values to produce; if `null` (the default) then use the entire range of + * integers (i.e. `Int.MIN_VALUE..Int.MAX_VALUE`). + * + * @see intWithEvenNumDigitsDistribution + */ +@JvmName("intWithEvenNumDigitsDistributionNullableRange") +fun Arb.Companion.intWithEvenNumDigitsDistribution(range: IntRange? = null): Arb = + intWithEvenNumDigitsDistribution(range ?: Int.MIN_VALUE..Int.MAX_VALUE) + +private val rangeByNumDigits: Map = buildMap { + put(1, 0..9) + put(2, 10..99) + put(3, 100..999) + put(4, 1_000..9_999) + put(5, 10_000..99_999) + put(6, 100_000..999_999) + put(7, 1_000_000..9_999_999) + put(8, 10_000_000..99_999_999) + put(9, 100_000_000..999_999_999) + put(10, 1_000_000_000..Int.MAX_VALUE) + put(-1, -9..-1) + put(-2, -99..-10) + put(-3, -999..-100) + put(-4, -9_999..-1_000) + put(-5, -99_999..-10_000) + put(-6, -999_999..-100_000) + put(-7, -9_999_999..-1_000_000) + put(-8, -99_999_999..-10_000_000) + put(-9, -999_999_999..-100_000_000) + put(-10, Int.MIN_VALUE..-1_000_000_000) +} diff --git a/firebase-dataconnect/testutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/property/arbitrary/ThreeTenBpArbs.kt b/firebase-dataconnect/testutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/property/arbitrary/ThreeTenBpArbs.kt new file mode 100644 index 00000000000..016808496ca --- /dev/null +++ b/firebase-dataconnect/testutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/property/arbitrary/ThreeTenBpArbs.kt @@ -0,0 +1,51 @@ +/* + * Copyright 2024 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +@file:Suppress("UnusedReceiverParameter") + +package com.google.firebase.dataconnect.testutil.property.arbitrary + +import com.google.firebase.dataconnect.testutil.dayRangeInYear +import io.kotest.property.Arb +import io.kotest.property.arbitrary.enum +import io.kotest.property.arbitrary.flatMap +import io.kotest.property.arbitrary.map +import org.threeten.bp.LocalDate +import org.threeten.bp.Month +import org.threeten.bp.Year + +val Arb.Companion.threeTenBp: ThreeTenBpArbs + get() = ThreeTenBpArbs + +@Suppress("MemberVisibilityCanBePrivate", "unused") +object ThreeTenBpArbs { + fun year(intArb: Arb = yearInt()): Arb = intArb.map(Year::of) + + fun yearInt(): Arb = Arb.intWithEvenNumDigitsDistribution(Year.MIN_VALUE..Year.MAX_VALUE) + + fun month(): Arb = Arb.enum() + + fun monthInt(monthArb: Arb = month()): Arb = monthArb.map(Month::getValue) + + fun localDate(yearArb: Arb = year(), monthArb: Arb = month()): Arb = + yearArb.flatMap { year -> + monthArb.flatMap { month -> + Arb.intWithEvenNumDigitsDistribution(month.dayRangeInYear(year)).map { day -> + LocalDate.of(year.value, month, day) + } + } + } +} diff --git a/firebase-dataconnect/testutil/testutil.gradle.kts b/firebase-dataconnect/testutil/testutil.gradle.kts index 9e03b210684..5f0629c2e87 100644 --- a/firebase-dataconnect/testutil/testutil.gradle.kts +++ b/firebase-dataconnect/testutil/testutil.gradle.kts @@ -54,6 +54,8 @@ dependencies { implementation("com.google.firebase:firebase-components:18.0.0") implementation("com.google.firebase:firebase-auth:22.3.1") + compileOnly(libs.kotlinx.datetime) + implementation(libs.androidx.test.junit) implementation(libs.kotest.assertions) implementation(libs.kotest.property) From 4e2dcd0b7518c5fb84d01036c2b196f3c1363473 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 12 Nov 2024 18:19:15 +0000 Subject: [PATCH 2/2] dataconnect: DataConnectCredentialsTokenManager: initialize synchronously to fix race condition (#6448) --- .../dataconnect/AuthIntegrationTest.kt | 2 + .../dataconnect/core/DataConnectAppCheck.kt | 16 +- .../dataconnect/core/DataConnectAuth.kt | 12 +- .../DataConnectCredentialsTokenManager.kt | 196 +++++------------- .../core/FirebaseDataConnectImpl.kt | 13 +- .../core/DataConnectAuthUnitTest.kt | 59 +----- 6 files changed, 80 insertions(+), 218 deletions(-) diff --git a/firebase-dataconnect/src/androidTest/kotlin/com/google/firebase/dataconnect/AuthIntegrationTest.kt b/firebase-dataconnect/src/androidTest/kotlin/com/google/firebase/dataconnect/AuthIntegrationTest.kt index e4dd0ad20ad..ba314d0f9db 100644 --- a/firebase-dataconnect/src/androidTest/kotlin/com/google/firebase/dataconnect/AuthIntegrationTest.kt +++ b/firebase-dataconnect/src/androidTest/kotlin/com/google/firebase/dataconnect/AuthIntegrationTest.kt @@ -17,6 +17,7 @@ package com.google.firebase.dataconnect import com.google.firebase.auth.FirebaseAuth +import com.google.firebase.dataconnect.core.FirebaseDataConnectInternal import com.google.firebase.dataconnect.testutil.DataConnectBackend import com.google.firebase.dataconnect.testutil.DataConnectIntegrationTestBase import com.google.firebase.dataconnect.testutil.InProcessDataConnectGrpcServer @@ -201,6 +202,7 @@ class AuthIntegrationTest : DataConnectIntegrationTestBase() { } private suspend fun signIn() { + (personSchema.dataConnect as FirebaseDataConnectInternal).awaitAuthReady() val authResult = auth.run { signInAnonymously().await() } withClue("authResult.user returned from signInAnonymously()") { authResult.user.shouldNotBeNull() diff --git a/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectAppCheck.kt b/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectAppCheck.kt index 176aba53832..abe60656f73 100644 --- a/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectAppCheck.kt +++ b/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectAppCheck.kt @@ -32,24 +32,20 @@ internal class DataConnectAppCheck( blockingDispatcher: CoroutineDispatcher, logger: Logger, ) : - DataConnectCredentialsTokenManager( + DataConnectCredentialsTokenManager( deferredProvider = deferredAppCheckTokenProvider, parentCoroutineScope = parentCoroutineScope, blockingDispatcher = blockingDispatcher, logger = logger, ) { - override fun newTokenListener(): AppCheckTokenListener = AppCheckTokenListenerImpl(logger) + private val appCheckTokenListener = AppCheckTokenListenerImpl(logger) @DeferredApi - override fun addTokenListener( - provider: InteropAppCheckTokenProvider, - listener: AppCheckTokenListener - ) = provider.addAppCheckTokenListener(listener) + override fun addTokenListener(provider: InteropAppCheckTokenProvider) = + provider.addAppCheckTokenListener(appCheckTokenListener) - override fun removeTokenListener( - provider: InteropAppCheckTokenProvider, - listener: AppCheckTokenListener - ) = provider.removeAppCheckTokenListener(listener) + override fun removeTokenListener(provider: InteropAppCheckTokenProvider) = + provider.removeAppCheckTokenListener(appCheckTokenListener) override suspend fun getToken(provider: InteropAppCheckTokenProvider, forceRefresh: Boolean) = provider.getToken(forceRefresh).await().let { GetTokenResult(it.token) } diff --git a/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectAuth.kt b/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectAuth.kt index 1efd00af83e..a3a27ccc31c 100644 --- a/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectAuth.kt +++ b/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectAuth.kt @@ -32,20 +32,20 @@ internal class DataConnectAuth( blockingDispatcher: CoroutineDispatcher, logger: Logger, ) : - DataConnectCredentialsTokenManager( + DataConnectCredentialsTokenManager( deferredProvider = deferredAuthProvider, parentCoroutineScope = parentCoroutineScope, blockingDispatcher = blockingDispatcher, logger = logger, ) { - override fun newTokenListener(): IdTokenListener = IdTokenListenerImpl(logger) + private val idTokenListener = IdTokenListenerImpl(logger) @DeferredApi - override fun addTokenListener(provider: InternalAuthProvider, listener: IdTokenListener) = - provider.addIdTokenListener(listener) + override fun addTokenListener(provider: InternalAuthProvider) = + provider.addIdTokenListener(idTokenListener) - override fun removeTokenListener(provider: InternalAuthProvider, listener: IdTokenListener) = - provider.removeIdTokenListener(listener) + override fun removeTokenListener(provider: InternalAuthProvider) = + provider.removeIdTokenListener(idTokenListener) override suspend fun getToken(provider: InternalAuthProvider, forceRefresh: Boolean) = provider.getAccessToken(forceRefresh).await().let { GetTokenResult(it.token) } diff --git a/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectCredentialsTokenManager.kt b/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectCredentialsTokenManager.kt index 61b0f9bd769..e6f4049c359 100644 --- a/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectCredentialsTokenManager.kt +++ b/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectCredentialsTokenManager.kt @@ -52,10 +52,10 @@ import kotlinx.coroutines.launch import kotlinx.coroutines.yield /** Base class that shares logic for managing the Auth token and AppCheck token. */ -internal sealed class DataConnectCredentialsTokenManager( +internal sealed class DataConnectCredentialsTokenManager( private val deferredProvider: com.google.firebase.inject.Deferred, parentCoroutineScope: CoroutineScope, - private val blockingDispatcher: CoroutineDispatcher, + blockingDispatcher: CoroutineDispatcher, protected val logger: Logger, ) { val instanceId: String @@ -78,24 +78,26 @@ internal sealed class DataConnectCredentialsTokenManager( } ) - private interface ProviderListenerPair { - val provider: T? - val tokenListener: L + init { + // Call `whenAvailable()` on a non-main thread because it accesses SharedPreferences, which + // performs disk i/o, violating the StrictMode policy android.os.strictmode.DiskReadViolation. + val coroutineName = CoroutineName("k6rwgqg9gh $instanceId whenAvailable") + coroutineScope.launch(coroutineName + blockingDispatcher) { + deferredProvider.whenAvailable(DeferredProviderHandlerImpl(weakThis)) + } } - private sealed interface State { + private interface ProviderProvider { + val provider: T? + } - /** State indicating that [initialize] has not yet been invoked. */ - object Uninitialized : State + private sealed interface State { /** State indicating that [close] has been invoked. */ - object Closed : State + object Closed : State - /** - * State indicating that [initialize] has been invoked and there is no outstanding "get token" - * request. - */ - class Ready( + /** State indicating that there is no outstanding "get token" request. */ + class Idle( /** * The [InternalAuthProvider] or [InteropAppCheckTokenProvider]; may be null if the deferred @@ -103,18 +105,12 @@ internal sealed class DataConnectCredentialsTokenManager( */ override val provider: T?, - /** The token listener that is, or will be, registered with [provider]. */ - override val tokenListener: L, - /** The value to specify for `forceRefresh` on the next invocation of [getToken]. */ val forceTokenRefresh: Boolean - ) : State, ProviderListenerPair + ) : State, ProviderProvider - /** - * State indicating that [initialize] has been invoked and there _is_ an outstanding "get token" - * request. - */ - class Active( + /** State indicating that there _is_ an outstanding "get token" request. */ + class Active( /** * The [InternalAuthProvider] or [InteropAppCheckTokenProvider] that is performing the "get @@ -122,12 +118,9 @@ internal sealed class DataConnectCredentialsTokenManager( */ override val provider: T, - /** The token listener that is registered with [provider]. */ - override val tokenListener: L, - /** The job that is performing the "get token" request. */ val job: Deferred>> - ) : State, ProviderListenerPair + ) : State, ProviderProvider } /** @@ -135,30 +128,22 @@ internal sealed class DataConnectCredentialsTokenManager( * in order to be thread-safe. Such a loop should call `yield()` on each iteration to allow other * coroutines to run on the thread. */ - private val state = AtomicReference>(State.Uninitialized) - - /** - * Creates and returns a new "token listener" that will be registered with the provider returned - * from the [deferredProvider] specified to the constructor. - * - * @see addTokenListener - * @see removeTokenListener - */ - protected abstract fun newTokenListener(): L + private val state = + AtomicReference>(State.Idle(provider = null, forceTokenRefresh = false)) /** - * Adds the given listener to the given provider. + * Adds the token listener to the given provider. * * @see removeTokenListener */ - @DeferredApi protected abstract fun addTokenListener(provider: T, listener: L) + @DeferredApi protected abstract fun addTokenListener(provider: T) /** - * Removes the given listener from the given provider. + * Removes the token listener from the given provider. * * @see addTokenListener */ - protected abstract fun removeTokenListener(provider: T, listener: L) + protected abstract fun removeTokenListener(provider: T) /** * Starts an asynchronous task to get a new access token from the given provider, forcing a token @@ -166,45 +151,6 @@ internal sealed class DataConnectCredentialsTokenManager( */ protected abstract suspend fun getToken(provider: T, forceRefresh: Boolean): GetTokenResult - /** - * Initializes this object, acquiring resources and registering necessary listeners. - * - * This method must be called exactly once before any other methods on this object. The only - * exception is that [close] may be invoked without having invoked this method. - * - * @throws IllegalStateException if invoked more than once or after [close]. - * - * @see close - */ - fun initialize() { - val newState = - State.Ready(provider = null, tokenListener = newTokenListener(), forceTokenRefresh = false) - - while (true) { - val oldState = state.get() - if (oldState != State.Uninitialized) { - throw IllegalStateException( - if (oldState == State.Closed) { - "initialize() may not be called after close()" - } else { - "initialize() has already been called" - } - ) - } - - if (state.compareAndSet(oldState, newState)) { - break - } - } - - // Call `whenAvailable()` on a non-main thread because it accesses SharedPreferences, which - // performs disk i/o, violating the StrictMode policy android.os.strictmode.DiskReadViolation. - val coroutineName = CoroutineName("k6rwgqg9gh $instanceId whenAvailable") - coroutineScope.launch(coroutineName + blockingDispatcher) { - deferredProvider.whenAvailable(DeferredProviderHandlerImpl(weakThis, newState.tokenListener)) - } - } - /** * Closes this object, releasing its resources, unregistering any registered listeners, and * cancelling any in-flight token requests. @@ -226,19 +172,16 @@ internal sealed class DataConnectCredentialsTokenManager( private fun setClosedState() { while (true) { val oldState = state.get() - val providerListenerPair: ProviderListenerPair? = + val providerProvider: ProviderProvider = when (oldState) { is State.Closed -> return - is State.Uninitialized -> null - is State.Ready -> oldState + is State.Idle -> oldState is State.Active -> oldState } if (state.compareAndSet(oldState, State.Closed)) { - providerListenerPair?.run { - provider?.let { provider -> removeTokenListener(provider, tokenListener) } - } - return + providerProvider.provider?.let { removeTokenListener(it) } + break } } } @@ -247,19 +190,15 @@ internal sealed class DataConnectCredentialsTokenManager( * Sets a flag to force-refresh the token upon the next call to [getToken]. * * If [close] has been called, this method does nothing. - * - * @throws IllegalStateException if [initialize] has not been called. */ suspend fun forceRefresh() { logger.debug { "forceRefresh()" } while (true) { val oldState = state.get() - val providerListenerPair: ProviderListenerPair = + val oldStateProviderProvider = when (oldState) { - is State.Uninitialized -> - throw IllegalStateException("forceRefresh() cannot be called before initialize()") is State.Closed -> return - is State.Ready -> oldState + is State.Idle -> oldState is State.Active -> { val message = "needs token refresh (wgrwbrvjxt)" oldState.job.cancel(message, ForceRefresh(message)) @@ -267,12 +206,7 @@ internal sealed class DataConnectCredentialsTokenManager( } } - val newState = - State.Ready( - providerListenerPair.provider, - providerListenerPair.tokenListener, - forceTokenRefresh = true - ) + val newState = State.Idle(oldStateProviderProvider.provider, forceTokenRefresh = true) if (state.compareAndSet(oldState, newState)) { break } @@ -284,9 +218,8 @@ internal sealed class DataConnectCredentialsTokenManager( private fun newActiveState( invocationId: String, provider: T, - tokenListener: L, forceRefresh: Boolean - ): State.Active { + ): State.Active { val coroutineName = CoroutineName( "$instanceId 535gmcvv5a $invocationId getToken(" + @@ -299,15 +232,14 @@ internal sealed class DataConnectCredentialsTokenManager( val result = runCatching { getToken(provider, forceRefresh) } SequencedReference(sequenceNumber, result) } - return State.Active(provider, tokenListener, job) + return State.Active(provider, job) } /** * Gets the access token, force-refreshing it if [forceRefresh] has been called. * - * @throws IllegalStateException if [initialize] has not been called. - * @throws DataConnectException if [close] has not been called or is called while the operation is - * in progress. + * @throws DataConnectException if [close] has been called or is called while the operation is in + * progress. */ suspend fun getToken(requestId: String): String? { val invocationId = "gat" + Random.nextAlphanumericString(length = 8) @@ -316,10 +248,8 @@ internal sealed class DataConnectCredentialsTokenManager( val attemptSequenceNumber = nextSequenceNumber() val oldState = state.get() - val newState: State.Active = + val newState: State.Active = when (oldState) { - is State.Uninitialized -> - throw IllegalStateException("getToken() cannot be called before initialize()") is State.Closed -> { logger.debug { "$invocationId getToken() throws CredentialsTokenManagerClosedException" + @@ -327,20 +257,14 @@ internal sealed class DataConnectCredentialsTokenManager( } throw CredentialsTokenManagerClosedException(this) } - is State.Ready -> { + is State.Idle -> { if (oldState.provider === null) { logger.debug { - "$invocationId getToken() returns null" + - " (token provider is not (yet?) available)" + "$invocationId getToken() returns null (token provider is not (yet?) available)" } return null } - newActiveState( - invocationId, - oldState.provider, - oldState.tokenListener, - oldState.forceTokenRefresh - ) + newActiveState(invocationId, oldState.provider, oldState.forceTokenRefresh) } is State.Active -> { if ( @@ -348,12 +272,7 @@ internal sealed class DataConnectCredentialsTokenManager( !oldState.job.isCancelled && oldState.job.await().sequenceNumber < attemptSequenceNumber ) { - newActiveState( - invocationId, - oldState.provider, - oldState.tokenListener, - forceRefresh = false - ) + newActiveState(invocationId, oldState.provider, forceRefresh = false) } else { oldState } @@ -390,7 +309,7 @@ internal sealed class DataConnectCredentialsTokenManager( continue } else if (exception is FirebaseNoSignedInUserException) { logger.debug { - "$invocationId getToken() returns null" + " (FirebaseAuth reports no signed-in user)" + "$invocationId getToken() returns null (FirebaseAuth reports no signed-in user)" } return null } else if (exception is CancellationException) { @@ -418,33 +337,28 @@ internal sealed class DataConnectCredentialsTokenManager( private class NewProvider(message: String) : GetTokenRetry(message) @DeferredApi - private fun onProviderAvailable(newProvider: T, tokenListener: L) { + private fun onProviderAvailable(newProvider: T) { logger.debug { "onProviderAvailable(newProvider=$newProvider)" } - addTokenListener(newProvider, tokenListener) + addTokenListener(newProvider) while (true) { val oldState = state.get() val newState = when (oldState) { - is State.Uninitialized -> - throw IllegalStateException( - "INTERNAL ERROR: onProviderAvailable() called before initialize()" - ) is State.Closed -> { logger.debug { "onProviderAvailable(newProvider=$newProvider)" + " unregistering token listener that was just added" } - removeTokenListener(newProvider, tokenListener) + removeTokenListener(newProvider) break } - is State.Ready -> - State.Ready(newProvider, oldState.tokenListener, oldState.forceTokenRefresh) + is State.Idle -> State.Idle(newProvider, oldState.forceTokenRefresh) is State.Active -> { val newProviderClassName = newProvider::class.qualifiedName val message = "a new provider $newProviderClassName is available (symhxtmazy)" oldState.job.cancel(message, NewProvider(message)) - State.Ready(newProvider, tokenListener, forceTokenRefresh = false) + State.Idle(newProvider, forceTokenRefresh = false) } } @@ -464,25 +378,23 @@ internal sealed class DataConnectCredentialsTokenManager( * strong reference to the [DataConnectCredentialsTokenManager] instance indefinitely, in the case * that the callback never occurs. */ - private class DeferredProviderHandlerImpl( - private val weakCredentialsTokenManagerRef: - WeakReference>, - private val tokenListener: L, + private class DeferredProviderHandlerImpl( + private val weakCredentialsTokenManagerRef: WeakReference> ) : DeferredHandler { override fun handle(provider: Provider) { - weakCredentialsTokenManagerRef.get()?.onProviderAvailable(provider.get(), tokenListener) + weakCredentialsTokenManagerRef.get()?.onProviderAvailable(provider.get()) } } private class CredentialsTokenManagerClosedException( - tokenProvider: DataConnectCredentialsTokenManager<*, *> + tokenProvider: DataConnectCredentialsTokenManager<*> ) : DataConnectException( - "DataConnectCredentialsTokenManager ${tokenProvider.instanceId} was closed" + "DataConnectCredentialsTokenManager ${tokenProvider.instanceId} was closed (code cqrbq4zfvy)" ) private class GetTokenCancelledException(cause: Throwable) : - DataConnectException("getToken() was cancelled, likely by close()", cause) + DataConnectException("getToken() was cancelled, likely by close() (code rqdd4jam9d)", cause) protected data class GetTokenResult(val token: String?) diff --git a/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/FirebaseDataConnectImpl.kt b/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/FirebaseDataConnectImpl.kt index b04b6aa6ead..d9bee50b89d 100644 --- a/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/FirebaseDataConnectImpl.kt +++ b/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/FirebaseDataConnectImpl.kt @@ -86,9 +86,8 @@ internal class FirebaseDataConnectImpl( override val config: ConnectorConfig, override val blockingExecutor: Executor, override val nonBlockingExecutor: Executor, - private val deferredAuthProvider: com.google.firebase.inject.Deferred, - private val deferredAppCheckProvider: - com.google.firebase.inject.Deferred, + deferredAuthProvider: com.google.firebase.inject.Deferred, + deferredAppCheckProvider: com.google.firebase.inject.Deferred, private val creator: FirebaseDataConnectFactory, override val settings: DataConnectSettings, ) : FirebaseDataConnectInternal { @@ -146,8 +145,8 @@ internal class FirebaseDataConnectImpl( } init { - coroutineScope.launch(CoroutineName("DataConnectAuth initializer for $instanceId")) { - dataConnectAuth.initialize() + val name = CoroutineName("DataConnectAuth isProviderAvailable pipe for $instanceId") + coroutineScope.launch(name) { dataConnectAuth.providerAvailable.collect { isProviderAvailable -> logger.debug { "authProviderAvailable=$isProviderAvailable" } authProviderAvailable.value = isProviderAvailable @@ -168,8 +167,8 @@ internal class FirebaseDataConnectImpl( } init { - coroutineScope.launch(CoroutineName("DataConnectAppCheck initializer for $instanceId")) { - dataConnectAppCheck.initialize() + val name = CoroutineName("DataConnectAppCheck isProviderAvailable pipe for $instanceId") + coroutineScope.launch(name) { dataConnectAppCheck.providerAvailable.collect { isProviderAvailable -> logger.debug { "appCheckProviderAvailable=$isProviderAvailable" } appCheckProviderAvailable.value = isProviderAvailable diff --git a/firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/core/DataConnectAuthUnitTest.kt b/firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/core/DataConnectAuthUnitTest.kt index b45a943a1f4..a30121a707f 100644 --- a/firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/core/DataConnectAuthUnitTest.kt +++ b/firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/core/DataConnectAuthUnitTest.kt @@ -32,6 +32,7 @@ import com.google.firebase.dataconnect.testutil.UnavailableDeferred import com.google.firebase.dataconnect.testutil.newBackgroundScopeThatAdvancesLikeForeground import com.google.firebase.dataconnect.testutil.newMockLogger import com.google.firebase.dataconnect.testutil.property.arbitrary.dataConnect +import com.google.firebase.dataconnect.testutil.shouldContainWithNonAbuttingTextIgnoringCase import com.google.firebase.dataconnect.testutil.shouldHaveLoggedAtLeastOneMessageContaining import com.google.firebase.dataconnect.testutil.shouldHaveLoggedExactlyOneMessageContaining import com.google.firebase.dataconnect.testutil.shouldNotHaveLoggedAnyMessagesContaining @@ -47,7 +48,6 @@ import io.kotest.matchers.collections.shouldContain import io.kotest.matchers.collections.shouldContainExactly import io.kotest.matchers.nulls.shouldBeNull import io.kotest.matchers.shouldBe -import io.kotest.matchers.throwable.shouldHaveMessage import io.kotest.matchers.types.shouldBeSameInstanceAs import io.kotest.property.Arb import io.kotest.property.RandomSource @@ -93,23 +93,14 @@ class DataConnectAuthUnitTest { private val mockLogger = newMockLogger("ecvqkga56c") @Test - fun `close() should succeed if called _before_ initialize()`() = runTest { + fun `close() should succeed if called on a brand new instance()`() = runTest { val dataConnectAuth = newDataConnectAuth() dataConnectAuth.close() } - @Test - fun `close() should succeed if called _after_ initialize()`() = runTest { - val dataConnectAuth = newDataConnectAuth() - dataConnectAuth.initialize() - - dataConnectAuth.close() - } - @Test fun `close() should log a message`() = runTest { val dataConnectAuth = newDataConnectAuth() - dataConnectAuth.initialize() dataConnectAuth.close() @@ -119,7 +110,6 @@ class DataConnectAuthUnitTest { @Test fun `close() should cancel in-flight requests to get a token`() = runTest { val dataConnectAuth = newDataConnectAuth() - dataConnectAuth.initialize() advanceUntilIdle() coEvery { mockInternalAuthProvider.getAccessToken(any()) } coAnswers @@ -133,7 +123,8 @@ class DataConnectAuthUnitTest { val exception = shouldThrow { dataConnectAuth.getToken(requestId) } - exception shouldHaveMessage "getToken() was cancelled, likely by close()" + exception.message shouldContainWithNonAbuttingTextIgnoringCase "getToken() was cancelled" + exception.message shouldContainWithNonAbuttingTextIgnoringCase "likely by close()" mockLogger.shouldHaveLoggedExactlyOneMessageContaining(requestId) mockLogger.shouldHaveLoggedExactlyOneMessageContaining("throws GetTokenCancelledException") } @@ -141,7 +132,6 @@ class DataConnectAuthUnitTest { @Test fun `close() should remove the IdTokenListener`() = runTest { val dataConnectAuth = newDataConnectAuth() - dataConnectAuth.initialize() advanceUntilIdle() val idTokenListenerSlot = slot() @@ -156,7 +146,6 @@ class DataConnectAuthUnitTest { @Test fun `close() should be callable multiple times, from multiple threads`() = runTest { val dataConnectAuth = newDataConnectAuth() - dataConnectAuth.initialize() advanceUntilIdle() val latch = SuspendingCountDownLatch(100) @@ -175,15 +164,6 @@ class DataConnectAuthUnitTest { jobs.forEach { it.await() } } - @Test - fun `forceRefresh() should throw if invoked before initialize()`() = runTest { - val dataConnectAuth = newDataConnectAuth() - - val exception = shouldThrow { dataConnectAuth.forceRefresh() } - - exception shouldHaveMessage "forceRefresh() cannot be called before initialize()" - } - @Test fun `forceRefresh() should do nothing if invoked after close()`() = runTest { val dataConnectAuth = newDataConnectAuth() @@ -195,7 +175,6 @@ class DataConnectAuthUnitTest { @Test fun `getToken() should return null if InternalAuthProvider is not available`() = runTest { val dataConnectAuth = newDataConnectAuth(deferredInternalAuthProvider = UnavailableDeferred()) - dataConnectAuth.initialize() advanceUntilIdle() val result = dataConnectAuth.getToken(requestId) @@ -206,15 +185,6 @@ class DataConnectAuthUnitTest { mockLogger.shouldHaveLoggedExactlyOneMessageContaining("token provider is not (yet?) available") } - @Test - fun `getToken() should throw if invoked before initialize()`() = runTest { - val dataConnectAuth = newDataConnectAuth() - - val exception = shouldThrow { dataConnectAuth.getToken(requestId) } - - exception shouldHaveMessage "getToken() cannot be called before initialize()" - } - @Test fun `getToken() should throw if invoked after close()`() = runTest { val dataConnectAuth = newDataConnectAuth() @@ -222,7 +192,7 @@ class DataConnectAuthUnitTest { val exception = shouldThrow { dataConnectAuth.getToken(requestId) } - exception shouldHaveMessage + exception.message shouldContainWithNonAbuttingTextIgnoringCase "DataConnectCredentialsTokenManager ${dataConnectAuth.instanceId} was closed" mockLogger.shouldHaveLoggedExactlyOneMessageContaining(requestId) mockLogger.shouldHaveLoggedExactlyOneMessageContaining( @@ -234,7 +204,6 @@ class DataConnectAuthUnitTest { @Test fun `getToken() should return null if no user is signed in`() = runTest { val dataConnectAuth = newDataConnectAuth() - dataConnectAuth.initialize() advanceUntilIdle() coEvery { mockInternalAuthProvider.getAccessToken(any()) } returns Tasks.forException(FirebaseNoSignedInUserException("j8rkghbcnz")) @@ -250,7 +219,6 @@ class DataConnectAuthUnitTest { @Test fun `getToken() should return the token returned from FirebaseAuth`() = runTest { val dataConnectAuth = newDataConnectAuth() - dataConnectAuth.initialize() advanceUntilIdle() coEvery { mockInternalAuthProvider.getAccessToken(any()) } returns taskForToken(accessToken) @@ -271,7 +239,6 @@ class DataConnectAuthUnitTest { val exception = TestException("xqtbckcn6w") val dataConnectAuth = newDataConnectAuth() - dataConnectAuth.initialize() advanceUntilIdle() coEvery { mockInternalAuthProvider.getAccessToken(any()) } returns Tasks.forException(exception) @@ -287,13 +254,12 @@ class DataConnectAuthUnitTest { } @Test - fun `getToken() should return re-throw the exception thrown by InternalAuthProvider getAccessToken()`() = + fun `getToken() should re-throw the exception thrown by InternalAuthProvider getAccessToken()`() = runTest { class TestException(message: String) : Exception(message) val exception = TestException("s4c4xr9z4p") val dataConnectAuth = newDataConnectAuth() - dataConnectAuth.initialize() advanceUntilIdle() coEvery { mockInternalAuthProvider.getAccessToken(any()) } answers { throw exception } @@ -310,7 +276,6 @@ class DataConnectAuthUnitTest { @Test fun `getToken() should force refresh the access token after calling forceRefresh()`() = runTest { val dataConnectAuth = newDataConnectAuth() - dataConnectAuth.initialize() advanceUntilIdle() coEvery { mockInternalAuthProvider.getAccessToken(any()) } returns taskForToken(accessToken) @@ -332,7 +297,6 @@ class DataConnectAuthUnitTest { fun `getToken() should NOT force refresh the access token without calling forceRefresh()`() = runTest { val dataConnectAuth = newDataConnectAuth() - dataConnectAuth.initialize() advanceUntilIdle() coEvery { mockInternalAuthProvider.getAccessToken(any()) } returns taskForToken(accessToken) @@ -347,7 +311,6 @@ class DataConnectAuthUnitTest { fun `getToken() should NOT force refresh the access token after it is force refreshed`() = runTest { val dataConnectAuth = newDataConnectAuth() - dataConnectAuth.initialize() advanceUntilIdle() coEvery { mockInternalAuthProvider.getAccessToken(any()) } returns taskForToken(accessToken) @@ -365,7 +328,6 @@ class DataConnectAuthUnitTest { @Test fun `getToken() should ask for a token from FirebaseAuth on every invocation`() = runTest { val dataConnectAuth = newDataConnectAuth() - dataConnectAuth.initialize() advanceUntilIdle() val tokens = CopyOnWriteArrayList() coEvery { mockInternalAuthProvider.getAccessToken(any()) } answers @@ -381,7 +343,6 @@ class DataConnectAuthUnitTest { @Test fun `getToken() should conflate concurrent requests`() = runTest { val dataConnectAuth = newDataConnectAuth() - dataConnectAuth.initialize() advanceUntilIdle() val tokens = CopyOnWriteArrayList() coEvery { mockInternalAuthProvider.getAccessToken(any()) } answers @@ -411,7 +372,6 @@ class DataConnectAuthUnitTest { @Test fun `getToken() should re-fetch token if invalidated concurrently`() = runTest { val dataConnectAuth = newDataConnectAuth() - dataConnectAuth.initialize() advanceUntilIdle() val invocationCount = AtomicInteger(0) val tokens = CopyOnWriteArrayList().apply { add(accessToken) } @@ -446,7 +406,6 @@ class DataConnectAuthUnitTest { @Test fun `getToken() should ignore results with lower sequence number`() = runTest { val dataConnectAuth = newDataConnectAuth() - dataConnectAuth.initialize() advanceUntilIdle() val invocationCount = AtomicInteger(0) val tokens = CopyOnWriteArrayList() @@ -488,7 +447,6 @@ class DataConnectAuthUnitTest { } val dataConnectAuth = newDataConnectAuth(deferredInternalAuthProvider = deferredInternalAuthProvider) - dataConnectAuth.initialize() advanceUntilIdle() val result = dataConnectAuth.getToken(requestId) @@ -509,7 +467,6 @@ class DataConnectAuthUnitTest { val deferredInternalAuthProvider: DeferredInternalAuthProvider = mockk(relaxed = true) val dataConnectAuth = newDataConnectAuth(deferredInternalAuthProvider = deferredInternalAuthProvider) - dataConnectAuth.initialize() advanceUntilIdle() dataConnectAuth.close() val deferredInternalAuthProviderHandlerSlot = slot>() @@ -531,7 +488,6 @@ class DataConnectAuthUnitTest { val deferredInternalAuthProvider = DelayedDeferred(mockInternalAuthProvider) val dataConnectAuth = newDataConnectAuth(deferredInternalAuthProvider = deferredInternalAuthProvider) - dataConnectAuth.initialize() advanceUntilIdle() every { mockInternalAuthProvider.addIdTokenListener(any()) } answers { @@ -571,9 +527,6 @@ class DataConnectAuthUnitTest { interval = 100.milliseconds } - val firebaseAppDeletedException - get() = java.lang.IllegalStateException("FirebaseApp was deleted") - fun taskForToken(token: String?): Task = Tasks.forResult(mockk(relaxed = true) { every { getToken() } returns token }) }