Move data schema to a separate db.#33
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR splits the system into separate application and data SQL databases, wires the new data connection through infrastructure and deployment workflows, updates import services to resolve that connection, and refreshes local configuration, scripts, and documentation. ChangesApplication vs Data Database Separation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/Import/FlatFileImportService.cs (1)
763-811: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftImport data swap and import log need one transactional boundary.
ReplaceTargetTableAsyncwrites to the Data DB, whileLogImportAttemptAsynclater callsdbContext.SaveChangesAsyncon App DB via a separate connection/transaction. A failure between them leaves the table swap and audit log out of sync. Move the log write into the same transaction/connection or add an outbox/reconciliation path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/Import/FlatFileImportService.cs` around lines 763 - 811, The import swap and audit log are using separate transactional boundaries, so a failure after ReplaceTargetTableAsync but before LogImportAttemptAsync leaves Data DB and App DB inconsistent. Update the flow around ReplaceTargetTableAsync and LogImportAttemptAsync so the import log write participates in the same transactional unit as the table replacement, or add a durable outbox/reconciliation step that guarantees both succeed or are recovered together. Use the existing ReplaceTargetTableAsync and LogImportAttemptAsync entry points to keep the fix localized.
🧹 Nitpick comments (3)
infrastructure/azure/modules/sql.bicep (1)
53-77: 🔒 Security & Privacy | 🔵 TrivialConsider server-level threat detection/alerting now that two databases share the server.
Checkov flags missing zone redundancy, alert recipients, and threat detection on the SQL databases. These gaps predate this PR (same as the original single-database resource), but doubling the databases on the same server doubles the exposed surface without these protections.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infrastructure/azure/modules/sql.bicep` around lines 53 - 77, Both Microsoft.Sql/servers/databases resources in the sql.bicep module still omit the server-level security settings flagged by Checkov. Update the SQL server/database provisioning around appDatabase and dataDatabase to include the missing zone redundancy, alert recipient configuration, and threat detection/alerting at the server level so both databases inherit the protections instead of leaving the shared sqlServer exposed.Source: Linters/SAST tools
.github/workflows/deploy-azure-appservice.yml (1)
228-231: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRemove the redundant
DATA_DB_CONNECTIONoverride. Bicep already sets the same app setting, so this workflow copy only adds a second place to keep the connection string in sync; let one layer own it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/deploy-azure-appservice.yml around lines 228 - 231, The deploy-azure-appservice workflow is redundantly setting DATA_DB_CONNECTION even though Bicep already owns that app setting. Remove the extra append_setting call in the deployment step and keep the connection string defined in only one place so the workflow no longer has to mirror the value. Use the existing data_db_connection assignment and the append_setting block near Notification__BaseUrl to locate the change.server.core/Data/DataDatabaseConnection.cs (1)
10-24: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
??chain skips fallback sources when a source is blank, not just null.
configuration[EnvironmentVariableName]can return an empty string (e.g., an env var explicitly set to""), which is non-null and short-circuits the??chain — soConnectionStrings:DataConnectionandfallbackConnectionStringare never consulted, and the method throws even if a valid connection string is available downstream.🛠️ Proposed fix using blank-aware fallback
- var connectionString = configuration[EnvironmentVariableName] - ?? configuration.GetConnectionString(ConnectionStringName) - ?? fallbackConnectionString; + var connectionString = configuration[EnvironmentVariableName]; + if (string.IsNullOrWhiteSpace(connectionString)) + { + connectionString = configuration.GetConnectionString(ConnectionStringName); + } + if (string.IsNullOrWhiteSpace(connectionString)) + { + connectionString = fallbackConnectionString; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server.core/Data/DataDatabaseConnection.cs` around lines 10 - 24, The Resolve method in DataDatabaseConnection should treat blank values as missing instead of letting the first non-null result short-circuit the lookup. Update the fallback logic so that configuration[EnvironmentVariableName], Configuration.GetConnectionString(ConnectionStringName), and fallbackConnectionString are only accepted when they contain non-whitespace text, and continue to the next source otherwise. Keep the existing InvalidOperationException behavior in Resolve, but ensure it is thrown only after all sources have been checked for a usable connection string.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@database/data/publish-local.sh`:
- Around line 9-10: The default CREATE_NEW_DATABASE value in publish-local.sh is
destructive because it causes DataDb to be dropped and recreated on each run.
Update the script’s create_new_database default to False, or make the
destructive behavior explicit in the script comments/help text so users must opt
in before invoking the publish flow.
---
Outside diff comments:
In `@server/Import/FlatFileImportService.cs`:
- Around line 763-811: The import swap and audit log are using separate
transactional boundaries, so a failure after ReplaceTargetTableAsync but before
LogImportAttemptAsync leaves Data DB and App DB inconsistent. Update the flow
around ReplaceTargetTableAsync and LogImportAttemptAsync so the import log write
participates in the same transactional unit as the table replacement, or add a
durable outbox/reconciliation step that guarantees both succeed or are recovered
together. Use the existing ReplaceTargetTableAsync and LogImportAttemptAsync
entry points to keep the fix localized.
---
Nitpick comments:
In @.github/workflows/deploy-azure-appservice.yml:
- Around line 228-231: The deploy-azure-appservice workflow is redundantly
setting DATA_DB_CONNECTION even though Bicep already owns that app setting.
Remove the extra append_setting call in the deployment step and keep the
connection string defined in only one place so the workflow no longer has to
mirror the value. Use the existing data_db_connection assignment and the
append_setting block near Notification__BaseUrl to locate the change.
In `@infrastructure/azure/modules/sql.bicep`:
- Around line 53-77: Both Microsoft.Sql/servers/databases resources in the
sql.bicep module still omit the server-level security settings flagged by
Checkov. Update the SQL server/database provisioning around appDatabase and
dataDatabase to include the missing zone redundancy, alert recipient
configuration, and threat detection/alerting at the server level so both
databases inherit the protections instead of leaving the shared sqlServer
exposed.
In `@server.core/Data/DataDatabaseConnection.cs`:
- Around line 10-24: The Resolve method in DataDatabaseConnection should treat
blank values as missing instead of letting the first non-null result
short-circuit the lookup. Update the fallback logic so that
configuration[EnvironmentVariableName],
Configuration.GetConnectionString(ConnectionStringName), and
fallbackConnectionString are only accepted when they contain non-whitespace
text, and continue to the next source otherwise. Keep the existing
InvalidOperationException behavior in Resolve, but ensure it is thrown only
after all sources have been checked for a usable connection string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3cbac33c-e574-4c20-a09b-1a6a13c3b4eb
📒 Files selected for processing (18)
.devcontainer/devcontainer.json.github/workflows/ci-cd.yml.github/workflows/deploy-azure-appservice.ymlREADME.mddatabase/data/Scripts/Script.PreDeployment.sqldatabase/data/publish-local.shdocs/ARCHITECTURE.mdinfrastructure/azure/README.mdinfrastructure/azure/main.bicepinfrastructure/azure/main.jsoninfrastructure/azure/modules/compute.bicepinfrastructure/azure/modules/sql.bicepserver.core/Data/DataDatabaseConnection.csserver.core/Import/PgmProjectsImportService.csserver/.env.exampleserver/Import/FlatFileImportService.csserver/appsettings.Development.jsontests/server.tests/Import/FlatFileImportServiceTests.cs
💤 Files with no reviewable changes (1)
- database/data/Scripts/Script.PreDeployment.sql
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server.core/Data/DataDatabaseTransaction.cs`:
- Around line 23-24: The import transaction is writing to a cross-database table
via QualifiedAppTableName()/InsertImportLog, which breaks on Azure SQL Database
when app and data live in separate databases. Update DataDatabaseTransaction so
it no longer inserts ImportLog from the data DB connection; instead either route
that write through an app-DB connection or move the log/outbox table into the
data database, and make sure FlatFileImportService uses the revised path.
In `@server/Import/FlatFileImportService.cs`:
- Around line 784-788: Pass the active transaction into the temp-table cleanup
in FlatFileImportService so the DROP TABLE IF EXISTS command is executed under
the same SqlTransaction as ReplaceTargetTableAsync. Update the
DropTempTableIfExistsAsync call to use the existing transaction object from
dataTransaction, or move the cleanup to after the transaction scope ends so it
no longer runs on an open connection without an associated transaction. Use the
local transaction/connection variables in this method to locate the affected
path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1cc50f9-1f4b-4611-9b49-b7c4cfdd1f0a
📒 Files selected for processing (5)
database/data/publish-local.shserver.core/Data/DataDatabaseTransaction.csserver/Import/FlatFileImportService.csserver/Program.cstests/server.tests/Import/FlatFileImportServiceTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/server.tests/Import/FlatFileImportServiceTests.cs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/Import/FlatFileImportService.cs`:
- Line 817: The persistence success path in ImportAsync should also handle
transaction-disposal failures after dataTransaction.Complete() because
TransactionScope disposal can throw TransactionException outside the current
filter. Update the persistence failure handling around the FlatFileImportService
import flow to include TransactionException alongside the existing
database/persistence exceptions so aborted transactions still map to the
graceful database-rejected-import response.
- Around line 812-815: The `beforeCommitAsync` callback in
`ReplaceTargetTableAsync` is writing the `ImportLog` through
`LogImportAttemptAsync` while `dataTransaction` is still open, which pulls
`AppDbContext` into the same ambient `TransactionScope` as `DataDb`. Move the
log write to after `dataTransaction.Complete()` (or otherwise ensure it uses the
same database/transaction) so the `ImportLog` persistence is not part of the
ambient data transaction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f664f66e-1122-4a6e-984b-754593ff06bb
📒 Files selected for processing (2)
server.core/Data/DataDatabaseTransaction.csserver/Import/FlatFileImportService.cs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server.core/Data/DataDbConnection.cs (1)
12-14: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEmpty env var short-circuits the fallback chain.
??only treatsnullas missing. IfDATA_DB_CONNECTIONis present but empty/whitespace (a common misconfiguration),configuration[EnvironmentVariableName]returns"", the chain stops, andGetConnectionString(ConnectionStringName)/fallbackConnectionStringare never consulted — the method then throws even thoughConnectionStrings:DataConnectionis configured.🛠️ Proposed fix
- var connectionString = configuration[EnvironmentVariableName] - ?? configuration.GetConnectionString(ConnectionStringName) - ?? fallbackConnectionString; - - if (string.IsNullOrWhiteSpace(connectionString)) + var connectionString = Coalesce( + configuration[EnvironmentVariableName], + configuration.GetConnectionString(ConnectionStringName), + fallbackConnectionString); + + if (string.IsNullOrWhiteSpace(connectionString)) { throw new InvalidOperationException( $"No data database connection string configured. Set the {EnvironmentVariableName} environment variable " + $"or configure ConnectionStrings:{ConnectionStringName}."); }Add a small helper (or inline the logic):
private static string? Coalesce(params string?[] values) => values.FirstOrDefault(v => !string.IsNullOrWhiteSpace(v));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server.core/Data/DataDbConnection.cs` around lines 12 - 14, The connection lookup in DataDbConnection is stopping too early when EnvironmentVariableName is set to an empty or whitespace value. Update the connection resolution logic so it skips blank entries and continues to ConnectionStringName and then fallbackConnectionString, using a small helper like Coalesce or equivalent inside the DataDbConnection initialization path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@server.core/Data/DataDbConnection.cs`:
- Around line 12-14: The connection lookup in DataDbConnection is stopping too
early when EnvironmentVariableName is set to an empty or whitespace value.
Update the connection resolution logic so it skips blank entries and continues
to ConnectionStringName and then fallbackConnectionString, using a small helper
like Coalesce or equivalent inside the DataDbConnection initialization path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19d70e3d-a654-4b86-bb5c-9ae52e9c342e
📒 Files selected for processing (4)
server.core/Data/DataDbConnection.csserver.core/Import/PgmProjectsImportService.csserver/Import/FlatFileImportService.cstests/server.tests/Import/FlatFileImportServiceTests.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/server.tests/Import/FlatFileImportServiceTests.cs
- server.core/Import/PgmProjectsImportService.cs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/server.tests/Data/DataDbConnectionTests.cs (1)
9-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for env-var precedence and the throw path.
The two tests cover "named connection wins over blank env var" and "fallback wins over blank named connection," but don't cover: (1) a non-blank env var taking precedence over both named connection and fallback, and (2)
ResolvethrowingInvalidOperationExceptionwhen everything is blank/unset. The throw path is the failure-safety guarantee of this helper and is worth locking in with a test.✅ Suggested additional tests
[Fact] public void Resolve_prefers_non_blank_environment_variable() { var configuration = BuildConfiguration(new Dictionary<string, string?> { [DataDbConnection.EnvironmentVariableName] = "env-connection", [$"ConnectionStrings:{DataDbConnection.ConnectionStringName}"] = "named-connection", }); var connectionString = DataDbConnection.Resolve(configuration, "fallback-connection"); connectionString.Should().Be("env-connection"); } [Fact] public void Resolve_throws_when_no_connection_string_is_configured() { var configuration = BuildConfiguration(new Dictionary<string, string?>()); Action act = () => DataDbConnection.Resolve(configuration, null); act.Should().Throw<InvalidOperationException>(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/server.tests/Data/DataDbConnectionTests.cs` around lines 9 - 35, Add tests in DataDbConnectionTests for the remaining Resolve behaviors: verify that DataDbConnection.Resolve prefers a non-blank value from DataDbConnection.EnvironmentVariableName over both the named connection and the fallback, and add a failure-case test that calls Resolve with no usable configuration so it throws InvalidOperationException. Use the existing DataDbConnection.Resolve helper and the same BuildConfiguration setup to cover both the precedence path and the throw path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/server.tests/Data/DataDbConnectionTests.cs`:
- Around line 9-35: Add tests in DataDbConnectionTests for the remaining Resolve
behaviors: verify that DataDbConnection.Resolve prefers a non-blank value from
DataDbConnection.EnvironmentVariableName over both the named connection and the
fallback, and add a failure-case test that calls Resolve with no usable
configuration so it throws InvalidOperationException. Use the existing
DataDbConnection.Resolve helper and the same BuildConfiguration setup to cover
both the precedence path and the throw path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97af49bd-5d04-4f41-9956-c239c96eb7d2
📒 Files selected for processing (2)
server.core/Data/DataDbConnection.cstests/server.tests/Data/DataDbConnectionTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- server.core/Data/DataDbConnection.cs
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
1 participant
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.
Summary by CodeRabbit
DATA_DB_CONNECTION/ConnectionStrings:DataConnection) for local, dev, and deployed environments.DropObjectsNotInSourceenabled).