fix: handle unknown metric types in API responses#40
Conversation
Add serde(other) catch-all variant to MetricType enum so the CLI gracefully handles new metric types added to the backend API without crashing on deserialization.
📝 WalkthroughWalkthroughA new Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/models/metric.rs`:
- Line 101: The Display implementation for the metric enum currently maps
Self::Unknown to "BUILT_IN", which mislabels any future/unrecognized server
metric types; update the mapping in the Display (the branch matching
Self::Unknown) to return a neutral label such as "UNKNOWN" (or similar) instead
of "BUILT_IN" so unknown variants are represented correctly and are easier to
spot.
- Around line 82-84: The MetricType enum's Unknown variant can be serialized
back into outbound requests (used in CreateMetricRequest and
UpdateMetricRequest) which yields an invalid literal; fix by preventing Unknown
from being written: either split into separate request/response enums so
requests never include Unknown, or implement a custom Serialize for MetricType
that returns an error (or Serde::serialize_str with an Err) when encountering
the Unknown variant; update references to MetricType in CreateMetricRequest and
UpdateMetricRequest to use the new request-only enum or ensure serialization
rejects Unknown so invalid "Unknown" strings are never sent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3287ffa9-85ff-4b9b-bddd-ec480b2bcf95
📒 Files selected for processing (1)
src/client/models/metric.rs
| #[serde(other)] | ||
| #[value(skip)] | ||
| Unknown, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In serde 1.x, when an enum derives Serialize/Deserialize and one unit variant is annotated with #[serde(other)], how is that catch-all variant serialized if it has no #[serde(rename = "...")]?
💡 Result:
#[serde(other)] only affects deserialization (it’s the fallback when the input tag doesn’t match any other variant). It does not change how the variant is serialized. So if you serialize that unit variant and it has no #[serde(rename = "...")], Serde will serialize it like any normal unit variant, using its (possibly rename_all-transformed) variant name as the tag value (e.g., Unknown → "Unknown" unless renamed by rules). [1]
Source:
[1] (serde.rs)
Citations:
Block Unknown from outbound request serialization.
The Unknown variant will be serialized as "Unknown" by the derived Serialize impl, which is not a valid backend enum literal. Since MetricType is used in CreateMetricRequest and UpdateMetricRequest, a deserialized unknown value can leak into outbound requests as invalid data. Either split request and response enums or add a manual Serialize impl that rejects Unknown on writes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/models/metric.rs` around lines 82 - 84, The MetricType enum's
Unknown variant can be serialized back into outbound requests (used in
CreateMetricRequest and UpdateMetricRequest) which yields an invalid literal;
fix by preventing Unknown from being written: either split into separate
request/response enums so requests never include Unknown, or implement a custom
Serialize for MetricType that returns an error (or Serde::serialize_str with an
Err) when encountering the Unknown variant; update references to MetricType in
CreateMetricRequest and UpdateMetricRequest to use the new request-only enum or
ensure serialization rejects Unknown so invalid "Unknown" strings are never
sent.
| Self::Regex => write!(f, "REGEX"), | ||
| Self::Pause => write!(f, "PAUSE"), | ||
| Self::CompositeEvaluation => write!(f, "COMPOSITE"), | ||
| Self::Unknown => write!(f, "BUILT_IN"), |
There was a problem hiding this comment.
Don't label every unknown server value as BUILT_IN.
Unknown now covers any future metric type, not just builtin ones. Rendering it as BUILT_IN turns the fallback into incorrect user-facing data and makes new backend types harder to spot. A neutral label like UNKNOWN would be safer here.
Suggested change
- Self::Unknown => write!(f, "BUILT_IN"),
+ Self::Unknown => write!(f, "UNKNOWN"),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Self::Unknown => write!(f, "BUILT_IN"), | |
| Self::Unknown => write!(f, "UNKNOWN"), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/models/metric.rs` at line 101, The Display implementation for the
metric enum currently maps Self::Unknown to "BUILT_IN", which mislabels any
future/unrecognized server metric types; update the mapping in the Display (the
branch matching Self::Unknown) to return a neutral label such as "UNKNOWN" (or
similar) instead of "BUILT_IN" so unknown variants are represented correctly and
are easier to spot.
2 participants
Summary
#[serde(other)]catch-allUnknownvariant to theMetricTypeenumMETRIC_TRACE_AUDIO_UPLOAD_STT_WER)--typeCLI arg via#[value(skip)]so users can't create metrics with unknown typeTest plan
cargo test— 17 passedcargo clippy -- -D warnings— cleancoval metrics list --include-builtin— returns results instead of crashingSummary by CodeRabbit