[COVAL-5821] Model the dropped persona request fields - #124
Conversation
CreatePersonaRequest and UpdatePersonaRequest declared seven fewer fields than the API publishes and serves, so `coval personas create/update` discarded them in silence: background_sound_volume, voice_volume, voice_speed, hold_music_timeout_seconds, situate_speaker, audio_degradation, and tags. The background-sound create and update requests dropped acoustic_source_type the same way. Null does not mean the same thing for every one of these, so the update request does not treat them alike. shared_requests.py applies voice_volume, voice_speed, hold_music_timeout_seconds, situate_speaker, and audio_degradation through `model_fields_set`, deleting the stored value when the key is present and null; those get the explicit_option treatment so an intentional clear survives. It applies background_sound_volume through an `is not None` guard, and tags follow the "None means don't update; [] clears" contract, so both stay plain Option. Modeling all seven the same way would have made half the clears no-ops. acoustic_source_type is likewise cleared by an explicit null on update. explicit_option moves from test_case.rs to common.rs now that a second resource needs it. --audio-degradation takes a bare preset id or the full JSON object, so the common case needs no JSON on the command line.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
The release bridge publishes any main version without a matching tag, so the first feature PR to merge would cut an incomplete release. Feature PRs stay at the base version; a dedicated version-bump PR releases the series.
1 participant
Depends on #122 for the audit exceptions; the code here stands alone. Touches
common.rsandtest_case.rs, so it conflicts with nothing in #123 but wants landing after it.Why
CreatePersonaRequestandUpdatePersonaRequestdeclared seven fewer fields than the API publishes and serves, and the background-sound requests droppedacoustic_source_type.input_json::finishdeserializes into the struct, so passing any of them by flag or--input-jsonreturned 200 and changed nothing.The part worth reviewing carefully: null is not one rule
backendsrc/services/api/v1/personas/common/shared_requests.pyapplies these fields two different ways, and the CLI has to match each:voice_volume,voice_speed,hold_music_timeout_seconds,situate_speaker,audio_degradationin model_fields_setandis None→ delete the stored valueOption<Option<T>>+explicit_optionbackground_sound_volumeis not Noneguard → null is the same as omittedOption<T>tags[]clears"Option<Vec<String>>Modeling all seven the same way would have turned half the clears into silent no-ops — the exact failure
skip_serializing_ifalone produces, and the reason COVAL-5805 introducedexplicit_option.acoustic_source_typeis cleared by an explicit null too (background_sounds/service.pyline 491).Note this is the opposite of metrics in #123, where null and omitted genuinely do mean the same thing, so
tagsthere is plainOption. The rule is per field, not per resource.Other changes
explicit_optionmoves fromtest_case.rstocommon.rsnow that a second resource needs it.test_case.rskeeps its behavior; its tests are untouched and pass.--audio-degradationtakes a bare preset id (cell-handoff) or the full{"preset": …}object, so the common case needs no JSON on the command line.--input-json '{"situate_speaker":null}'. Documented in the README.Verification
Eight new tests. Two matter most: one asserts an explicit null is actually sent as null rather than dropped, and one asserts an unset clearable field is still omitted — the two halves of the contract.
Running the #122 audit against this tree moves modeled request fields from 280/360 to 296/360 and reports exactly these 16 entries as stale exceptions, with no new extras.
Minor version bump to 0.8.0.
On merge order
Cut from
main, so it does not carry #122'sapi-coverage.tomlsections. After #122 merges, rebase and delete the 16[[known_field_gap]]entries forPOST/PATCH /personasandPOST/PATCH /personas/background-sounds, regenerateapi-coverage-report.md, and re-apply the version bump.test_update_check_notifies_when_outdatedfails under parallelcargo testhere exactly as on cleanmain. Pre-existing, COVAL-5826.