Conversation
📝 WalkthroughWalkthroughThe changes introduce a document database update feature by implementing a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Command/Service
participant DbRepo as IDocumentDbRepository
participant Config as DataConfig
participant PgConn as NpgsqlConnection
participant DB as PostgreSQL Database
Client->>DbRepo: UpdateDocumentDatabaseAsync()
DbRepo->>Config: Check DocDatabaseType
alt Not Postgres
DbRepo-->>Client: return true
else Is Postgres
DbRepo->>DbRepo: Load migration script from embedded resource
DbRepo->>DbRepo: Validate connection string & script
DbRepo->>PgConn: Create connection
DbRepo->>PgConn: Open()
DbRepo->>PgConn: BeginTransaction()
DbRepo->>PgConn: ExecuteNonQueryAsync(migration script)
PgConn->>DB: Execute DDL/DML commands
DB-->>PgConn: Success/Failure
alt Command succeeded
DbRepo->>PgConn: CommitAsync()
PgConn->>DB: Commit transaction
DbRepo-->>Client: return true
else Command failed
DbRepo->>PgConn: RollbackAsync()
PgConn->>DB: Rollback transaction
DbRepo->>DbRepo: Log exception
DbRepo-->>Client: return false
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
There was a problem hiding this comment.
🧹 Nitpick comments (6)
Repositories/Resgrid.Repositories.NoSqlRepository/DocumentDbRepository.cs (3)
35-44: Consider an explicitCommandTimeoutfor the migration command.
ExecuteNonQueryAsync()will use the default Npgsql command timeout (30s). If the embedded migration grows and takes longer (e.g., when applied to a populated database in the future), it may time out partway through, leading to a transaction rollback that the caller sees only as a genericfalse. Setting an explicit, generous timeout (or0for no timeout) makes the intent clear and avoids surprises in production.♻️ Proposed change
cmd.Transaction = tran; cmd.CommandText = migrationScript; + cmd.CommandTimeout = 0; // or a sufficiently large value for migrations await cmd.ExecuteNonQueryAsync();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Repositories/Resgrid.Repositories.NoSqlRepository/DocumentDbRepository.cs` around lines 35 - 44, The migration command currently relies on Npgsql's default timeout; set an explicit timeout on the command before executing to avoid mid-migration timeouts: in the DocumentDbRepository where you create the NpgsqlConnection and command (the cmd variable that runs migrationScript and calls ExecuteNonQueryAsync), assign cmd.CommandTimeout = 0 (or a generous seconds value) prior to ExecuteNonQueryAsync and leave transaction handling unchanged.
23-24: Prefertypeof(...).Assemblyover string-basedAssembly.Load.
Assembly.Load("Resgrid.Providers.MigrationsPg")is brittle: a rename or refactor of the assembly will fail only at runtime with no compile-time hint. The existingOidcRepository.UpdateOidcDatabaseAsync(the reference for this pattern) usestypeof(M0001_InitialMigrationPg).Assembly, which is type-safe and discoverable via the IDE. Aligning here keeps the two repositories consistent.♻️ Proposed change
- var assembly = Assembly.Load("Resgrid.Providers.MigrationsPg"); + var assembly = typeof(Resgrid.Providers.MigrationsPg.Migrations.M0001_InitialMigrationPg).Assembly;You may also drop the
using System.Reflection;if no longer needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Repositories/Resgrid.Repositories.NoSqlRepository/DocumentDbRepository.cs` around lines 23 - 24, The code in DocumentDbRepository.cs uses Assembly.Load("Resgrid.Providers.MigrationsPg") which is brittle; change it to use a type-based assembly reference like typeof(M0001_InitialMigrationPg).Assembly (same pattern as OidcRepository.UpdateOidcDatabaseAsync) to make the reference type-safe and discoverable; replace the Assembly.Load call with typeof(<an existing migration type in Resgrid.Providers.MigrationsPg>).Assembly and keep the const resourceName as-is, and remove the System.Reflection using if it becomes unused.
55-59: Swallowing the exception loses caller-visible context.
UpdateDocumentDatabaseAsynccatches everything, logs viaFramework.Logging.LogException, and returnsfalse. Callers (DbUpdateCommand,MigrateDocsDbCommand,DatabaseUpgradeService) only get a boolean and log a generic "did not complete successfully" message — operators have to cross-reference the exception sink to diagnose a failed schema upgrade. Consider either letting the exception propagate (each caller already has a top-leveltry/catch) or attaching the failure reason to the result so the caller can log a meaningful error. This is consistent with howIOidcRepositoryis currently structured, so it's optional, but worth reconsidering for this new contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Repositories/Resgrid.Repositories.NoSqlRepository/DocumentDbRepository.cs` around lines 55 - 59, UpdateDocumentDatabaseAsync currently catches all exceptions, logs them via Framework.Logging.LogException, and returns false which hides the error from callers (DbUpdateCommand, MigrateDocsDbCommand, DatabaseUpgradeService); either remove the broad catch so exceptions propagate to callers' existing try/catch blocks, or change the method signature/contract to return an enriched result (e.g., a Result/Outcome object or an out/ref error string) that carries the exception message/stack so callers can log a meaningful error; if you choose to propagate, ensure you don’t swallow the exception (rethrow or omit the catch) and if you choose the enriched result, capture the exception details in that result before returning false/failed.Workers/Resgrid.Workers.Console/Program.cs (3)
442-452: Inconsistent shutdown semantics between success and failure paths.On success the service uses
_hostApplicationLifetime.StopApplication()(graceful), but on failure it callsEnvironment.Exit(1)(abrupt — bypasses host shutdown, hosted serviceStopAsync, logger flushing, etc.). For a one-shot upgrade host, prefer signalling failure with a non-zero exit code while still letting the host shut down gracefully (e.g., setEnvironment.ExitCode = 1;and callStopApplication()), or at least flush Sentry/Serilog beforeEnvironment.Exit. As-is, error diagnostics may be lost.♻️ Proposed change
catch (Exception ex) { _logger.Log(LogLevel.Error, ex, "There was an error trying to update the Resgrid Database."); - Environment.Exit(1); + Environment.ExitCode = 1; + _hostApplicationLifetime.StopApplication(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Workers/Resgrid.Workers.Console/Program.cs` around lines 442 - 452, The failure path abruptly calls Environment.Exit(1) which bypasses graceful shutdown; replace this by setting Environment.ExitCode = 1 and invoking _hostApplicationLifetime.StopApplication() so the host and hosted services can run StopAsync and loggers/Sentry can flush; ensure the catch block still logs the exception via _logger.Log and, if you have explicit flush methods (e.g., for Serilog/Sentry), call them before returning so diagnostics are not lost.
74-74: Use a culture-invariant comparison for the env var.
upgradeDatabase.ToLower() == "true"is culture-sensitive (the classic Turkish-i hazard with"I"/"i"). Use an ordinal comparison orbool.TryParseso the flag is interpreted identically regardless of the host locale.♻️ Proposed change
- var runDatabaseUpgrade = !String.IsNullOrWhiteSpace(upgradeDatabase) && upgradeDatabase.ToLower() == "true"; + var runDatabaseUpgrade = bool.TryParse(upgradeDatabase, out var doUpgrade) && doUpgrade;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Workers/Resgrid.Workers.Console/Program.cs` at line 74, The current culture-sensitive check var runDatabaseUpgrade = !String.IsNullOrWhiteSpace(upgradeDatabase) && upgradeDatabase.ToLower() == "true"; should be replaced with a culture-invariant parse or comparison; use bool.TryParse(upgradeDatabase, out var runDatabaseUpgrade) (or string.Equals(upgradeDatabase, "true", StringComparison.OrdinalIgnoreCase)) so the code uses an ordinal/Invariant comparison and avoids locale issues—update the usage in Program.cs where runDatabaseUpgrade and upgradeDatabase are referenced.
495-517:serviceProviderparameter is unused — resolve from it instead ofBootstrapper.
UpdateDocumentDatabaseAsyncacceptsIServiceProvider serviceProviderbut ignores it and pulls the repository fromBootstrapper.GetKernel().Resolve<IDocumentDbRepository>(). This mirrorsUpdateOidcDatabaseAsyncso it's not a regression, but it does leak the static service-locator dependency and makes the method harder to test. Consider either dropping the parameter or, better, resolving from the passed-in scope so the lifetime matches the FluentMigrator scope already created by the caller.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Workers/Resgrid.Workers.Console/Program.cs` around lines 495 - 517, The method UpdateDocumentDatabaseAsync currently ignores the IServiceProvider parameter and uses Bootstrapper.GetKernel().Resolve<IDocumentDbRepository(), which introduces a static service locator; change it to resolve IDocumentDbRepository from the passed-in serviceProvider (preferably by using the scope provided or calling serviceProvider.GetRequiredService<IDocumentDbRepository>() / serviceProvider.CreateScope().ServiceProvider.GetRequiredService<IDocumentDbRepository>() so the repository lifetime matches the caller's FluentMigrator scope), then call UpdateDocumentDatabaseAsync on that instance and keep the existing logging/exception behavior; remove the Bootstrapper.GetKernel().Resolve usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Repositories/Resgrid.Repositories.NoSqlRepository/DocumentDbRepository.cs`:
- Around line 35-44: The migration command currently relies on Npgsql's default
timeout; set an explicit timeout on the command before executing to avoid
mid-migration timeouts: in the DocumentDbRepository where you create the
NpgsqlConnection and command (the cmd variable that runs migrationScript and
calls ExecuteNonQueryAsync), assign cmd.CommandTimeout = 0 (or a generous
seconds value) prior to ExecuteNonQueryAsync and leave transaction handling
unchanged.
- Around line 23-24: The code in DocumentDbRepository.cs uses
Assembly.Load("Resgrid.Providers.MigrationsPg") which is brittle; change it to
use a type-based assembly reference like
typeof(M0001_InitialMigrationPg).Assembly (same pattern as
OidcRepository.UpdateOidcDatabaseAsync) to make the reference type-safe and
discoverable; replace the Assembly.Load call with typeof(<an existing migration
type in Resgrid.Providers.MigrationsPg>).Assembly and keep the const
resourceName as-is, and remove the System.Reflection using if it becomes unused.
- Around line 55-59: UpdateDocumentDatabaseAsync currently catches all
exceptions, logs them via Framework.Logging.LogException, and returns false
which hides the error from callers (DbUpdateCommand, MigrateDocsDbCommand,
DatabaseUpgradeService); either remove the broad catch so exceptions propagate
to callers' existing try/catch blocks, or change the method signature/contract
to return an enriched result (e.g., a Result/Outcome object or an out/ref error
string) that carries the exception message/stack so callers can log a meaningful
error; if you choose to propagate, ensure you don’t swallow the exception
(rethrow or omit the catch) and if you choose the enriched result, capture the
exception details in that result before returning false/failed.
In `@Workers/Resgrid.Workers.Console/Program.cs`:
- Around line 442-452: The failure path abruptly calls Environment.Exit(1) which
bypasses graceful shutdown; replace this by setting Environment.ExitCode = 1 and
invoking _hostApplicationLifetime.StopApplication() so the host and hosted
services can run StopAsync and loggers/Sentry can flush; ensure the catch block
still logs the exception via _logger.Log and, if you have explicit flush methods
(e.g., for Serilog/Sentry), call them before returning so diagnostics are not
lost.
- Line 74: The current culture-sensitive check var runDatabaseUpgrade =
!String.IsNullOrWhiteSpace(upgradeDatabase) && upgradeDatabase.ToLower() ==
"true"; should be replaced with a culture-invariant parse or comparison; use
bool.TryParse(upgradeDatabase, out var runDatabaseUpgrade) (or
string.Equals(upgradeDatabase, "true", StringComparison.OrdinalIgnoreCase)) so
the code uses an ordinal/Invariant comparison and avoids locale issues—update
the usage in Program.cs where runDatabaseUpgrade and upgradeDatabase are
referenced.
- Around line 495-517: The method UpdateDocumentDatabaseAsync currently ignores
the IServiceProvider parameter and uses
Bootstrapper.GetKernel().Resolve<IDocumentDbRepository(), which introduces a
static service locator; change it to resolve IDocumentDbRepository from the
passed-in serviceProvider (preferably by using the scope provided or calling
serviceProvider.GetRequiredService<IDocumentDbRepository>() /
serviceProvider.CreateScope().ServiceProvider.GetRequiredService<IDocumentDbRepository>()
so the repository lifetime matches the caller's FluentMigrator scope), then call
UpdateDocumentDatabaseAsync on that instance and keep the existing
logging/exception behavior; remove the Bootstrapper.GetKernel().Resolve usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1b11612b-b4b4-41b2-893b-fadd23893513
📒 Files selected for processing (7)
Core/Resgrid.Model/Repositories/IDocumentDbRepository.csProviders/Resgrid.Providers.MigrationsPg/Sql/EF0003_PopulateDocDb.sqlRepositories/Resgrid.Repositories.NoSqlRepository/DocumentDbRepository.csRepositories/Resgrid.Repositories.NoSqlRepository/NoSqlDataModule.csTools/Resgrid.Console/Commands/DbUpdateCommand.csTools/Resgrid.Console/Commands/MigrateDocsDbCommand.csWorkers/Resgrid.Workers.Console/Program.cs
|
Approve |
Summary by CodeRabbit
New Features
Bug Fixes
Chores