Skip to content

Commit

Permalink
Fix mis-merge with main, and improve timeseries name regex
Browse files Browse the repository at this point in the history
  • Loading branch information
bnaecker committed Dec 11, 2023
1 parent 07a2023 commit 8871f11
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 35 deletions.
23 changes: 0 additions & 23 deletions oximeter/db/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -281,31 +281,8 @@ mod tests {
use super::*;
use crate::model::DbFieldList;
use crate::model::DbTimeseriesSchema;
use std::convert::TryFrom;
use uuid::Uuid;

#[test]
fn test_timeseries_name() {
let name = TimeseriesName::try_from("foo:bar").unwrap();
assert_eq!(format!("{}", name), "foo:bar");
}

#[test]
fn test_timeseries_name_from_str() {
assert!(TimeseriesName::try_from("a:b").is_ok());
assert!(TimeseriesName::try_from("a_a:b_b").is_ok());
assert!(TimeseriesName::try_from("a0:b0").is_ok());
assert!(TimeseriesName::try_from("a_0:b_0").is_ok());

assert!(TimeseriesName::try_from("_:b").is_err());
assert!(TimeseriesName::try_from("a_:b").is_err());
assert!(TimeseriesName::try_from("0:b").is_err());
assert!(TimeseriesName::try_from(":b").is_err());
assert!(TimeseriesName::try_from("a:").is_err());
assert!(TimeseriesName::try_from("123").is_err());
assert!(TimeseriesName::try_from("no:no:no").is_err());
}

// Validates that the timeseries_key stability for a sample is stable.
#[test]
fn test_timeseries_key_sample_stability() {
Expand Down
29 changes: 18 additions & 11 deletions oximeter/db/src/sql/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -756,6 +756,23 @@ impl RestrictedQuery {
}
}

// Verify that the identifier is a single, concrete timeseries name.
fn extract_timeseries_name(
from: &[Ident],
) -> Result<IndexSet<TimeseriesName>, OxdbError> {
if from.len() != 1 {
return unsupported!(
"Query must select from single named \
timeseries, with no database"
);
}
from[0]
.value
.parse()
.map(|n| indexmap::indexset! { n })
.map_err(|_| MetricsError::InvalidTimeseriesName.into())
}

// Process a single "table factor", the <object> in `FROM <object>` to
// extract the names of the timeseries it refers to.
//
Expand All @@ -770,17 +787,7 @@ impl RestrictedQuery {
"Table functions and hints are not supported"
);
}
if name.0.len() != 1 {
return unsupported!(
"Query must select from single named \
table, with no database"
);
}
let timeseries_name = name.0[0]
.value
.parse()
.map(|n| indexmap::indexset! { n })
.map_err(|_| MetricsError::InvalidTimeseriesName)?;
let timeseries_name = Self::extract_timeseries_name(&name.0)?;
// Rewrite the quote style to be backticks, so that the
// resulting actual query translates into a valid identifier for
// ClickHouse, naming the CTE's well generate later.
Expand Down
3 changes: 2 additions & 1 deletion oximeter/oximeter/src/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ impl PartialEq for TimeseriesSchema {
//
// That describes the target/metric name, and the timeseries is two of those, joined with ':'.
const TIMESERIES_NAME_REGEX: &str =
"(([a-z]+[a-z0-9]*)(_([a-z0-9]+))*):(([a-z]+[a-z0-9]*)(_([a-z0-9]+))*)";
"^(([a-z]+[a-z0-9]*)(_([a-z0-9]+))*):(([a-z]+[a-z0-9]*)(_([a-z0-9]+))*)$";

/// A set of timeseries schema, useful for testing changes to targets or
/// metrics.
Expand Down Expand Up @@ -502,6 +502,7 @@ mod tests {
assert!(TimeseriesName::try_from(":b").is_err());
assert!(TimeseriesName::try_from("a:").is_err());
assert!(TimeseriesName::try_from("123").is_err());
assert!(TimeseriesName::try_from("x.a:b").is_err());
}

#[derive(Target)]
Expand Down

0 comments on commit 8871f11

Please sign in to comment.