fix(api): let optional fields be cleared on member, group and system edits

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<Any>,
+) {
+    /** 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<MemberUpdate>() {
+    private val anyAdapter: JsonAdapter<Any> 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<GroupUpdate>() {
+    private val anyAdapter: JsonAdapter<Any> 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<SystemUpdate>() {
+    private val anyAdapter: JsonAdapter<Any> 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
+        }
+    }
+}

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()
 

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 == '}' })
+            }
+    }
+}