[COVAL-5822] Model the dropped review-project and report request fields - #125
Conversation
|
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.
Review projects lost project_rules, blind_labeling_shown_metric_ids, and enforced_collaboration on create, and additionally add_linked_simulation_ids, remove_linked_simulation_ids, and metric_addition_completion_action on update. The two membership deltas are the ones that cost something: without them the CLI could only replace linked_simulation_ids wholesale, so there was no way to add or remove one simulation without re-sending the whole set. The API rejects a delta combined with the replacement, so the flags conflict with --simulation-ids. Reports lost simulation_output_ids and source_human_review_project_id on create, and additionally view_config on update. The report PATCH applies simulation_output_ids and source_human_review_project_id through model_fields_set, so an explicit null unpins the report; both take explicit_option to keep that distinct from an omission. view_config is a nested partial-update object the API validates and never accepts as null, so it stays a plain optional JSON value behind --view-config. Every review-project field is applied under an `is not None` guard, so none of them need the null-versus-omitted treatment. explicit_option moves from test_case.rs to common.rs. COVAL-5821 makes the same move; the content is identical, so whichever lands second drops the hunk.
c68e717 to
ac3e4a1
Compare
1 participant
Depends on #122 for the audit exceptions; the code here stands alone. Shares the
explicit_optionmove with #124 (identical content, so the second one to land drops that hunk).Why
Nine review-project fields and five report fields were published and served but absent from the CLI's request structs, so
input_json::finishdropped them and the call returned 200 having ignored them.Review projects
Create gains
project_rules,blind_labeling_shown_metric_ids,enforced_collaboration. Update gains those plusadd_linked_simulation_ids,remove_linked_simulation_ids,metric_addition_completion_action.The two membership deltas are the ones that actually cost something. Without them the CLI could only replace
linked_simulation_idswholesale, so adding one simulation to a project meant re-sending the entire set — and racing anyone else editing it. The API rejects a delta combined with the replacement, so--add-simulation-ids/--remove-simulation-idsareconflicts_with = "simulation_ids"and fail locally with a clear message.Every review-project field is applied under an
is not Noneguard inupdate_core.py, so none of them need null-versus-omitted handling.Reports
Create gains
simulation_output_ids,source_human_review_project_id. Update gains those plusview_config.The report PATCH is different from the review-project one:
api/v1/reports/update/service.pyappliessimulation_output_idsandsource_human_review_project_idthroughmodel_fields_set, so an explicit null unpins the report. Both takeexplicit_option(Option<Option<T>>) so a deliberate unpin is not silently downgraded to "leave it alone".view_configis a nested partial-update object (ReportViewConfigurationPatch) withextra="forbid"and its own validators, and it explicitly cannot be null. Modeling it as a typed Rust struct would duplicate validation the API already owns, so it stays a plain optional JSON value behind--view-config, like the existing--target-condition.Verification
Nine new tests, including the explicit-null unpin, the "unset stays omitted" counterpart, and the delta/replacement conflict.
Running the #122 audit against this tree moves modeled request fields from 280/360 to 294/360 and reports exactly these 14 entries as stale exceptions, with no new extras.
Minor version bump to 0.8.0.
On merge order
Cut from
main. After #122 merges, rebase and delete the 14[[known_field_gap]]entries forPOST/PATCH /review-projectsandPOST/PATCH /reports, 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.