[COVAL-5820] Model the dropped metric request fields - #123
Conversation
CreateMetricRequest and UpdateMetricRequest declared 16 fewer fields than the
API publishes and serves, so `coval metrics create/update` discarded them in
silence: max_silence_duration_seconds, min_silence_gap_seconds,
frequency_threshold, direction, success_sentiments, percent_above,
success_end_reasons, observation_name, expected_body, match_path,
min_volume_change_for_pitch_misalignment, threshold, operator, sql_query,
runtime_config, and tags. Every one is present on the served model in
src/services/api/v1/metrics/common/metric_request_schemas.py.
Each gets a flag alongside the existing ones. expected_body is a string or a
JSON object, so unparseable input is sent as the literal string rather than
rejected. runtime_config parses as JSON like target_condition already does.
tags takes plain Option here, not the explicit_option treatment: the metric
update contract is "None means don't update; [] clears", so absent and null
already mean the same thing and only an empty list clears.
POST /v1/metrics/{metric_id}/test gains simulation_output_ids. That makes the
singular field optional, so the command validates that exactly one target is
given rather than relaying a 400. The batch response reports per simulation
output in results and leaves the deprecated metric_output_ulid null, so the
response models both; a single-target call still returns the ulid it always did.
Two served fields stay unmodeled deliberately: aggregation_method and unit are
absent from the published schema, so adding them would trade a silent drop for
an unreviewed extra.
|
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 |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Depends on #122 for the audit exceptions; the code here stands alone.
Why
CreateMetricRequestandUpdateMetricRequestdeclared 16 fewer fields than the API publishes and serves.input_json::finishdeserializes the assembled JSON into the struct, so a caller passing any of them — by flag or--input-json— got a 200 and no effect.Verified against the served models in
backendsrc/services/api/v1/metrics/common/metric_request_schemas.py, not just the published YAML.What changed
Both request structs gain:
max_silence_duration_seconds,min_silence_gap_seconds,frequency_threshold,direction,success_sentiments,percent_above,success_end_reasons,observation_name,expected_body,match_path,min_volume_change_for_pitch_misalignment,threshold,operator,sql_query,runtime_config,tags— each with a matching flag.--expected-bodyaccepts a JSON value or a plain string, matching the API'soneOf. Unparseable input is sent as the literal string rather than rejected.--runtime-configparses as JSON, like the existing--target-condition.tagsuses plainOption, not theexplicit_optiontreatment from COVAL-5805. The metric update contract is "None means don't update;[]clears", so absent and null already mean the same thing and only an empty list clears. The agent merge-patch is the opposite and will needOption<Option<T>>in COVAL-5823 — worth not copying the wrong pattern across resources.POST /v1/metrics/{metric_id}/testgainssimulation_output_ids. That makes the singular field optional, so the command validates that exactly one target is given rather than relaying a 400. The batch response reports per simulation output inresultsand leaves the deprecatedmetric_output_ulidnull, soTestMetricResponsemodels both; a single-target call still returns the ulid it always did, and the two existingmetrics testtests pass unchanged.Deliberately not modeled
aggregation_methodandunitare served but absent from the published schema (model_extra_fieldin the backend's ownopenapi_parity_baseline.txt). Adding them would trade a silent drop for an unreviewed extra, so they stay out until the spec catches up.Verification
Ten new tests, including one that asserts every newly modeled field survives the round trip and one that asserts an unset field is still omitted.
Running the #122 audit against this tree moves modeled request fields from 280/360 to 313/360 and reports exactly these 33 entries as stale exceptions — nothing else changes, and no new extras appear.
Minor version bump to 0.8.0.
On merge order
This branch is cut from
main, so it does not carry #122'sapi-coverage.tomlsections. After #122 merges, rebase and delete the 33[[known_field_gap]]entries forPOST /metrics,PATCH /metrics/{metric_id}, andPOST /metrics/{metric_id}/test simulation_output_ids, then regenerateapi-coverage-report.mdand re-apply the version bump.cargo testalso failstest_update_check_notifies_when_outdatedunder parallel execution here, exactly as it does on cleanmain. Pre-existing, filed as COVAL-5826.