Uses raw databaseName for fetching state #107
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors how database names are handled by removing the previous escaping logic when running queries and introducing a new property (EscapedName) in the Database model for cases where escaping is required.
- Removed local escaping logic in KustoSchemaHandler and now uses the raw databaseName
- Introduced Database.EscapedName for on-demand escaping when necessary
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| KustoSchemaTools/Model/Database.cs | Added a new property EscapedName computed using Name.BracketIfIdentifier() |
| KustoSchemaTools/KustoSchemaHandler.cs | Replaced escaped database names with the raw databaseName in logs, handler creation, and diff generation |
Comments suppressed due to low confidence (1)
KustoSchemaTools/KustoSchemaHandler.cs:41
- The PR description introduces the EscapedName property for escaped identifiers. Please verify if using the raw databaseName in logging and factory calls is intentional, or if the EscapedName property should be used when an escaped identifier is required.
Log.LogInformation($"Generating diff markdown for {Path.Combine(path, databaseName)} => {cluster}/{databaseName}");
| foreach (var cluster in clusters.Connections) | ||
| { | ||
| Log.LogInformation($"Generating diff markdown for {Path.Combine(path, databaseName)} => {cluster}/{escapedDbName}"); | ||
| Log.LogInformation($"Generating diff markdown for {Path.Combine(path, databaseName)} => {cluster}/{databaseName}"); |
Check warning
Code scanning / CodeQL
Use of default ToString() Warning
Copilot Autofix
AI about 20 hours ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| @@ -36,5 +37,6 @@ public class Database | |||
|
|
|||
| public Dictionary<string, FollowerDatabase> Followers { get; set; } = new Dictionary<string, FollowerDatabase>(); | |||
|
|
|||
| public string EscapedName => Name.BracketIfIdentifier(); | |||
There was a problem hiding this comment.
where this property is used?
| foreach (var cluster in clusters.Connections) | ||
| { | ||
| Log.LogInformation($"Generating diff markdown for {Path.Combine(path, databaseName)} => {cluster}/{escapedDbName}"); | ||
| Log.LogInformation($"Generating diff markdown for {Path.Combine(path, databaseName)} => {cluster}/{databaseName}"); |
Check notice
Code scanning / CodeQL
Call to System.IO.Path.Combine Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 20 hours ago
To fix the issue, replace the use of Path.Combine with Path.Join. The Path.Join method does not treat any of its arguments as absolute paths, ensuring that all arguments are concatenated as intended. This change will prevent the silent dropping of earlier arguments if a later argument is an absolute path.
Specifically:
- Replace the
Path.Combine(path, databaseName)call on line 41 withPath.Join(path, databaseName). - Ensure that the
System.IOnamespace is already imported (it is in this case).
| @@ -40,3 +40,3 @@ | ||
| { | ||
| Log.LogInformation($"Generating diff markdown for {Path.Combine(path, databaseName)} => {cluster}/{databaseName}"); | ||
| Log.LogInformation($"Generating diff markdown for {Path.Join(path, databaseName)} => {cluster}/{databaseName}"); | ||
|
|
This pull request changes the handling of database names by removing escaping logic when loading the database state and introducing a new property for escaped names to allow access to the escaped name when necessary.