-
Notifications
You must be signed in to change notification settings - Fork 33
Fix to (re)enable lower-privilege connection strings #234
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -177,10 +177,22 @@ async Task<DatabaseLock> AcquireDatabaseLockAsync(bool createDatabaseIfNotExists | |
|
||
async Task EnsureDatabaseExistsAsync() | ||
{ | ||
// Note that we may not be able to connect to the DB, let alone obtain the lock, | ||
// Note that we may not be able to connect to the target DB, let alone obtain the lock, | ||
// if the database does not exist yet. So we obtain a connection to the 'master' database for now. | ||
// If the current connection doesn't have permission to access the master database, this will fail. | ||
// The exact error we see is hard to predict, so we catch SqlException and log a generic message. | ||
using SqlConnection connection = this.settings.CreateConnection("master"); | ||
await connection.OpenAsync(); | ||
try | ||
{ | ||
await connection.OpenAsync(); | ||
} | ||
catch (SqlException) | ||
{ | ||
this.traceHelper.GenericInfoEvent( | ||
"Failed to connect to the master database. Skipping database exists check.", | ||
instanceId: null); | ||
return; | ||
} | ||
Comment on lines
-180
to
+195
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Skipping the check somewhat reduces the effectiveness of the check. I wonder if there's a bigger refactor here that would allow us to ensure the DB exists without trying to connect to the master DB, which we know may not be allowed by some companies. If you get my point and think this may exist, but is costly, I'd appreciate if you could add a You said this probably used to work before - curious if you know what the code used to look like for this. I want to understand if this is essentially reverting back to the old code. |
||
|
||
if (!await this.DoesDatabaseExistAsync(this.settings.DatabaseName, connection)) | ||
{ | ||
|
Uh oh!
There was an error while loading. Please reload this page.