Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
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
Original file line number Diff line number Diff line change
Expand Up @@ -724,7 +724,6 @@ impl DocumentTypeV1 {
#[cfg(test)]
mod tests {
use super::*;
use crate::data_contract::document_type::DocumentTypeV0;
use assert_matches::assert_matches;
use platform_value::platform_value;

Expand Down Expand Up @@ -1006,13 +1005,14 @@ mod tests {
let config = DataContractConfig::default_for_version(platform_version)
.expect("should create a default config");

let result = DocumentTypeV0::try_from_schema(
let result = DocumentTypeV1::try_from_schema(
Identifier::new([1; 32]),
1,
config.version(),
"invalid name",
schema.clone(),
None,
&BTreeMap::new(),
&config,
true,
&mut vec![],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,11 @@ pub use traversal_validator::*;
#[cfg(test)]
mod test {
use super::*;
use crate::consensus::basic::data_contract::IncompatibleRe2PatternError;
use crate::consensus::basic::json_schema_compilation_error::JsonSchemaCompilationError;
use crate::consensus::basic::BasicError;
use crate::consensus::codes::ErrorWithCode;
use crate::consensus::ConsensusError;
use assert_matches::assert_matches;
use platform_value::{platform_value, Value};
use platform_version::version::PlatformVersion;

Expand All @@ -17,9 +18,100 @@ mod test {
.try_init();
}

#[ignore]
// ----------------------------------------------------------------
// Test-only sub-validator callbacks.
//
// IMPORTANT: The helpers below are NOT production validators and
// are NOT wired into document-type validation. Production
// document-type schema validation does not use `traversal_validator`
// at all — it relies on `validate_max_depth`, JSON Schema meta-schema
// validation, and `json_schema_validator.compile()`.
//
// These callbacks exist solely as fixtures to exercise the
// `traversal_validator` mechanism: they prove that traversal walks
// every map/array node, invokes each provided sub-validator with the
// expected `(path, key, parent, value)` arguments, and that any
// `ConsensusError`s a callback adds (or `ProtocolError`s it returns)
// are propagated correctly. They happen to be shaped after legacy
// validators (byteArray-with-items, regex compatibility) only because
// those produce well-known consensus error variants that are
// convenient to assert on; do not read the assertions below as
// coverage of the corresponding production validation rules.
// ----------------------------------------------------------------

/// Test callback fixture: when traversal visits a map that contains
/// `"byteArray"` alongside `"items"` or `"prefixItems"`, emit a
/// `JsonSchemaCompilationError`. Used purely to verify the traversal
/// mechanism propagates consensus errors added by callbacks; this is
/// not the production byteArray-vs-items rule.
fn test_callback_flag_byte_array_with_items(
path: &str,
key: &str,
parent: &Value,
_value: &Value,
result: &mut crate::validation::SimpleConsensusValidationResult,
_platform_version: &PlatformVersion,
) -> Result<(), crate::ProtocolError> {
if key != "byteArray" {
return Ok(());
}
let Value::Map(parent_map) = parent else {
return Ok(());
};
let has_items = parent_map.iter().any(
|(k, _)| matches!(k.to_str(), Ok(name) if name == "items" || name == "prefixItems"),
);
if has_items {
let message = format!(
"invalid path: '{}': byteArray cannot be used with `items` or `prefixItems`",
path
);
result.add_error(ConsensusError::BasicError(
BasicError::JsonSchemaCompilationError(JsonSchemaCompilationError::new(message)),
));
}
Ok(())
}

/// Test callback fixture: when traversal visits a `"pattern"` string
/// that fails to compile under Rust's `regex` crate (which, like
/// Re2, rejects lookarounds), emit an `IncompatibleRe2PatternError`.
/// Used purely to verify the traversal mechanism propagates
/// consensus errors added by callbacks; this is not the production
/// pattern-validation rule.
fn test_callback_flag_invalid_regex_pattern(
path: &str,
key: &str,
_parent: &Value,
value: &Value,
result: &mut crate::validation::SimpleConsensusValidationResult,
_platform_version: &PlatformVersion,
) -> Result<(), crate::ProtocolError> {
if key != "pattern" {
return Ok(());
}
let Value::Text(pattern) = value else {
return Ok(());
};
if let Err(err) = regex::Regex::new(pattern) {
result.add_error(ConsensusError::BasicError(
BasicError::IncompatibleRe2PatternError(IncompatibleRe2PatternError::new(
pattern.clone(),
path.to_string(),
err.to_string(),
)),
));
}
Ok(())
}
Comment on lines +21 to +106
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

🟡 Suggestion: The restored recursive-schema rejection tests exercise test-local validator copies rather than production validation

The restored tests now define byte_array_has_no_items_as_parent_validator and pattern_is_valid_regex_validator inside the test module, and the in-file comment explicitly says the original production modules were removed and these helpers exist only to preserve the historical assertions. A repository search shows these validators do not exist anywhere outside this test file. That means the named rejection cases no longer protect production byte-array or regex validation behavior; they only verify that traversal_validator invokes arbitrary callbacks and propagates the constructed consensus errors. That is useful traversal coverage, but it is narrower than the test names and comments imply.

source: ['claude']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-dpp/src/data_contract/document_type/schema/recursive_schema_validator/mod.rs`:
- [SUGGESTION] lines 21-89: The restored recursive-schema rejection tests exercise test-local validator copies rather than production validation
  The restored tests now define `byte_array_has_no_items_as_parent_validator` and `pattern_is_valid_regex_validator` inside the test module, and the in-file comment explicitly says the original production modules were removed and these helpers exist only to preserve the historical assertions. A repository search shows these validators do not exist anywhere outside this test file. That means the named rejection cases no longer protect production byte-array or regex validation behavior; they only verify that `traversal_validator` invokes arbitrary callbacks and propagates the constructed consensus errors. That is useful traversal coverage, but it is narrower than the test names and comments imply.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good call on the scope here. I did not reintroduce removed production validator modules just to satisfy these tests; those validators no longer exist outside this test file. I kept the traversal/error-propagation coverage test-local and only made small clarity adjustments around the surrounding traversal-only cases, while fixing the other actionable findings (toolchain churn, duplicate constant, and fixed external JSON fixture coverage).

Comment on lines +21 to +106
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

🟡 Suggestion: Recursive-schema rejection tests now exercise test-local mirrors, not production validation

The restored byteArray/regex rejection tests define byte_array_has_no_items_as_parent_validator and pattern_is_valid_regex_validator locally inside the test module (lines 32-89), and the comment at lines 21-26 confirms the original production validators were removed. A repository search confirms these validators exist only in this test file, and traversal_validator itself is not called from any production path — the production document-type validation in class_methods/try_from_schema/v0/mod.rs and .../v1/mod.rs runs validate_max_depth, meta-schema validation, and json_schema_validator.compile() instead. As a result the rejection tests at lines 91-251 prove only that traversal_validator propagates errors from arbitrary callbacks; they no longer guard production byteArray-with-items or Re2-incompatible-pattern behavior. Either restore the underlying production check (and route document-type validation through it), or rename the tests and the module comment to make clear they are traversal-mechanism tests rather than DPP schema validation tests, so future readers don't over-attribute coverage to them.

source: ['codex-general']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-dpp/src/data_contract/document_type/schema/recursive_schema_validator/mod.rs`:
- [SUGGESTION] lines 21-89: Recursive-schema rejection tests now exercise test-local mirrors, not production validation
  The restored byteArray/regex rejection tests define `byte_array_has_no_items_as_parent_validator` and `pattern_is_valid_regex_validator` locally inside the test module (lines 32-89), and the comment at lines 21-26 confirms the original production validators were removed. A repository search confirms these validators exist only in this test file, and `traversal_validator` itself is not called from any production path — the production document-type validation in `class_methods/try_from_schema/v0/mod.rs` and `.../v1/mod.rs` runs `validate_max_depth`, meta-schema validation, and `json_schema_validator.compile()` instead. As a result the rejection tests at lines 91-251 prove only that `traversal_validator` propagates errors from arbitrary callbacks; they no longer guard production byteArray-with-items or Re2-incompatible-pattern behavior. Either restore the underlying production check (and route document-type validation through it), or rename the tests and the module comment to make clear they are traversal-mechanism tests rather than DPP schema validation tests, so future readers don't over-attribute coverage to them.


#[test]
fn should_return_error_if_bytes_array_parent_contains_items_or_prefix_items() {
fn traversal_propagates_byte_array_callback_errors_for_each_offending_parent() {
// Verifies traversal-mechanism behavior only: that the test
// callback is invoked at every map node and that each
// ConsensusError it adds reaches the result. Does not assert
// anything about production document-type byteArray validation,
// which does not run through traversal_validator.
let schema: Value = platform_value!(
{
"type": "object",
Expand All @@ -36,25 +128,31 @@ mod test {
"additionalProperties": false,
}
);
let mut result = traversal_validator(&schema, &[], PlatformVersion::first())
.expect("expected traversal validator to succeed");
let mut result = traversal_validator(
&schema,
&[test_callback_flag_byte_array_with_items],
PlatformVersion::first(),
)
.expect("expected traversal validator to succeed");
assert_eq!(2, result.errors.len());
let first_error = get_basic_error(result.errors.pop().unwrap());
let second_error = get_basic_error(result.errors.pop().unwrap());

assert_matches!(
assert!(matches!(
first_error,
BasicError::JsonSchemaCompilationError(msg) if msg.compilation_error().starts_with("invalid path: '/properties/bar': byteArray cannot")
);
assert_matches!(
BasicError::JsonSchemaCompilationError(msg) if msg.compilation_error().starts_with("invalid path: '/properties/bar': byteArray cannot"),
));
assert!(matches!(
second_error,
BasicError::JsonSchemaCompilationError(msg) if msg.compilation_error().starts_with("invalid path: '/properties': byteArray cannot")
);
BasicError::JsonSchemaCompilationError(msg) if msg.compilation_error().starts_with("invalid path: '/properties': byteArray cannot"),
));
}

#[ignore]
#[test]
fn should_return_valid_result() {
fn traversal_pattern_callback_adds_no_errors_for_compilable_regex() {
// Traversal-mechanism check only: verifies the test callback
// returns no errors when every visited "pattern" compiles.
// Not a production pattern-validation assertion.
let schema: Value = platform_value!(
{
"type": "object",
Expand All @@ -69,63 +167,79 @@ mod test {
"additionalProperties": false,
}
);
assert!(traversal_validator(&schema, &[], PlatformVersion::first())
.expect("expected traversal validator to succeed")
.is_valid());
assert!(traversal_validator(
&schema,
&[test_callback_flag_invalid_regex_pattern],
PlatformVersion::first()
)
.expect("expected traversal validator to succeed")
.is_valid());
}

#[ignore]
#[test]
fn should_return_invalid_result() {
fn traversal_propagates_pattern_callback_error_for_top_level_uncompilable_regex() {
// Traversal-mechanism check only: confirms the IncompatibleRe2
// ConsensusError that the test callback emits propagates with
// the correct path/pattern. Production document-type schema
// validation does not invoke traversal_validator.
let unsafe_pattern = "^((?!-|_)[a-zA-Z0-9-_]{0,62}[a-zA-Z0-9])$";
let schema: Value = platform_value!({
"type": "object",
"properties": {
"foo": { "type": "integer" },
"bar": {
"type": "string",
"pattern": "^((?!-|_)[a-zA-Z0-9-_]{0,62}[a-zA-Z0-9])$",
"pattern": unsafe_pattern,
},
},
"required": ["foo"],
"additionalProperties": false,

});
let result = traversal_validator(&schema, &[], PlatformVersion::first())
.expect("expected traversal validator to succeed");
let result = traversal_validator(
&schema,
&[test_callback_flag_invalid_regex_pattern],
PlatformVersion::first(),
)
.expect("expected traversal validator to succeed");
let consensus_error = result.errors.first().expect("the error should be returned");

match consensus_error {
ConsensusError::BasicError(BasicError::IncompatibleRe2PatternError(err)) => {
assert_eq!(err.path(), "/properties/bar".to_string());
assert_eq!(
err.pattern(),
"^((?!-|_)[a-zA-Z0-9-_]{0,62}[a-zA-Z0-9])$".to_string()
);
assert_eq!(err.pattern(), unsafe_pattern.to_string());
assert_eq!(consensus_error.code(), 10202);
}
_ => panic!("Expected error to be IncompatibleRe2PatternError"),
}
}

#[ignore]
#[test]
fn should_be_valid_complex_for_complex_schema() {
fn traversal_with_no_validators_visits_complex_schema_without_errors() {
let schema = get_document_schema();

assert!(traversal_validator(&schema, &[], PlatformVersion::first())
.expect("expected traversal validator to exist for first protocol version")
.is_valid())
}

#[ignore]
#[test]
fn invalid_result_for_array_of_object() {
fn traversal_propagates_pattern_callback_error_inside_array_items_object() {
// Traversal-mechanism check only: confirms traversal descends
// into `items` of an array-of-object schema and that the test
// callback's ConsensusError carries the expected path.
// Not a production pattern-validation assertion.
let unsafe_pattern = "^((?!-|_)[a-zA-Z0-9-_]{0,62}[a-zA-Z0-9])$";
let mut schema = get_document_schema();
schema["properties"]["arrayOfObject"]["items"]["properties"]["simple"]["pattern"] =
platform_value!("^((?!-|_)[a-zA-Z0-9-_]{0,62}[a-zA-Z0-9])$");

let result = traversal_validator(&schema, &[], PlatformVersion::first())
.expect("expected traversal validator to exist for first protocol version");
platform_value!(unsafe_pattern);

let result = traversal_validator(
&schema,
&[test_callback_flag_invalid_regex_pattern],
PlatformVersion::first(),
)
.expect("expected traversal validator to exist for first protocol version");
let consensus_error = result.errors.first().expect("the error should be returned");

match consensus_error {
Expand All @@ -134,25 +248,30 @@ mod test {
err.path(),
"/properties/arrayOfObject/items/properties/simple".to_string()
);
assert_eq!(
err.pattern(),
"^((?!-|_)[a-zA-Z0-9-_]{0,62}[a-zA-Z0-9])$".to_string()
);
assert_eq!(err.pattern(), unsafe_pattern.to_string());
assert_eq!(consensus_error.code(), 10202);
}
_ => panic!("Expected error to be IncompatibleRe2PatternError"),
}
}

#[ignore]
#[test]
fn invalid_result_for_array_of_objects() {
fn traversal_propagates_pattern_callback_error_inside_array_items_tuple() {
// Traversal-mechanism check only: confirms traversal descends
// into the indexed entries of a tuple-form `items` array and
// that the path passed to the test callback includes the
// numeric index. Not a production pattern-validation assertion.
let unsafe_pattern = "^((?!-|_)[a-zA-Z0-9-_]{0,62}[a-zA-Z0-9])$";
let mut schema = get_document_schema();
schema["properties"]["arrayOfObjects"]["items"][0]["properties"]["simple"]["pattern"] =
platform_value!("^((?!-|_)[a-zA-Z0-9-_]{0,62}[a-zA-Z0-9])$");

let result = traversal_validator(&schema, &[], PlatformVersion::first())
.expect("expected traversal validator to exist for first protocol version");
platform_value!(unsafe_pattern);

let result = traversal_validator(
&schema,
&[test_callback_flag_invalid_regex_pattern],
PlatformVersion::first(),
)
.expect("expected traversal validator to exist for first protocol version");
let consensus_error = result.errors.first().expect("the error should be returned");

match consensus_error {
Expand All @@ -161,16 +280,20 @@ mod test {
err.path(),
"/properties/arrayOfObjects/items/[0]/properties/simple".to_string()
);
assert_eq!(
err.pattern(),
"^((?!-|_)[a-zA-Z0-9-_]{0,62}[a-zA-Z0-9])$".to_string()
);
assert_eq!(err.pattern(), unsafe_pattern.to_string());
assert_eq!(consensus_error.code(), 10202);
}
_ => panic!("Expected error to be IncompatibleRe2PatternError"),
}
}

fn get_basic_error(error: ConsensusError) -> BasicError {
if let ConsensusError::BasicError(err) = error {
return err;
}
panic!("the error: {:?} isn't a BasicError", error)
}
Comment on lines 108 to 295
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

🟡 Suggestion: Several un-ignored validator tests now assert the opposite of what their names describe

Four tests were brought back from #[ignore], but their assertions were weakened from checking specific validator failures to bare .is_valid() checks even though the names still describe invalid/error cases: should_return_invalid_result, invalid_result_for_array_of_object, invalid_result_for_array_of_objects, and should_return_error_if_bytes_array_parent_contains_items_or_prefix_items. Those tests now pass while asserting validity on exactly the schemas they previously treated as invalid, which turns them into rubber stamps rather than regression coverage. If these schemas are intentionally valid now, rename the tests and document the behavior change; otherwise, restore assertions that pin the actual failure mode or the later layer that now rejects them.

source: ['claude']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-dpp/src/data_contract/document_type/schema/recursive_schema_validator/mod.rs`:
- [SUGGESTION] lines 18-111: Several un-ignored validator tests now assert the opposite of what their names describe
  Four tests were brought back from `#[ignore]`, but their assertions were weakened from checking specific validator failures to bare `.is_valid()` checks even though the names still describe invalid/error cases: `should_return_invalid_result`, `invalid_result_for_array_of_object`, `invalid_result_for_array_of_objects`, and `should_return_error_if_bytes_array_parent_contains_items_or_prefix_items`. Those tests now pass while asserting validity on exactly the schemas they previously treated as invalid, which turns them into rubber stamps rather than regression coverage. If these schemas are intentionally valid now, rename the tests and document the behavior change; otherwise, restore assertions that pin the actual failure mode or the later layer that now rejects them.

Comment on lines 108 to 295
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

🟡 Suggestion: Restored traversal_validator tests no longer assert original error variants

The original commented-out versions of traversal_validator_with_no_sub_validators_accepts_byte_array_parent_with_items, ..._accepts_schema_with_unsafe_pattern, ..._accepts_array_of_object_with_unsafe_pattern, and ..._accepts_array_of_objects_with_unsafe_pattern asserted specific consensus-critical error variants (BasicError::JsonSchemaCompilationError with a precise message prefix, and BasicError::IncompatibleRe2PatternError with path / pattern / code 10202). The restored tests only check .is_valid() is true with an empty &[] sub-validator slice, which by construction can never produce errors — so they no longer guard against regressions in the validators that produced those errors. The module-level NOTE correctly states this is traversal-only and that 'rejection of these shapes lives in the callers that supply sub-validators, not in traversal_validator itself', but no replacement tests were added that pass a real sub-validator and re-assert the original error codes/paths. Either parameterise these tests with a real byte_array_has_no_items_as_parent_validator / pattern_is_re2_compatible_validator to restore the original error-variant assertions, or delete the now-trivial cases and add one focused test per real sub-validator. As is, four tests sit under suggestive names but contribute almost nothing beyond traversal_validator(...).is_ok().

source: ['claude']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-dpp/src/data_contract/document_type/schema/recursive_schema_validator/mod.rs`:
- [SUGGESTION] lines 24-117: Restored traversal_validator tests no longer assert original error variants
  The original commented-out versions of `traversal_validator_with_no_sub_validators_accepts_byte_array_parent_with_items`, `..._accepts_schema_with_unsafe_pattern`, `..._accepts_array_of_object_with_unsafe_pattern`, and `..._accepts_array_of_objects_with_unsafe_pattern` asserted specific consensus-critical error variants (`BasicError::JsonSchemaCompilationError` with a precise message prefix, and `BasicError::IncompatibleRe2PatternError` with `path` / `pattern` / `code 10202`). The restored tests only check `.is_valid()` is true with an empty `&[]` sub-validator slice, which by construction can never produce errors — so they no longer guard against regressions in the validators that produced those errors. The module-level NOTE correctly states this is traversal-only and that 'rejection of these shapes lives in the callers that supply sub-validators, not in `traversal_validator` itself', but no replacement tests were added that pass a real sub-validator and re-assert the original error codes/paths. Either parameterise these tests with a real `byte_array_has_no_items_as_parent_validator` / `pattern_is_re2_compatible_validator` to restore the original error-variant assertions, or delete the now-trivial cases and add one focused test per real sub-validator. As is, four tests sit under suggestive names but contribute almost nothing beyond `traversal_validator(...).is_ok()`.


fn get_document_schema() -> Value {
platform_value!({
"properties": {
Expand Down Expand Up @@ -251,13 +374,6 @@ mod test {
})
}

fn get_basic_error(error: ConsensusError) -> BasicError {
if let ConsensusError::BasicError(err) = error {
return err;
}
panic!("the error: {:?} isn't a BasicError", error)
}

// ================================================================
// New tests that actually run (not #[ignore]'d) to cover the
// traversal_validator_v0 traversal logic.
Expand Down
Loading
Loading