Catch exceptions and log when parsing graph models and snapshots#98
Merged
Conversation
Co-authored-by: Copilot <copilot@github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Improves server-side schema loading robustness for graph models by preventing JSON parsing failures (graph model content and snapshot lists) from breaking schema loading, and adds regression tests covering snapshot parsing edge cases.
Changes:
- Added MSTest coverage for graph model snapshot loading scenarios (no snapshot rows, null snapshot field, malformed snapshot JSON, mixed-good/bad models).
- Wrapped snapshot JSON deserialization in
ServerSchemaSourcewith try/catch and added logging on failures. - Updated
GraphModel.TryParseto catch deserialization exceptions and returnfalseinstead of propagating.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/ServerTests/Features/ServerSchemaSourceTests.cs | New tests validating schema loading resilience when snapshot data is missing/null/malformed. |
| src/Server/Utilities/GraphModel.cs | Makes TryParse resilient to malformed graph model JSON by catching exceptions. |
| src/Server/Schema/ServerSchemaSource.cs | Adds guarded snapshot parsing + logging; changes graph model parsing flow (currently drops models on parse failure). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+23
to
+27
| catch | ||
| { | ||
| model = null!; | ||
| return false; | ||
| } |
Comment on lines
16
to
+27
| public static bool TryParse(string text, out GraphModel model) | ||
| { | ||
| model = JsonConvert.DeserializeObject<GraphModel>(text); | ||
| return model != null; | ||
| try | ||
| { | ||
| model = JsonConvert.DeserializeObject<GraphModel>(text); | ||
| return model != null; | ||
| } | ||
| catch | ||
| { | ||
| model = null!; | ||
| return false; | ||
| } |
| } | ||
| catch (JsonException je) | ||
| { | ||
| _logger?.Log($"ServerSchemaSource: Failed to parse snapshots for graph model '{e.EntityName}': {je.Message}"); |
Comment on lines
+4
to
+12
| using System.Collections.Immutable; | ||
| using System.Data; | ||
| using System.Diagnostics.CodeAnalysis; | ||
| using Kusto.Data; | ||
| using Kusto.Data.Common; | ||
| using Kusto.Data.Data; | ||
| using Kusto.Language; | ||
| using Kusto.Language.Editor; | ||
| using Kusto.Vscode; |
Comment on lines
+239
to
+241
| public IConnection WithCluster(string clusterName) => this; | ||
| public IConnection WithDatabase(string databaseName) => this; | ||
|
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Add additional tests for graph models and snapshot schema loading