-
Notifications
You must be signed in to change notification settings - Fork 12
Add initial support for structured diffs #127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| return new List<string>(); | ||
| } | ||
|
|
||
| var differ = new Differ(); |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning
differ
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 23 hours ago
To fix the issue, simply remove the assignment and local declaration for the unused variable differ. In BuildDiffPreview, eliminate the line var differ = new Differ(); entirely. No other changes are required; the InlineDiffBuilder.Diff invocation works independently and does not depend on Differ. This change should be limited to the relevant lines in the method body and requires no additional imports or method definitions.
| @@ -132,7 +132,6 @@ | ||
| return new List<string>(); | ||
| } | ||
|
|
||
| var differ = new Differ(); | ||
| var diff = InlineDiffBuilder.Diff(before, after, false); | ||
|
|
||
| var preview = diff.Lines |
| ?? new Dictionary<string, DatabaseScriptContainer>(); | ||
|
|
||
| var sb = new StringBuilder(); | ||
| var differ = new Differ(); |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning
differ
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 23 hours ago
To fix the problem, simply remove the unnecessary assignment to the differ variable in the BuildDiffMarkdown method. Specifically, delete line 168 (var differ = new Differ();). No additional imports or logic changes are required, as the Differ instance is not used elsewhere in the method and its removal has no impact on program correctness.
| @@ -165,7 +165,6 @@ | ||
| var previousScripts = BuildPreviousScripts(change.From, change.Entity); | ||
|
|
||
| var sb = new StringBuilder(); | ||
| var differ = new Differ(); | ||
| foreach (var script in change.Scripts) | ||
| { | ||
| var before = previousScripts.TryGetValue(script.Kind, out var prior) |
| foreach (var script in comparison.NewScripts) | ||
| { | ||
| if (previousScripts.TryGetValue(script.Kind, out var previous)) | ||
| { | ||
| comparison.OldScripts.Add(CloneScript(previous)); | ||
| } | ||
| } |
Check notice
Code scanning / CodeQL
Missed opportunity to use Where Note
implicitly filters its target sequence
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.
Pull request overview
This pull request introduces a structured diff generation feature for Kusto schema changes, alongside important refactorings to improve type safety and API consistency. The changes enable programmatic consumption of schema differences through a structured JSON format while maintaining backward compatibility with the existing markdown output.
Key Changes
- Refactored script property access: All references to
TextandOrderproperties onDatabaseScriptContainernow consistently useScript.TextandScript.Order, improving encapsulation and type safety - Added structured diff API: New
GenerateStructuredDiffmethod returns schema differences in a structured format withStructuredDiffResult,StructuredDiff, and related models, enabling programmatic diff consumption - Enhanced diagnostics handling: Introduced
ScriptDiagnosticmodel andDiagnosticsproperty onDatabaseScriptContainerto capture and expose detailed script validation information (partially implemented)
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
KustoSchemaTools/Parser/KustoWriter/DefaultDatabaseWriter.cs |
Updated to access script properties via Script.Text and Script.Order instead of direct properties |
KustoSchemaTools/Parser/KustoClusterHandler.cs |
Updated to access script properties via Script.Text and Script.Order instead of direct properties |
KustoSchemaTools/Model/StructuredDiff.cs |
New file defining models for structured diff output including StructuredDiffResult, StructuredDiff, StructuredChange, StructuredScriptComparison, and StructuredComment |
KustoSchemaTools/Model/ScriptDiagnostic.cs |
New file defining model for capturing script diagnostic information (start, end, description) |
KustoSchemaTools/Model/DatabaseScript.cs |
Added JSON serialization attributes for text and order properties |
KustoSchemaTools/KustoSchemaHandler.cs |
Refactored to extract common diff computation logic, added GenerateStructuredDiff method, improved Apply method with parallel execution, and added helper classes for diff context |
KustoSchemaTools/Changes/StructuredChangeExtensions.cs |
New file providing extension methods to convert IChange objects to structured representations with diff previews and validation payloads |
KustoSchemaTools/Changes/ScriptCompareChange.cs |
Updated property access to Script.Text/Script.Order and added logic to populate Diagnostics property when validation errors occur |
KustoSchemaTools/Changes/EntityGroupChange.cs |
Updated to access script text via Script.Text property |
KustoSchemaTools/Changes/DeletionChange.cs |
Updated to access script text via Script.Text property |
KustoSchemaTools/Changes/DatabaseScriptContainer.cs |
Added JSON attributes for all properties, introduced Diagnostics property for storing script validation details, removed convenience properties Text and Order |
KustoSchemaTools/Changes/DatabaseChanges.cs |
Updated to access script text via Script.Text property |
KustoSchemaTools/Changes/ClusterChanges.cs |
Updated to access script text via Script.Text property |
Comments suppressed due to low confidence (2)
KustoSchemaTools/Changes/StructuredChangeExtensions.cs:142
- This assignment to differ is useless, since its value is never read.
var differ = new Differ();
KustoSchemaTools/Changes/StructuredChangeExtensions.cs:180
- This assignment to differ is useless, since its value is never read.
var differ = new Differ();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ?? new Dictionary<string, DatabaseScriptContainer>(); | ||
|
|
||
| var sb = new StringBuilder(); | ||
| var differ = new Differ(); |
Copilot
AI
Dec 5, 2025
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.
The differ variable is instantiated but never used. The InlineDiffBuilder.Diff method on line 193 doesn't require this variable. Consider removing:
var differ = new Differ();| var differ = new Differ(); |
| Scripts.Add(toScript); | ||
| var code = KustoCode.Parse(toScript.Text); | ||
| var code = KustoCode.Parse(toScript.Script.Text); | ||
| var diagnostics = code.GetDiagnostics(); |
Copilot
AI
Dec 5, 2025
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.
Diagnostics should be captured in the Diagnostics property for consistency with ScriptCompareChange. Currently, diagnostics are only used to set IsValid but not stored. Consider adding:
toScript.Diagnostics = diagnostics.Any()
? diagnostics.Select(diagnostic => new ScriptDiagnostic
{
Start = diagnostic.Start,
End = diagnostic.End,
Description = diagnostic.Description
}).ToList()
: null;| var diagnostics = code.GetDiagnostics(); | |
| var diagnostics = code.GetDiagnostics(); | |
| toScript.Diagnostics = diagnostics.Any() | |
| ? diagnostics.Select(diagnostic => new ScriptDiagnostic | |
| { | |
| Start = diagnostic.Start, | |
| End = diagnostic.End, | |
| Description = diagnostic.Description | |
| }).ToList() | |
| : null; |
| var code = KustoCode.Parse(sc.Text); | ||
| var code = KustoCode.Parse(sc.Script.Text); | ||
| var diagnostics = code.GetDiagnostics(); | ||
| sc.IsValid = diagnostics.Any() == false; |
Copilot
AI
Dec 5, 2025
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.
Diagnostics should be captured in the Diagnostics property for consistency with ScriptCompareChange. Currently, diagnostics are only used to set IsValid but not stored. Consider adding:
sc.Diagnostics = diagnostics.Any()
? diagnostics.Select(diagnostic => new ScriptDiagnostic
{
Start = diagnostic.Start,
End = diagnostic.End,
Description = diagnostic.Description
}).ToList()
: null;| sc.IsValid = diagnostics.Any() == false; | |
| sc.IsValid = diagnostics.Any() == false; | |
| sc.Diagnostics = diagnostics.Any() | |
| ? diagnostics.Select(diagnostic => new ScriptDiagnostic | |
| { | |
| Start = diagnostic.Start, | |
| End = diagnostic.End, | |
| Description = diagnostic.Description | |
| }).ToList() | |
| : null; |
| var code = KustoCode.Parse(script.Text); | ||
| var code = KustoCode.Parse(script.Script.Text); | ||
| var diagnostics = code.GetDiagnostics(); | ||
| script.IsValid = diagnostics.Any() == false; |
Copilot
AI
Dec 5, 2025
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.
Diagnostics should be captured in the Diagnostics property for consistency with ScriptCompareChange. Currently, diagnostics are only used to set IsValid but not stored. Consider adding:
script.Diagnostics = diagnostics.Any()
? diagnostics.Select(diagnostic => new ScriptDiagnostic
{
Start = diagnostic.Start,
End = diagnostic.End,
Description = diagnostic.Description
}).ToList()
: null;| script.IsValid = diagnostics.Any() == false; | |
| script.IsValid = diagnostics.Any() == false; | |
| script.Diagnostics = diagnostics.Any() | |
| ? diagnostics.Select(diagnostic => new ScriptDiagnostic | |
| { | |
| Start = diagnostic.Start, | |
| End = diagnostic.End, | |
| Description = diagnostic.Description | |
| }).ToList() | |
| : null; |
| var code = KustoCode.Parse(script.Text); | ||
| var code = KustoCode.Parse(script.Script.Text); | ||
| var diagnostics = code.GetDiagnostics(); | ||
| script.IsValid = !diagnostics.Any(); |
Copilot
AI
Dec 5, 2025
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.
Diagnostics should be captured in the Diagnostics property for consistency with ScriptCompareChange. Currently, diagnostics are only used to set IsValid but not stored. Consider adding:
script.Diagnostics = diagnostics.Any()
? diagnostics.Select(diagnostic => new ScriptDiagnostic
{
Start = diagnostic.Start,
End = diagnostic.End,
Description = diagnostic.Description
}).ToList()
: null;| script.IsValid = !diagnostics.Any(); | |
| script.IsValid = !diagnostics.Any(); | |
| script.Diagnostics = diagnostics.Any() | |
| ? diagnostics.Select(diagnostic => new ScriptDiagnostic | |
| { | |
| Start = diagnostic.Start, | |
| End = diagnostic.End, | |
| Description = diagnostic.Description | |
| }).ToList() | |
| : null; |
| Log.LogInformation($"Generating diff markdown for {Path.Combine(path, databaseName)} => {cluster}/{databaseName}"); | ||
| try | ||
| { | ||
| Log.LogInformation($"Generating and applying script for {Path.Combine(path, databaseName)} => {cluster}/{databaseName}"); |
Copilot
AI
Dec 5, 2025
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.
Default 'ToString()': Cluster inherits 'ToString()' from 'Object', and so is not suitable for printing.
| var payload = new Dictionary<string, object?>(); | ||
| foreach (var script in newScripts) | ||
| { | ||
| var oldScript = previousScripts.ContainsKey(script.Kind) ? previousScripts[script.Kind] : null; |
Copilot
AI
Dec 5, 2025
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.
Inefficient use of 'ContainsKey' and indexer.
| var oldScript = previousScripts.ContainsKey(script.Kind) ? previousScripts[script.Kind] : null; | |
| previousScripts.TryGetValue(script.Kind, out var oldScript); |
| var diagnostics = code.GetDiagnostics(); | ||
| change.IsValid = diagnostics.Any() == false || change.Order == -1; | ||
| var hasDiagnostics = diagnostics.Any(); | ||
| change.IsValid = hasDiagnostics == false || change.Script.Order == -1; |
Copilot
AI
Dec 5, 2025
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.
The expression 'A == false' can be simplified to '!A'.
| change.IsValid = hasDiagnostics == false || change.Script.Order == -1; | |
| change.IsValid = !hasDiagnostics || change.Script.Order == -1; |
| var dbHandler = KustoDatabaseHandlerFactory.Create(clusters.Connections[0].Url, databaseName); | ||
|
|
||
| var db = await dbHandler.LoadAsync(); | ||
| if (includeColumns == false) |
Copilot
AI
Dec 5, 2025
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.
The expression 'A == false' can be simplified to '!A'.
| if (includeColumns == false) | |
| if (!includeColumns) |
| var commentsHealthy = changeList | ||
| .Select(change => change.Comment) | ||
| .Where(comment => comment != null) | ||
| .All(comment => comment.FailsRollout == false); |
Copilot
AI
Dec 5, 2025
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.
The expression 'A == false' can be simplified to '!A'.
| .All(comment => comment.FailsRollout == false); | |
| .All(comment => !comment.FailsRollout); |
| Log.LogInformation($"Generating diff markdown for {Path.Combine(path, databaseName)} => {cluster}/{databaseName}"); | ||
| try | ||
| { | ||
| Log.LogInformation($"Generating and applying script for {Path.Join(path, databaseName)} => {cluster}/{databaseName}"); |
Check warning
Code scanning / CodeQL
Use of default ToString() Warning
Cluster
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 23 hours ago
To fix the problem, override the default ToString() method in the Cluster class to return a meaningful string representation, such as its Url property or a combination of its key identifying properties. No changes to the log statement are needed—the log output will automatically use the new ToString() implementation. The override should be added within the code for the Cluster class, which is not present in the code snippet, but is likely defined in the appropriate model or domain folder. If you can only edit within the region shown above, an alternative is to perform a bespoke string conversion in the log statement itself (e.g., output cluster.Url or similar). Given the limitations (only shown code can be edited), the log line should be changed to manually reference a property like cluster.Url, ensuring the log output is clear and useful.
-
Copy modified line R102
| @@ -99,7 +99,7 @@ | ||
| { | ||
| try | ||
| { | ||
| Log.LogInformation($"Generating and applying script for {Path.Join(path, databaseName)} => {cluster}/{databaseName}"); | ||
| Log.LogInformation($"Generating and applying script for {Path.Join(path, databaseName)} => {cluster.Url}/{databaseName}"); | ||
| var dbHandler = KustoDatabaseHandlerFactory.Create(cluster.Url, databaseName); | ||
| await dbHandler.WriteAsync(yamlDb); | ||
| results.TryAdd(cluster.Url, null!); |
This pull request introduces several improvements and refactors to the Kusto schema tooling codebase, focusing on script handling, diagnostics, and structured diff generation. The most significant changes are grouped below.
Refactoring of script handling and diagnostics:
DatabaseScriptContainerand related change classes to referenceScript.Textinstead of a directTextproperty, improving type safety and consistency. [1] [2] [3] [4] [5] [6] [7]Diagnosticsproperty (with JSON serialization attributes) toDatabaseScriptContainerto store detailed script diagnostics, and updated code to populate this property when diagnostics are present. [1] [2]Structured diff computation and output:
GenerateStructuredDifftoKustoSchemaHandler, which computes structured diffs for clusters and followers, returning aStructuredDiffResultwith validity status and an optional message if the output is too long.StructuredChangeExtensionsclass, which provides conversion of change objects to structured representations, including script comparisons, validation payloads, and diff previews in markdown format.Infrastructure and code organization:
KustoSchemaHandlerto use a new internal methodBuildDiffComputationResultfor shared diff computation logic, and updated methods to leverage this for markdown and structured diff generation.Applymethod for cluster updates, usingParallel.ForEachAsyncand a concurrent dictionary for results.General improvements:
usingstatements for collections and LINQ, and improved serialization attributes for better API consistency. [1] [2] [3]