Ignore write-only properties in schema-aware diffs #1674
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,7 +11,7 @@ use rust_i18n::t; | |
| use schemars::JsonSchema; | ||
| use serde::{Deserialize, Serialize}; | ||
| use serde_json::{Map, Value}; | ||
| use std::collections::{HashMap, HashSet}; | ||
| use std::path::PathBuf; | ||
| use tracing::{debug, info, trace, warn}; | ||
|
|
||
|
|
@@ -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. | ||
| /// 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 identify write-only properties and default values | ||
| /// | ||
| /// # Returns | ||
| /// | ||
|
|
@@ -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; | ||
|
|
@@ -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 value != &default_value { | ||
| info!("{}", t!("dscresources.dscresource.diffKeyMissing", key = key)); | ||
| diff_properties.push(key.to_string()); | ||
|
|
@@ -764,6 +769,42 @@ 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`, directly or | ||
| /// through a local JSON Pointer reference. | ||
| fn is_schema_write_only(schema: Option<&Value>, property_name: &str) -> bool { | ||
| let Some(schema) = schema else { | ||
| return false; | ||
| }; | ||
| let Some(mut property_schema) = schema | ||
| .get("properties") | ||
| .and_then(Value::as_object) | ||
| .and_then(|properties| properties.get(property_name)) | ||
| else { | ||
| return false; | ||
| }; | ||
| let mut visited_references = HashSet::new(); | ||
|
|
||
| loop { | ||
| if property_schema.get("writeOnly").and_then(Value::as_bool) == Some(true) { | ||
| return true; | ||
| } | ||
|
|
||
| let Some(reference) = property_schema.get("$ref").and_then(Value::as_str) else { | ||
| return false; | ||
| }; | ||
| let Some(pointer) = reference.strip_prefix('#') else { | ||
| return false; | ||
| }; | ||
|
Comment on lines
+795
to
+797
Collaborator
There was a problem hiding this comment. This works for pointer references like The formalized bundling format for JSON Schema 2020-12 (known more correctly as a compound schema document) is to ensure that you include any external schemas in the For example, the following non-bundled schema: {
"$id": "https://jsonschema.dev/schemas/examples/non-negative-integer",
"$schema": "https://json-schema.org/draft/2020-12/schema",
"description": "Must be a non-negative integer",
"$ref": "#/$defs/nonNegativeInteger"
"$defs": {
"nonNegativeInteger": {
"allOf": [
{ "$ref": "/schemas/mixins/integer" },
{ "$ref": "/schemas/mixins/non-negative" }
]
}
},
}Bundles to the following compound schema document: {
"$id": "https://jsonschema.dev/schemas/examples/non-negative-integer-bundle",
"$schema": "https://json-schema.org/draft/2020-12/schema",
"description": "Must be a non-negative integer",
"$ref": "#/$defs/nonNegativeInteger"
"$defs": {
"nonNegativeInteger": {
"allOf": [
{ "$ref": "/schemas/mixins/integer" },
{ "$ref": "/schemas/mixins/non-negative" }
]
},
"https://jsonschema.dev/schemas/mixins/integer": {
"$schema": "https://json-schema.org/draft/2020-12/schema",
"$id": "https://jsonschema.dev/schemas/mixins/integer",
"description": "Must be an integer",
"type": "integer"
},
"https://jsonschema.dev/schemas/mixins/non-negative": {
"$schema": "https://json-schema.org/draft/2020-12/schema",
"$id": "https://jsonschema.dev/schemas/mixins/non-negative",
"description": "Not allowed to be negative",
"minimum": 0
}
},
}When a validator resolves a relative URI like The key in the We have an extension method on the DSC/lib/dsc-lib-jsonschema/src/schema_utility_extensions.rs Lines 1619 to 1666 in 88ca706
Member
Author
There was a problem hiding this comment. One of the limitations we have is we CAN'T do a HTTP call to retrieve URLs due to compliance restrictions |
||
| if !visited_references.insert(pointer) { | ||
| return false; | ||
| } | ||
| let Some(resolved_schema) = schema.pointer(pointer) else { | ||
| return false; | ||
| }; | ||
| property_schema = resolved_schema; | ||
| } | ||
| } | ||
|
|
||
| /// Validates the properties of a resource against its schema. | ||
| /// | ||
| /// # Arguments | ||
|
|
@@ -1023,6 +1064,73 @@ 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_local_ref_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": { "$ref": "#/$defs/action" } | ||
| }, | ||
| "$defs": { | ||
| "action": { "type": "string", "writeOnly": true } | ||
| } | ||
| }); | ||
| let diff = get_diff_with_schema(&expected, &actual, Some(&schema)); | ||
| assert!(diff.is_empty(), "Expected referenced 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; | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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-jsonschemacrate, 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 calldeference()to get the full property definition for verification.