From f68622ec5fa7d626406f45dba7ef0138f412ccba Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Thu, 13 Apr 2023 15:24:13 -0400 Subject: [PATCH 1/4] fix(config): recurse through schema refs when determining eligibility for unevaluated properties --- .../src/schema/visitors/unevaluated.rs | 53 ++++++++++++++++--- 1 file changed, 47 insertions(+), 6 deletions(-) diff --git a/lib/vector-config/src/schema/visitors/unevaluated.rs b/lib/vector-config/src/schema/visitors/unevaluated.rs index cfab8822df5be..2073ddfc105a9 100644 --- a/lib/vector-config/src/schema/visitors/unevaluated.rs +++ b/lib/vector-config/src/schema/visitors/unevaluated.rs @@ -237,12 +237,22 @@ fn build_closed_schema_flatten_eligibility_mappings( Schema::Object(schema) => schema, }; + debug!( + "Evaluating schema definition '{}' for markability.", + definition_name + ); + // If a schema itself would not be considered markable, then we don't need to consider the // eligibility between parent/child since there's nothing to drive the "now unmark the child // schemas" logic. if !is_markable_schema(&root_schema.definitions, parent_schema) { debug!("Schema definition '{}' not markable.", definition_name); continue; + } else { + debug!( + "Schema definition '{}' markable. Collecting referents.", + definition_name + ); } // Collect all referents for this definition, which includes both property-based referents @@ -251,6 +261,13 @@ fn build_closed_schema_flatten_eligibility_mappings( let mut referents = HashSet::new(); get_referents(parent_schema, &mut referents); + debug!( + "Collected {} referents for '{}': {:?}", + referents.len(), + definition_name, + referents + ); + // Store the parent/child mapping. parent_to_child.insert(SchemaReference::from(definition_name), referents); } @@ -328,12 +345,14 @@ fn is_markable_schema(definitions: &Map, schema: &SchemaObject) // subschemas: { "type": "null" } and { "$ref": "#/definitions/T" }. If the schema for `T` is, // say, just a scalar schema, instead of an object schema... then it wouldn't be marked, and in // turn, we wouldn't need to mark the schema for `Option`: there's no properties at all. - // - // We'll follow schema references in subschemas up to one level deep trying to figure this out. if let Some(subschema) = schema.subschemas.as_ref() { let subschemas = get_object_subschemas_from_parent(subschema).collect::>(); - let has_object_subschema = subschemas.iter().any(|schema| is_object_schema(schema)); + debug!("{} subschemas detected.", subschemas.len()); + + let has_object_subschema = subschemas + .iter() + .any(|schema| is_markable_schema(definitions, schema)); let has_referenced_object_subschema = subschemas .iter() .map(|subschema| { @@ -342,13 +361,35 @@ fn is_markable_schema(definitions: &Map, schema: &SchemaObject) .as_ref() .and_then(|reference| { let reference = get_cleaned_schema_reference(reference); - definitions.get(reference) + definitions.get(reference).map(|d| (reference, d)) + }) + .and_then(|(definition_name, schema_definition)| { + schema_definition.as_object().map(|s| (definition_name, s)) + }) + .map_or(false, |(name, schema)| { + debug!( + "Following schema reference '{}' for subschema markability.", + name + ); + is_markable_schema(definitions, schema) }) - .and_then(Schema::as_object) - .map_or(false, is_object_schema) }) .any(identity); + debug!( + "Schema {} object subschema(s) and {} referenced subschemas.", + if has_object_subschema { + "has" + } else { + "does not have" + }, + if has_referenced_object_subschema { + "has" + } else { + "does not have" + }, + ); + if has_object_subschema || has_referenced_object_subschema { return true; } From 6dd4f0690df0d2df22774303d4f530d7c394b85f Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Thu, 13 Apr 2023 15:34:42 -0400 Subject: [PATCH 2/4] rename visitor --- lib/vector-config/src/schema/helpers.rs | 4 +-- lib/vector-config/src/schema/visitors/mod.rs | 2 +- .../src/schema/visitors/unevaluated.rs | 30 +++++++++---------- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/lib/vector-config/src/schema/helpers.rs b/lib/vector-config/src/schema/helpers.rs index c1baffd0731a3..61f8cccba65ea 100644 --- a/lib/vector-config/src/schema/helpers.rs +++ b/lib/vector-config/src/schema/helpers.rs @@ -12,7 +12,7 @@ use crate::{ num::ConfigurableNumber, Configurable, ConfigurableRef, GenerateError, Metadata, ToValue, }; -use super::visitors::{DisallowedUnevaluatedPropertiesVisitor, InlineSingleUseReferencesVisitor}; +use super::visitors::{DisallowUnevaluatedPropertiesVisitor, InlineSingleUseReferencesVisitor}; /// Applies metadata to the given schema. /// @@ -482,7 +482,7 @@ pub fn generate_internal_tagged_variant_schema( pub fn default_schema_settings() -> SchemaSettings { SchemaSettings::new() .with_visitor(InlineSingleUseReferencesVisitor::from_settings) - .with_visitor(DisallowedUnevaluatedPropertiesVisitor::from_settings) + .with_visitor(DisallowUnevaluatedPropertiesVisitor::from_settings) } pub fn generate_root_schema() -> Result diff --git a/lib/vector-config/src/schema/visitors/mod.rs b/lib/vector-config/src/schema/visitors/mod.rs index 59802e0ebe10b..3d919f2dea740 100644 --- a/lib/vector-config/src/schema/visitors/mod.rs +++ b/lib/vector-config/src/schema/visitors/mod.rs @@ -7,4 +7,4 @@ mod unevaluated; pub(self) mod test; pub use self::inline_single::InlineSingleUseReferencesVisitor; -pub use self::unevaluated::DisallowedUnevaluatedPropertiesVisitor; +pub use self::unevaluated::DisallowUnevaluatedPropertiesVisitor; diff --git a/lib/vector-config/src/schema/visitors/unevaluated.rs b/lib/vector-config/src/schema/visitors/unevaluated.rs index 2073ddfc105a9..321c1d104dacb 100644 --- a/lib/vector-config/src/schema/visitors/unevaluated.rs +++ b/lib/vector-config/src/schema/visitors/unevaluated.rs @@ -27,12 +27,12 @@ use super::scoped_visit::{ /// with advanced subschema validation, such as `oneOf` or `allOf`, as `unevaluatedProperties` /// cannot simply be applied to any and all schemas indiscriminately. #[derive(Debug, Default)] -pub struct DisallowedUnevaluatedPropertiesVisitor { +pub struct DisallowUnevaluatedPropertiesVisitor { scope_stack: SchemaScopeStack, eligible_to_flatten: HashMap>, } -impl DisallowedUnevaluatedPropertiesVisitor { +impl DisallowUnevaluatedPropertiesVisitor { pub fn from_settings(_: &SchemaSettings) -> Self { Self { scope_stack: SchemaScopeStack::default(), @@ -41,7 +41,7 @@ impl DisallowedUnevaluatedPropertiesVisitor { } } -impl Visitor for DisallowedUnevaluatedPropertiesVisitor { +impl Visitor for DisallowUnevaluatedPropertiesVisitor { fn visit_root_schema(&mut self, root: &mut RootSchema) { let eligible_to_flatten = build_closed_schema_flatten_eligibility_mappings(root); @@ -132,7 +132,7 @@ impl Visitor for DisallowedUnevaluatedPropertiesVisitor { } } -impl ScopedVisitor for DisallowedUnevaluatedPropertiesVisitor { +impl ScopedVisitor for DisallowUnevaluatedPropertiesVisitor { fn push_schema_scope>(&mut self, scope: S) { self.scope_stack.push(scope.into()); } @@ -545,7 +545,7 @@ mod tests { use crate::schema::visitors::test::{as_schema, assert_schemas_eq}; - use super::DisallowedUnevaluatedPropertiesVisitor; + use super::DisallowUnevaluatedPropertiesVisitor; #[test] fn basic_object_schema() { @@ -556,7 +556,7 @@ mod tests { } })); - let mut visitor = DisallowedUnevaluatedPropertiesVisitor::default(); + let mut visitor = DisallowUnevaluatedPropertiesVisitor::default(); visitor.visit_root_schema(&mut actual_schema); let expected_schema = as_schema(json!({ @@ -584,7 +584,7 @@ mod tests { } })); - let mut visitor = DisallowedUnevaluatedPropertiesVisitor::default(); + let mut visitor = DisallowUnevaluatedPropertiesVisitor::default(); visitor.visit_root_schema(&mut actual_schema); let expected_schema = as_schema(json!({ @@ -621,7 +621,7 @@ mod tests { }] })); - let mut visitor = DisallowedUnevaluatedPropertiesVisitor::default(); + let mut visitor = DisallowUnevaluatedPropertiesVisitor::default(); visitor.visit_root_schema(&mut actual_schema); let expected_schema = as_schema(json!({ @@ -662,7 +662,7 @@ mod tests { }] })); - let mut visitor = DisallowedUnevaluatedPropertiesVisitor::default(); + let mut visitor = DisallowUnevaluatedPropertiesVisitor::default(); visitor.visit_root_schema(&mut actual_schema); let expected_schema = as_schema(json!({ @@ -703,7 +703,7 @@ mod tests { }] })); - let mut visitor = DisallowedUnevaluatedPropertiesVisitor::default(); + let mut visitor = DisallowUnevaluatedPropertiesVisitor::default(); visitor.visit_root_schema(&mut actual_schema); let expected_schema = as_schema(json!({ @@ -737,7 +737,7 @@ mod tests { })); let expected_schema = actual_schema.clone(); - let mut visitor = DisallowedUnevaluatedPropertiesVisitor::default(); + let mut visitor = DisallowUnevaluatedPropertiesVisitor::default(); visitor.visit_root_schema(&mut actual_schema); assert_schemas_eq(expected_schema, actual_schema); @@ -753,7 +753,7 @@ mod tests { "additionalProperties": false })); - let mut visitor = DisallowedUnevaluatedPropertiesVisitor::default(); + let mut visitor = DisallowUnevaluatedPropertiesVisitor::default(); visitor.visit_root_schema(&mut actual_schema); let expected_schema = as_schema(json!({ @@ -799,7 +799,7 @@ mod tests { } })); - let mut visitor = DisallowedUnevaluatedPropertiesVisitor::default(); + let mut visitor = DisallowUnevaluatedPropertiesVisitor::default(); visitor.visit_root_schema(&mut actual_schema); let expected_schema = as_schema(json!({ @@ -854,7 +854,7 @@ mod tests { } })); - let mut visitor = DisallowedUnevaluatedPropertiesVisitor::default(); + let mut visitor = DisallowUnevaluatedPropertiesVisitor::default(); visitor.visit_root_schema(&mut actual_schema); let expected_schema = as_schema(json!({ @@ -920,7 +920,7 @@ mod tests { } })); - let mut visitor = DisallowedUnevaluatedPropertiesVisitor::default(); + let mut visitor = DisallowUnevaluatedPropertiesVisitor::default(); visitor.visit_root_schema(&mut actual_schema); // Expectations: From 8ebe68d456b9e4eaa5bbf66249a0732b20aef3b0 Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Thu, 13 Apr 2023 16:19:19 -0400 Subject: [PATCH 3/4] update spell checker dict --- .github/actions/spelling/expect.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/actions/spelling/expect.txt b/.github/actions/spelling/expect.txt index 8f2ee3b2e4d6f..be592b9ac0b25 100644 --- a/.github/actions/spelling/expect.txt +++ b/.github/actions/spelling/expect.txt @@ -723,6 +723,7 @@ MAINPID majorly MAKECMDGOALS Makefiles +markability markdownify markdownlintrc marketo From 712ca0d32b6a31eb34576abaa9b11c871c153aea Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Fri, 14 Apr 2023 09:58:14 -0400 Subject: [PATCH 4/4] PR feedback --- lib/vector-config/src/schema/visitors/unevaluated.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/vector-config/src/schema/visitors/unevaluated.rs b/lib/vector-config/src/schema/visitors/unevaluated.rs index 321c1d104dacb..1619465cde24a 100644 --- a/lib/vector-config/src/schema/visitors/unevaluated.rs +++ b/lib/vector-config/src/schema/visitors/unevaluated.rs @@ -361,11 +361,9 @@ fn is_markable_schema(definitions: &Map, schema: &SchemaObject) .as_ref() .and_then(|reference| { let reference = get_cleaned_schema_reference(reference); - definitions.get(reference).map(|d| (reference, d)) - }) - .and_then(|(definition_name, schema_definition)| { - schema_definition.as_object().map(|s| (definition_name, s)) + definitions.get_full(reference) }) + .and_then(|(_, name, schema)| schema.as_object().map(|schema| (name, schema))) .map_or(false, |(name, schema)| { debug!( "Following schema reference '{}' for subschema markability.",