DB Exists check fix #1626

DavidWiseman · 2025-11-07T15:44:10Z

DB exists check against master DB might fail for contained databases or if the user doesn't have permissions against master. If we can connect to the database, then it exists. Use both checks as trying to connect to a database that doesn't exist can introduce a delay. #1609

DB exists check against master DB might fail for contained databases or if the user doesn't have permissions against master. If we can connect to the database, then it exists. Use both checks as trying to connect to a database that doesn't exist can introduce a delay. trimble-oss#1609

Copilot

Pull Request Overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-07T16:13:36Z

DBADashSharedGUI/DBConnection.cs

+                {
+                    // Success on first completed (either DB or master)
+                    DialogResult = DialogResult.OK;
+                    cts.Cancel(); // cancel other pending attempt if still running


After canceling at line 202, the code immediately returns without waiting for the cancelled task to complete. The cancelled task may still be executing and could throw an exception. Consider using try/finally or awaiting with exception suppression to ensure clean resource disposal.

Suggested change

cts.Cancel(); // cancel other pending attempt if still running

cts.Cancel(); // cancel other pending attempt if still running

try

{

// Await the other task to ensure clean completion and observe exceptions

var other = first == dbTask ? masterTask : dbTask;

await other.ContinueWith(t => { var _ = t.Exception; }, TaskContinuationOptions.OnlyOnFaulted);

}

catch

{

// Suppress any exceptions from the cancelled task

}

Created extension method to simplify

Copilot · 2025-11-07T16:13:37Z

DBADashServiceConfig/ServiceConfig.cs

+                catch
                {
-                    InvokeSetStatus(lblServerNameWarning, lblServerNameWarning.Text, lblServerNameWarning.ForeColor, lblServerNameWarning.Font.Style);
-                    InvokeSetVisible(lblServerNameWarning, true);
+                    // Ignore error.  We might not be able to connect to master database


Nested try-catch blocks with empty catch clause on the inner block. The outer try at line 399 appears redundant since it doesn't have a matching catch block in the visible diff. Consider simplifying this structure or documenting the purpose of the outer try block.

Suggested change

catch

{

InvokeSetStatus(lblServerNameWarning, lblServerNameWarning.Text, lblServerNameWarning.ForeColor, lblServerNameWarning.Font.Style);

InvokeSetVisible(lblServerNameWarning, true);

// Ignore error. We might not be able to connect to master database

catch (Exception ex)

{

// Exception ignored intentionally: unable to connect to master database is expected in some scenarios (e.g., invalid credentials, network issues).

// If logging is available, consider logging ex at a debug or trace level for troubleshooting.

Outer is not redundant

Copilot · 2025-11-07T16:13:37Z

DBADash/Upgrade/DBValidations.cs

+            builder.InitialCatalog = "master";
+            await using var cn = new SqlConnection(builder.ConnectionString);
+            await using var cmd = new SqlCommand(
+                "SELECT CASE WHEN EXISTS(SELECT 1 FROM sys.databases WHERE name = @db) THEN 1 ELSE 0 END",


[nitpick] The query returns an int (1 or 0) but the original code expected a BIT/bool. The change from CAST(1 as BIT) to just 1 requires casting the result to int at line 58. This is correctly handled, but consider adding a comment explaining this type change for future maintainers.

Doesn't make much difference. Either is fine. 🤷

Copilot · 2025-11-07T16:13:37Z

DBADash/Upgrade/DBValidations.cs

-            else
-            {
-                return Task.CompletedTask;
+                await Task.Run(() => dac.ProcessDacPac(connectionString, db, DACPackPath, status.VersionStatus));


Wrapping a synchronous method ProcessDacPac in Task.Run creates a new thread. If ProcessDacPac is a long-running or frequently called operation, consider converting it to a truly async method rather than using thread pool offloading.

Suggested change

await Task.Run(() => dac.ProcessDacPac(connectionString, db, DACPackPath, status.VersionStatus));

await dac.ProcessDacPacAsync(connectionString, db, DACPackPath, status.VersionStatus);

Out of scope. Could maybe be refactored but no async methods are currently provided.

Parser.Default.ParseArguments<Options>(args).WithParsed(async o => ...). An async lambda passed to WithParsed (which expects Action<Options>) becomes async void. Exceptions inside async void bypass normal Task exception propagation and can terminate the process.

Add ObserveFault() extension methods as a neater way to apply suggestion in PR trimble-oss#1626

Copilot

Pull Request Overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-08T13:26:59Z

DBADash/Upgrade/DBValidations.cs

+            builder.InitialCatalog = "master";
+            await using var cn = new SqlConnection(builder.ConnectionString);
+            await using var cmd = new SqlCommand(
+                "SELECT CASE WHEN EXISTS(SELECT 1 FROM sys.databases WHERE name = @db) THEN 1 ELSE 0 END",


The query returns an int value instead of a bit. Consider using CAST(1 AS BIT) ELSE CAST(0 AS BIT) to match the original return type and avoid the type check result is int i && i == 1 in favor of a simpler (bool)result.

Copilot · 2025-11-08T13:26:59Z

DBADashServiceConfig/ServiceConfig.cs

            InvokeSetStatus(lblVersionInfo, "Validating...", DashColors.TrimbleBlue, FontStyle.Regular);
-            _ = Task.Run(() =>
-            {
+            _ = Task.Run(async ()=>{


Fire-and-forget task pattern detected. The underscore discard indicates the task result is intentionally ignored, but exceptions in this task will not be observed. Consider using .ContinueWith with error handling or storing the task reference to observe exceptions.

Out of scope

Copilot AI review requested due to automatic review settings November 7, 2025 15:44

This comment was marked as resolved.

Sign in to view

DavidWiseman force-pushed the 1609_DBExistsCheck branch from 5f64eb5 to ef4cb2e Compare November 7, 2025 15:59

DavidWiseman requested a review from Copilot November 7, 2025 15:59

This comment was marked as resolved.

Sign in to view

DavidWiseman force-pushed the 1609_DBExistsCheck branch from ef4cb2e to 667f419 Compare November 7, 2025 16:09

DavidWiseman requested a review from Copilot November 7, 2025 16:10

This comment was marked as resolved.

Sign in to view

DavidWiseman force-pushed the 1609_DBExistsCheck branch from 667f419 to a32abdf Compare November 7, 2025 16:11

DavidWiseman requested a review from Copilot November 7, 2025 16:12

Copilot AI reviewed Nov 7, 2025

View reviewed changes

DBADashConfig async fix

02885f7

Parser.Default.ParseArguments<Options>(args).WithParsed(async o => ...). An async lambda passed to WithParsed (which expects Action<Options>) becomes async void. Exceptions inside async void bypass normal Task exception propagation and can terminate the process.

DavidWiseman force-pushed the 1609_DBExistsCheck branch from 9f03bde to 02885f7 Compare November 7, 2025 21:30

Observe exceptions

2482586

Add ObserveFault() extension methods as a neater way to apply suggestion in PR trimble-oss#1626

DavidWiseman requested a review from Copilot November 8, 2025 13:26

Copilot AI reviewed Nov 8, 2025

View reviewed changes

DavidWiseman merged commit cf0af31 into trimble-oss:main Nov 8, 2025

DavidWiseman deleted the 1609_DBExistsCheck branch November 8, 2025 13:47

-                    cts.Cancel(); // cancel other pending attempt if still running
+                    cts.Cancel(); // cancel other pending attempt if still running
+                    try
+                    {
+                        // Await the other task to ensure clean completion and observe exceptions
+                        var other = first == dbTask ? masterTask : dbTask;
+                        await other.ContinueWith(t => { var _ = t.Exception; }, TaskContinuationOptions.OnlyOnFaulted);
+                    }
+                    catch
+                    {
+                        // Suppress any exceptions from the cancelled task
+                    }

	await Task.Run(() => dac.ProcessDacPac(connectionString, db, DACPackPath, status.VersionStatus));
	await dac.ProcessDacPacAsync(connectionString, db, DACPackPath, status.VersionStatus);

Uh oh!

DB Exists check fix #1626

DB Exists check fix #1626

Uh oh!

Conversation

DavidWiseman commented Nov 7, 2025

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

DavidWiseman Nov 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

DavidWiseman Nov 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

DavidWiseman Nov 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

DavidWiseman Nov 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 8, 2025

Choose a reason for hiding this comment

Uh oh!

DavidWiseman Nov 8, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant