From aa3d4fc40f9691ac63c519e79e44f50b2f171ea6 Mon Sep 17 00:00:00 2001 From: SiteRelEnby <125829806+SiteRelEnby@users.noreply.github.com> Date: Tue, 1 Sep 2026 20:05:13 -0400 Subject: [PATCH] fix(api): let optional fields be cleared on member, group and system edits Emptying an optional field and saving did nothing: the old value came back. This affected a member's display name, pronouns, colour, birthday and description; a group's description and colour; and a system's description, tag and colour. Removing an avatar or banner was broken the same way, on both a member and the system profile. These endpoints read the body with exclude_unset, so presence is the contract: omitted leaves a field alone, an explicit null clears it. Moshi omits null fields, so a cleared field was simply absent from the request and the server correctly left it as it was. Confirmed by probing the serialiser, which turned a member edit with five cleared fields into {"name":"Alex"}. The obvious shortcut does not work. Turning on null serialisation for these bodies would also null the fields backed by NOT NULL columns that the server explicitly rejects an explicit null for, failing the whole save instead of one field. So each field is either clearable or omit-when-null, matching the server's _reject_explicit_null lists, and the two helpers are named after which is which so a field added later has to make the choice. No ViewModel changes: they already produce null for an emptied field, which now means what it always should have. Found by auditing for the failure mode behind the custom-field-value fix. That one is the same bug in a different shape, where the server requires the key rather than treating its absence as unchanged. --- .../sheaf/data/api/PatchJsonAdapters.kt | 153 ++++++++++++++++++ .../systems/lupine/sheaf/di/NetworkModule.kt | 9 ++ .../sheaf/data/api/PatchJsonAdaptersTest.kt | 106 ++++++++++++ 3 files changed, 268 insertions(+) create mode 100644 sheaf/app/src/main/java/systems/lupine/sheaf/data/api/PatchJsonAdapters.kt create mode 100644 sheaf/app/src/test/java/systems/lupine/sheaf/data/api/PatchJsonAdaptersTest.kt 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..c862500 --- /dev/null +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/PatchJsonAdapters.kt @@ -0,0 +1,153 @@ +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) + // NOTE: sheaf-project/android#69 adds `emoji` to MemberUpdate. When + // that lands, add clears("emoji", ...) here and drop the + // empty-string workaround it uses to clear, which this makes + // unnecessary. + } + 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 97fafc3..8b857d3 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 @@ -5,6 +5,9 @@ import systems.lupine.sheaf.BuildConfig 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.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 @@ -43,6 +46,12 @@ object NetworkModule { fun provideMoshi(): Moshi = Moshi.Builder() .add(Date::class.java, Rfc3339DateJsonAdapter().nullSafe()) .add(FrontUpdate::class.java, FrontUpdateJsonAdapter()) + // PATCH 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(MemberUpdateJsonAdapter.FACTORY) + .add(GroupUpdateJsonAdapter.FACTORY) + .add(SystemUpdateJsonAdapter.FACTORY) .addLast(KotlinJsonAdapterFactory()) .build() 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..3cc7f79 --- /dev/null +++ b/sheaf/app/src/test/java/systems/lupine/sheaf/data/api/PatchJsonAdaptersTest.kt @@ -0,0 +1,106 @@ +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") + .forEach { assertTrue("\"$it\":null" in json, "$it should clear, got: $json") } + } + + @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 == '}' }) + } + } +}