From 2162e03e1f5d626a3aee52c4a58764c964afd22e Mon Sep 17 00:00:00 2001 From: Nik Clayton Date: Wed, 21 Feb 2024 23:36:24 +0100 Subject: [PATCH] fix: Handle JSON enums with unknown values (#462) Previous code expected all incoming enums values to map directly to Kotlin enum constants. This is a problem for servers with additional features -- e.g., "reaction" as a notification type. Fix this with a new Moshi adapter that will set the incoming value to a given constant if it's not recognised. Apply this to the enum constants in core.network to ensure they are handled. Clean up enum handling in Converters.kt, ComposeViewModel.kt, and Status.kt by using the existing `.ordinal` property and some extension functions for idiomatic code. Fixes #461 --- .../components/compose/ComposeViewModel.kt | 6 +- .../core/common/extensions/EnumExtensions.kt | 54 +++++++ .../app/pachli/core/database/Converters.kt | 4 +- .../pachli/core/network/di/NetworkModule.kt | 2 + .../pachli/core/network/json/HasDefault.kt | 133 ++++++++++++++++++ .../pachli/core/network/model/Attachment.kt | 4 + .../app/pachli/core/network/model/Filter.kt | 13 +- .../pachli/core/network/model/Notification.kt | 18 +-- .../app/pachli/core/network/model/Status.kt | 32 ++--- .../pachli/core/network/model/TrendsLink.kt | 6 + .../core/network/json/HasDefaultTest.kt | 109 ++++++++++++++ 11 files changed, 346 insertions(+), 35 deletions(-) create mode 100644 core/common/src/main/kotlin/app/pachli/core/common/extensions/EnumExtensions.kt create mode 100644 core/network/src/main/kotlin/app/pachli/core/network/json/HasDefault.kt create mode 100644 core/network/src/test/kotlin/app/pachli/core/network/json/HasDefaultTest.kt diff --git a/app/src/main/java/app/pachli/components/compose/ComposeViewModel.kt b/app/src/main/java/app/pachli/components/compose/ComposeViewModel.kt index e134b9d44..4d6680c8f 100644 --- a/app/src/main/java/app/pachli/components/compose/ComposeViewModel.kt +++ b/app/src/main/java/app/pachli/components/compose/ComposeViewModel.kt @@ -427,8 +427,8 @@ class ComposeViewModel @Inject constructor( val preferredVisibility = accountManager.activeAccount!!.defaultPostPrivacy val replyVisibility = composeOptions?.replyVisibility ?: Status.Visibility.UNKNOWN - startingVisibility = Status.Visibility.byNum( - preferredVisibility.num.coerceAtLeast(replyVisibility.num), + startingVisibility = Status.Visibility.getOrUnknown( + preferredVisibility.ordinal.coerceAtLeast(replyVisibility.ordinal), ) inReplyToId = composeOptions?.inReplyToId @@ -471,7 +471,7 @@ class ComposeViewModel @Inject constructor( postLanguage = composeOptions?.language val tootVisibility = composeOptions?.visibility ?: Status.Visibility.UNKNOWN - if (tootVisibility.num != Status.Visibility.UNKNOWN.num) { + if (tootVisibility != Status.Visibility.UNKNOWN) { startingVisibility = tootVisibility } statusVisibility.value = startingVisibility diff --git a/core/common/src/main/kotlin/app/pachli/core/common/extensions/EnumExtensions.kt b/core/common/src/main/kotlin/app/pachli/core/common/extensions/EnumExtensions.kt new file mode 100644 index 000000000..0786de171 --- /dev/null +++ b/core/common/src/main/kotlin/app/pachli/core/common/extensions/EnumExtensions.kt @@ -0,0 +1,54 @@ +/* + * Copyright 2024 Pachli Association + * + * This file is a part of Pachli. + * + * This program is free software; you can redistribute it and/or modify it under the terms of the + * GNU General Public License as published by the Free Software Foundation; either version 3 of the + * License, or (at your option) any later version. + * + * Pachli is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even + * the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General + * Public License for more details. + * + * You should have received a copy of the GNU General Public License along with Pachli; if not, + * see . + */ + +package app.pachli.core.common.extensions + +// Note: Technically these don't have to be extension methods on +// Enum.Companion. You could remove that and write: +// +// get(0) +// +// But the bare "get" (getOrElse, getOrNull) in calling code strikes +// me as a little weird. Installing these on Enum.Companion makes +// the calling code: +// +// Enum.get(0) +// +// which is a little more legible. + +/** + * Returns the enum constant with the given [ordinal] value. + * + * If the ordinal is out of bounds of this enum, throws + * an IndexOutOfBoundsException except in Kotlin/JS where the behavior is unspecified. + * + * @see [kotlin.Array.get] + */ +inline fun > Enum.Companion.get(ordinal: Int) = enumValues()[ordinal] + +/** + * Returns the enum constant with the given [ordinal] value or the result + * of calling the [defaultValue] function if the [ordinal] is out of bounds of + * this enum. + */ +inline fun > Enum.Companion.getOrElse(ordinal: Int, defaultValue: (Int) -> E) = enumValues().getOrElse(ordinal, defaultValue) + +/** + * Returns the enum constant with the given [ordinal] value or `null` if the + * [ordinal] is out of bounds of this enum + */ +inline fun > Enum.Companion.getOrNull(ordinal: Int) = enumValues().getOrNull(ordinal) diff --git a/core/database/src/main/kotlin/app/pachli/core/database/Converters.kt b/core/database/src/main/kotlin/app/pachli/core/database/Converters.kt index 6d8e9948c..1e5da5ebf 100644 --- a/core/database/src/main/kotlin/app/pachli/core/database/Converters.kt +++ b/core/database/src/main/kotlin/app/pachli/core/database/Converters.kt @@ -58,12 +58,12 @@ class Converters @Inject constructor( @TypeConverter fun visibilityToInt(visibility: Status.Visibility?): Int { - return visibility?.num ?: Status.Visibility.UNKNOWN.num + return visibility?.ordinal ?: Status.Visibility.UNKNOWN.ordinal } @TypeConverter fun intToVisibility(visibility: Int): Status.Visibility { - return Status.Visibility.byNum(visibility) + return Status.Visibility.getOrUnknown(visibility) } @TypeConverter diff --git a/core/network/src/main/kotlin/app/pachli/core/network/di/NetworkModule.kt b/core/network/src/main/kotlin/app/pachli/core/network/di/NetworkModule.kt index 5939a2acb..30cea5824 100644 --- a/core/network/src/main/kotlin/app/pachli/core/network/di/NetworkModule.kt +++ b/core/network/src/main/kotlin/app/pachli/core/network/di/NetworkModule.kt @@ -25,6 +25,7 @@ import app.pachli.core.network.BuildConfig import app.pachli.core.network.json.BooleanIfNull import app.pachli.core.network.json.DefaultIfNull import app.pachli.core.network.json.Guarded +import app.pachli.core.network.json.HasDefault import app.pachli.core.network.retrofit.InstanceSwitchAuthInterceptor import app.pachli.core.network.retrofit.MastodonApi import app.pachli.core.preferences.PrefKeys.HTTP_PROXY_ENABLED @@ -65,6 +66,7 @@ object NetworkModule { fun providesMoshi(): Moshi = Moshi.Builder() .add(Date::class.java, Rfc3339DateJsonAdapter()) .add(Guarded.Factory()) + .add(HasDefault.Factory()) .add(DefaultIfNull.Factory()) .add(BooleanIfNull.Factory()) .build() diff --git a/core/network/src/main/kotlin/app/pachli/core/network/json/HasDefault.kt b/core/network/src/main/kotlin/app/pachli/core/network/json/HasDefault.kt new file mode 100644 index 000000000..d6de50315 --- /dev/null +++ b/core/network/src/main/kotlin/app/pachli/core/network/json/HasDefault.kt @@ -0,0 +1,133 @@ +/* + * Copyright 2024 Pachli Association + * + * This file is a part of Pachli. + * + * This program is free software; you can redistribute it and/or modify it under the terms of the + * GNU General Public License as published by the Free Software Foundation; either version 3 of the + * License, or (at your option) any later version. + * + * Pachli is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even + * the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General + * Public License for more details. + * + * You should have received a copy of the GNU General Public License along with Pachli; if not, + * see . + */ + +package app.pachli.core.network.json + +import com.squareup.moshi.JsonAdapter +import com.squareup.moshi.JsonDataException +import com.squareup.moshi.JsonQualifier +import com.squareup.moshi.JsonReader +import com.squareup.moshi.JsonWriter +import com.squareup.moshi.Moshi +import com.squareup.moshi.Types +import java.lang.reflect.Type + +/** + * Marks the enum constant to use as the default value if the parsed JSON + * contains an invalid value. + * + * The `enum class` must be annotated with [HasDefault]. + */ +@Retention(AnnotationRetention.RUNTIME) +@Target(AnnotationTarget.FIELD) +annotation class Default + +/** + * A [JsonQualifier] for use with [Enum] declarations to indicate that incoming + * JSON values that are not valid enum constants should be mapped to a default + * value instead of throwing a [JsonDataException]. + * + * Usage: + * ``` + * val moshi = Moshi.Builder() + * .add(HasDefault.Factory()) + * .build() + * + * @HasDefault + * enum class SomeEnum { + * @Default + * FOO, + * BAR + * } + * + * @JsonClass(generateAdapter = true) + * data class Data( + * @Json(name = "some_enum") someEnum: SomeEnum + * ) + * ``` + * + * JSON of the form `{ "some_enum": "unknown" }` will parse to a + * + * ``` + * Data(someEnum = SomeEnum.FOO) + * ``` + * + * because `SomeEnum.FOO` has the `@Default` annotation. Move it to another constant + * to change it. + * + * This is similar to Moshi's existing [com.squareup.moshi.adapters.EnumJsonAdapter] + * which also supports fallbacks. The primary difference is that you define the + * default value at the point where the `enum class` is declared, not at the point + * where the Moshi instance is created. + */ +@Retention(AnnotationRetention.RUNTIME) +@Target(AnnotationTarget.CLASS) +@JsonQualifier +annotation class HasDefault() { + class Factory : JsonAdapter.Factory { + override fun create( + type: Type, + annotations: MutableSet, + moshi: Moshi, + ): JsonAdapter<*>? { + if (annotations.isNotEmpty()) return null + val rawType = Types.getRawType(type) + if (!rawType.isEnum) return null + + rawType.getAnnotation(HasDefault::class.java) ?: return null + + val delegateAnnotations = Types.nextAnnotations( + annotations, + HasDefault::class.java, + ) ?: emptySet() + + val delegate = moshi.nextAdapter( + this, + type, + delegateAnnotations, + ) + + val enumType = rawType as Class> + + val default = enumType.enumConstants.firstOrNull { + it::class.java.getField(it.name).getAnnotation(Default::class.java) != null + } ?: throw AssertionError("Missing @Default on ${enumType.name}") + + return Adapter(delegate, default) + } + + private class Adapter>( + private val delegate: JsonAdapter, + val default: Enum<*>, + ) : JsonAdapter() { + override fun fromJson(reader: JsonReader): T { + val peeked = reader.peekJson() + val result = try { + delegate.fromJson(peeked) as T + } catch (_: JsonDataException) { + default + } finally { + peeked.close() + } + reader.skipValue() + return result as T + } + + override fun toJson(writer: JsonWriter, value: T?) = delegate.toJson(writer, value) + } + } +} diff --git a/core/network/src/main/kotlin/app/pachli/core/network/model/Attachment.kt b/core/network/src/main/kotlin/app/pachli/core/network/model/Attachment.kt index 84db4e753..19ee16a79 100644 --- a/core/network/src/main/kotlin/app/pachli/core/network/model/Attachment.kt +++ b/core/network/src/main/kotlin/app/pachli/core/network/model/Attachment.kt @@ -18,7 +18,9 @@ package app.pachli.core.network.model import android.os.Parcelable +import app.pachli.core.network.json.Default import app.pachli.core.network.json.DefaultIfNull +import app.pachli.core.network.json.HasDefault import com.squareup.moshi.Json import com.squareup.moshi.JsonClass import kotlinx.parcelize.Parcelize @@ -36,6 +38,7 @@ data class Attachment( val blurhash: String?, ) : Parcelable { + @HasDefault enum class Type { @Json(name = "image") IMAGE, @@ -50,6 +53,7 @@ data class Attachment( AUDIO, @Json(name = "unknown") + @Default UNKNOWN, } diff --git a/core/network/src/main/kotlin/app/pachli/core/network/model/Filter.kt b/core/network/src/main/kotlin/app/pachli/core/network/model/Filter.kt index e1efd1954..344717f1a 100644 --- a/core/network/src/main/kotlin/app/pachli/core/network/model/Filter.kt +++ b/core/network/src/main/kotlin/app/pachli/core/network/model/Filter.kt @@ -1,6 +1,8 @@ package app.pachli.core.network.model import android.os.Parcelable +import app.pachli.core.network.json.Default +import app.pachli.core.network.json.HasDefault import com.squareup.moshi.Json import com.squareup.moshi.JsonClass import java.util.Date @@ -22,26 +24,33 @@ data class Filter( val keywords: List = emptyList(), // val statuses: List, ) : Parcelable { + @HasDefault enum class Action(val action: String) { NONE("none"), + + @Default WARN("warn"), HIDE("hide"), ; companion object { - fun from(action: String): Action = values().firstOrNull { it.action == action } ?: WARN + fun from(action: String): Action = entries.firstOrNull { it.action == action } ?: WARN } } + + @HasDefault enum class Kind(val kind: String) { HOME("home"), NOTIFICATIONS("notifications"), + + @Default PUBLIC("public"), THREAD("thread"), ACCOUNT("account"), ; companion object { - fun from(kind: String): Kind = values().firstOrNull { it.kind == kind } ?: PUBLIC + fun from(kind: String): Kind = entries.firstOrNull { it.kind == kind } ?: PUBLIC fun from(kind: TimelineKind): Kind = when (kind) { is TimelineKind.Home, is TimelineKind.UserList -> HOME diff --git a/core/network/src/main/kotlin/app/pachli/core/network/model/Notification.kt b/core/network/src/main/kotlin/app/pachli/core/network/model/Notification.kt index 57619c8ed..dd9af0e02 100644 --- a/core/network/src/main/kotlin/app/pachli/core/network/model/Notification.kt +++ b/core/network/src/main/kotlin/app/pachli/core/network/model/Notification.kt @@ -17,6 +17,8 @@ package app.pachli.core.network.model +import app.pachli.core.network.json.Default +import app.pachli.core.network.json.HasDefault import com.squareup.moshi.Json import com.squareup.moshi.JsonClass @@ -33,8 +35,10 @@ data class Notification( /** From https://docs.joinmastodon.org/entities/Notification/#type */ @JsonClass(generateAdapter = false) + @HasDefault enum class Type(val presentation: String) { @Json(name = "unknown") + @Default UNKNOWN("unknown"), /** Someone mentioned you */ @@ -80,14 +84,7 @@ data class Notification( companion object { @JvmStatic - fun byString(s: String): Type { - values().forEach { - if (s == it.presentation) { - return it - } - } - return UNKNOWN - } + fun byString(s: String) = entries.firstOrNull { s == it.presentation } ?: UNKNOWN /** Notification types for UI display (omits UNKNOWN) */ val visibleTypes = listOf(MENTION, REBLOG, FAVOURITE, FOLLOW, FOLLOW_REQUEST, POLL, STATUS, SIGN_UP, UPDATE, REPORT) @@ -113,10 +110,7 @@ data class Notification( // for Pleroma compatibility that uses Mention type fun rewriteToStatusTypeIfNeeded(accountId: String): Notification { if (type == Type.MENTION && status != null) { - return if (status.mentions.any { - it.id == accountId - } - ) { + return if (status.mentions.any { it.id == accountId }) { this } else { copy(type = Type.STATUS) diff --git a/core/network/src/main/kotlin/app/pachli/core/network/model/Status.kt b/core/network/src/main/kotlin/app/pachli/core/network/model/Status.kt index ff2780633..f0f37eaf4 100644 --- a/core/network/src/main/kotlin/app/pachli/core/network/model/Status.kt +++ b/core/network/src/main/kotlin/app/pachli/core/network/model/Status.kt @@ -19,6 +19,9 @@ package app.pachli.core.network.model import android.text.SpannableStringBuilder import android.text.style.URLSpan +import app.pachli.core.common.extensions.getOrNull +import app.pachli.core.network.json.Default +import app.pachli.core.network.json.HasDefault import app.pachli.core.network.parseAsMastodonHtml import com.squareup.moshi.Json import com.squareup.moshi.JsonClass @@ -64,20 +67,26 @@ data class Status( val actionableStatus: Status get() = reblog ?: this - enum class Visibility(val num: Int) { - UNKNOWN(0), + // Note: These are deliberately listed in order, most public to least public. + // These are currently serialised to the database by the ordinal value, and + // compared by ordinal value, so be extremely careful when adding anything + // to this list. + @HasDefault + enum class Visibility { + @Default + UNKNOWN, @Json(name = "public") - PUBLIC(1), + PUBLIC, @Json(name = "unlisted") - UNLISTED(2), + UNLISTED, @Json(name = "private") - PRIVATE(3), + PRIVATE, @Json(name = "direct") - DIRECT(4), + DIRECT, ; fun serverString(): String { @@ -92,16 +101,7 @@ data class Status( companion object { @JvmStatic - fun byNum(num: Int): Visibility { - return when (num) { - 4 -> DIRECT - 3 -> PRIVATE - 2 -> UNLISTED - 1 -> PUBLIC - 0 -> UNKNOWN - else -> UNKNOWN - } - } + fun getOrUnknown(index: Int) = Enum.getOrNull(index) ?: UNKNOWN @JvmStatic fun byString(s: String): Visibility { diff --git a/core/network/src/main/kotlin/app/pachli/core/network/model/TrendsLink.kt b/core/network/src/main/kotlin/app/pachli/core/network/model/TrendsLink.kt index 87fec767d..4ecf7906c 100644 --- a/core/network/src/main/kotlin/app/pachli/core/network/model/TrendsLink.kt +++ b/core/network/src/main/kotlin/app/pachli/core/network/model/TrendsLink.kt @@ -17,10 +17,16 @@ package app.pachli.core.network.model +import app.pachli.core.network.json.Default +import app.pachli.core.network.json.HasDefault import com.squareup.moshi.Json import com.squareup.moshi.JsonClass +@HasDefault enum class PreviewCardKind { + @Default + UNKNOWN, + @Json(name = "link") LINK, diff --git a/core/network/src/test/kotlin/app/pachli/core/network/json/HasDefaultTest.kt b/core/network/src/test/kotlin/app/pachli/core/network/json/HasDefaultTest.kt new file mode 100644 index 000000000..61b042f47 --- /dev/null +++ b/core/network/src/test/kotlin/app/pachli/core/network/json/HasDefaultTest.kt @@ -0,0 +1,109 @@ +/* + * Copyright 2024 Pachli Association + * + * This file is a part of Pachli. + * + * This program is free software; you can redistribute it and/or modify it under the terms of the + * GNU General Public License as published by the Free Software Foundation; either version 3 of the + * License, or (at your option) any later version. + * + * Pachli is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even + * the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General + * Public License for more details. + * + * You should have received a copy of the GNU General Public License along with Pachli; if not, + * see . + */ + +package app.pachli.core.network.json + +import com.google.common.truth.Truth.assertThat +import com.squareup.moshi.Json +import com.squareup.moshi.JsonClass +import com.squareup.moshi.JsonDataException +import com.squareup.moshi.Moshi +import com.squareup.moshi.adapter +import kotlin.test.fail +import org.junit.Test + +@OptIn(ExperimentalStdlibApi::class) +class HasDefaultTest { + private val moshi = Moshi.Builder() + .add(HasDefault.Factory()) + .build() + + enum class NoAnnotation { FOO, BAR } + + @HasDefault + enum class Annotated { + @Default + FOO, + BAR, + } + + @Test + fun `unannotated enum parses`() { + assertThat(moshi.adapter().fromJson("\"FOO\"")).isEqualTo(NoAnnotation.FOO) + } + + @Test + fun `unannotated enum throws an exception on unrecognised value`() { + try { + moshi.adapter().fromJson("\"UNKNOWN\"") + fail() + } catch (e: Exception) { + assertThat(e).isInstanceOf(JsonDataException::class.java) + } + } + + @Test + fun `annotated enum parses as normal`() { + val adapter = moshi.adapter() + assertThat(adapter.fromJson("\"FOO\"")).isEqualTo(Annotated.FOO) + assertThat(adapter.fromJson("\"BAR\"")).isEqualTo(Annotated.BAR) + } + + @Test + fun `annotated enum with unknown value parses as FOO`() { + val adapter = moshi.adapter() + assertThat(adapter.fromJson("\"unknown\"")).isEqualTo(Annotated.FOO) + } + + @Test + fun `annotated enum emits correct JSON for valid values`() { + val adapter = moshi.adapter() + assertThat(adapter.toJson(Annotated.FOO)).isEqualTo("\"FOO\"") + assertThat(adapter.toJson(Annotated.BAR)).isEqualTo("\"BAR\"") + } + + @JsonClass(generateAdapter = true) + data class Data(@Json(name = "some_enum") val someEnum: Annotated) + + @Test + fun `annotated enum as property of class with unknown value parses as FOO`() { + val adapter = moshi.adapter() + assertThat(adapter.fromJson("{ \"some_enum\": \"unknown\" }")).isEqualTo( + Data(someEnum = Annotated.FOO), + ) + } + + // This definition has @HasDefault but no @Default constant, so should error + @HasDefault + enum class MissingDefaultAnnotation { + FOO, + BAR, + } + + @Test + fun `unannotated enum constant throws exception when creating adapter`() { + try { + assertThat(moshi.adapter().fromJson("\"FOO\"")).isEqualTo( + MissingDefaultAnnotation.FOO, + ) + fail() + } catch (e: Error) { + assertThat(e).isInstanceOf(AssertionError::class.java) + assertThat(e.message).contains("Missing @Default") + } + } +}