Skip to content

fix: handle unknown metric types in API responses #40

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lorenjphillips merged 2 commits into from
Mar 12, 2026
Merged

fix: handle unknown metric types in API responses #40

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
83f5a3c
fix: handle unknown metric types in API responses
lorenjphillips Mar 11, 2026
9cdeaa5
fix: display unknown metric types as BUILT_IN
lorenjphillips Mar 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/client/models/metric.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@ pub enum MetricType {
#[serde(rename = "METRIC_COMPOSITE_EVALUATION")]
#[value(name = "composite")]
CompositeEvaluation,
#[serde(other)]
#[value(skip)]
Unknown,
Comment on lines +82 to +84

@coderabbitai coderabbitai Bot Mar 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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.

}

impl std::fmt::Display for MetricType {
Expand All @@ -95,6 +98,7 @@ impl std::fmt::Display for MetricType {
Self::Regex => write!(f, "REGEX"),
Self::Pause => write!(f, "PAUSE"),
Self::CompositeEvaluation => write!(f, "COMPOSITE"),
Self::Unknown => write!(f, "BUILT_IN"),

@coderabbitai coderabbitai Bot Mar 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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.

}
}
}
Expand Down
Loading