diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/PatchJsonAdapters.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/PatchJsonAdapters.kt new file mode 100644 index 0000000..9e08a93 --- /dev/null +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/PatchJsonAdapters.kt @@ -0,0 +1,150 @@ +package systems.lupine.sheaf.data.api + +import com.squareup.moshi.JsonAdapter +import com.squareup.moshi.JsonReader +import com.squareup.moshi.JsonWriter +import com.squareup.moshi.Moshi +import com.squareup.moshi.Types +import systems.lupine.sheaf.data.model.GroupUpdate +import systems.lupine.sheaf.data.model.MemberUpdate +import systems.lupine.sheaf.data.model.SystemUpdate + +/** + * Hand-written Moshi adapters for the PATCH bodies whose fields clear on an + * explicit null. + * + * These endpoints read the body with `exclude_unset`, so presence is the + * contract: + * - omitted -> leave the existing value alone + * - JSON null -> clear it + * - a value -> set it + * + * Moshi omits null fields, which expresses the first two-thirds and leaves no + * way to clear anything. Emptying a member's pronouns, a group's description or + * a system's tag therefore did nothing: the field was dropped from the request + * and the old value survived. + * + * The obvious shortcut, turning on null serialisation for the whole body, does + * not work. Each of these bodies also carries fields backed by NOT NULL columns + * that the server rejects an explicit null for (a member's name and privacy, a + * system's date_format, and so on). Blanket nulls would start sending those and + * fail the whole save. So each field is either clearable or omit-when-null, and + * the split has to match the server's `_reject_explicit_null` lists. + * + * [PatchWriter.clears] and [PatchWriter.omitsWhenNull] name which is which, so + * a field added later has to make the choice explicitly. + */ +internal class PatchWriter( + private val writer: JsonWriter, + private val anyAdapter: JsonAdapter, +) { + /** Server clears the column on an explicit null, so always write it. */ + fun clears(name: String, value: Any?) { + val previous = writer.serializeNulls + writer.serializeNulls = true + writer.name(name) + if (value == null) writer.nullValue() else anyAdapter.toJson(writer, value) + writer.serializeNulls = previous + } + + /** Server rejects an explicit null here, so send nothing at all. */ + fun omitsWhenNull(name: String, value: Any?) { + if (value == null) return + writer.name(name) + anyAdapter.toJson(writer, value) + } +} + +private fun requestOnly(name: String): Nothing = + throw UnsupportedOperationException("$name is a request body; it is never parsed") + +class MemberUpdateJsonAdapter(moshi: Moshi) : JsonAdapter() { + private val anyAdapter: JsonAdapter by lazy { moshi.adapter(Any::class.java) } + + override fun toJson(writer: JsonWriter, value: MemberUpdate?) { + if (value == null) { writer.nullValue(); return } + writer.beginObject() + PatchWriter(writer, anyAdapter).apply { + // NOT NULL server-side: name, privacy. + omitsWhenNull("name", value.name) + omitsWhenNull("privacy", value.privacy) + clears("display_name", value.displayName) + clears("description", value.description) + clears("pronouns", value.pronouns) + clears("avatar_url", value.avatarUrl) + clears("banner_url", value.bannerUrl) + clears("color", value.color) + clears("birthday", value.birthday) + clears("note", value.note) + clears("emoji", value.emoji) + } + writer.endObject() + } + + override fun fromJson(reader: JsonReader) = requestOnly("MemberUpdate") + + companion object { + /** Needs the Moshi instance, so it registers as a factory. */ + val FACTORY = Factory { type, _, moshi -> + if (Types.getRawType(type) == MemberUpdate::class.java) MemberUpdateJsonAdapter(moshi) else null + } + } +} + +class GroupUpdateJsonAdapter(moshi: Moshi) : JsonAdapter() { + private val anyAdapter: JsonAdapter by lazy { moshi.adapter(Any::class.java) } + + override fun toJson(writer: JsonWriter, value: GroupUpdate?) { + if (value == null) { writer.nullValue(); return } + writer.beginObject() + PatchWriter(writer, anyAdapter).apply { + omitsWhenNull("name", value.name) + clears("description", value.description) + clears("color", value.color) + // Clearing this is how a subgroup is promoted back to top level. + clears("parent_id", value.parentId) + } + writer.endObject() + } + + override fun fromJson(reader: JsonReader) = requestOnly("GroupUpdate") + + companion object { + /** Needs the Moshi instance, so it registers as a factory. */ + val FACTORY = Factory { type, _, moshi -> + if (Types.getRawType(type) == GroupUpdate::class.java) GroupUpdateJsonAdapter(moshi) else null + } + } +} + +class SystemUpdateJsonAdapter(moshi: Moshi) : JsonAdapter() { + private val anyAdapter: JsonAdapter by lazy { moshi.adapter(Any::class.java) } + + override fun toJson(writer: JsonWriter, value: SystemUpdate?) { + if (value == null) { writer.nullValue(); return } + writer.beginObject() + PatchWriter(writer, anyAdapter).apply { + // NOT NULL server-side: name, privacy, show_member_created_date + // (alongside date_format and the front defaults, which this client + // does not send). + omitsWhenNull("name", value.name) + omitsWhenNull("privacy", value.privacy) + omitsWhenNull("show_member_created_date", value.showMemberCreatedDate) + clears("description", value.description) + clears("tag", value.tag) + clears("avatar_url", value.avatarUrl) + clears("color", value.color) + clears("note", value.note) + } + writer.endObject() + } + + override fun fromJson(reader: JsonReader) = requestOnly("SystemUpdate") + + companion object { + /** Needs the Moshi instance, so it registers as a factory. */ + val FACTORY = Factory { type, _, moshi -> + if (Types.getRawType(type) == SystemUpdate::class.java) SystemUpdateJsonAdapter(moshi) else null + } + } +} diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/di/NetworkModule.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/di/NetworkModule.kt index 2ce9f32..e123572 100644 --- a/sheaf/app/src/main/java/systems/lupine/sheaf/di/NetworkModule.kt +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/di/NetworkModule.kt @@ -6,6 +6,9 @@ import systems.lupine.sheaf.data.api.AuthInterceptor import systems.lupine.sheaf.data.api.BaseUrlInterceptor import systems.lupine.sheaf.data.api.CredentialGuardInterceptor import systems.lupine.sheaf.data.api.CustomFieldValueSetJsonAdapter +import systems.lupine.sheaf.data.api.SystemUpdateJsonAdapter +import systems.lupine.sheaf.data.api.GroupUpdateJsonAdapter +import systems.lupine.sheaf.data.api.MemberUpdateJsonAdapter import systems.lupine.sheaf.data.api.FrontUpdateJsonAdapter import systems.lupine.sheaf.data.model.FrontUpdate import systems.lupine.sheaf.data.api.SheafApiService @@ -44,7 +47,13 @@ object NetworkModule { fun provideMoshi(): Moshi = Moshi.Builder() .add(Date::class.java, Rfc3339DateJsonAdapter().nullSafe()) .add(FrontUpdate::class.java, FrontUpdateJsonAdapter()) + // Bodies whose optional fields clear on an explicit null. Moshi drops + // those nulls, so without these the field is simply absent from the + // request and the old value survives. .add(CustomFieldValueSetJsonAdapter.FACTORY) + .add(MemberUpdateJsonAdapter.FACTORY) + .add(GroupUpdateJsonAdapter.FACTORY) + .add(SystemUpdateJsonAdapter.FACTORY) .addLast(KotlinJsonAdapterFactory()) .build() diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/members/MembersViewModel.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/members/MembersViewModel.kt index ca5e815..72e66ad 100644 --- a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/members/MembersViewModel.kt +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/members/MembersViewModel.kt @@ -475,13 +475,12 @@ class MemberDetailViewModel @Inject constructor( val update = MemberUpdate( name = f.name.trim(), displayName = f.displayName.takeIf { it.isNotBlank() }, - // Empty string rather than null, so removing an emoji - // actually removes it. The member PATCH is - // omit-means-unchanged and Moshi drops nulls, so a null - // here would silently leave the old emoji in place. Same - // trick the scratchpad note uses. Reads back as "no - // emoji" everywhere, since every client blank-checks it. - emoji = f.emoji.trim(), + // Null clears it. That used to need an empty string, + // because Moshi drops nulls and the member PATCH reads + // an absent field as "leave it alone", so clearing an + // emoji silently did nothing; the hand-written adapter + // now writes this one out even when it is null. + emoji = f.emoji.trim().takeIf { it.isNotEmpty() }, pronouns = f.pronouns.takeIf { it.isNotBlank() }, description = f.description.takeIf { it.isNotBlank() }, avatarUrl = f.avatarUrl, diff --git a/sheaf/app/src/test/java/systems/lupine/sheaf/data/api/PatchJsonAdaptersTest.kt b/sheaf/app/src/test/java/systems/lupine/sheaf/data/api/PatchJsonAdaptersTest.kt new file mode 100644 index 0000000..a4443e9 --- /dev/null +++ b/sheaf/app/src/test/java/systems/lupine/sheaf/data/api/PatchJsonAdaptersTest.kt @@ -0,0 +1,114 @@ +package systems.lupine.sheaf.data.api + +import systems.lupine.sheaf.data.model.GroupUpdate +import systems.lupine.sheaf.data.model.MemberUpdate +import systems.lupine.sheaf.data.model.SystemUpdate +import systems.lupine.sheaf.di.NetworkModule +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +/** + * These PATCH bodies are read with `exclude_unset`, so an omitted field means + * "leave it alone" and a null means "clear it". Moshi drops nulls, which left + * no way to clear anything: emptying a member's pronouns silently kept the old + * value. + * + * The split matters in both directions. A field that should clear but is + * omitted fails quietly, which is the bug. A field the server rejects an + * explicit null for fails the entire save if a null is sent, which would be a + * louder bug introduced by an over-broad fix. + * + * Built through the app's own Moshi, so a missing registration fails here + * rather than only in the field. + */ +class PatchJsonAdaptersTest { + + private val moshi = NetworkModule.provideMoshi() + + private fun member(u: MemberUpdate) = moshi.adapter(MemberUpdate::class.java).toJson(u) + private fun group(u: GroupUpdate) = moshi.adapter(GroupUpdate::class.java).toJson(u) + private fun system(u: SystemUpdate) = moshi.adapter(SystemUpdate::class.java).toJson(u) + + // ── Member ──────────────────────────────────────────────────────────────── + + @Test fun `emptying a member's optional fields clears them`() { + val json = member(MemberUpdate(name = "Alex")) + listOf("display_name", "description", "pronouns", "color", "birthday", "note", "emoji") + .forEach { assertTrue("\"$it\":null" in json, "$it should clear, got: $json") } + } + + @Test fun `removing a member's emoji removes it`() { + // This one needed an empty string before the adapter existed, because a + // dropped null read as "leave it alone" and the old emoji stayed put. + assertTrue("\"emoji\":null" in member(MemberUpdate(name = "Alex", emoji = null))) + val wolf = "\uD83D\uDC3A" + assertTrue("\"emoji\":\"$wolf\"" in member(MemberUpdate(emoji = wolf))) + } + + @Test fun `a member's NOT NULL fields are omitted rather than nulled`() { + // Sending these as null fails the whole save server-side. + val json = member(MemberUpdate(displayName = "D")) + assertFalse("\"name\"" in json, json) + assertFalse("\"privacy\"" in json, json) + } + + @Test fun `a member's set values still serialise`() { + val json = member(MemberUpdate(name = "Alex", pronouns = "they/them")) + assertTrue("\"name\":\"Alex\"" in json, json) + assertTrue("\"pronouns\":\"they/them\"" in json, json) + } + + @Test fun `clearing an avatar reaches the wire`() { + // Removing a picture is a clear, not an omission. + assertTrue("\"avatar_url\":null" in member(MemberUpdate(name = "Alex"))) + assertTrue("\"banner_url\":null" in member(MemberUpdate(name = "Alex"))) + } + + // ── Group ───────────────────────────────────────────────────────────────── + + @Test fun `a group's description and colour clear`() { + val json = group(GroupUpdate(name = "G")) + assertTrue("\"description\":null" in json, json) + assertTrue("\"color\":null" in json, json) + } + + @Test fun `clearing a group's parent promotes it to top level`() { + assertTrue("\"parent_id\":null" in group(GroupUpdate(name = "G"))) + } + + @Test fun `a group's name is omitted when null`() { + assertFalse("\"name\"" in group(GroupUpdate(description = "d"))) + } + + // ── System ──────────────────────────────────────────────────────────────── + + @Test fun `a system's tag, colour and description clear`() { + val json = system(SystemUpdate(name = "S")) + listOf("description", "tag", "color", "note") + .forEach { assertTrue("\"$it\":null" in json, "$it should clear, got: $json") } + } + + @Test fun `a system's NOT NULL fields are omitted rather than nulled`() { + val json = system(SystemUpdate(tag = "t")) + assertFalse("\"name\"" in json, json) + assertFalse("\"privacy\"" in json, json) + // Absent from the body, not sent as null: the column is NOT NULL, and + // the toggle only ever ships an actual value. + assertFalse("\"show_member_created_date\"" in json, json) + } + + @Test fun `a system's created-date toggle serialises both ways when set`() { + assertTrue("\"show_member_created_date\":true" in system(SystemUpdate(showMemberCreatedDate = true))) + assertTrue("\"show_member_created_date\":false" in system(SystemUpdate(showMemberCreatedDate = false))) + } + + @Test fun `the bodies stay valid JSON objects`() { + listOf(member(MemberUpdate(name = "A")), group(GroupUpdate(name = "G")), system(SystemUpdate(name = "S"))) + .forEach { + assertTrue(it.startsWith("{") && it.endsWith("}"), it) + assertEquals(it.count { c -> c == '{' }, it.count { c -> c == '}' }) + } + } +}