Add JSON schema validation for rules and tests #626
Conversation
Introduces JSON schema validation for Application Inspector rules using JsonSchema.Net. Adds RuleSchemaProvider and rule-schema.json, updates RulesVerifier and RuleStatus to support schema validation, and extends RulesVerifierOptions for configuration. Includes unit tests for schema validation in SchemaValidationTests.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces JSON schema validation for Application Inspector rules using JsonSchema.Net library. It adds comprehensive schema validation capabilities to ensure rules conform to a defined JSON schema, helping catch structural issues early in the development process.
- Adds RuleSchemaProvider class for validating rules against embedded JSON schema
- Updates RulesVerifier and RuleStatus to support schema validation reporting
- Extends RulesVerifierOptions with new configuration options for schema validation
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| AppInspector.Tests/RuleProcessor/SchemaValidationTests.cs | Comprehensive unit tests for schema validation functionality |
| AppInspector.RulesEngine/Schema/rule-schema.json | JSON schema definition for Application Inspector rules |
| AppInspector.RulesEngine/Schema/RuleSchemaProvider.cs | Provider class for loading and validating rules against schema |
| AppInspector.RulesEngine/RulesVerifierOptions.cs | Configuration options for enabling schema validation |
| AppInspector.RulesEngine/RulesVerifier.cs | Integration of schema validation into rule verification process |
| AppInspector.RulesEngine/RuleStatus.cs | Schema validation result tracking in rule status |
| AppInspector.RulesEngine/AppInspector.RulesEngine.csproj | Package reference for JsonSchema.Net library |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Debug: Check what JSON is generated | ||
| var json = System.Text.Json.JsonSerializer.Serialize(new[] { rule }, new System.Text.Json.JsonSerializerOptions | ||
| { | ||
| WriteIndented = true | ||
| }); | ||
|
|
||
| var result = provider.ValidateRule(rule); | ||
|
|
||
| // Debug: Show what errors we're getting | ||
| if (!result.IsValid) | ||
| { | ||
| var errorMessages = string.Join("; ", result.Errors.Select(e => $"'{e.Message}' at '{e.Path}' (Type: {e.ErrorType})")); | ||
| throw new Exception($"Schema validation failed. JSON: {json}\n\nErrors: {errorMessages}"); | ||
| } |
There was a problem hiding this comment.
Debug code should be removed from unit tests. The JSON serialization and debug exception are not needed for the test and add unnecessary complexity.
| // Debug: Check what JSON is generated | |
| var json = System.Text.Json.JsonSerializer.Serialize(new[] { rule }, new System.Text.Json.JsonSerializerOptions | |
| { | |
| WriteIndented = true | |
| }); | |
| var result = provider.ValidateRule(rule); | |
| // Debug: Show what errors we're getting | |
| if (!result.IsValid) | |
| { | |
| var errorMessages = string.Join("; ", result.Errors.Select(e => $"'{e.Message}' at '{e.Path}' (Type: {e.ErrorType})")); | |
| throw new Exception($"Schema validation failed. JSON: {json}\n\nErrors: {errorMessages}"); | |
| } | |
| var result = provider.ValidateRule(rule); | |
| var provider = new RuleSchemaProvider(); | ||
|
|
||
| // Load one of the actual default rule files | ||
| var ruleFilePath = Path.Combine("AppInspector", "rules", "default", "components", "load_dll.json"); |
There was a problem hiding this comment.
Hard-coded file path makes the test brittle and dependent on specific project structure. Consider using a relative path or test resource that's guaranteed to exist.
| { | ||
| var json = JsonSerializer.Serialize(rules, new JsonSerializerOptions | ||
| { | ||
| WriteIndented = true |
There was a problem hiding this comment.
Using WriteIndented = true adds unnecessary formatting overhead for validation purposes. Consider removing this option since the JSON is only used for validation, not display.
| WriteIndented = true |
| { | ||
| foreach (var error in validationResult.Errors) | ||
| { | ||
| var errorMessage = $"Schema validation error at {error.Path}: {error.Message} (Line: {error.LineNumber}, Position: {error.LinePosition})"; |
There was a problem hiding this comment.
The LineNumber and LinePosition properties are always 0 based on the SchemaValidationError class definition, making this error message misleading. These properties aren't populated by the validation logic.
Changed 'must-match' and 'must-not-match' in rule-schema.json to arrays of strings with updated descriptions. Enhanced SchemaValidationTests to validate all embedded default rules using RuleSetUtils and provide detailed error reporting for failed schema validations.
Adds a SchemaValidationResult property to Rule and updates TypedRuleSet to validate and store schema results when loading rules from JSON. RulesVerifier now uses the cached schema validation result if available, avoiding redundant validations. Also updates constructors to pass the schema provider and simplifies JSON serialization in RuleSchemaProvider.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Eliminated the inclusion of line number and position details from schema validation error and warning messages in RulesVerifier. Also removed the corresponding properties from the RuleSchemaProvider error class, simplifying error reporting.
Eliminated commented-out and unused debug code related to JSON serialization and error output in SchemaValidationTests. This cleanup improves test readability and removes unnecessary code.
Introduces multiple unit tests to verify that invalid rule definitions fail schema validation as expected. Tests cover invalid pattern types, confidence levels, severities, scopes, empty arrays, missing required fields, and schema validation error handling at both error and warning levels.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Moved rule-schema.json to the project root and updated its $id. Added rule-schema-v1.json as a new versioned schema file. Updated project and code references to use the new rule-schema-v1.json as the embedded resource.
…pplicationInspector into gfs/SchemaValidation
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Check if there are any details with errors | ||
| if (result.Details != null) | ||
| { | ||
| foreach (var detail in result.Details) | ||
| { | ||
| if (!detail.IsValid) | ||
| { | ||
| var path = detail.InstanceLocation?.ToString() ?? currentPath; | ||
| var schemaPath = detail.SchemaLocation?.ToString() ?? ""; | ||
|
|
||
| errors.Add(new SchemaValidationError | ||
| { | ||
| Message = $"Validation failed at schema location '{schemaPath}'", | ||
| Path = path, | ||
| ErrorType = "SchemaViolation" | ||
| }); | ||
|
|
||
| // Recursively collect errors from nested details | ||
| CollectErrorsFromResult(detail, errors, path); |
There was a problem hiding this comment.
The recursive error collection method could potentially cause stack overflow with deeply nested schema violations. Consider implementing an iterative approach using a stack or queue to avoid deep recursion.
| // Check if there are any details with errors | |
| if (result.Details != null) | |
| { | |
| foreach (var detail in result.Details) | |
| { | |
| if (!detail.IsValid) | |
| { | |
| var path = detail.InstanceLocation?.ToString() ?? currentPath; | |
| var schemaPath = detail.SchemaLocation?.ToString() ?? ""; | |
| errors.Add(new SchemaValidationError | |
| { | |
| Message = $"Validation failed at schema location '{schemaPath}'", | |
| Path = path, | |
| ErrorType = "SchemaViolation" | |
| }); | |
| // Recursively collect errors from nested details | |
| CollectErrorsFromResult(detail, errors, path); | |
| // Use an explicit stack to avoid recursion and potential stack overflow | |
| var stack = new Stack<(EvaluationResults node, string path)>(); | |
| stack.Push((result, currentPath)); | |
| while (stack.Count > 0) | |
| { | |
| var (currentResult, pathSoFar) = stack.Pop(); | |
| if (currentResult.Details != null) | |
| { | |
| foreach (var detail in currentResult.Details) | |
| { | |
| if (!detail.IsValid) | |
| { | |
| var path = detail.InstanceLocation?.ToString() ?? pathSoFar; | |
| var schemaPath = detail.SchemaLocation?.ToString() ?? ""; | |
| errors.Add(new SchemaValidationError | |
| { | |
| Message = $"Validation failed at schema location '{schemaPath}'", | |
| Path = path, | |
| ErrorType = "SchemaViolation" | |
| }); | |
| // Push nested details onto the stack for further processing | |
| stack.Push((detail, path)); | |
| } |
Replaces deprecated Assert.Empty with Assert.Fail in SchemaValidationTests and simplifies type references for schema validation result and error classes. Also removes the rule-schema.json file, possibly indicating a migration to a different schema management approach.
Replaces recursive calls in CollectErrorsFromResult with an explicit stack to avoid potential stack overflow on deeply nested validation results. This change improves robustness when processing large or deeply nested schemas.
Introduces JSON schema validation for Application Inspector rules using JsonSchema.Net. Adds RuleSchemaProvider and rule-schema.json, updates RulesVerifier and RuleStatus to support schema validation, and extends RulesVerifierOptions for configuration. Includes unit tests for schema validation in SchemaValidationTests.