Adds model unit tests and Update Policy schema validation #103
Conversation
sdseaton
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| foreach (var targetColumn in targetTable.Columns!) | ||
| { | ||
| // If the query explicitly projects this column, validate its type compatibility | ||
| if (queryColumns.ContainsKey(targetColumn.Key)) |
Check notice
Code scanning / CodeQL
Inefficient use of ContainsKey Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 22 hours ago
To fix the issue, replace the ContainsKey check followed by the indexer access with a single call to TryGetValue. This eliminates the redundant dictionary lookup and improves efficiency. Specifically, modify the code on line 269 to use TryGetValue to check for the existence of the key and retrieve its value in one operation. Ensure that the retrieved value is used in subsequent logic.
| @@ -268,5 +268,4 @@ | ||
| // If the query explicitly projects this column, validate its type compatibility | ||
| if (queryColumns.ContainsKey(targetColumn.Key)) | ||
| if (queryColumns.TryGetValue(targetColumn.Key, out var queryColumnType)) | ||
| { | ||
| var queryColumnType = queryColumns[targetColumn.Key]; | ||
| if (!AreTypesCompatible(queryColumnType, targetColumn.Value, config)) | ||
| @@ -281,3 +280,3 @@ | ||
| { | ||
| if (!targetTable.Columns.ContainsKey(queryColumn.Key)) | ||
| if (!targetTable.Columns.TryGetValue(queryColumn.Key, out _)) | ||
| { |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the KustoSchemaTools project by introducing parser-based update policy validation, extending script generation methods, and adding comprehensive unit tests with FluentAssertions.
- Added
UpdatePolicyValidationConfigdocumentation and config options for strict vs. permissive validation. - Extended model APIs (
Table,TablePolicy) to support optional update policy validation during script creation. - Introduced
KustoQuerySchemaExtractorandEnhancedUpdatePolicyValidatorfor KQL parsing-based schema extraction and validation. - Added a wide suite of unit tests across models, utilities, and integration flows, and integrated
FluentAssertions.
Reviewed Changes
Copilot reviewed 22 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/README_UpdatePolicyValidationConfig.md | New guide for UpdatePolicyValidationConfig options and usage examples |
| KustoSchemaTools/Model/Table.cs | Extended CreateScripts to accept database context and validate policies |
| KustoSchemaTools/Model/Policy.cs | Added ValidateUpdatePolicies and script overloads with optional validation |
| KustoSchemaTools/Model/KustoQuerySchemaExtractor.cs | New static class for KQL schema extraction and query validation |
| KustoSchemaTools/Model/EnhancedUpdatePolicyValidator.cs | Parser-based update policy validator integrating new extractor |
| KustoSchemaTools/Examples/README_KustoParser.md | Expanded examples for parser usage |
| KustoSchemaTools/Examples/KustoParserExamples.cs | Added C# code samples demonstrating new parsing and validation features |
| KustoSchemaTools.Tests/KustoSchemaTools.Tests.csproj | Added FluentAssertions dependency for expressive testing assertions |
| KustoSchemaTools.Tests/ (multiple files) | Comprehensive unit tests for models, validators, and parser integration |
Comments suppressed due to low confidence (1)
KustoSchemaTools/Examples/README_KustoParser.md:347
- This example attempts to instantiate
EnhancedUpdatePolicyValidator, but it is declared as a static class. Update the example to call the static method directly, e.g.var result = EnhancedUpdatePolicyValidator.ValidatePolicyWithKustoParser(...);.
var enhancedValidator = new EnhancedUpdatePolicyValidator();
| /// <param name="database">The database context (optional, for update policy validation)</param> | ||
| /// <param name="validateUpdatePolicies">Whether to validate update policies before creating scripts</param> | ||
| /// <returns>List of database script containers</returns> | ||
| public List<DatabaseScriptContainer> CreateScripts(string name, bool isNew, Database? database = null, bool validateUpdatePolicies = false) |
There was a problem hiding this comment.
The isNew parameter is declared but never used in the method body. Consider removing it or implementing logic to handle new vs. existing tables to avoid confusion.
| public List<DatabaseScriptContainer> CreateScripts(string name, bool isNew, Database? database = null, bool validateUpdatePolicies = false) | |
| public List<DatabaseScriptContainer> CreateScripts(string name, Database? database = null, bool validateUpdatePolicies = false) |
Copilot uses AI. Check for mistakes.
This pull request adds comprehensive unit tests to validate the behavior of various models and utilities in the
KustoSchemaToolsproject. The changes include new test classes for models such asAADObject,Cluster,Database,ContinuousExport, andFunction, as well as for utilities likeKustoQuerySchemaExtractor. Additionally, theFluentAssertionslibrary has been added as a dependency to improve test readability and assertion capabilities.Dependency Addition:
FluentAssertionspackage to the test project for expressive assertion syntax (KustoSchemaTools.Tests.csproj).Model-Specific Unit Tests:
AADObject Model:
AADObject.Cluster Model:
Cluster.ContinuousExport Model:
ContinuousExport.Database Model:
Database, including handling of collections likeAdminsandTables.Function Model:
Function.Query Schema Extraction and Validation:
KustoQuerySchemaExtractorto validate schema extraction, column references, and query validation, including handling of syntax errors, type conversions, and operations likesummarizeandjoin.