fix(config): recurse through schema refs when determining eligibility for unevaluated properties (vectordotdev#17150)

Author: tobz

.github/actions/spelling/expect.txt

@@ -723,6 +723,7 @@ MAINPID
 majorly
 MAKECMDGOALS
 Makefiles
+markability
 markdownify
 markdownlintrc
 marketo

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<T>() -> Result<RootSchema, GenerateError>

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;

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<String, HashSet<SchemaReference>>,
 }
 
-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<S: Into<SchemaReference>>(&mut self, scope: S) {
         self.scope_stack.push(scope.into());
     }
@@ -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<String, Schema>, 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<T>`: 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::<Vec<_>>();
 
-        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,33 @@ fn is_markable_schema(definitions: &Map<String, Schema>, schema: &SchemaObject)
                     .as_ref()
                     .and_then(|reference| {
                         let reference = get_cleaned_schema_reference(reference);
-                        definitions.get(reference)
+                        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.",
+                            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;
         }
@@ -504,7 +543,7 @@ mod tests {
 
     use crate::schema::visitors::test::{as_schema, assert_schemas_eq};
 
-    use super::DisallowedUnevaluatedPropertiesVisitor;
+    use super::DisallowUnevaluatedPropertiesVisitor;
 
     #[test]
     fn basic_object_schema() {
@@ -515,7 +554,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!({
@@ -543,7 +582,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!({
@@ -580,7 +619,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 +660,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 +701,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!({
@@ -696,7 +735,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);
@@ -712,7 +751,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!({
@@ -758,7 +797,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!({
@@ -813,7 +852,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!({
@@ -879,7 +918,7 @@ mod tests {
             }
         }));
 
-        let mut visitor = DisallowedUnevaluatedPropertiesVisitor::default();
+        let mut visitor = DisallowUnevaluatedPropertiesVisitor::default();
         visitor.visit_root_schema(&mut actual_schema);
 
         // Expectations: