Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 69 additions & 4 deletions lib/dsc-lib/src/dscresources/dscresource.rs
Original file line number Diff line number Diff line change
Expand Up @@ -657,14 +657,15 @@ pub fn get_diff(expected: &Value, actual: &Value) -> Vec<String> {

#[must_use]
/// Performs a comparison of two JSON Values using an optional JSON Schema.
/// If a property exists in `expected` but not in `actual`, the schema's `default` value
/// for that property is used for comparison when available.
/// Properties whose schema sets `writeOnly` to `true` are ignored. If a property exists
/// in `expected` but not in `actual`, the schema's `default` value for that property is
/// used for comparison when available.
///
/// # Arguments
///
/// * `expected` - The expected value
/// * `actual` - The actual value
/// * `schema` - Optional JSON Schema to look up default values for missing properties
/// * `schema` - Optional JSON Schema to identify write-only properties and default values
///
/// # Returns
///
Expand All @@ -691,6 +692,10 @@ pub(crate) fn get_diff_with_schema(expected: &Value, actual: &Value, schema: Opt
}

for (key, value) in &*map {
if is_schema_write_only(schema, key) {
continue;
}

if is_secure_value(value) {
// skip secure values as they are not comparable
continue;
Expand Down Expand Up @@ -726,7 +731,7 @@ pub(crate) fn get_diff_with_schema(expected: &Value, actual: &Value, schema: Opt
}
} else {
// Property not in actual - check schema for a default value
if let Some(default_value) = get_schema_default(schema, key) {
if let Some(default_value) = get_schema_default(schema, key) {
if value != &default_value {
info!("{}", t!("dscresources.dscresource.diffKeyMissing", key = key));
diff_properties.push(key.to_string());
Expand Down Expand Up @@ -764,6 +769,18 @@ fn get_schema_default(schema: Option<&Value>, property_name: &str) -> Option<Val
property_schema.get("default").cloned()
}

/// Returns whether a property's JSON Schema sets `writeOnly` to `true`.
fn is_schema_write_only(schema: Option<&Value>, property_name: &str) -> bool {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially, I was going to recommend implementing this would be easier in the dsc-lib-jsonschema crate, but thinking about this a bit more, I think the ergonomics and simplicity will mostly improve when we implement the in-memory schema registry and retriever, when we can call deference() to get the full property definition for verification.

schema
.and_then(|schema| schema.get("properties"))
.and_then(Value::as_object)
.and_then(|properties| properties.get(property_name))
.and_then(Value::as_object)
.and_then(|property_schema| property_schema.get("writeOnly"))
.and_then(Value::as_bool)
.unwrap_or(false)
}
Comment thread
SteveL-MSFT marked this conversation as resolved.

/// Validates the properties of a resource against its schema.
///
/// # Arguments
Expand Down Expand Up @@ -1023,6 +1040,54 @@ fn diff_with_schema_no_default_reports_missing_property() {
assert_eq!(diff, vec!["enabled".to_string()]);
}

#[test]
fn diff_with_schema_write_only_ignores_differing_property() {
use serde_json::json;
let expected = json!({"name": "test", "action": "remove"});
let actual = json!({"name": "test", "action": "ignore"});
let schema = json!({
"type": "object",
"properties": {
"name": { "type": "string" },
"action": { "type": "string", "writeOnly": true }
}
});
let diff = get_diff_with_schema(&expected, &actual, Some(&schema));
assert!(diff.is_empty(), "Expected write-only property to be ignored, got: {diff:?}");
}

#[test]
fn diff_with_schema_write_only_ignores_missing_property() {
use serde_json::json;
let expected = json!({"name": "test", "action": "remove"});
let actual = json!({"name": "test"});
let schema = json!({
"type": "object",
"properties": {
"name": { "type": "string" },
"action": { "type": "string", "writeOnly": true }
}
});
let diff = get_diff_with_schema(&expected, &actual, Some(&schema));
assert!(diff.is_empty(), "Expected write-only property to be ignored, got: {diff:?}");
}

#[test]
fn diff_with_schema_write_only_false_reports_missing_property() {
use serde_json::json;
let expected = json!({"name": "test", "action": "remove"});
let actual = json!({"name": "test"});
let schema = json!({
"type": "object",
"properties": {
"name": { "type": "string" },
"action": { "type": "string", "writeOnly": false }
}
});
let diff = get_diff_with_schema(&expected, &actual, Some(&schema));
assert_eq!(diff, vec!["action".to_string()]);
}

#[test]
fn diff_without_schema_reports_missing_property() {
use serde_json::json;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ Describe 'Microsoft.Windows/FirewallRuleList - synthetic test with schema defaul
$result.differingProperties | Should -Not -Contain 'unspecifiedRulesAction'
}

It 'non-default unspecifiedRulesAction "disable" is reported as differing' {
It 'non-default unspecifiedRulesAction "disable" is ignored for comparison' {
$json = @{
unspecifiedRulesAction = 'disable'
rules = @(@{
Expand All @@ -81,10 +81,11 @@ Describe 'Microsoft.Windows/FirewallRuleList - synthetic test with schema defaul
$LASTEXITCODE | Should -Be 0 -Because (Get-Content -Raw $testdrive/error.log)

$result = $out | ConvertFrom-Json
$result.differingProperties | Should -Contain 'unspecifiedRulesAction'
$result.inDesiredState | Should -Be $true
$result.differingProperties | Should -Not -Contain 'unspecifiedRulesAction'
}

It 'non-default unspecifiedRulesAction "remove" is reported as differing' {
It 'non-default unspecifiedRulesAction "remove" is ignored for comparison' {
$json = @{
unspecifiedRulesAction = 'remove'
rules = @(@{
Expand All @@ -100,6 +101,7 @@ Describe 'Microsoft.Windows/FirewallRuleList - synthetic test with schema defaul
$LASTEXITCODE | Should -Be 0 -Because (Get-Content -Raw $testdrive/error.log)

$result = $out | ConvertFrom-Json
$result.differingProperties | Should -Contain 'unspecifiedRulesAction'
$result.inDesiredState | Should -Be $true
$result.differingProperties | Should -Not -Contain 'unspecifiedRulesAction'
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
"properties": {
"unspecifiedRulesAction": {
"type": "string",
"writeOnly": true,
"title": "Unspecified rules action",
"description": "The action to take on firewall rules not explicitly listed in the rules array. 'ignore' (default) leaves them unchanged, 'disable' disables them, and 'remove' deletes them.",
"default": "ignore",
Expand Down
Loading