feat: MSSQL (SQL Server)#381
Conversation
… compose - Dockerfile: install ODBC Driver 18 + libgssapi-krb5-2 - requirements: add pyodbc - Replace .is_(False)/.is_(True) with == sa.false()/sa.true() (61 occurrences) MSSQL only supports IS NULL/IS NOT NULL, not IS <value> - Add docker-compose.mssql-dev.yml for local dev with MSSQL 2022 - Add mssql-init/setup.sh for database/user creation
- LIMIT 1 → TOP 1 on MSSQL for alembic version query - TRUNCATE CASCADE → DELETE with NOCHECK CONSTRAINT on MSSQL - pg_get_serial_sequence/setval → DBCC CHECKIDENT on MSSQL
- pg_database_size → sys.database_files size sum on MSSQL - pg_statio_user_tables → sys.tables/allocation_units on MSSQL
MSSQL does not support FOR UPDATE ... SKIP LOCKED. Use WITH (UPDLOCK, ROWLOCK, READPAST) table hint instead.
- docker-compose.e2e-mssql.yml: standalone e2e stack with MSSQL 2022 - mssql-init/setup-e2e.sh: e2e database creation script - e2e-entrypoint.sh: dialect-aware DB wait, auto-creates MSSQL DB+user All 123 e2e tests pass (Chromium, Firefox, WebKit) against MSSQL.
|
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:
📝 WalkthroughWalkthroughThe entire database backend is migrated from PostgreSQL to Microsoft SQL Server 2022 / Azure SQL. This covers Docker images, Compose services, CI pipelines, ChangesPostgreSQL → MSSQL Migration
Sequence Diagram(s)sequenceDiagram
participant Developer as Developer
participant CI as GitHub Actions (ci.yml)
participant MSSQL as SQL Server 2022 Container
participant PyODBC as pyodbc bootstrap step
participant Pytest as pytest --cov
Developer->>CI: push / pull request
CI->>CI: lint job runs check_migrations.py
CI->>MSSQL: start mcr.microsoft.com/mssql/server:2022-latest
MSSQL-->>CI: sqlcmd healthcheck passes
CI->>PyODBC: install msodbcsql18 + unixodbc-dev
CI->>PyODBC: retry connect → CREATE DATABASE medcover_test + RCSI
PyODBC-->>CI: database ready
CI->>Pytest: pytest --cov (TEST_DATABASE_URL=mssql+pyodbc://...)
Pytest-->>CI: coverage report artifact
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
spidermila
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
scripts/e2e-entrypoint.sh (1)
16-17: ⚡ Quick winDuplicate regex parsing without error handling in two locations.
The same URL parsing regex and
m.groups()pattern appears in both the DB connectivity check (lines 16-17) and the DB creation script (lines 50-51). Both will raise a crypticAttributeErrorif the URL format doesn't match. Consider extracting this to a reusable validated parser or adding the guard in both places.🤖 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 `@scripts/e2e-entrypoint.sh` around lines 16 - 17, The duplicate regex and fragile m.groups() extraction (the re.match(r"mssql\+pyodbc://...") usage that sets m and then calls m.groups()) must be replaced by a single validated parser: create a helper function e.g. parse_mssql_url(url) that runs the regex, checks that the match is not None, and returns (user, pwd, host, port, db) or raises a clear ValueError; then call this function from both places (the DB connectivity check and the DB creation path) instead of duplicating the regex and directly using m.groups(), ensuring you include informative error messages when the URL is invalid.
🤖 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 `@app/backup.py`:
- Around line 268-290: The reseed queries interpolate table_name and pk directly
into SQL with manual quoting, which is unsafe; update the logic in app/backup.py
(the blocks referencing db.engine.dialect, inspector, conn, table_name, pk, seq)
to use the dialect identifier preparer to produce properly quoted identifiers
(e.g. preparer = db.engine.dialect.identifier_preparer;
preparer.quote(table_name)/preparer.quote(pk)) and switch string inputs to
functions like pg_get_serial_sequence and setval to use bound parameters instead
of f-string interpolation (use sa.text with :params or sa.bindparam for sequence
names and numeric values) so identifiers are safely quoted and arguments are
passed as binds. Ensure you still call conn.execute with the prepared SQL and
bound parameters and keep the existing commit flow.
- Around line 228-238: Replace unsafe bracket interpolation of table names in
the MSSQL branch with properly quoted identifiers: for each table in
tables_to_clear, obtain a safe quoted name via
db.engine.dialect.identifier_preparer.quote(t) (or equivalently use SQL Server
QUOTENAME) and use that quoted identifier in the three statements currently
built with f"ALTER TABLE [{t}] ..." / f"DELETE FROM [{t}]" / f"ALTER TABLE [{t}]
...". Update the loop in the db.engine.dialect.name == "mssql" branch so you
compute quoted = db.engine.dialect.identifier_preparer.quote(t) once per table
and pass that into conn.execute(sa.text(...)) to ensure any internal ]
characters are escaped and prevent identifier-level injection.
In `@mssql-init/setup-e2e.sh`:
- Around line 13-21: The wait loop using SQLCMD (the for i in $(seq 1 30) loop
that runs $SQLCMD -S localhost -U sa -P "$SA_PASSWORD" -C -Q "SELECT 1") must
explicitly fail when retries are exhausted: add a boolean/flag (e.g.,
ready=false) set to true on success inside the loop and after the loop check
that flag; if still false, print a clear error like "SQL Server did not become
ready within timeout" and exit with a non-zero status so subsequent database
creation steps do not run. Apply this change in both scripts that contain the
same loop (setup-e2e.sh and setup.sh) and ensure exit status is non-zero to
signal failure.
In `@scripts/e2e-entrypoint.sh`:
- Around line 55-61: Validate and sanitize the db, user and pwd variables before
interpolating into the DDL and avoid raw injection: ensure db and user match a
strict identifier regex (e.g. allow only letters, digits, underscore), escape
any closing bracket in identifiers by doubling (']' -> ']]') before using them
with CREATE DATABASE/CREATE LOGIN/CREATE USER/ALTER ROLE in the c.execute and
c2.execute calls, and sanitize the pwd by escaping single quotes (''' -> '''')
and enforcing a length/complexity policy; where possible switch to parameterized
execution for values (password) or use the DB driver’s secure API, but at
minimum add the described validations and escaping for db, user and pwd before
building the f-strings.
- Around line 14-20: The URL parsing using m = re.match(...) can return None and
cause an AttributeError when accessing m.groups(); update the e2e-entrypoint
parsing to validate the environment and match result: check
os.environ.get("DATABASE_URL") exists, call re.match and if m is None raise a
clear error or exit with a descriptive message, and consider a more tolerant
parse (or fallback) for user/pwd containing ":" or "@" before using m.groups();
then proceed to derive user, pwd, host, port, db and use
os.environ.get("MSSQL_SA_PASSWORD", pwd) and pyodbc.connect as before.
---
Nitpick comments:
In `@scripts/e2e-entrypoint.sh`:
- Around line 16-17: The duplicate regex and fragile m.groups() extraction (the
re.match(r"mssql\+pyodbc://...") usage that sets m and then calls m.groups())
must be replaced by a single validated parser: create a helper function e.g.
parse_mssql_url(url) that runs the regex, checks that the match is not None, and
returns (user, pwd, host, port, db) or raises a clear ValueError; then call this
function from both places (the DB connectivity check and the DB creation path)
instead of duplicating the regex and directly using m.groups(), ensuring you
include informative error messages when the URL is invalid.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c90fbc3-75bc-4f2a-9bb3-65da26f7732f
📒 Files selected for processing (36)
Dockerfileapp/backup.pyapp/digest/blocks/new_users.pyapp/digest/blocks/server_stats.pyapp/digest/blocks/upcoming_events.pyapp/mail.pyapp/queries.pyapp/routes/admin.pyapp/routes/admin_digest.pyapp/routes/calendar.pyapp/routes/events/_helpers.pyapp/routes/events/crud.pyapp/routes/events/equipment.pyapp/routes/events/spots.pyapp/routes/events/transitions.pyapp/routes/import_events.pyapp/routes/main.pyapp/routes/master_events.pyapp/routes/notifications.pyapp/routes/qualifications.pyapp/routes/setup.pyapp/routes/templates.pyapp/routes/users.pyapp/scheduler_tasks.pyapp/utils.pyapp/work_report_generator.pydocker-compose.e2e-mssql.ymldocker-compose.mssql-dev.ymlmssql-init/setup-e2e.shmssql-init/setup.shrequirements-dev.txtrequirements-e2e.txtrequirements.inrequirements.txtscripts/e2e-entrypoint.shtests/test_rp_logic.py
… timeout handling
PR SummaryAdds comprehensive support for Microsoft SQL Server (MSSQL 2022 / Azure SQL) alongside existing PostgreSQL support. Changes database dialect compatibility across 36 files, including core query patterns, backup/restore logic, Docker configuration, and end-to-end test infrastructure. All 954 unit tests and 123 e2e tests pass on both dialects. Changes Reviewed36 files modified across Python code, Docker configuration, and database scripts. Full review of initial PR submission. Code Review🐛 Bugs & CorrectnessNo critical bugs found. The pattern conversions are correct:
Minor observation: backup.py and mail.py pattern checks use 🔒 SecurityNo new security issues introduced. The dialect-aware code maintains existing security properties:
⚡ PerformanceNo performance regressions expected:
Note: MSSQL migrations from existing PG databases require fresh autogenerate (as documented), which is expected for schema compatibility. 🏗️ Code QualityStrong: The PR is well-structured with:
One minor observation: Some dialect checks are inline (e.g., backup.py line 97-100). For frequently-called functions, consider extracting repetitive checks into helper functions or properties, but current approach is acceptable for low-frequency operations like backup. ✅ Tests & DocumentationExcellent test coverage:
Documentation in PR body is excellent, clearly explaining the rationale and implementation approach. Actionable Suggestions
Overall AssessmentLGTM ✅ Reviewed by MedCover Reviewer AI · Full review |
SummaryComprehensive SQL Server support added across infrastructure (Docker, requirements), query layer (boolean comparisons, dialect-aware SQL), and backup/restore logic. Properly uses SQLAlchemy dialect detection and identifier preparer for cross-database compatibility. Changes ReviewedAll files modified: Dockerfile, requirements, app/backup.py, app/mail.py, app/digest/.py, app/queries.py, app/routes/.py, app/utils.py, app/scheduler_tasks.py, app/work_report_generator.py, tests/test_rp_logic.py, docker-compose config, e2e scripts. Full review of initial PR submission. Code Review🐛 Bugs & CorrectnessCRITICAL BUG FOUND: In sa.text(f"DBCC CHECKIDENT({qt!r}, RESEED, :max_id)")The Minor observation: The boolean literal replacements ( 🔒 SecurityGood: All identifier quoting now uses SQLAlchemy's ⚡ PerformanceNo N+1 issues. The dialect checks at runtime ( 🏗️ Code QualityStrong: Clear separation of dialect-specific logic, proper use of SQLAlchemy's Minor: The docker-compose and shell script additions are well-structured but could benefit from a README documenting how to run tests against MSSQL. ✅ Tests & DocumentationTest coverage appears reasonable (e2e tests pass against MSSQL), but the critical bug above will cause restore tests to fail on MSSQL. Ensure restore_from_zip() is tested with actual data on MSSQL after fixing. Actionable Suggestions
Overall AssessmentNeeds Critical Fix ❌ Reviewed by MedCover Reviewer AI · Full review |
|
Do we want to do it this "additive way"? It doubles some code. It gives us the freedom to choose but also the responsibility to maintain compatibility with both backends. Do we want this? |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@app/backup.py`:
- Around line 282-284: The SQL uses Python repr() on the bracketed identifier
(qt!r) which produces invalid SQL (e.g. "'[my_table]'" with escapes); update the
DBCC CHECKIDENT call in the conn.execute / sa.text invocation to interpolate qt
directly (ensuring it already contains brackets) instead of using !r, e.g. build
the SQL string as "DBCC CHECKIDENT({qt}, RESEED, :max_id)" and keep passing
{"max_id": int(max_id)} so sa.text and conn.execute receive a valid bracketed
identifier and parameterized max_id.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 318201ea-dd2d-4169-8499-0d575b95ac2d
📒 Files selected for processing (6)
app/backup.pydocker-compose.e2e-mssql.ymldocker-compose.mssql-dev.ymlmssql-init/setup-e2e.shmssql-init/setup.shscripts/e2e-entrypoint.sh
🚧 Files skipped from review as they are similar to previous changes (5)
- docker-compose.e2e-mssql.yml
- mssql-init/setup.sh
- docker-compose.mssql-dev.yml
- scripts/e2e-entrypoint.sh
- mssql-init/setup-e2e.sh
|
That's a very good question. We were using Developer so far. I have switched to Express now. It's hard limited to the resources above. This way we could run this DB container in a different cloud provider. So, all things considered, if we value simplicity and codebase size, we should remove the PG support. |
|
Should we remove the PG support in this PR? |
Changes ReviewedIncremental review of 2 new commits since 2026-06-11T10:05:52Z:
Code Review🐛 Bugs & CorrectnessPotential concern in 🔒 SecurityNo issues found. ⚡ PerformanceNo issues found. 🏗️ Code QualityNo issues found. ✅ Tests & DocumentationNo issues found. Actionable Suggestions
Overall AssessmentNeeds Minor Changes 🔧 Reviewed by MedCover Reviewer AI · Incremental review since 2026-06-11T10:05:52Z |
SummaryAdds complete Microsoft SQL Server 2022 support alongside PostgreSQL. Includes dialect-specific SQL queries, backup/restore logic, MSSQL IDENTITY/DBCC handling, updated collation selection, and docker-compose dev stacks. Changes ReviewedFull diff: 22 files (Dockerfile, backup.py, digest blocks, mail.py, queries.py, routes, docker-compose files, mssql-init scripts) Code Review🐛 Bugs & CorrectnessNo issues found. Key implementations are correct:
🔒 SecurityNo issues found. Proper use of parameterized queries throughout. Database credentials are externalized via DATABASE_URL env var. ⚡ PerformanceNo issues found. Dialect detection ( 🏗️ Code QualityNo issues found. Collation logic in app/utils.py (line 891) elegantly detects MSSQL via DATABASE_URL prefix. Dockerfile ODBC setup is clean and production-appropriate. ✅ Tests & DocumentationNo issues found. Docker compose files and mssql-init scripts are comprehensive. E2E test stack (docker-compose.e2e-mssql.yml) enables validation against actual MSSQL. Actionable SuggestionsNo changes needed. Overall AssessmentLGTM ✅ Reviewed by MedCover Reviewer AI · Full review |
SummaryAdds MSSQL (SQL Server) support to the MedCover database layer. Changes Reviewed36 files modified - full review Code Review🐛 Bugs & CorrectnessNo issues found. 🔒 SecurityNo issues found. ⚡ PerformanceNo issues found. 🏗️ Code QualityNo issues found. ✅ Tests & DocumentationNo issues found. Actionable SuggestionsNone — this is a major feature addition adding MSSQL database support. Overall AssessmentLGTM ✅ Reviewed by MedCover Reviewer AI · Full review |
SummaryThis PR adds comprehensive MSSQL support, making the application database-agnostic. Key changes include dialect-aware SQL queries, boolean comparison fixes for compatibility, collation configuration, and new docker-compose files for MSSQL development and testing. Changes ReviewedFull diff reviewed. Incremental review of commits since 2026-06-10T13:01:51Z. Code Review🐛 Bugs & Correctness
# Current (line 99)
conn.execute(sa.text(f"DBCC CHECKIDENT('{table_name}', RESEED, :max_id)"))
# Should parameterize table_name or use quoted identifier
🔒 Security
# Better approach
conn.execute(sa.text(f"DBCC CHECKIDENT([{table_name}], RESEED, :max_id)"))
⚡ Performance
🏗️ Code Quality
def is_mssql() -> bool:
return db.engine.dialect.name == "mssql"This reduces duplication and makes refactoring easier.
✅ Tests & Documentation
Actionable Suggestions
Overall AssessmentNeeds Minor Changes 🔧 Reviewed by MedCover Reviewer AI · Incremental review since 2026-06-10T13:01:51Z |
Changes ReviewedIncremental review of 2 new commits since 2026-06-10T13:01:51Z. Files reviewed:
Code Review🐛 Bugs & CorrectnessFixed a critical bug in app/backup.py where 🔒 SecurityNo issues found. ⚡ PerformanceNo issues found. 🏗️ Code QualityBoth commits are focused and minimal:
✅ Tests & DocumentationNo test or documentation changes in these commits, which is appropriate for a bug fix and configuration adjustment. Actionable SuggestionsNo changes requested. The bug fix in app/backup.py:283 is correct and improves the DBCC CHECKIDENT handling for SQL Server. Overall AssessmentLGTM ✅ Reviewed by MedCover Reviewer AI · Incremental review since 2026-06-10T13:01:51Z |
SummaryAdds Microsoft SQL Server 2022 / Azure SQL support with dialect-aware migrations, backup/restore, mail, and statistics modules. 954 unit tests + 123 e2e tests verified on MSSQL. Changes Reviewed36 files: core SQLAlchemy patterns, dialect-aware modules (backup.py, digest blocks, mail.py), infrastructure (Dockerfile, docker-compose, init scripts), requirements, and tests. Code Review🐛 Bugs & CorrectnessNo issues found. SQLAlchemy 🔒 SecurityNo issues found. SQL statements use parameterized queries and dialect.identifier_preparer for safe quoting across databases. ⚡ PerformanceNo issues found. Dialect-specific optimizations (e.g., FOR UPDATE SKIP LOCKED → UPDLOCK/ROWLOCK/READPAST in mail.py) preserve locking semantics. 🏗️ Code QualityNo issues found. Dialect detection uses ✅ Tests & DocumentationExcellent test coverage: all tests pass on MSSQL (e2e on 3 browsers). PR description clearly explains the migration story (stamp-and-autogenerate for MSSQL). Actionable SuggestionsNone. Overall AssessmentLGTM ✅ Major feature, well-executed with comprehensive dialect support, solid test coverage, and clear documentation. PostgreSQL path unchanged; purely additive. Reviewed by MedCover Reviewer AI · Full review |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docker-compose.prod.yml (1)
76-76: ⚡ Quick winConsider increasing
stop_grace_periodfor production.30 seconds may be insufficient for MSSQL to gracefully complete in-flight transactions and checkpoints before forceful termination, especially under load. The default for SQL Server shutdown is typically 60 seconds or more. Consider setting this to 60s or higher to reduce the risk of data corruption during restarts.
Suggested change
- stop_grace_period: 30s + stop_grace_period: 60s🤖 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 `@docker-compose.prod.yml` at line 76, The stop_grace_period for the MSSQL service is currently set to 30 seconds, which is insufficient for SQL Server to gracefully complete in-flight transactions and checkpoints during shutdown in a production environment. Increase the stop_grace_period value from 30s to at least 60s or higher to allow adequate time for proper database shutdown and reduce the risk of data corruption during container restarts.
🤖 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 `@docker-compose.prod.yml`:
- Line 76: The stop_grace_period for the MSSQL service is currently set to 30
seconds, which is insufficient for SQL Server to gracefully complete in-flight
transactions and checkpoints during shutdown in a production environment.
Increase the stop_grace_period value from 30s to at least 60s or higher to allow
adequate time for proper database shutdown and reduce the risk of data
corruption during container restarts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d35eed80-e1ec-4459-bd5b-c9acd84e88ac
📒 Files selected for processing (19)
.env.example.github/workflows/ci.ymlCHANGELOG.mdapp/backup.pyapp/config.pyapp/digest/blocks/server_stats.pyapp/mail.pyapp/models/qualification.pyapp/utils.pydb-init/01-create-test-db.sqldocker-compose.e2e.ymldocker-compose.prod.ymldocker-compose.ymlpostgres.confrequirements.inrequirements.txtscripts/e2e-entrypoint.shtests/conftest.pytests/test_config.py
💤 Files with no reviewable changes (4)
- requirements.in
- postgres.conf
- db-init/01-create-test-db.sql
- requirements.txt
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- app/digest/blocks/server_stats.py
|
@frenzymadness I hope the PR is ready for review. please check. the app is running on zerver and on local. looks like nothing broke. |
|
I'm not going to read the whole diff but I have a couple of questions:
|
…vers Production runs on Azure Container Apps + the managed Azure SQL service (passwordless via managed identity), not a containerized database. Several files still described the old Postgres/containerized model after the MSSQL migration: - .env.prod.example: replace POSTGRES_* vars and postgresql:// URL with the Azure SQL managed-identity connection string (Authentication=ActiveDirectoryMsi) - docker-compose.prod.yml: remove the bundled MSSQL db service/volume/depends_on; prod connects to managed Azure SQL - .dockerignore: drop deleted postgres.conf and db-init/ entries - .pre-commit-config.yaml: drop types-psycopg2 (psycopg2 removed) - digest template: "PostgreSQL databáze" -> "databáze" tooltip Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1TSmeNk897sPHRWD6ny4G
|
number 2 was already fixed as I am working on the PR today. numbers 1 and 3 are valid and will be addressed. |
…eline The 45 migrations in migrations/versions/ were written for PostgreSQL and could not run on a fresh MSSQL database (they used postgresql_where=, PG enum/boolean DDL, etc.), forcing a manual "stamp head + autogenerate" bootstrap before first deploy and breaking clean dev re-spins (alembic walked the old PG-only files). Replace the entire history with a single autogenerated MSSQL baseline (down_revision = None) created against an empty SQL Server database: - 30 tables, MSSQL-native types, mssql_where partial unique index on qualification (matching the model) - Validated: `flask db upgrade` on an empty DB followed by re-running autogenerate reports "No changes in schema detected", and `flask verify-schema` confirms all 30 tables/columns present `flask db upgrade` now works on a clean MSSQL database with no manual bootstrap. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1TSmeNk897sPHRWD6ny4G
The MSSQL container has no auto-init directory (unlike Postgres' docker-entrypoint-initdb.d), so the previous setup required a manual `docker compose exec` step after first startup — `docker compose up` alone left the app without its database/login. The old `./mssql-init:/docker- entrypoint-initdb.d` mount was misleading: that path does nothing in the MSSQL image. Add a dedicated one-shot `db-init` service that waits for the db to be healthy, runs setup.sh (create medcover_dev with Czech collation + RCSI, create the app login/user), then exits. `web` now depends on it via service_completed_successfully, so `docker compose up` provisions everything automatically. setup.sh is parameterized (MSSQL_HOST / MSSQL_SA_PASSWORD / SQLCMD) and remains idempotent. Verified: `docker compose up` brings up db -> db-init (runs & exits 0) -> web (healthy) with no manual intervention. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1TSmeNk897sPHRWD6ny4G
…driver - Remove the obsolete "one-time MSSQL bootstrap" (stamp head + autogenerate); the single MSSQL baseline now applies via `flask db upgrade` on a fresh DB - Document the host ODBC Driver 18 + unixODBC prerequisite for running the test suite (pyodbc needs libodbc.so.2 at runtime; pip alone is insufficient) - Correct the "Run tests" section: the prod-lean image has no pytest/tox, so tests run on the host venv (as CI does), not via `docker compose exec web` - Reflect the new one-shot db-init service in the compose snippet Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1TSmeNk897sPHRWD6ny4G
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)
app/__init__.py (1)
136-136:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix Python 2 exception syntax on lines 136 and 160.
Both exception handlers use the Python 2 comma syntax (
except TypeError, ValueError:), which is invalid in Python 3.0+. This will cause aSyntaxErroron module import and prevents the application from running.🔴 Proposed fix for Python 3 exception syntax
- except TypeError, ValueError: + except (TypeError, ValueError):Apply the same fix to line 160:
- except ValueError, TypeError: + except (ValueError, TypeError):Also applies to: 160-160
🤖 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 `@app/__init__.py` at line 136, The exception handlers at lines 136 and 160 use Python 2 comma syntax (except TypeError, ValueError:) which is invalid in Python 3. Replace the comma between exception types with the Python 3 syntax by wrapping the exception types in parentheses, changing the syntax to except (TypeError, ValueError):. Apply this same fix to both occurrences to ensure the module can be imported without SyntaxError.
🤖 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 `@app/__init__.py`:
- Line 136: The exception handlers at lines 136 and 160 use Python 2 comma
syntax (except TypeError, ValueError:) which is invalid in Python 3. Replace the
comma between exception types with the Python 3 syntax by wrapping the exception
types in parentheses, changing the syntax to except (TypeError, ValueError):.
Apply this same fix to both occurrences to ensure the module can be imported
without SyntaxError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f03aa959-41d6-4c61-8c6b-3d910253f690
📒 Files selected for processing (62)
.dockerignore.env.prod.example.github/workflows/ci.yml.pre-commit-config.yamlDEVOPS.mdREADME.mdapp/__init__.pyapp/templates/admin/digest/index.htmlarchitecture.mddocker-compose.prod.ymldocker-compose.ymlmigrations/versions/150aa748aa4f_add_ical_token_to_user_account.pymigrations/versions/1613fcb025fb_add_password_reset_nonce_to_user_account.pymigrations/versions/1a19cb432044_add_dev_email_block_to_app_settings.pymigrations/versions/20dbd30fbcfe_debriefing_redesign_new_fields_on_.pymigrations/versions/2b096852ef02_template_equipment_plan.pymigrations/versions/2dbbf9629c3f_mssql_baseline_schema.pymigrations/versions/36dc707e1bbc_add_can_be_rp_to_qualification.pymigrations/versions/4ec25aa941b8_add_invite_cancelled_at.pymigrations/versions/5a65ab309cfc_add_dashboard_horizon_days_to_user_.pymigrations/versions/5bf568d4f009_add_dark_mode_to_user_account.pymigrations/versions/661e4a600825_add_last_login_at_to_user_account.pymigrations/versions/67dfa2385f16_add_scheduler_last_seen_to_app_settings.pymigrations/versions/6afec7c90ac1_qualification_soft_delete.pymigrations/versions/7fd90ec4167e_add_user_role_permission_tables.pymigrations/versions/86ba4668fbf9_add_backup_config_to_app_settings.pymigrations/versions/8af3e067c0c6_add_invite_email_tracking_columns.pymigrations/versions/904fa21b5ed3_rename_preferred_hour_utc_to_preferred_.pymigrations/versions/9159338cf432_add_reminder_sent_json_to_event.pymigrations/versions/92f9337e848e_add_instance_name_to_outbox_email.pymigrations/versions/953cffa1cb85_add_vykaz_generate_permission.pymigrations/versions/9b422409dc10_add_session_timeout_hours_to_app_.pymigrations/versions/9d1ee14eb241_notification_toggles_and_outbox_type.pymigrations/versions/9fd3c08b3d50_add_app_base_url_to_app_settings.pymigrations/versions/a1b2c3d4e5f6_rename_credential_to_qualification.pymigrations/versions/a2f3c4d5e6f7_lowercase_existing_user_emails.pymigrations/versions/a730e4af594e_add_version_to_event_template.pymigrations/versions/ac1ab7d64f6c_add_outbox_email_table_for_queued_email_.pymigrations/versions/ad27f656e221_add_invite_custom_subject_and_message.pymigrations/versions/b1c2d3e4f5a6_add_performance_indexes.pymigrations/versions/b39e55598ad1_add_equipment_models.pymigrations/versions/b801953d30cb_add_equipment_item_status_and_.pymigrations/versions/bbd2db07fc29_add_user_feedback_table.pymigrations/versions/bbd3aa765181_add_is_archived_to_user_account.pymigrations/versions/c11afc34311c_add_event_type_enum_planned_.pymigrations/versions/c384ea97aef5_add_notify_event_changed_to_app_settings.pymigrations/versions/c7d8e9f0a1b2_add_login_lockout_fields_to_user_account.pymigrations/versions/ca37e9989a8a_split_notify_event_lifecycle_into_.pymigrations/versions/d1e2f3a4b5c6_merge_indexes_and_login_lockout_heads.pymigrations/versions/d37d41a4e38d_add_all_domain_models.pymigrations/versions/d697cc60c5d2_add_ical_all_token.pymigrations/versions/d946eda56491_add_feedback_version_and_enabled_toggle.pymigrations/versions/dfc13fca938f_add_app_settings_table.pymigrations/versions/ebe3ddc11f1e_optional_spots.pymigrations/versions/f0c168424643_add_digest_tables_and_outbox_html_body.pymigrations/versions/f8edde653722_add_version_columns_for_optimistic_.pymigrations/versions/merge_heads_ical_all.pymssql-init/setup.shrequirements-dev.txtrequirements-e2e.txtrequirements.txttests/conftest.py
💤 Files with no reviewable changes (47)
- migrations/versions/2b096852ef02_template_equipment_plan.py
- migrations/versions/67dfa2385f16_add_scheduler_last_seen_to_app_settings.py
- migrations/versions/5a65ab309cfc_add_dashboard_horizon_days_to_user_.py
- migrations/versions/1a19cb432044_add_dev_email_block_to_app_settings.py
- migrations/versions/9d1ee14eb241_notification_toggles_and_outbox_type.py
- migrations/versions/b1c2d3e4f5a6_add_performance_indexes.py
- migrations/versions/b39e55598ad1_add_equipment_models.py
- migrations/versions/merge_heads_ical_all.py
- migrations/versions/a2f3c4d5e6f7_lowercase_existing_user_emails.py
- migrations/versions/953cffa1cb85_add_vykaz_generate_permission.py
- migrations/versions/8af3e067c0c6_add_invite_email_tracking_columns.py
- migrations/versions/b801953d30cb_add_equipment_item_status_and_.py
- migrations/versions/661e4a600825_add_last_login_at_to_user_account.py
- migrations/versions/dfc13fca938f_add_app_settings_table.py
- migrations/versions/a730e4af594e_add_version_to_event_template.py
- migrations/versions/9fd3c08b3d50_add_app_base_url_to_app_settings.py
- migrations/versions/150aa748aa4f_add_ical_token_to_user_account.py
- migrations/versions/d697cc60c5d2_add_ical_all_token.py
- migrations/versions/9159338cf432_add_reminder_sent_json_to_event.py
- .pre-commit-config.yaml
- migrations/versions/6afec7c90ac1_qualification_soft_delete.py
- migrations/versions/20dbd30fbcfe_debriefing_redesign_new_fields_on_.py
- migrations/versions/f0c168424643_add_digest_tables_and_outbox_html_body.py
- migrations/versions/86ba4668fbf9_add_backup_config_to_app_settings.py
- migrations/versions/f8edde653722_add_version_columns_for_optimistic_.py
- migrations/versions/ca37e9989a8a_split_notify_event_lifecycle_into_.py
- migrations/versions/c11afc34311c_add_event_type_enum_planned_.py
- migrations/versions/a1b2c3d4e5f6_rename_credential_to_qualification.py
- migrations/versions/9b422409dc10_add_session_timeout_hours_to_app_.py
- migrations/versions/92f9337e848e_add_instance_name_to_outbox_email.py
- migrations/versions/bbd2db07fc29_add_user_feedback_table.py
- migrations/versions/ac1ab7d64f6c_add_outbox_email_table_for_queued_email_.py
- .dockerignore
- migrations/versions/c384ea97aef5_add_notify_event_changed_to_app_settings.py
- migrations/versions/d1e2f3a4b5c6_merge_indexes_and_login_lockout_heads.py
- migrations/versions/5bf568d4f009_add_dark_mode_to_user_account.py
- migrations/versions/d37d41a4e38d_add_all_domain_models.py
- migrations/versions/bbd3aa765181_add_is_archived_to_user_account.py
- migrations/versions/7fd90ec4167e_add_user_role_permission_tables.py
- migrations/versions/904fa21b5ed3_rename_preferred_hour_utc_to_preferred_.py
- migrations/versions/c7d8e9f0a1b2_add_login_lockout_fields_to_user_account.py
- migrations/versions/36dc707e1bbc_add_can_be_rp_to_qualification.py
- migrations/versions/d946eda56491_add_feedback_version_and_enabled_toggle.py
- migrations/versions/1613fcb025fb_add_password_reset_nonce_to_user_account.py
- migrations/versions/4ec25aa941b8_add_invite_cancelled_at.py
- migrations/versions/ad27f656e221_add_invite_custom_subject_and_message.py
- migrations/versions/ebe3ddc11f1e_optional_spots.py
✅ Files skipped from review due to trivial changes (1)
- app/templates/admin/digest/index.html
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/ci.yml
- docker-compose.yml
- mssql-init/setup.sh
- tests/conftest.py
… dance The e2e entrypoint still ran the pre-squash bootstrap (`flask db stamp head` + `flask db migrate` + `flask db upgrade`), which broke after the migration squash: stamping/autogenerating against the single baseline failed with `KeyError: 'a1f2e3d4c5b6'` (a deleted PG-era revision), so the web-e2e container exited 1 and the E2E job failed. Replace the dance with a plain `flask db upgrade` — identical to the production entrypoint — which creates the full schema from the single MSSQL baseline on the fresh e2e database. Verified locally: `docker compose -f docker-compose.e2e.yml up --build` (chromium) applies `2dbbf9629c3f` cleanly, seeds, and passes 41/41 e2e tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1TSmeNk897sPHRWD6ny4G
…color) Merging main brought in migration e1f2a3b4c5d6 (add color to event), whose down_revision points into the PG chain this branch squashed away — so the PR merge ref had a dangling revision and `flask db upgrade` failed in CI with KeyError 'a1f2e3d4c5b6'. Local branch-only runs missed it because CI tests the branch merged with main. Regenerate the single MSSQL baseline from the merged models so it includes event.color, and drop the now-redundant e1f2a3b4c5d6. Validated: upgrade on an empty DB + re-run autogenerate = "No changes detected"; verify-schema confirms 30 tables. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1TSmeNk897sPHRWD6ny4G
There was a problem hiding this comment.
🧹 Nitpick comments (3)
DEVOPS.md (3)
845-845: 💤 Low valueMinor wordiness in migration governance section.
Static analysis flagged two phrasing opportunities:
- Line 845: "can't merge by accident" → consider "can't merge unintentionally" or "can't merge without review"
- Line 887: "that is exactly the failure" → consider "that is precisely the failure" (or simply "that is the failure")
These are style preferences and do not affect technical accuracy or clarity. Address only if you prefer tighter wording.
Also applies to: 887-887
🤖 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 `@DEVOPS.md` at line 845, Update the phrasing in the migration governance section of DEVOPS.md to improve clarity and conciseness. Replace the phrase "can't merge by accident" with either "can't merge unintentionally" or "can't merge without review" to be more precise. Similarly, replace "that is exactly the failure" with "that is precisely the failure" or simply "that is the failure" for tighter wording. These changes improve readability without affecting technical accuracy.Source: Linters/SAST tools
184-217: ⚡ Quick winTest instructions need platform-specific ODBC driver installation guidance.
The docs correctly identify that the host needs Microsoft ODBC Driver 18 + unixODBC, but only show the Ubuntu command. Developers on macOS or Windows WSL2 need explicit steps (brew, apt, system package manager, etc.). Consider adding platform-specific installation subsections or a link to Microsoft's official ODBC driver documentation.
🤖 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 `@DEVOPS.md` around lines 184 - 217, The ODBC driver installation guidance in the "Tests run on the host" section provides detailed steps only for Debian/Ubuntu, with a minimal macOS comment and no Windows WSL2 guidance. Expand the installation instructions by adding platform-specific subsections for macOS (with detailed brew installation steps similar to the Ubuntu format) and Windows WSL2 (with explicit apt-get commands for WSL2 systems), or alternatively add a reference link to Microsoft's official ODBC driver documentation where developers can find platform-specific installation guidance. Ensure each platform has clear, actionable commands that developers can copy and run.
338-341: 💤 Low valueClarify db-init idempotency and recovery behavior.
The comment states "idempotent, so re-runs are safe," but the docs don't explain what "safe" means when re-running: Does the init script skip existing databases, drop and recreate, or fail silently? This matters when developers restart containers or re-run
docker compose up. A brief note on the behavior (e.g., "checks for existing database and login before creating") would reduce confusion.🤖 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 `@DEVOPS.md` around lines 338 - 341, Update the documentation comment for the db-init service to explicitly clarify its idempotency behavior by explaining what checks or mechanisms prevent issues during re-runs. Specifically, add details about how the initialization script handles existing databases and logins (for example, whether it checks for their existence before creating, skips creation if they already exist, or uses conditional logic to make the process safe for repeated executions). This will help developers understand what to expect when restarting containers or re-running docker compose up.
🤖 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 `@DEVOPS.md`:
- Line 845: Update the phrasing in the migration governance section of DEVOPS.md
to improve clarity and conciseness. Replace the phrase "can't merge by accident"
with either "can't merge unintentionally" or "can't merge without review" to be
more precise. Similarly, replace "that is exactly the failure" with "that is
precisely the failure" or simply "that is the failure" for tighter wording.
These changes improve readability without affecting technical accuracy.
- Around line 184-217: The ODBC driver installation guidance in the "Tests run
on the host" section provides detailed steps only for Debian/Ubuntu, with a
minimal macOS comment and no Windows WSL2 guidance. Expand the installation
instructions by adding platform-specific subsections for macOS (with detailed
brew installation steps similar to the Ubuntu format) and Windows WSL2 (with
explicit apt-get commands for WSL2 systems), or alternatively add a reference
link to Microsoft's official ODBC driver documentation where developers can find
platform-specific installation guidance. Ensure each platform has clear,
actionable commands that developers can copy and run.
- Around line 338-341: Update the documentation comment for the db-init service
to explicitly clarify its idempotency behavior by explaining what checks or
mechanisms prevent issues during re-runs. Specifically, add details about how
the initialization script handles existing databases and logins (for example,
whether it checks for their existence before creating, skips creation if they
already exist, or uses conditional logic to make the process safe for repeated
executions). This will help developers understand what to expect when restarting
containers or re-running docker compose up.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 333b5ad9-31ce-4e66-ba59-85457ad54229
📒 Files selected for processing (10)
.github/workflows/ci.yml.pre-commit-config.yamlDEVOPS.mdapp/routes/events/_helpers.pyapp/routes/events/crud.pyapp/routes/events/spots.pyapp/routes/master_events.pymigrations/versions/2c159bca01be_mssql_baseline_schema.pyscripts/check_migrations.pyscripts/e2e-entrypoint.sh
🚧 Files skipped from review as they are similar to previous changes (4)
- app/routes/events/_helpers.py
- app/routes/events/spots.py
- app/routes/master_events.py
- app/routes/events/crud.py
A re-squash rewrites the single MSSQL baseline's revision id (and can fold in new columns), stranding every durable DB: alembic_version points at a deleted revision and flask db upgrade aborts on deploy. This hit the zerver dev deploy of feat/mssql-support (the "include color" re-squash left dev DBs without event.color). The risk applies to prod too — an identical-schema re-squash still breaks it, since the revision id changes. Add scripts/check_migrations.py enforcing: one root, root id == frozen EXPECTED_BASELINE_REVISION, single head. Wire into CI lint job and pre-commit (on migrations/versions changes). Document the failure mode, prod exposure, and a Sanctioned re-baseline procedure (schema-neutral squash + re-stamp every durable DB) in DEVOPS.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
spidermila
force-pushed
the
feat/mssql-support
branch
from
7772a54 to
f1d4a9a
Compare
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ps while allowing dual-connection paths
frenzymadness
left a comment
frenzymadness
left a comment
There was a problem hiding this comment.
Just a few comments now as I see fresh commits landing here. Let me know when you are done here.
…it mount, clarify setup-e2e.sh
2 participants
Summary
Migrates MedCover from PostgreSQL to Microsoft SQL Server (MSSQL 2022 / Azure SQL) as the sole supported database engine. PostgreSQL is fully removed.
What changed
Core dialect changes
sa.false()/sa.true()— replaced.is_(False/True)(generated invalidIS 0on MSSQL) across 22 filesCS_COLLATION— hardcoded toCzech_100_CI_AS_SC_UTF8; PG ICU collation removedFOR UPDATE SKIP LOCKED→WITH (UPDLOCK, ROWLOCK, READPAST)postgresql_where=→mssql_where=Dialect-specific modules simplified (PG branches removed)
app/backup.py— IDENTITY_INSERT for restore, DBCC CHECKIDENT for reseed, DELETE+NOCHECK for clear, TOP 1 for alembic headapp/digest/blocks/server_stats.py—sys.database_filesandsys.tables/allocation_unitsonlyapp/mail.py— READPAST hint onlyapp/config.py— removed_fix_db_url()(postgres:// → postgresql:// shim); production warning updated forEncrypt=yesInfrastructure
Dockerfile— ODBC Driver 18 +libgssapi-krb5-2requirements.in/txt— removedpsycopg2-binary;pyodbconlydocker-compose.yml— MSSQL 2022 Express replaces PostgreSQLdocker-compose.prod.yml— MSSQLdocker-compose.e2e.yml— MSSQL (merged from formerdocker-compose.e2e-mssql.yml)docker-compose.mssql-dev.yml,docker-compose.e2e-mssql.yml,db-init/,postgres.confscripts/e2e-entrypoint.sh— MSSQL-only (PG case branch removed)mssql-init/setup.sh,setup-e2e.sh— explicit timeout failure handling.github/workflows/ci.yml— MSSQL service container + ODBC install step.env.example— updated to MSSQL connection stringTest suite
tests/conftest.py— PostgresContainer → SqlServerContainer (testcontainers);_ensure_db_exists/_drop_dbrewritten for MSSQL;TRUNCATE CASCADE→ FK-disable + DELETE; xdist worker DB URLs correctly handle MSSQL query paramstests/test_config.py— test URLs and warning assertions updated for MSSQLTest results
Security review
See
azure-sql-security-review.mdandAzure-SQL-DB.parameters.jsonin the medcover-infra repo for ARM template hardening (TLS 1.2, ADS, auditing, VA).Summary by CodeRabbit
Release Notes
Changed
Chores