Skip to content

Commit

Permalink
More review feedback
Browse files Browse the repository at this point in the history 
- Actually rename authz variant
- Inline schema error variant creation
- Add inline-error-chain to preserve TOML failures more accurately
  • Loading branch information
@bnaecker
bnaecker committed Jun 18, 2024
1 parent dca0355 commit a410171
Show file tree
Hide file tree
Showing 4 changed files with 68 additions and 56 deletions.
1 change: 1 addition & 0 deletions Cargo.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions oximeter/impl/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ regex.workspace = true
schemars = { workspace = true, features = [ "uuid1", "bytes", "chrono" ] }
serde.workspace = true
serde_json.workspace = true
slog-error-chain.workspace = true
strum.workspace = true
syn.workspace = true
toml.workspace = true
Expand Down
120 changes: 65 additions & 55 deletions oximeter/impl/src/schema/ir.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,14 +78,14 @@ impl TimeseriesDefinition {
self,
) -> Result<Vec<TimeseriesSchema>, MetricsError> {
if self.target.versions.is_empty() {
return defn_error(String::from(
return Err(MetricsError::SchemaDefinition(String::from(
"At least one target version must be defined",
));
)));
}
if self.metrics.is_empty() {
return defn_error(String::from(
return Err(MetricsError::SchemaDefinition(String::from(
"At least one metric must be defined",
));
)));
}
let mut timeseries = BTreeMap::new();
let target_name = &self.target.name;
Expand All @@ -105,11 +105,11 @@ impl TimeseriesDefinition {
.iter()
.any(|fields| fields.version.get() > 1)
{
return defn_error(String::from(
return Err(MetricsError::SchemaDefinition(String::from(
"Exactly one timeseries version, with version number 1, \
may currently be specified. Updates will be supported \
in future releases.",
));
)));
}

// First create a map from target version to the fields in it.
Expand All @@ -121,30 +121,30 @@ impl TimeseriesDefinition {
(1u8..).zip(self.target.versions.iter())
{
if expected_version != target_fields.version.get() {
return defn_error(format!(
return Err(MetricsError::SchemaDefinition(format!(
"Target '{}' versions should be sequential \
and monotonically increasing (expected {}, found {})",
target_name, expected_version, target_fields.version,
));
)));
}

let fields: BTreeSet<_> =
target_fields.fields.iter().cloned().collect();
if fields.len() != target_fields.fields.len() {
return defn_error(format!(
return Err(MetricsError::SchemaDefinition(format!(
"Target '{}' version {} lists duplicate field names",
target_name, expected_version,
));
)));
}

if target_fields_by_version
.insert(expected_version, fields)
.is_some()
{
return defn_error(format!(
return Err(MetricsError::SchemaDefinition(format!(
"Target '{}' version {} is duplicated",
target_name, expected_version,
));
)));
}
}

Expand Down Expand Up @@ -186,10 +186,14 @@ impl TimeseriesDefinition {
let Some(target_fields) =
target_fields_by_version.get(&last_target_version)
else {
return defn_error(format!(
"Metric '{}' version {} does not have \
return Err(MetricsError::SchemaDefinition(
format!(
"Metric '{}' version {} does not have \
a matching version in the target '{}'",
metric_name, last_target_version, target_name,
metric_name,
last_target_version,
target_name,
),
));
};
let field_schema = construct_field_schema(
Expand Down Expand Up @@ -226,9 +230,11 @@ impl TimeseriesDefinition {
if let Some(old) = timeseries
.insert((timeseries_name, version), schema)
{
return defn_error(format!(
"Timeseries '{}' version {} is duplicated",
old.timeseries_name, old.version,
return Err(MetricsError::SchemaDefinition(
format!(
"Timeseries '{}' version {} is duplicated",
old.timeseries_name, old.version,
),
));
}
}
Expand All @@ -252,11 +258,11 @@ impl TimeseriesDefinition {
let Some(target_fields) =
target_fields_by_version.get(&last_target_version)
else {
return defn_error(format!(
return Err(MetricsError::SchemaDefinition(format!(
"Metric '{}' version {} does not have \
a matching version in the target '{}'",
metric_name, last_target_version, target_name,
));
)));
};
let field_schema = construct_field_schema(
&self.fields,
Expand Down Expand Up @@ -292,10 +298,10 @@ impl TimeseriesDefinition {
if let Some(old) =
timeseries.insert((timeseries_name, version), schema)
{
return defn_error(format!(
return Err(MetricsError::SchemaDefinition(format!(
"Timeseries '{}' version {} is duplicated",
old.timeseries_name, old.version,
));
)));
}
}

Expand Down Expand Up @@ -358,9 +364,11 @@ impl TimeseriesDefinition {
if let Some(old) = timeseries
.insert((timeseries_name, version), schema)
{
return defn_error(format!(
return Err(MetricsError::SchemaDefinition(
format!(
"Timeseries '{}' version {} is duplicated",
old.timeseries_name, old.version,
),
));
}
}
Expand Down Expand Up @@ -394,16 +402,16 @@ impl CurrentVersion {
}
}

fn defn_error<T>(s: String) -> Result<T, MetricsError> {
Err(MetricsError::SchemaDefinition(s))
}

/// Load the list of timeseries schema from a schema definition in TOML format.
pub fn load_schema(
contents: &str,
) -> Result<Vec<TimeseriesSchema>, MetricsError> {
toml::from_str::<TimeseriesDefinition>(contents)
.map_err(|e| MetricsError::Toml(e.to_string()))
.map_err(|e| {
MetricsError::Toml(
slog_error_chain::InlineErrorChain::new(&e).to_string(),
)
})
.and_then(TimeseriesDefinition::into_schema_list)
}

Expand Down Expand Up @@ -447,11 +455,11 @@ fn extract_authz_scope(
}) {
Ok(())
} else {
defn_error(format!(
Err(MetricsError::SchemaDefinition(format!(
"Metric '{}' has '{}' authorization scope, and so must \
contain a field '{}' of UUID type",
metric_name, scope, key,
))
)))
}
};
match authz_scope {
Expand All @@ -470,11 +478,11 @@ fn construct_field_schema(
metric_field_names: &BTreeSet<String>,
) -> Result<BTreeSet<FieldSchema>, MetricsError> {
if let Some(dup) = target_fields.intersection(&metric_field_names).next() {
return defn_error(format!(
return Err(MetricsError::SchemaDefinition(format!(
"Field '{}' is duplicated between target \
'{}' and metric '{}'",
dup, target_name, metric_name,
));
)));
}

let mut field_schema = BTreeSet::new();
Expand All @@ -491,11 +499,11 @@ fn construct_field_schema(
} else {
("metric", metric_name)
};
return defn_error(format!(
return Err(MetricsError::SchemaDefinition(format!(
"Field '{}' is referenced in the {} '{}', but it \
does not appear in the set of all fields.",
field_name, kind, name,
));
)));
};
validate_field_name(field_name, metadata)?;
field_schema.insert(FieldSchema {
Expand All @@ -521,18 +529,18 @@ fn validate_field_name(
metadata: &FieldMetadata,
) -> Result<(), MetricsError> {
if !is_valid_ident_name(field_name) {
return defn_error(format!(
return Err(MetricsError::SchemaDefinition(format!(
"Field name '{}' should be a valid identifier in snake_case",
field_name,
));
)));
}
if metadata.type_ == FieldType::Uuid
&& !(field_name.ends_with("_id") || field_name == "id")
{
return defn_error(format!(
return Err(MetricsError::SchemaDefinition(format!(
"Uuid field '{}' should end with '_id' or equal 'id'",
field_name,
));
)));
}
Ok(())
}
Expand All @@ -550,49 +558,49 @@ fn extract_metric_fields<'a>(
// removed. Bump the version and mark it active,
// but there are no fields to return here.
if removed_in <= version {
return defn_error(format!(
return Err(MetricsError::SchemaDefinition(format!(
"Metric '{}' is removed in version \
{}, which is not strictly after the \
current active version, {}",
metric_name, removed_in, version,
));
)));
}
CurrentVersion::Inactive { removed_in: *removed_in }
}
Some(CurrentVersion::Inactive { removed_in }) => {
return defn_error(format!(
return Err(MetricsError::SchemaDefinition(format!(
"Metric '{}' was already removed in \
version {}, it cannot be removed again",
metric_name, removed_in,
));
)));
}
None => {
return defn_error(format!(
return Err(MetricsError::SchemaDefinition(format!(
"Metric {} has no previous version, \
it cannot be removed.",
metric_name,
));
)));
}
}
}
MetricFields::Added { added_in, fields } => {
match current_version {
Some(CurrentVersion::Active { .. }) => {
return defn_error(format!(
return Err(MetricsError::SchemaDefinition(format!(
"Metric '{}' is already active, it \
cannot be added again until it is removed.",
metric_name,
));
)));
}
Some(CurrentVersion::Inactive { removed_in }) => {
// The metric is currently inactive, just check
// that the newly-active version is greater.
if added_in <= removed_in {
return defn_error(format!(
return Err(MetricsError::SchemaDefinition(format!(
"Re-added metric '{}' must appear in a later \
version than the one in which it was removed ({})",
metric_name, removed_in,
));
)));
}
CurrentVersion::Active {
version: *added_in,
Expand All @@ -615,13 +623,13 @@ fn extract_metric_fields<'a>(
// The happy-path, we're stepping the version
// and possibly modifying the fields.
if new_fields.version <= *version {
return defn_error(format!(
return Err(MetricsError::SchemaDefinition(format!(
"Metric '{}' version should increment, \
expected at least {}, found {}",
metric_name,
version.checked_add(1).expect("version < 256"),
new_fields.version,
));
)));
}
CurrentVersion::Active {
version: new_fields.version,
Expand All @@ -634,20 +642,20 @@ fn extract_metric_fields<'a>(
Some(CurrentVersion::Inactive { removed_in }) => {
// The metric has been removed in the past, it
// needs to be added again first.
return defn_error(format!(
return Err(MetricsError::SchemaDefinition(format!(
"Metric '{}' was removed in version {}, \
it must be added again first",
metric_name, removed_in,
));
)));
}
None => {
// The metric never existed, it must be added
// first.
return defn_error(format!(
return Err(MetricsError::SchemaDefinition(format!(
"Metric '{}' must be added in at its first \
version, and can then be modified",
metric_name,
));
)));
}
}
}
Expand All @@ -661,7 +669,9 @@ fn to_unique_field_names(
) -> Result<BTreeSet<String>, MetricsError> {
let set: BTreeSet<_> = fields.iter().cloned().collect();
if set.len() != fields.len() {
return defn_error(format!("Object '{name}' has duplicate fields"));
return Err(MetricsError::SchemaDefinition(format!(
"Object '{name}' has duplicate fields"
)));
}
Ok(set)
}
Expand Down
2 changes: 1 addition & 1 deletion oximeter/impl/src/schema/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,7 @@ pub enum AuthzScope {
/// Timeseries data is limited to the authorized projects for a user.
Project,
/// The timeseries is viewable to all without limitation.
None,
ViewableToAll,
}

impl AuthzScope {
Expand Down

0 comments on commit a410171

Please sign in to comment.