refactor resource schema validation and use before inserting metadata

tgauth · tgauth · commit fcbaa32d58cd · 2025-09-02T17:05:35.000-04:00
diff --git a/dsc/src/subcommand.rs b/dsc/src/subcommand.rs
@@ -5,7 +5,7 @@ use crate::args::{ConfigSubCommand, SchemaType, ExtensionSubCommand, FunctionSub
 use crate::resolve::{get_contents, Include};
 use crate::resource_command::{get_resource, self};
 use crate::tablewriter::Table;
-use crate::util::{get_input, get_schema, in_desired_state, set_dscconfigroot, validate_json, write_object, DSC_CONFIG_ROOT, EXIT_DSC_ASSERTION_FAILED, EXIT_DSC_ERROR, EXIT_INVALID_ARGS, EXIT_INVALID_INPUT, EXIT_JSON_ERROR};
+use crate::util::{get_input, get_schema, in_desired_state, set_dscconfigroot, write_object, DSC_CONFIG_ROOT, EXIT_DSC_ASSERTION_FAILED, EXIT_DSC_ERROR, EXIT_INVALID_ARGS, EXIT_INVALID_INPUT, EXIT_JSON_ERROR};
 use dsc_lib::functions::FunctionArgKind;
 use dsc_lib::{
     configure::{
@@ -26,8 +26,8 @@ use dsc_lib::{
         TestResult,
         ValidateResult,
     },
-    dscresources::dscresource::{Capability, ImplementedAs, Invoke},
-    dscresources::resource_manifest::{import_manifest, ResourceManifest},
+    dscresources::dscresource::{Capability, ImplementedAs, validate_json, validate_properties},
+    dscresources::resource_manifest::import_manifest,
     extensions::dscextension::Capability as ExtensionCapability,
     functions::FunctionDispatcher,
     progress::ProgressFormat,
@@ -516,34 +516,7 @@ pub fn validate_config(config: &Configuration, progress_format: ProgressFormat)
 
         // see if the resource is command based
         if resource.implemented_as == ImplementedAs::Command {
-            // if so, see if it implements validate via the resource manifest
-            if let Some(manifest) = resource.manifest.clone() {
-                // convert to resource_manifest
-                let manifest: ResourceManifest = serde_json::from_value(manifest)?;
-                if manifest.validate.is_some() {
-                    debug!("{}: {type_name} ", t!("subcommand.resourceImplementsValidate"));
-                    // get the resource's part of the config
-                    let resource_config = resource_block["properties"].to_string();
-                    let result = resource.validate(&resource_config)?;
-                    if !result.valid {
-                        let reason = result.reason.unwrap_or(t!("subcommand.noReason").to_string());
-                        let type_name = resource.type_name.clone();
-                        return Err(DscError::Validation(format!("{}: {type_name} {reason}", t!("subcommand.resourceValidationFailed"))));
-                    }
-                }
-                else {
-                    // use schema validation
-                    trace!("{}: {type_name}", t!("subcommand.resourceDoesNotImplementValidate"));
-                    let Ok(schema) = resource.schema() else {
-                        return Err(DscError::Validation(format!("{}: {type_name}", t!("subcommand.noSchemaOrValidate"))));
-                    };
-                    let schema = serde_json::from_str(&schema)?;
-
-                    validate_json(&resource.type_name, &schema, &resource_block["properties"])?;
-                }
-            } else {
-                return Err(DscError::Validation(format!("{}: {type_name}", t!("subcommand.noManifest"))));
-            }
+            validate_properties(resource, &resource_block["properties"])?;
         }
     }
 
diff --git a/dsc/src/util.rs b/dsc/src/util.rs
@@ -41,12 +41,10 @@ use dsc_lib::{
         parse_input_to_json,
     },
 };
-use jsonschema::Validator;
 use path_absolutize::Absolutize;
 use rust_i18n::t;
 use schemars::{Schema, schema_for};
 use serde::Deserialize;
-use serde_json::Value;
 use std::collections::HashMap;
 use std::env;
 use std::io::{IsTerminal, Read};
@@ -423,39 +421,6 @@ pub fn enable_tracing(trace_level_arg: Option<&TraceLevel>, trace_format_arg: Op
     info!("Trace-level is {:?}", tracing_setting.level);
 }
 
-/// Validate the JSON against the schema.
-///
-/// # Arguments
-///
-/// * `source` - The source of the JSON
-/// * `schema` - The schema to validate against
-/// * `json` - The JSON to validate
-///
-/// # Returns
-///
-/// Nothing on success.
-///
-/// # Errors
-///
-/// * `DscError` - The JSON is invalid
-pub fn validate_json(source: &str, schema: &Value, json: &Value) -> Result<(), DscError> {
-    debug!("{}: {source}", t!("util.validatingSchema"));
-    trace!("JSON: {json}");
-    trace!("Schema: {schema}");
-    let compiled_schema = match Validator::new(schema) {
-        Ok(compiled_schema) => compiled_schema,
-        Err(err) => {
-            return Err(DscError::Validation(format!("{}: {err}", t!("util.failedToCompileSchema"))));
-        }
-    };
-
-    if let Err(err) = compiled_schema.validate(json) {
-        return Err(DscError::Validation(format!("{}: '{source}' {err}", t!("util.validationFailed"))));
-    }
-
-    Ok(())
-}
-
 pub fn get_input(input: Option<&String>, file: Option<&String>, parameters_from_stdin: bool) -> String {
     trace!("Input: {input:?}, File: {file:?}");
     let value = if let Some(input) = input {
diff --git a/dsc/tests/dsc_metadata.tests.ps1 b/dsc/tests/dsc_metadata.tests.ps1
@@ -2,6 +2,23 @@
 # Licensed under the MIT License.
 
 Describe 'metadata tests' {
+    It 'metadata not provided if not declared in resource schema' {
+        $configYaml = @'
+        $schema: https://aka.ms/dsc/schemas/v3/bundled/config/document.json
+        resources:
+          - name: test
+            type: Microsoft.DSC.Debug/Echo
+            metadata:
+              ignoreKey: true
+            properties:
+              output: hello world
+'@
+        $out = dsc config get -i $configYaml 2>$TestDrive/error.log | ConvertFrom-Json
+        $LASTEXITCODE | Should -Be 0
+        (Get-Content $TestDrive/error.log) | Should -BeLike "*WARN*Will not add '_metadata' to properties because resource schema does not support it*"
+        $out.results.result.actualState.output | Should -BeExactly 'hello world'
+    }
+
     It 'resource can provide high-level metadata for <operation>' -TestCases @(
         @{ operation = 'get' }
         @{ operation = 'set' }
diff --git a/dsc_lib/locales/en-us.toml b/dsc_lib/locales/en-us.toml
@@ -71,6 +71,7 @@ propertyNotString = "Property '%{name}' with value '%{value}' is not a string"
 metadataMicrosoftDscIgnored = "Resource returned '_metadata' property 'Microsoft.DSC' which is ignored"
 metadataNotObject = "Resource returned '_metadata' property which is not an object"
 metadataRestartRequiredInvalid = "Resource returned '_metadata' property '_restartRequired' which contains invalid value: %{value}"
+schemaExcludesMetadata = "Will not add '_metadata' to properties because resource schema does not support it"
 
 [discovery.commandDiscovery]
 couldNotReadSetting = "Could not read 'resourcePath' setting"
diff --git a/dsc_lib/src/configure/mod.rs b/dsc_lib/src/configure/mod.rs
@@ -6,7 +6,7 @@ use crate::configure::{config_doc::RestartRequired, parameters::Input};
 use crate::dscerror::DscError;
 use crate::dscresources::invoke_result::ExportResult;
 use crate::dscresources::{
-    {dscresource::{Capability, Invoke, get_diff},
+    {dscresource::{Capability, Invoke, get_diff, validate_properties},
     invoke_result::{GetResult, SetResult, TestResult,  ResourceSetResponse}},
     resource_manifest::Kind,
 };
@@ -170,8 +170,8 @@ fn escape_property_values(properties: &Map<String, Value>) -> Result<Option<Map<
     Ok(Some(result))
 }
 
-fn add_metadata(kind: &Kind, mut properties: Option<Map<String, Value>>, resource_metadata: Option<Metadata> ) -> Result<String, DscError> {
-    if *kind == Kind::Adapter {
+fn add_metadata(dsc_resource: &DscResource, mut properties: Option<Map<String, Value>>, resource_metadata: Option<Metadata> ) -> Result<String, DscError> {
+    if dsc_resource.kind == Kind::Adapter {
         // add metadata to the properties so the adapter knows this is a config
         let mut metadata: Map<String, Value> = Map::new();
         if let Some(resource_metadata) = resource_metadata {
@@ -191,15 +191,19 @@ fn add_metadata(kind: &Kind, mut properties: Option<Map<String, Value>>, resourc
     }
 
     if let Some(resource_metadata) = resource_metadata {
-        let other_metadata = resource_metadata.other.clone();
-        if let Some(mut properties) = properties {
-            properties.insert("_metadata".to_string(), Value::Object(other_metadata));
-            return Ok(serde_json::to_string(&properties)?);
-        }
-        let mut props = Map::new();
+        let other_metadata = resource_metadata.other;
+        let mut props = if let Some(props) = properties {
+            props
+        } else {
+            Map::new()
+        };
         props.insert("_metadata".to_string(), Value::Object(other_metadata));
-        properties = Some(props);
-        return Ok(serde_json::to_string(&properties)?);
+        let modified_props = Value::from(props.clone());
+        if let Ok(()) = validate_properties(dsc_resource, &modified_props) {} else {
+            warn!("{}", t!("configure.mod.schemaExcludesMetadata"));
+            props.remove("_metadata");
+        }
+        return Ok(serde_json::to_string(&props)?);
     }
 
     match properties {
@@ -347,7 +351,7 @@ impl Configurator {
             };
             let properties = self.get_properties(&resource, &dsc_resource.kind)?;
             debug!("resource_type {}", &resource.resource_type);
-            let filter = add_metadata(&dsc_resource.kind, properties, resource.metadata.clone())?;
+            let filter = add_metadata(dsc_resource, properties, resource.metadata.clone())?;
             trace!("filter: {filter}");
             let start_datetime = chrono::Local::now();
             let mut get_result = match dsc_resource.get(&filter) {
@@ -442,7 +446,7 @@ impl Configurator {
                 }
             };
 
-            let desired = add_metadata(&dsc_resource.kind, properties, resource.metadata.clone())?;
+            let desired = add_metadata(dsc_resource, properties, resource.metadata.clone())?;
             trace!("{}", t!("configure.mod.desired", state = desired));
 
             let start_datetime;
@@ -579,7 +583,7 @@ impl Configurator {
             };
             let properties = self.get_properties(&resource, &dsc_resource.kind)?;
             debug!("resource_type {}", &resource.resource_type);
-            let expected = add_metadata(&dsc_resource.kind, properties, resource.metadata.clone())?;
+            let expected = add_metadata(dsc_resource, properties, resource.metadata.clone())?;
             trace!("{}", t!("configure.mod.expectedState", state = expected));
             let start_datetime = chrono::Local::now();
             let mut test_result = match dsc_resource.test(&expected) {
@@ -655,7 +659,7 @@ impl Configurator {
                 return Err(DscError::ResourceNotFound(resource.resource_type.clone()));
             };
             let properties = self.get_properties(resource, &dsc_resource.kind)?;
-            let input = add_metadata(&dsc_resource.kind, properties, resource.metadata.clone())?;
+            let input = add_metadata(dsc_resource, properties, resource.metadata.clone())?;
             trace!("{}", t!("configure.mod.exportInput", input = input));
             let export_result = match add_resource_export_results_to_configuration(dsc_resource, &mut conf, input.as_str()) {
                 Ok(result) => result,
diff --git a/dsc_lib/src/dscresources/dscresource.rs b/dsc_lib/src/dscresources/dscresource.rs
@@ -4,14 +4,24 @@
 use crate::{configure::{config_doc::{Configuration, ExecutionKind, Resource}, Configurator}, dscresources::resource_manifest::Kind};
 use crate::dscresources::invoke_result::{ResourceGetResponse, ResourceSetResponse};
 use dscerror::DscError;
+use jsonschema::Validator;
 use rust_i18n::t;
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
 use serde_json::{Map, Value};
 use std::collections::HashMap;
-use tracing::{debug, info};
-
-use super::{command_resource, dscerror, invoke_result::{ExportResult, GetResult, ResolveResult, ResourceTestResponse, SetResult, TestResult, ValidateResult}, resource_manifest::import_manifest};
+use tracing::{debug, info, trace};
+
+use super::{
+    command_resource,
+    dscerror,
+    invoke_result::{
+        ExportResult, GetResult, ResolveResult, ResourceTestResponse, SetResult, TestResult, ValidateResult
+    },
+    resource_manifest::{
+        import_manifest, ResourceManifest
+    }
+};
 
 #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
 #[serde(deny_unknown_fields)]
@@ -557,6 +567,80 @@ pub fn get_diff(expected: &Value, actual: &Value) -> Vec<String> {
     diff_properties
 }
 
+/// Validates the properties of a resource against its schema.
+///
+/// # Arguments
+///
+/// * `properties` - The properties of the resource to validate.
+/// * `schema` - The schema to validate against.
+///
+/// # Returns
+///
+/// * `Result<(), DscError>` - Ok if valid, Err with message if invalid.
+///
+/// # Errors
+///
+/// * `DscError` - Error if the schema is invalid
+pub fn validate_properties(resource: &DscResource, properties: &Value) -> Result<(), DscError> {
+    // if so, see if it implements validate via the resource manifest
+    let type_name = resource.type_name.clone();
+    if let Some(manifest) = resource.manifest.clone() {
+        // convert to resource_manifest``
+        let manifest: ResourceManifest = serde_json::from_value(manifest)?;
+        if manifest.validate.is_some() {
+            debug!("{}: {type_name} ", t!("subcommand.resourceImplementsValidate"));
+            let resource_config = properties.to_string();
+            let result = resource.validate(&resource_config)?;
+            if !result.valid {
+                let reason = result.reason.unwrap_or(t!("subcommand.noReason").to_string());
+                return Err(DscError::Validation(format!("{}: {type_name} {reason}", t!("subcommand.resourceValidationFailed"))));
+            }
+            return Ok(())
+        }
+        // use schema validation
+        trace!("{}: {type_name}", t!("subcommand.resourceDoesNotImplementValidate"));
+        let Ok(schema) = resource.schema() else {
+            return Err(DscError::Validation(format!("{}: {type_name}", t!("subcommand.noSchemaOrValidate"))));
+        };
+        let schema = serde_json::from_str(&schema)?;
+        return validate_json(&resource.type_name, &schema, properties)
+    }
+    Err(DscError::Validation(format!("{}: {type_name}", t!("subcommand.noManifest"))))
+}
+
+/// Validate the JSON against the schema.
+///
+/// # Arguments
+///
+/// * `source` - The source of the JSON
+/// * `schema` - The schema to validate against
+/// * `json` - The JSON to validate
+///
+/// # Returns
+///
+/// Nothing on success.
+///
+/// # Errors
+///
+/// * `DscError` - The JSON is invalid
+pub fn validate_json(source: &str, schema: &Value, json: &Value) -> Result<(), DscError> {
+    debug!("{}: {source}", t!("util.validatingSchema"));
+    trace!("JSON: {json}");
+    trace!("Schema: {schema}");
+    let compiled_schema = match Validator::new(schema) {
+        Ok(compiled_schema) => compiled_schema,
+        Err(err) => {
+            return Err(DscError::Validation(format!("{}: {err}", t!("util.failedToCompileSchema"))));
+        }
+    };
+
+    if let Err(err) = compiled_schema.validate(json) {
+        return Err(DscError::Validation(format!("{}: '{source}' {err}", t!("util.validationFailed"))));
+    }
+
+    Ok(())
+}
+
 /// Compares two arrays independent of order
 fn is_same_array(expected: &Vec<Value>, actual: &Vec<Value>) -> bool {
     if expected.len() != actual.len() {
