diff --git a/crates/weaver_resolved_schema/src/instrumentation_library.rs b/crates/weaver_resolved_schema/src/instrumentation_library.rs index 3ba47f4fa..cf16410a7 100644 --- a/crates/weaver_resolved_schema/src/instrumentation_library.rs +++ b/crates/weaver_resolved_schema/src/instrumentation_library.rs @@ -9,7 +9,7 @@ use serde::{Deserialize, Serialize}; /// An instrumentation library specification. /// MUST be used both by applications and libraries. -#[derive(Serialize, Deserialize, Debug, JsonSchema)] +#[derive(Serialize, Deserialize, Debug, Clone, JsonSchema)] #[serde(deny_unknown_fields)] pub struct InstrumentationLibrary { /// An optional name for the instrumentation library. diff --git a/crates/weaver_resolved_schema/src/lib.rs b/crates/weaver_resolved_schema/src/lib.rs index 72a99612e..202884587 100644 --- a/crates/weaver_resolved_schema/src/lib.rs +++ b/crates/weaver_resolved_schema/src/lib.rs @@ -46,7 +46,7 @@ pub(crate) const V2_RESOLVED_FILE_FORMAT: &str = "resolved/2.0"; /// A Resolved Telemetry Schema. /// A Resolved Telemetry Schema is self-contained and doesn't contain any /// external references to other schemas or semantic conventions. -#[derive(Debug)] +#[derive(Debug, Clone)] pub struct ResolvedTelemetrySchema { /// Version of the file structure. pub file_format: String, diff --git a/crates/weaver_resolved_schema/src/signal.rs b/crates/weaver_resolved_schema/src/signal.rs index 22a39adf0..379f71311 100644 --- a/crates/weaver_resolved_schema/src/signal.rs +++ b/crates/weaver_resolved_schema/src/signal.rs @@ -11,7 +11,7 @@ use crate::metric::MetricRef; use crate::tags::Tags; /// A univariate metric signal. -#[derive(Serialize, Deserialize, Debug, JsonSchema)] +#[derive(Serialize, Deserialize, Debug, Clone, JsonSchema)] #[serde(deny_unknown_fields)] pub struct UnivariateMetric { /// References to attributes defined in the catalog. @@ -25,7 +25,7 @@ pub struct UnivariateMetric { } /// A multivariate metric signal. -#[derive(Serialize, Deserialize, Debug, JsonSchema)] +#[derive(Serialize, Deserialize, Debug, Clone, JsonSchema)] #[serde(deny_unknown_fields)] pub struct MultivariateMetric { /// The name of the multivariate metric. @@ -46,7 +46,7 @@ pub struct MultivariateMetric { } /// An event specification, used for both Span and Log events. -#[derive(Serialize, Deserialize, Debug, JsonSchema)] +#[derive(Serialize, Deserialize, Debug, Clone, JsonSchema)] #[serde(deny_unknown_fields)] pub struct Event { /// The name of the event @@ -68,7 +68,7 @@ pub struct Event { } /// A span signal. -#[derive(Serialize, Deserialize, Debug, JsonSchema)] +#[derive(Serialize, Deserialize, Debug, Clone, JsonSchema)] #[serde(deny_unknown_fields)] pub struct Span { /// The name of the span. @@ -111,7 +111,7 @@ pub enum SpanKind { } /// A span link specification. -#[derive(Serialize, Deserialize, Debug, JsonSchema)] +#[derive(Serialize, Deserialize, Debug, Clone, JsonSchema)] #[serde(deny_unknown_fields)] pub struct SpanLink { /// The name of the span link. diff --git a/crates/weaver_resolver/data/compatible-version-conflict/main/registry_manifest.yaml b/crates/weaver_resolver/data/compatible-version-conflict/main/registry_manifest.yaml new file mode 100644 index 000000000..ffff9aed6 --- /dev/null +++ b/crates/weaver_resolver/data/compatible-version-conflict/main/registry_manifest.yaml @@ -0,0 +1,9 @@ +name: main +description: Main Registry +semconv_version: 0.1.0 +schema_base_url: https://example.com/main/ +dependencies: + - schema_url: https://example.com/a/0.1.0 + registry_path: data/compatible-version-conflict/registry_a + - schema_url: https://example.com/b/0.1.0 + registry_path: data/compatible-version-conflict/registry_b diff --git a/crates/weaver_resolver/data/compatible-version-conflict/registry_a/registry_manifest.yaml b/crates/weaver_resolver/data/compatible-version-conflict/registry_a/registry_manifest.yaml new file mode 100644 index 000000000..00edfcdbb --- /dev/null +++ b/crates/weaver_resolver/data/compatible-version-conflict/registry_a/registry_manifest.yaml @@ -0,0 +1,7 @@ +name: registry_a +description: Registry A +semconv_version: 0.1.0 +schema_base_url: https://example.com/a/ +dependencies: + - schema_url: https://example.com/c/1.1.0 + registry_path: data/compatible-version-conflict/registry_c_v1_1 diff --git a/crates/weaver_resolver/data/compatible-version-conflict/registry_b/registry_manifest.yaml b/crates/weaver_resolver/data/compatible-version-conflict/registry_b/registry_manifest.yaml new file mode 100644 index 000000000..cebb5ca7e --- /dev/null +++ b/crates/weaver_resolver/data/compatible-version-conflict/registry_b/registry_manifest.yaml @@ -0,0 +1,7 @@ +name: registry_b +description: Registry B +semconv_version: 0.1.0 +schema_base_url: https://example.com/b/ +dependencies: + - schema_url: https://example.com/c/1.2.0 + registry_path: data/compatible-version-conflict/registry_c_v1_2 diff --git a/crates/weaver_resolver/data/compatible-version-conflict/registry_c_v1_1/registry_manifest.yaml b/crates/weaver_resolver/data/compatible-version-conflict/registry_c_v1_1/registry_manifest.yaml new file mode 100644 index 000000000..0f2f5759e --- /dev/null +++ b/crates/weaver_resolver/data/compatible-version-conflict/registry_c_v1_1/registry_manifest.yaml @@ -0,0 +1,4 @@ +name: registry_c +description: Registry C v1.1 +semconv_version: 1.1.0 +schema_base_url: https://example.com/c/ diff --git a/crates/weaver_resolver/data/compatible-version-conflict/registry_c_v1_2/registry_manifest.yaml b/crates/weaver_resolver/data/compatible-version-conflict/registry_c_v1_2/registry_manifest.yaml new file mode 100644 index 000000000..087c89e6c --- /dev/null +++ b/crates/weaver_resolver/data/compatible-version-conflict/registry_c_v1_2/registry_manifest.yaml @@ -0,0 +1,4 @@ +name: registry_c +description: Registry C v1.2 +semconv_version: 1.2.0 +schema_base_url: https://example.com/c/ diff --git a/crates/weaver_resolver/data/incompatible-version-conflict/main/registry_manifest.yaml b/crates/weaver_resolver/data/incompatible-version-conflict/main/registry_manifest.yaml new file mode 100644 index 000000000..c51d5ac79 --- /dev/null +++ b/crates/weaver_resolver/data/incompatible-version-conflict/main/registry_manifest.yaml @@ -0,0 +1,9 @@ +name: main +description: Main Registry +semconv_version: 0.1.0 +schema_base_url: https://example.com/main/ +dependencies: + - schema_url: https://example.com/a/0.1.0 + registry_path: data/incompatible-version-conflict/registry_a + - schema_url: https://example.com/b/0.1.0 + registry_path: data/incompatible-version-conflict/registry_b diff --git a/crates/weaver_resolver/data/incompatible-version-conflict/registry_a/registry_manifest.yaml b/crates/weaver_resolver/data/incompatible-version-conflict/registry_a/registry_manifest.yaml new file mode 100644 index 000000000..59a2c17e4 --- /dev/null +++ b/crates/weaver_resolver/data/incompatible-version-conflict/registry_a/registry_manifest.yaml @@ -0,0 +1,7 @@ +name: registry_a +description: Registry A +semconv_version: 0.1.0 +schema_base_url: https://example.com/a/ +dependencies: + - schema_url: https://example.com/c/1.0.0 + registry_path: data/incompatible-version-conflict/registry_c_v1 diff --git a/crates/weaver_resolver/data/incompatible-version-conflict/registry_b/registry_manifest.yaml b/crates/weaver_resolver/data/incompatible-version-conflict/registry_b/registry_manifest.yaml new file mode 100644 index 000000000..878b4c307 --- /dev/null +++ b/crates/weaver_resolver/data/incompatible-version-conflict/registry_b/registry_manifest.yaml @@ -0,0 +1,7 @@ +name: registry_b +description: Registry B +semconv_version: 0.1.0 +schema_base_url: https://example.com/b/ +dependencies: + - schema_url: https://example.com/c/2.0.0 + registry_path: data/incompatible-version-conflict/registry_c_v2 diff --git a/crates/weaver_resolver/data/incompatible-version-conflict/registry_c_v1/registry_manifest.yaml b/crates/weaver_resolver/data/incompatible-version-conflict/registry_c_v1/registry_manifest.yaml new file mode 100644 index 000000000..38393f4bd --- /dev/null +++ b/crates/weaver_resolver/data/incompatible-version-conflict/registry_c_v1/registry_manifest.yaml @@ -0,0 +1,4 @@ +name: registry_c +description: Registry C v1 +semconv_version: 1.0.0 +schema_base_url: https://example.com/c/ diff --git a/crates/weaver_resolver/data/incompatible-version-conflict/registry_c_v2/registry_manifest.yaml b/crates/weaver_resolver/data/incompatible-version-conflict/registry_c_v2/registry_manifest.yaml new file mode 100644 index 000000000..20ef2f3d2 --- /dev/null +++ b/crates/weaver_resolver/data/incompatible-version-conflict/registry_c_v2/registry_manifest.yaml @@ -0,0 +1,4 @@ +name: registry_c +description: Registry C v2 +semconv_version: 2.0.0 +schema_base_url: https://example.com/c/ diff --git a/crates/weaver_resolver/src/attribute.rs b/crates/weaver_resolver/src/attribute.rs index cc247132a..2b7e4c507 100644 --- a/crates/weaver_resolver/src/attribute.rs +++ b/crates/weaver_resolver/src/attribute.rs @@ -16,6 +16,7 @@ use weaver_semconv::attribute::AttributeSpec; use weaver_semconv::schema_url::SchemaUrl; use crate::dependency::ResolvedDependency; +use crate::Error; /// A catalog of deduplicated resolved attributes with their corresponding reference. #[derive(Deserialize, Debug, Default, PartialEq)] @@ -153,7 +154,7 @@ impl AttributeCatalog { attr: &AttributeSpec, lineage: Option<&mut GroupLineage>, dependencies: &Vec, - ) -> Option { + ) -> Result, Error> { match attr { AttributeSpec::Ref { r#ref, @@ -173,7 +174,7 @@ impl AttributeCatalog { let mut root_attr: Option<&AttributeWithSource> = self.root_attributes.get(r#ref); // If we fail to find an attribute, check dependencies first. if root_attr.is_none() { - if let Some(at) = dependencies.lookup_attribute(r#ref) { + if let Some(at) = dependencies.lookup_attribute(r#ref)? { _ = self.root_attributes.insert(r#ref.to_owned(), at); root_attr = self.root_attributes.get(r#ref); } @@ -248,9 +249,9 @@ impl AttributeCatalog { ); } - Some(attr_ref) + Ok(Some(attr_ref)) } else { - None + Ok(None) } } AttributeSpec::Id { @@ -298,7 +299,7 @@ impl AttributeCatalog { }, }, ); - Some(self.attribute_ref(attr)) + Ok(Some(self.attribute_ref(attr))) } } } @@ -327,19 +328,67 @@ impl From for Catalog { } } +/// Helper trait for abstracting over V1 and V2 schema. /// Helper trait for abstracting over V1 and V2 schema. trait AttributeLookup { - fn lookup_attribute(&self, key: &str) -> Option; + fn lookup_attribute(&self, key: &str) -> Result, Error>; } impl AttributeLookup for Vec { - fn lookup_attribute(&self, key: &str) -> Option { - self.iter().find_map(|d| d.lookup_attribute(key)) + fn lookup_attribute(&self, key: &str) -> Result, Error> { + let mut matches = vec![]; + for d in self.iter() { + if let Some(at) = d.lookup_attribute(key)? { + matches.push(at); + } + } + // Check provenance of each match. + matches + .iter() + .try_fold(None, |acc: Option, m| { + match acc { + // No previous match, latest takes over. + None => Ok(Some(m.clone())), + // We have a previous match - we need to deal with conflicts. + Some(existing) => { + let result: Result, Error> = + match (&m.source, &existing.source) { + // This should be infeasible if code is accurate. Just pick existing. + (AttributeSource::Local { .. }, AttributeSource::Local { .. }) => { + Ok(Some(existing)) + } + ( + AttributeSource::Local { .. }, + AttributeSource::Dependency { schema_url }, + ) + | ( + AttributeSource::Dependency { schema_url }, + AttributeSource::Local { .. }, + ) => Err(Error::AmbiguousReference { + r#ref: key.to_owned(), + schema_url1: "local".to_owned(), // TODO - pull this form the current schema if we can. + schema_url2: schema_url.to_string(), + }), + ( + AttributeSource::Dependency { schema_url }, + AttributeSource::Dependency { + schema_url: schema_url2, + }, + ) => Err(Error::AmbiguousReference { + r#ref: key.to_owned(), + schema_url1: schema_url.to_string(), + schema_url2: schema_url2.to_string(), + }), + }; + result + } + } + }) } } impl AttributeLookup for ResolvedDependency { - fn lookup_attribute(&self, key: &str) -> Option { + fn lookup_attribute(&self, key: &str) -> Result, Error> { match self { ResolvedDependency::V1(schema) => schema.lookup_attribute(key), ResolvedDependency::V2(schema) => schema.lookup_attribute(key), @@ -348,21 +397,22 @@ impl AttributeLookup for ResolvedDependency { } impl AttributeLookup for V1Schema { - fn lookup_attribute(&self, key: &str) -> Option { - self.catalog + fn lookup_attribute(&self, key: &str) -> Result, Error> { + Ok(self + .catalog .root_attribute(key) .map(|(attr, group_id)| AttributeWithSource { attribute: attr.clone(), source: AttributeSource::Local { group_id: group_id.to_owned(), }, - }) + })) } } impl AttributeLookup for V2Schema { - fn lookup_attribute(&self, key: &str) -> Option { - self.registry.attributes.iter().find_map(|attr_ref| { + fn lookup_attribute(&self, key: &str) -> Result, Error> { + Ok(self.registry.attributes.iter().find_map(|attr_ref| { let attr = self.attribute_catalog.get(attr_ref.0 as usize)?; if attr.key == key { Some(AttributeWithSource { @@ -392,7 +442,7 @@ impl AttributeLookup for V2Schema { } else { None } - }) + })) } } @@ -524,4 +574,167 @@ mod tests { role: None, } } + + #[test] + fn test_lookup_attribute_ambiguous_reference() { + use weaver_resolved_schema::v2::attribute::Attribute as AttributeV2; + use weaver_resolved_schema::v2::ResolvedTelemetrySchema as V2Schema; + + let attr1 = AttributeV2 { + key: "error.type".to_owned(), + r#type: AttributeType::PrimitiveOrArray(PrimitiveOrArrayTypeSpec::String), + examples: None, + common: Default::default(), + provenance: Default::default(), + }; + + let schema1 = V2Schema { + file_format: "resolved/2.0".to_owned(), + schema_url: "http://test/schema/v1".try_into().unwrap(), + attribute_catalog: vec![attr1.clone()], + registry: weaver_resolved_schema::v2::registry::Registry { + attributes: vec![weaver_resolved_schema::v2::attribute::AttributeRef(0)], + spans: vec![], + metrics: vec![], + events: vec![], + entities: vec![], + attribute_groups: vec![], + }, + refinements: weaver_resolved_schema::v2::refinements::Refinements { + spans: vec![], + metrics: vec![], + events: vec![], + }, + dependencies: Default::default(), + }; + + let schema2 = V2Schema { + file_format: "resolved/2.0".to_owned(), + schema_url: "http://test/schema/v2".try_into().unwrap(), + attribute_catalog: vec![attr1.clone()], + registry: weaver_resolved_schema::v2::registry::Registry { + attributes: vec![weaver_resolved_schema::v2::attribute::AttributeRef(0)], + spans: vec![], + metrics: vec![], + events: vec![], + entities: vec![], + attribute_groups: vec![], + }, + refinements: weaver_resolved_schema::v2::refinements::Refinements { + spans: vec![], + metrics: vec![], + events: vec![], + }, + dependencies: Default::default(), + }; + + let dep1 = ResolvedDependency::V2(Box::new(schema1)); + let dep2 = ResolvedDependency::V2(Box::new(schema2)); + + let deps = vec![dep1, dep2]; + + let result = deps.lookup_attribute("error.type"); + assert!(result.is_err()); + if let Err(Error::AmbiguousReference { .. }) = result { + // ok + } else { + panic!("Expected AmbiguousReference error, got {:?}", result); + } + } + + #[test] + fn test_lookup_attribute_local_vs_dependency_conflict() { + use std::collections::HashMap; + use weaver_resolved_schema::v2::attribute::Attribute as AttributeV2; + use weaver_resolved_schema::v2::ResolvedTelemetrySchema as V2Schema; + use weaver_resolved_schema::ResolvedTelemetrySchema as V1Schema; + + let attr_name = "error.type"; + + // Create V1 Schema (acting as Local source) + let mut root_attributes = HashMap::new(); + let attr_v1 = attribute::Attribute { + name: attr_name.to_owned(), + r#type: AttributeType::PrimitiveOrArray(PrimitiveOrArrayTypeSpec::String), + brief: "brief v1".to_owned(), + examples: None, + tag: None, + requirement_level: RequirementLevel::Basic(Recommended), + sampling_relevant: None, + note: "".to_owned(), + stability: None, + deprecated: None, + prefix: false, + tags: None, + annotations: None, + value: None, + role: None, + }; + _ = root_attributes.insert(attr_name.to_owned(), (attr_v1.clone(), "group1".to_owned())); + + let schema_v1 = V1Schema { + file_format: "resolved/1.0".to_owned(), + schema_url: "http://test/schema/v1".to_owned(), + registry_id: "test-registry".to_owned(), + registry: weaver_resolved_schema::registry::Registry { + registry_url: "v1-example".to_owned(), + groups: vec![], + }, + catalog: Catalog::new(vec![attr_v1], root_attributes), + resource: None, + instrumentation_library: None, + dependencies: Default::default(), + versions: None, + registry_manifest: None, + }; + + // Create V2 Schema (acting as Dependency source) + let attr_v2 = AttributeV2 { + key: attr_name.to_owned(), + r#type: AttributeType::PrimitiveOrArray(PrimitiveOrArrayTypeSpec::String), + examples: None, + common: Default::default(), + provenance: Default::default(), + }; + + let schema_v2 = V2Schema { + file_format: "resolved/2.0".to_owned(), + schema_url: "http://test/schema/v2".try_into().unwrap(), + attribute_catalog: vec![attr_v2], + registry: weaver_resolved_schema::v2::registry::Registry { + attributes: vec![weaver_resolved_schema::v2::attribute::AttributeRef(0)], + spans: vec![], + metrics: vec![], + events: vec![], + entities: vec![], + attribute_groups: vec![], + }, + refinements: weaver_resolved_schema::v2::refinements::Refinements { + spans: vec![], + metrics: vec![], + events: vec![], + }, + dependencies: Default::default(), + }; + + let dep1 = ResolvedDependency::V1(Box::new(schema_v1)); + let dep2 = ResolvedDependency::V2(Box::new(schema_v2)); + + let deps = vec![dep1, dep2]; + + let result = deps.lookup_attribute(attr_name); + assert!(result.is_err()); + if let Err(Error::AmbiguousReference { + r#ref, + schema_url1, + schema_url2, + }) = result + { + assert_eq!(r#ref, attr_name); + assert_eq!(schema_url1, "local"); + assert_eq!(schema_url2, "http://test/schema/v2"); + } else { + panic!("Expected AmbiguousReference error, got {:?}", result); + } + } } diff --git a/crates/weaver_resolver/src/error.rs b/crates/weaver_resolver/src/error.rs index fd25de0b8..770005269 100644 --- a/crates/weaver_resolver/src/error.rs +++ b/crates/weaver_resolver/src/error.rs @@ -196,6 +196,30 @@ pub enum Error { attribute_ref: u32, }, + /// We discovered duplicate dependencies with different versions. + #[error( + "Duplicate dependency '{name}' found with different versions: {version1} and {version2}" + )] + DuplicateDependency { + /// The name of the dependency. + name: String, + /// The first version found. + version1: String, + /// The second version found. + version2: String, + }, + + /// We found multiple matches for a reference in dependencies with different SchemaURLs. + #[error("Ambiguous reference '{ref}' found in multiple dependencies with different SchemaURLs: {schema_url1} and {schema_url2}")] + AmbiguousReference { + /// The reference that is ambiguous. + r#ref: String, + /// The first SchemaURL found. + schema_url1: String, + /// The second SchemaURL found. + schema_url2: String, + }, + /// A container for multiple errors. #[error("{}", format_errors(.0))] CompoundError(#[related] Vec), diff --git a/crates/weaver_resolver/src/loader.rs b/crates/weaver_resolver/src/loader.rs index e25209a38..006388124 100644 --- a/crates/weaver_resolver/src/loader.rs +++ b/crates/weaver_resolver/src/loader.rs @@ -3,7 +3,6 @@ use itertools::Itertools; use rayon::iter::ParallelIterator; use rayon::iter::{IntoParallelIterator, ParallelBridge}; -use std::collections::HashSet; use std::fmt::Display; use std::path::MAIN_SEPARATOR; use weaver_common::vdir::{VirtualDirectory, VirtualDirectoryPath}; @@ -14,6 +13,7 @@ use weaver_common::result::WResult; use weaver_resolved_schema::v2::ResolvedTelemetrySchema as V2Schema; use weaver_resolved_schema::ResolvedTelemetrySchema as V1Schema; use weaver_semconv::registry_repo::{RegistryRepo, LEGACY_REGISTRY_MANIFEST, REGISTRY_MANIFEST}; +use weaver_semconv::schema_url::SchemaUrl; use weaver_semconv::{group::ImportsWithProvenance, semconv::SemConvSpecWithProvenance}; use crate::Error; @@ -23,6 +23,7 @@ const MAX_DEPENDENCY_DEPTH: u32 = 10; /// The result of loading a semantic convention URL prior to resolution. #[allow(clippy::large_enum_variant)] +#[derive(Clone)] pub enum LoadedSemconvRegistry { /// The semconv repository was unresolved and needs to be run through resolution. Unresolved { @@ -163,7 +164,7 @@ pub(crate) fn load_semconv_repository( follow_symlinks: bool, ) -> WResult { // This method simply sets up the resolution state and delegates to the actual work. - let mut visited_registries = HashSet::new(); + let mut visited_registries = std::collections::HashMap::new(); let mut dependency_chain = Vec::new(); load_semconv_repository_recursive( registry_repo, @@ -180,7 +181,7 @@ fn load_semconv_repository_recursive( registry_repo: RegistryRepo, follow_symlinks: bool, max_dependency_depth: u32, - visited_registries: &mut HashSet, + visited_registries: &mut std::collections::HashMap, dependency_chain: &mut Vec, ) -> WResult { // Make sure we don't go past our max dependency depth. @@ -190,8 +191,10 @@ fn load_semconv_repository_recursive( }); } let registry_name = registry_repo.name().to_owned(); - // Check for circular dependency - if visited_registries.contains(®istry_name) { + let schema_url = registry_repo.schema_url().clone(); + + // Check for circular dependency in the current path + if dependency_chain.contains(®istry_name) { dependency_chain.push(registry_name.clone()); let chain_str = dependency_chain.join(" → "); return WResult::FatalErr(Error::CircularDependency { @@ -199,22 +202,55 @@ fn load_semconv_repository_recursive( chain: chain_str, }); } - // Add current registry to visited set and dependency chain - let _ = visited_registries.insert(registry_name.clone()); + + // Check for conflict across the graph + if let Some(prev_schema_url) = visited_registries.get(®istry_name) { + if prev_schema_url != &schema_url { + // TODO: Address version conflicts. + // For now, fail fast on any duplicate name with different version. + return WResult::FatalErr(Error::DuplicateDependency { + name: registry_name, + version1: prev_schema_url.version().to_owned(), + version2: schema_url.version().to_owned(), + }); + } + } else { + let _ = visited_registries.insert(registry_name.clone(), schema_url.clone()); + } + + // Add current registry to dependency chain dependency_chain.push(registry_name.clone()); // Either load a fully resolved repository, or read in raw files. if let Some(manifest) = registry_repo.manifest() { if let Some(resolved_url) = registry_repo.resolved_schema_uri() { - load_resolved_repository(&resolved_url) + let res = load_resolved_repository(&resolved_url); + let _ = dependency_chain.pop(); + res } else { - if manifest.dependencies().len() > 1 { - todo!("Multiple dependencies is not supported yet.") - } // Load dependencies. let mut loaded_dependencies = vec![]; let mut non_fatal_errors: Vec = vec![]; + let mut seen_dependencies: std::collections::HashMap = + std::collections::HashMap::new(); + for d in manifest.dependencies().iter() { + let dep_name = d.schema_url.name().to_owned(); + + if let Some(prev_schema_url) = seen_dependencies.get(&dep_name) { + if prev_schema_url != &d.schema_url { + // TODO: Address version conflicts. + // For now, fail fast on any duplicate name with different version. + let _ = dependency_chain.pop(); + return WResult::FatalErr(Error::DuplicateDependency { + name: dep_name, + version1: prev_schema_url.version().to_owned(), + version2: d.schema_url.version().to_owned(), + }); + } + } else { + let _ = seen_dependencies.insert(dep_name, d.schema_url.clone()); + } let mut semconv_nfes: Vec = vec![]; match RegistryRepo::try_new_dependency(d, &mut semconv_nfes) { Ok(d_repo) => { @@ -233,21 +269,32 @@ fn load_semconv_repository_recursive( loaded_dependencies.push(d); non_fatal_errors.extend(nfes); } - WResult::FatalErr(err) => return WResult::FatalErr(err), + WResult::FatalErr(err) => { + let _ = dependency_chain.pop(); + return WResult::FatalErr(err); + } } } - Err(err) => return WResult::FatalErr(err.into()), + Err(err) => { + let _ = dependency_chain.pop(); + return WResult::FatalErr(err.into()); + } } } // Now load the raw repository. // TODO - Allow ignoring dependency warnings - https://github.com/open-telemetry/weaver/issues/1126. - load_definition_repository(registry_repo, follow_symlinks, loaded_dependencies) - .extend_non_fatal_errors(non_fatal_errors) + let res = + load_definition_repository(registry_repo, follow_symlinks, loaded_dependencies) + .extend_non_fatal_errors(non_fatal_errors); + let _ = dependency_chain.pop(); + res } } else { // This is a raw repository with *no* manifest. // TODO - issue a warning that manifest will be required w/ 2.0 to allow publishing. - load_definition_repository(registry_repo, follow_symlinks, vec![]) + let res = load_definition_repository(registry_repo, follow_symlinks, vec![]); + let _ = dependency_chain.pop(); + res } } @@ -393,7 +440,7 @@ fn load_definition_repository( #[cfg(test)] mod tests { - use std::collections::HashSet; + use weaver_semconv::schema_url::SchemaUrl; use weaver_common::{ diagnostic::DiagnosticMessages, result::WResult, vdir::VirtualDirectoryPath, @@ -455,7 +502,8 @@ mod tests { let registry_repo = RegistryRepo::try_new(None, ®istry_path, &mut vec![])?; // Try with depth limit of 1 - should fail at acme->otel transition - let mut visited_registries = HashSet::new(); + let mut visited_registries: std::collections::HashMap = + std::collections::HashMap::new(); let mut dependency_chain = Vec::new(); let result = load_semconv_repository_recursive( registry_repo, @@ -507,4 +555,58 @@ mod tests { Ok(()) } + + #[test] + fn test_incompatible_version_conflict() -> Result<(), Error> { + let registry_path = VirtualDirectoryPath::LocalFolder { + path: "data/incompatible-version-conflict/main".to_owned(), + }; + let registry_repo = RegistryRepo::try_new(None, ®istry_path, &mut vec![])?; + let result = load_semconv_repository(registry_repo, true); + + match result { + WResult::FatalErr(fatal) => { + let error_msg = fatal.to_string(); + assert!( + error_msg.contains("Duplicate dependency") && + error_msg.contains("example.com/c") && + error_msg.contains("1.0.0") && + error_msg.contains("2.0.0"), + "Expected duplicate dependency error mentioning both versions, got: {error_msg}" + ); + } + _ => { + panic!("Expected fatal error due to duplicate dependency, but got success"); + } + } + + Ok(()) + } + + #[test] + fn test_compatible_version_conflict_todo() -> Result<(), Error> { + let registry_path = VirtualDirectoryPath::LocalFolder { + path: "data/compatible-version-conflict/main".to_owned(), + }; + let registry_repo = RegistryRepo::try_new(None, ®istry_path, &mut vec![])?; + let result = load_semconv_repository(registry_repo, true); + + match result { + WResult::FatalErr(fatal) => { + let error_msg = fatal.to_string(); + assert!( + error_msg.contains("Duplicate dependency") + && error_msg.contains("example.com/c"), + "Expected duplicate dependency error, got: {error_msg}" + ); + } + _ => { + panic!( + "Expected fatal error due to duplicate dependency (for now), but got success" + ); + } + } + + Ok(()) + } } diff --git a/crates/weaver_resolver/src/registry.rs b/crates/weaver_resolver/src/registry.rs index 981cf5ebd..69f4fd2cb 100644 --- a/crates/weaver_resolver/src/registry.rs +++ b/crates/weaver_resolver/src/registry.rs @@ -404,41 +404,30 @@ fn resolve_attribute_references( // Remove attributes that are resolved and keep unresolved attributes // in the group for the next iteration. - unresolved_group.attributes = unresolved_group - .attributes - .clone() - .into_iter() - .filter_map(|attr| { - let attr_ref = attr_catalog.resolve( - &unresolved_group.group.id, - &unresolved_group.group.prefix, - &attr.spec, - unresolved_group.group.lineage.as_mut(), - &ureg.dependencies, - ); - if let Some(attr_ref) = attr_ref { - // Attribute reference resolved successfully. - resolved_attr.push(attr_ref); - resolved_attr_count += 1; - - // Return None to remove this attribute from the - // unresolved group. - None - } else { - // Attribute reference could not be resolved. - if let AttributeSpec::Ref { r#ref, .. } = &attr.spec { - // Keep track of unresolved attribute references in - // the errors. - errors.push(Error::UnresolvedAttributeRef { - group_id: unresolved_group.group.id.clone(), - attribute_ref: r#ref.clone(), - provenance: unresolved_group.provenance.clone().map(Box::new), - }); - } - Some(attr) + let mut still_unresolved = vec![]; + for attr in unresolved_group.attributes.clone() { + let attr_ref = attr_catalog.resolve( + &unresolved_group.group.id, + &unresolved_group.group.prefix, + &attr.spec, + unresolved_group.group.lineage.as_mut(), + &ureg.dependencies, + )?; + if let Some(attr_ref) = attr_ref { + resolved_attr.push(attr_ref); + resolved_attr_count += 1; + } else { + if let AttributeSpec::Ref { r#ref, .. } = &attr.spec { + errors.push(Error::UnresolvedAttributeRef { + group_id: unresolved_group.group.id.clone(), + attribute_ref: r#ref.clone(), + provenance: unresolved_group.provenance.clone().map(Box::new), + }); } - }) - .collect(); + still_unresolved.push(attr); + } + } + unresolved_group.attributes = still_unresolved; unresolved_group.group.attributes.extend(resolved_attr); }