fix(members): let a populated custom field be cleared again

sheaf/app/src/main/java/systems/lupine/sheaf/data/api/CustomFieldValueSetJsonAdapter.kt

@@ -0,0 +1,80 @@
+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.CustomFieldValueSet
+
+/**
+ * Hand-written Moshi adapter for [CustomFieldValueSet].
+ *
+ * `PUT /v1/members/{id}/fields` upserts whatever `value` it is handed, and a
+ * null is how a populated field gets cleared: the server stores the null rather
+ * than deleting the row. There is no "omit to leave this one alone" mode, so
+ * `value` is required on every entry the request carries.
+ *
+ * Moshi omits null fields by default, which turned a clear into
+ * `{"field_id": "..."}` with no `value` at all. That is not a weaker request,
+ * it is an invalid one, and clearing a field failed outright.
+ *
+ * This adapter always writes `value`, null included. Same trap the fronts PATCH
+ * hit (see [FrontUpdateJsonAdapter]) from the other side: there the fix was to
+ * allow an explicit null through, here it is to stop dropping one.
+ */
+class CustomFieldValueSetJsonAdapter(moshi: Moshi) : JsonAdapter<CustomFieldValueSet>() {
+
+    // Values are type-erased on the wire (string, number, boolean, or a list
+    // for multiselect), so delegate rather than guessing at the shape. Resolved
+    // lazily: the factory runs while Moshi is still assembling itself.
+    private val anyAdapter: JsonAdapter<Any> by lazy { moshi.adapter(Any::class.java) }
+
+    override fun toJson(writer: JsonWriter, value: CustomFieldValueSet?) {
+        if (value == null) {
+            writer.nullValue()
+            return
+        }
+        writer.beginObject()
+        writer.name("field_id").value(value.fieldId)
+        // Force the null through regardless of the writer's global setting,
+        // which is what drops it otherwise.
+        val previous = writer.serializeNulls
+        writer.serializeNulls = true
+        writer.name("value")
+        if (value.value == null) writer.nullValue() else anyAdapter.toJson(writer, value.value)
+        writer.serializeNulls = previous
+        writer.endObject()
+    }
+
+    override fun fromJson(reader: JsonReader): CustomFieldValueSet {
+        var fieldId: String? = null
+        var parsed: Any? = null
+        reader.beginObject()
+        while (reader.hasNext()) {
+            when (reader.nextName()) {
+                "field_id" -> fieldId = reader.nextString()
+                "value" -> parsed =
+                    if (reader.peek() == JsonReader.Token.NULL) reader.nextNull<Any>()
+                    else anyAdapter.fromJson(reader)
+                else -> reader.skipValue()
+            }
+        }
+        reader.endObject()
+        return CustomFieldValueSet(
+            fieldId = requireNotNull(fieldId) { "field_id missing" },
+            value = parsed,
+        )
+    }
+
+    companion object {
+        /** Registered on the app's Moshi; needs the instance, hence a factory. */
+        val FACTORY = Factory { type, _, moshi ->
+            if (Types.getRawType(type) == CustomFieldValueSet::class.java) {
+                CustomFieldValueSetJsonAdapter(moshi)
+            } else {
+                null
+            }
+        }
+    }
+}

sheaf/app/src/main/java/systems/lupine/sheaf/di/NetworkModule.kt

@@ -5,6 +5,7 @@ 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.CustomFieldValueSetJsonAdapter
 import systems.lupine.sheaf.data.api.FrontUpdateJsonAdapter
 import systems.lupine.sheaf.data.model.FrontUpdate
 import systems.lupine.sheaf.data.api.SheafApiService
