Issue #956: Disable SQLAlchemy echo in MDR API (env-driven)

[dereck-symmetry]

Include:

• This is a fix for Issue MDR API SQLAlchemy engine uses echo=True — logs every SQL statement to CloudWatch #956. The default behavior for SQLAlchemy going forward will be for the echo to be set to "False". However, it is possible to turn the echo to True if that would be preferred in development environments.

Related Issues

Closes #956

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality
  to not work as expected)
• Documentation update
• Infrastructure/deployment change
• Performance improvement
• Code refactoring

Project Area(s) Affected

• bases/
• components/
• projects/
• orchestrators/
• frontends/
• deployments/
• cloudformation/ or sam/ templates
• reference_data/
• scripts/
• test/ or e2e/
• Database schema (migrations)
• API endpoints
• Documentation (docs/, READMEs, ARCHITECTURE.md, CLAUDE.md)

Checklist

• commit message follows commit guidelines (see commitlint.config.mjs)
• [X ] code passes linting checks (uv run ruff check)
• code passes formatting checks (uv run ruff format)
• code passes type checking (uv run ty check)
• pre-commit hooks have been run successfully
• configuration changes: relevant folder README updated

Testing

• Manual testing performed
• Automated tests added/updated
• Integration testing completed

Additional Notes

Configuration updates done with the help of LLM. Did a complete deploy locally with docker to assure that the data was working.

[bjagg]

Overview

Makes SQLAlchemy echo env-driven and defaults it off, fixing #956. This is the most complete of the three PRs currently open from this branch series — the change itself is two lines, and the rest is the follow-through that usually gets skipped:

• deployments/advisor-demo-docker/docker-compose.yml — wired with a false default
• projects/lif_mdr_api/mdr-api.env.example — documented with a "keep false" note
• projects/lif_mdr_api/README.md — added to the env-var table
• test/bases/lif/mdr_restapi/conftest.py — the test engine's hardcoded echo=True picked up too, which is easy to miss
• New tests for default-off, on-for-true, and off-for-anything-else

The inline comment explaining why (echo emits every statement plus bound parameters at INFO) is the right thing to leave behind. Deliberately not adding SQLALCHEMY_ECHO to the CloudFormation task definitions is also correct — absent means false, which is the desired deployed behavior.

One required change: importlib.reload() in tests

test/components/lif/mdr_utils/test_database_setup.py — the new TestEchoToggle class reloads the module under each env condition:

return importlib.reload(database_setup).engine.echo

CLAUDE.md:125 prohibits this directly:

Avoid importlib.reload() in tests — it breaks isinstance()/pytest.raises() matching. Use mock.patch.object(module, "VAR_NAME", value) instead.

git grep importlib.reload -- test/ returns nothing on main, so this would introduce the only use in the suite. Beyond the convention, reloading database_setup rebinds module-level engine and async_session for every test that imports them afterward, which makes suite behavior order-dependent — the class docstring acknowledges the engine is built at import time, which is exactly the hazard.

The underlying difficulty is real: engine is constructed at module import, so there's no seam to patch. Two ways out:

1. Test the predicate rather than the engine — extract the parse (os.getenv("SQLALCHEMY_ECHO", "false").lower() == "true") into a small function and test that directly. It's the only logic here; create_async_engine faithfully storing echo is framework behavior.
2. Or mock.patch.object(database_setup, "SQLALCHEMY_ECHO", True) per the convention, if what's being asserted is downstream use of the flag.

Option 1 is probably the better shape — it also gives the flag a name that can be reused if another engine needs it.

Minor

• cspell.json adds SQLALCHEMY — fine, though sqlalchemy may already be present in a different case; worth a quick check to avoid a duplicate entry.
• Consider whether MDR__ prefixing would be more consistent with the surrounding settings (MDR__AUTH__*, MDR__COOKIE__*). SQLALCHEMY_ECHO is unprefixed like POSTGRESQL_*, so either convention has precedent — just worth a deliberate choice rather than an accident.

Verdict

Approve once the importlib.reload() test is reworked. Everything else here is ready, and the completeness of the non-code follow-through is worth calling out — the conftest.py catch in particular.

Note this PR is currently BEHIND main; it'll need a branch update before merge.

[dereck-symmetry]

Thanks @bjagg — all points addressed:

• importlib.reload() (required): removed entirely. Extracted the parse into _echo_from_env() in database_setup.py, and TestEchoToggle now asserts on the predicate directly under each env condition via monkeypatch — same five cases (default-off, true/TRUE/True, off-for-anything-else), no reload, no order dependence. Took your option 1; option 2 wouldn't have asserted anything real, since patching the flag post-import can't affect the already-built engine. → 7e8110c
• cspell duplicate: verified — exactly one entry exists (grep -in sqlal cspell.json → single hit, the new "SQLALCHEMY"); no lowercase sqlalchemy elsewhere in the words list. No change made.
• Naming: keeping SQLALCHEMY_ECHO unprefixed deliberately — like POSTGRESQL_*, it's consumed directly by engine construction in database_setup.py, outside the pydantic Settings MDR__* tree, so its closest precedent is unprefixed infra vars.
• Branch update: merged latest main into the branch (7006934), clean.

One extra commit flagged for transparency: 3d9457c removes .claude/plans/772-import-transformation-groups.md — agent-planning scratch that was inadvertently committed during #772 follow-through (#1136/#1143) — and adds .claude/plans/ to .gitignore. Unrelated to #956, isolated as a chore: commit so it's easy to review around.

Re-requesting quick review per repo rules.