Repository navigation
Stop partial PATCH bodies clearing fields they never set - #108
Merged
Merged
Conversation
Two bugs, one cause. These bodies clear a column by sending an explicit null, and the hand-written adapters wrote that null whether or not the caller had populated the field. That is right for a screen that owns the whole form and wrong for one that patches a single setting. Flipping either switch under Settings, Fronting sent the new value plus nulls for description, tag, colour, note, avatar and banner, and the server cleared all six. Shipped in 1.5.0. Verified against a real instance before and after. Unticking quiet hours on a channel had the opposite problem: Moshi's generated adapter dropped the null, so the field never reached the server and the window stayed. That one needs an explicit flag, because null there means both "clear it" and "not editing it" and the body had no way to say which. Clearing is now opt-in per body, defaulting to off, so a screen that sends part of a form cannot destroy the rest of it. The three full-form callers opt in. Member and group had no partial caller yet, but were one quick-rename action away from the same bug. Also adds a reflective parity test over all four bodies. The existing ones check a hand-written list of names, so a field added to a data class and not to its adapter passes them by construction, which is how the ceilings and the step-up credentials went missing before. This one asks the class what its fields are and names the missing one.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two bugs with one cause. These bodies clear a column by sending an explicit null, and the hand-written adapters wrote that null whether or not the caller had populated the field. Right for a screen that owns the whole form, wrong for one patching a single setting.
Flipping either switch under Settings, Fronting sent the new value plus nulls for description, tag, colour, note, avatar and banner, and the server cleared all six. Shipped in 1.5.0, so it has been destroying data rather than just failing to save. Checked against a real instance before and after.
Unticking quiet hours on a channel had the opposite problem: Moshi dropped the null, so the field never reached the server and the window stayed on. That one needs an explicit flag, since null there means both "clear it" and "not editing it" and the body had no way to say which.
Clearing is now opt-in per body and defaults to off, so a screen sending part of a form cannot destroy the rest. The three full-form callers opt in. Member and group had no partial caller yet but were one quick-rename action away from the same thing. The gate sits in
PatchWriter.clears()rather than per field, so fields added later inherit it.Also adds a reflective parity test over the four bodies. The existing tests check a hand-written list of names, so a field added to a data class and not to its adapter passes them by construction, which is how the step-up credentials went missing before. This one asks the class what its fields are and names the missing one.