@@ -43,6 +44,7 @@ object NetworkModule {
     fun provideMoshi(): Moshi = Moshi.Builder()
         .add(Date::class.java, Rfc3339DateJsonAdapter().nullSafe())
         .add(FrontUpdate::class.java, FrontUpdateJsonAdapter())
+        .add(CustomFieldValueSetJsonAdapter.FACTORY)
         .addLast(KotlinJsonAdapterFactory())
         .build()
 

sheaf/app/src/test/java/systems/lupine/sheaf/data/api/CustomFieldValueSetJsonAdapterTest.kt

@@ -0,0 +1,84 @@
+package systems.lupine.sheaf.data.api
+
+import com.squareup.moshi.Moshi
+import com.squareup.moshi.Types
+import com.squareup.moshi.kotlin.reflect.KotlinJsonAdapterFactory
+import systems.lupine.sheaf.data.model.CustomFieldValueSet
+import kotlin.test.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertTrue
+
+/**
+ * Clearing a populated custom field is expressed as `value: null`, and the
+ * server requires the key to be present: an entry without it is rejected
+ * outright, not treated as a smaller change. Moshi drops null fields by
+ * default, so this is the difference between clearing working and failing.
+ */
+class CustomFieldValueSetJsonAdapterTest {
+
+    private val moshi = Moshi.Builder()
+        .add(CustomFieldValueSetJsonAdapter.FACTORY)
+        .addLast(KotlinJsonAdapterFactory())
+        .build()
+
+    private val listType =
+        Types.newParameterizedType(List::class.java, CustomFieldValueSet::class.java)
+    private val adapter = moshi.adapter<List<CustomFieldValueSet>>(listType)
+
+    private fun json(vararg items: CustomFieldValueSet) = adapter.toJson(items.toList())
+
+    @Test fun `clearing a field sends an explicit null`() {
+        assertEquals(
+            """[{"field_id":"f1","value":null}]""",
+            json(CustomFieldValueSet("f1", null)),
+        )
+    }
+
+    @Test fun `the value key is never dropped`() {
+        // The regression this exists for: without the adapter this serialised
+        // to {"field_id":"f1"} and the server rejected the whole request.
+        assertTrue("\"value\"" in json(CustomFieldValueSet("f1", null)))
+    }
+
+    @Test fun `a set value still serialises normally`() {
+        assertEquals(
+            """[{"field_id":"f1","value":"hello"}]""",
+            json(CustomFieldValueSet("f1", "hello")),
+        )
+    }
+
+    @Test fun `non-string value types survive`() {
+        // Fields are type-erased on the wire; numbers, booleans and the list
+        // a multiselect carries all go through the same path.
+        assertTrue("\"value\":true" in json(CustomFieldValueSet("f1", true)))
+        assertTrue("\"value\":[" in json(CustomFieldValueSet("f1", listOf("a", "b"))))
+    }
+
+    @Test fun `a mixed batch clears one field while setting another`() {
+        // What a real save looks like when someone empties one field and edits
+        // another in the same edit.
+        assertEquals(
+            """[{"field_id":"f1","value":null},{"field_id":"f2","value":"kept"}]""",
+            json(CustomFieldValueSet("f1", null), CustomFieldValueSet("f2", "kept")),
+        )
+    }
+
+    @Test fun `the app's own Moshi has the adapter registered`() {
+        // The tests above prove the adapter; this proves it is actually wired
+        // into the instance the app uses. Forgetting the registration would
+        // leave every other test here passing while clearing stayed broken.
+        val appMoshi = systems.lupine.sheaf.di.NetworkModule.provideMoshi()
+        val appAdapter = appMoshi.adapter<List<CustomFieldValueSet>>(listType)
+        assertEquals(
+            """[{"field_id":"f1","value":null}]""",
+            appAdapter.toJson(listOf(CustomFieldValueSet("f1", null))),
+        )
+    }
+
+    @Test fun `round-trips back through fromJson`() {
+        val parsed = adapter.fromJson("""[{"field_id":"f1","value":null}]""")
+        assertEquals(1, parsed?.size)
+        assertEquals("f1", parsed?.first()?.fieldId)
+        assertEquals(null, parsed?.first()?.value)
+    }
+}