From 27c85767420bc34860941fa317b20a82db62e168 Mon Sep 17 00:00:00 2001 From: SiteRelEnby <125829806+SiteRelEnby@users.noreply.github.com> Date: Tue, 1 Sep 2026 10:25:48 -0400 Subject: [PATCH] fix(members): let a populated custom field be cleared again Emptying a custom field on a member did nothing: the save failed and the old value stayed. Clearing is expressed as `value: null` on PUT /v1/members/{id}/fields, which the server upserts as a null rather than deleting the row. `value` is required on every entry; there is no "omit to leave this one alone" mode. Moshi omits null fields by default, so a clear serialised to [{"field_id": "..."}] with no `value` at all. That is not a smaller request, it is an invalid one, and the server rejected it. Verified both halves: the serialisation by probing the adapter, and the rejection against the server's own Pydantic, which raises a validation error for a missing `value` and accepts an explicit null. Adds a hand-written adapter that always writes `value`, null included. Same trap the fronts PATCH hit, from the other side: there the fix was to let an explicit null through, here it is to stop dropping one. Tests cover the cleared entry, a set entry, non-string value types, and a mixed batch that clears one field while setting another in the same save. One of them builds the app's own Moshi rather than a local one, so forgetting the registration fails a test instead of quietly restoring the bug. Worth doing on the server too, as defence for any other client whose serialiser omits nulls: accepting a missing `value` as null costs nothing, since the endpoint has no other meaning for its absence. That is not a substitute for this, though, since it would only help once an instance upgrades. --- .../api/CustomFieldValueSetJsonAdapter.kt | 80 ++++++++++++++++++ .../systems/lupine/sheaf/di/NetworkModule.kt | 2 + .../api/CustomFieldValueSetJsonAdapterTest.kt | 84 +++++++++++++++++++ 3 files changed, 166 insertions(+) create mode 100644 sheaf/app/src/main/java/systems/lupine/sheaf/data/api/CustomFieldValueSetJsonAdapter.kt create mode 100644 sheaf/app/src/test/java/systems/lupine/sheaf/data/api/CustomFieldValueSetJsonAdapterTest.kt diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/CustomFieldValueSetJsonAdapter.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/CustomFieldValueSetJsonAdapter.kt new file mode 100644 index 0000000..d95a67e --- /dev/null +++ b/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() { + + // 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 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() + 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 + } + } + } +} 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..2ce9f32 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,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() diff --git a/sheaf/app/src/test/java/systems/lupine/sheaf/data/api/CustomFieldValueSetJsonAdapterTest.kt b/sheaf/app/src/test/java/systems/lupine/sheaf/data/api/CustomFieldValueSetJsonAdapterTest.kt new file mode 100644 index 0000000..2c9581c --- /dev/null +++ b/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>(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>(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) + } +}