From 11bf47e3b370f1272025d4b015174ecb147a4aa6 Mon Sep 17 00:00:00 2001 From: Nik Clayton Date: Thu, 15 Feb 2024 20:32:22 +0100 Subject: [PATCH] fix: Handle null properties in `Attachment.Focus` (#449) The `x` and `y` properties in `Attachment.Focus` may be null (not documented as such, but observed in the wild). Provide a `DefaultIfNull` adapter that can be applied to these to replace null values with a sensible default. --- .../java/app/pachli/StatusComparisonTest.kt | 5 +- .../CachedTimelineRemoteMediatorTest.kt | 2 + .../components/timeline/StatusMocker.kt | 2 + .../pachli/core/network/di/NetworkModule.kt | 2 + .../core/network/json/DefaultIfNullAdapter.kt | 66 ++++++++ .../pachli/core/network/model/Attachment.kt | 7 +- .../core/network/json/DefaultIfNullTest.kt | 145 ++++++++++++++++++ 7 files changed, 226 insertions(+), 3 deletions(-) create mode 100644 core/network/src/main/kotlin/app/pachli/core/network/json/DefaultIfNullAdapter.kt create mode 100644 core/network/src/test/kotlin/app/pachli/core/network/json/DefaultIfNullTest.kt diff --git a/app/src/test/java/app/pachli/StatusComparisonTest.kt b/app/src/test/java/app/pachli/StatusComparisonTest.kt index 1796b7f3d..b7bdad5e9 100644 --- a/app/src/test/java/app/pachli/StatusComparisonTest.kt +++ b/app/src/test/java/app/pachli/StatusComparisonTest.kt @@ -2,6 +2,7 @@ package app.pachli import androidx.test.ext.junit.runners.AndroidJUnit4 import app.pachli.core.database.model.TranslationState +import app.pachli.core.network.json.DefaultIfNullAdapter.Companion.DefaultIfNullAdapterFactory import app.pachli.core.network.json.GuardedAdapter.Companion.GuardedAdapterFactory import app.pachli.core.network.model.Status import app.pachli.viewdata.StatusViewData @@ -18,7 +19,9 @@ import org.junit.runner.RunWith class StatusComparisonTest { private val moshi = Moshi.Builder() .add(Date::class.java, Rfc3339DateJsonAdapter()) - .add(GuardedAdapterFactory()).build() + .add(GuardedAdapterFactory()) + .add(DefaultIfNullAdapterFactory()) + .build() @Test fun `two equal statuses - should be equal`() { diff --git a/app/src/test/java/app/pachli/components/timeline/CachedTimelineRemoteMediatorTest.kt b/app/src/test/java/app/pachli/components/timeline/CachedTimelineRemoteMediatorTest.kt index 117ec0c53..80f20ab4f 100644 --- a/app/src/test/java/app/pachli/components/timeline/CachedTimelineRemoteMediatorTest.kt +++ b/app/src/test/java/app/pachli/components/timeline/CachedTimelineRemoteMediatorTest.kt @@ -20,6 +20,7 @@ import app.pachli.core.database.model.AccountEntity import app.pachli.core.database.model.RemoteKeyEntity import app.pachli.core.database.model.RemoteKeyKind import app.pachli.core.database.model.TimelineStatusWithAccount +import app.pachli.core.network.json.DefaultIfNullAdapter.Companion.DefaultIfNullAdapterFactory import app.pachli.core.network.json.GuardedAdapter.Companion.GuardedAdapterFactory import com.google.common.truth.Truth.assertThat import com.squareup.moshi.Moshi @@ -66,6 +67,7 @@ class CachedTimelineRemoteMediatorTest { private val moshi: Moshi = Moshi.Builder() .add(Date::class.java, Rfc3339DateJsonAdapter()) .add(GuardedAdapterFactory()) + .add(DefaultIfNullAdapterFactory()) .build() @Before diff --git a/app/src/test/java/app/pachli/components/timeline/StatusMocker.kt b/app/src/test/java/app/pachli/components/timeline/StatusMocker.kt index de45d949a..a6e75d4ef 100644 --- a/app/src/test/java/app/pachli/components/timeline/StatusMocker.kt +++ b/app/src/test/java/app/pachli/components/timeline/StatusMocker.kt @@ -5,6 +5,7 @@ import app.pachli.core.database.model.TimelineAccountEntity import app.pachli.core.database.model.TimelineStatusEntity import app.pachli.core.database.model.TimelineStatusWithAccount import app.pachli.core.database.model.TranslationState +import app.pachli.core.network.json.DefaultIfNullAdapter.Companion.DefaultIfNullAdapterFactory import app.pachli.core.network.json.GuardedAdapter.Companion.GuardedAdapterFactory import app.pachli.core.network.model.Status import app.pachli.core.network.model.TimelineAccount @@ -101,6 +102,7 @@ fun mockStatusEntityWithAccount( val moshi = Moshi.Builder() .add(Date::class.java, Rfc3339DateJsonAdapter()) .add(GuardedAdapterFactory()) + .add(DefaultIfNullAdapterFactory()) .build() return TimelineStatusWithAccount( 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 1697d6983..9503230f8 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 @@ -22,6 +22,7 @@ import android.os.Build import app.pachli.core.common.util.versionName import app.pachli.core.mastodon.model.MediaUploadApi import app.pachli.core.network.BuildConfig +import app.pachli.core.network.json.DefaultIfNullAdapter.Companion.DefaultIfNullAdapterFactory import app.pachli.core.network.json.GuardedAdapter.Companion.GuardedAdapterFactory import app.pachli.core.network.retrofit.InstanceSwitchAuthInterceptor import app.pachli.core.network.retrofit.MastodonApi @@ -63,6 +64,7 @@ object NetworkModule { fun providesMoshi(): Moshi = Moshi.Builder() .add(Date::class.java, Rfc3339DateJsonAdapter()) .add(GuardedAdapterFactory()) + .add(DefaultIfNullAdapterFactory()) .build() @Provides diff --git a/core/network/src/main/kotlin/app/pachli/core/network/json/DefaultIfNullAdapter.kt b/core/network/src/main/kotlin/app/pachli/core/network/json/DefaultIfNullAdapter.kt new file mode 100644 index 000000000..a5fa44eb1 --- /dev/null +++ b/core/network/src/main/kotlin/app/pachli/core/network/json/DefaultIfNullAdapter.kt @@ -0,0 +1,66 @@ +/* + * 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.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 + +@Retention(AnnotationRetention.RUNTIME) +@JsonQualifier +annotation class DefaultIfNull + +class DefaultIfNullAdapter(private val delegate: JsonAdapter) : JsonAdapter() { + override fun fromJson(reader: JsonReader): Any? { + val value = reader.readJsonValue() + if (value is Map<*, *>) { + val withoutNulls = value.filterValues { it != null } + return delegate.fromJsonValue(withoutNulls) + } + return delegate.fromJsonValue(value) + } + + override fun toJson(writer: JsonWriter, value: Any?) { + return delegate.toJson(writer, value) + } + + companion object { + class DefaultIfNullAdapterFactory : Factory { + override fun create( + type: Type, + annotations: MutableSet, + moshi: Moshi, + ): JsonAdapter<*>? { + val delegateAnnotations = Types.nextAnnotations( + annotations, + DefaultIfNull::class.java, + ) ?: return null + val delegate = moshi.nextAdapter( + this, + type, + delegateAnnotations, + ) + return DefaultIfNullAdapter(delegate) + } + } + } +} 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 5c0ae2109..d71fc5fb3 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,6 +18,7 @@ package app.pachli.core.network.model import android.os.Parcelable +import app.pachli.core.network.json.DefaultIfNull import com.squareup.moshi.Json import com.squareup.moshi.JsonClass import kotlinx.parcelize.Parcelize @@ -58,6 +59,8 @@ data class Attachment( @Parcelize @JsonClass(generateAdapter = true) data class MetaData( + // Fields in Focus may be null, see https://github.com/mastodon/mastodon/issues/29222 + @DefaultIfNull val focus: Focus?, val duration: Float?, val original: Size?, @@ -73,8 +76,8 @@ data class Attachment( @Parcelize @JsonClass(generateAdapter = true) data class Focus( - val x: Float, - val y: Float, + val x: Float = 0f, + val y: Float = 0f, ) : Parcelable { fun toMastodonApiString(): String = "$x,$y" } diff --git a/core/network/src/test/kotlin/app/pachli/core/network/json/DefaultIfNullTest.kt b/core/network/src/test/kotlin/app/pachli/core/network/json/DefaultIfNullTest.kt new file mode 100644 index 000000000..d6d23cdb3 --- /dev/null +++ b/core/network/src/test/kotlin/app/pachli/core/network/json/DefaultIfNullTest.kt @@ -0,0 +1,145 @@ +package app.pachli.core.network.json + +import app.pachli.core.network.json.DefaultIfNullAdapter.Companion.DefaultIfNullAdapterFactory +import com.google.common.truth.Truth.assertThat +import com.squareup.moshi.JsonClass +import com.squareup.moshi.Moshi +import com.squareup.moshi.adapter +import org.junit.Test + +@OptIn(ExperimentalStdlibApi::class) +class DefaultIfNullTest { + + private val moshi = Moshi.Builder() + .add(DefaultIfNullAdapterFactory()) + .build() + + @JsonClass(generateAdapter = true) + data class Wrapper( + @DefaultIfNull + val data: Data, + ) + + @JsonClass(generateAdapter = true) + data class Data( + val x: Int = 1, + val y: Float = 2f, + val z: String = "hello, world", + val nullable: String? = null, + ) + + @Test + fun `null x defaults to 1`() { + val jsonInput = """ + { + "data": { + "x": null, + "y": 3, + "z": "foo", + "nullable": "bar" + } + } + """.trimIndent() + assertThat(moshi.adapter().fromJson(jsonInput)).isEqualTo( + Wrapper( + data = Data( + x = 1, + y = 3f, + z = "foo", + nullable = "bar", + ), + ), + ) + } + + @Test + fun `null y defaults to 2f`() { + val jsonInput = """ + { + "data": { + "x": 1, + "y": null, + "z": "foo", + "nullable": "bar" + } + } + """.trimIndent() + assertThat(moshi.adapter().fromJson(jsonInput)).isEqualTo( + Wrapper( + data = Data( + x = 1, + y = 2f, + z = "foo", + nullable = "bar", + ), + ), + ) + } + + @Test + fun `null z defaults to "hello, world"`() { + val jsonInput = """ + { + "data": { + "x": 1, + "y": 2, + "z": null, + "nullable": "bar" + } + } + """.trimIndent() + assertThat(moshi.adapter().fromJson(jsonInput)).isEqualTo( + Wrapper( + data = Data( + x = 1, + y = 2f, + z = "hello, world", + nullable = "bar", + ), + ), + ) + } + + @Test + fun `nullable remains null`() { + val jsonInput = """ + { + "data": { + "x": 1, + "y": 2, + "z": "foo", + "nullable": null + } + } + """.trimIndent() + assertThat(moshi.adapter().fromJson(jsonInput)).isEqualTo( + Wrapper( + data = Data( + x = 1, + y = 2f, + z = "foo", + nullable = null, + ), + ), + ) + } + + @Test + fun `null everything returns default`() { + val jsonInput = """ + { + "data": { + "x": null, + "y": null, + "z": null, + "nullable": null + } + } + """.trimIndent() + assertThat(moshi.adapter().fromJson(jsonInput)).isEqualTo( + Wrapper( + data = Data(), + ), + ) + } +}