chore: harden production schema compatibility (Phase 5)#124
Merged
Conversation
Contributor
Reviewer's GuideFinalizes production hardening by centralizing environment detection and password-column migration SQL, and tightening behavior for unsupported dialects in production, with accompanying unit tests for the new helpers. Sequence diagram for password column capacity hardening logicsequenceDiagram
participant AppStartup
participant EnsurePasswordColumn as _ensure_password_column_capacity
participant EnvHelper as _is_production_environment
participant SqlHelper as _password_column_migration_sql
participant DB as db.engine
participant Logger
AppStartup->>EnsurePasswordColumn: call
EnsurePasswordColumn->>DB: inspect user.password length
DB-->>EnsurePasswordColumn: current_length
Note over EnsurePasswordColumn: If current_length >= 255
EnsurePasswordColumn-->>AppStartup: return
Note over EnsurePasswordColumn: Legacy length detected
EnsurePasswordColumn->>DB: get dialect.name
DB-->>EnsurePasswordColumn: dialect
EnsurePasswordColumn->>SqlHelper: _password_column_migration_sql(dialect)
SqlHelper-->>EnsurePasswordColumn: alter_sql or None
alt Supported dialect
Note over EnsurePasswordColumn: alter_sql is not None
EnsurePasswordColumn->>Logger: info Expanding user.password column
EnsurePasswordColumn->>DB: execute(alter_sql)
DB-->>EnsurePasswordColumn: ok
EnsurePasswordColumn->>DB: session.commit()
DB-->>EnsurePasswordColumn: ok
EnsurePasswordColumn-->>AppStartup: return
else Unsupported dialect
Note over EnsurePasswordColumn: alter_sql is None
EnsurePasswordColumn->>EnvHelper: _is_production_environment()
EnvHelper-->>EnsurePasswordColumn: True or False
alt Production environment
Note over EnsurePasswordColumn: Raise instead of continuing
EnsurePasswordColumn->>EnsurePasswordColumn: raise RuntimeError
EnsurePasswordColumn-->>AppStartup: exception
else Non production environment
EnsurePasswordColumn->>Logger: warning manual migration required
EnsurePasswordColumn-->>AppStartup: return
end
end
Flow diagram for production environment detection and config usageflowchart TD
A[Start] --> B[Read APP_ENV]
B --> C[Is APP_ENV set?]
C -->|Yes| D[environment = APP_ENV]
C -->|No| E[Read FLASK_ENV or default to development]
E --> D
D --> F[Lowercase environment]
F --> G{environment in production or prod?}
G -->|Yes| H[is_production_environment returns True]
G -->|No| I[is_production_environment returns False]
H --> J[Use config.ProductionConfig]
I --> K[Use config.DevelopmentConfig]
H --> L[Do not set AUTHLIB_INSECURE_TRANSPORT]
I --> M[Set AUTHLIB_INSECURE_TRANSPORT=1]
J --> N[Flask app created with production config]
K --> N
N --> O[End]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors environment detection and database migration logic by introducing helper functions _is_production_environment and _password_column_migration_sql. It also improves error handling by raising a RuntimeError when an unsupported database dialect is encountered in production and includes new unit tests for these helpers. A review comment suggests adding a return type hint to the _password_column_migration_sql function to maintain consistency with the rest of the codebase.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider normalizing the
dialectargument in_password_column_migration_sql(e.g.,dialect.lower()) so that case differences in engine names don’t silently bypass the supported-dialect logic. - In
_is_production_environment(), you might want to strip whitespace from the resolved environment value before comparing, to avoid misclassification when env vars include trailing or leading spaces.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider normalizing the `dialect` argument in `_password_column_migration_sql` (e.g., `dialect.lower()`) so that case differences in engine names don’t silently bypass the supported-dialect logic.
- In `_is_production_environment()`, you might want to strip whitespace from the resolved environment value before comparing, to avoid misclassification when env vars include trailing or leading spaces.
## Individual Comments
### Comment 1
<location path="tests/test_app.py" line_range="92-101" />
<code_context>
+class AppHardeningHelpersTest(unittest.TestCase):
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests covering `_ensure_password_column_capacity` behavior for unsupported dialects in production vs non-production environments.
The tests currently verify `_password_column_migration_sql`’s dialect mapping but not `_ensure_password_column_capacity`’s new behavior. Please add tests that directly exercise `_ensure_password_column_capacity` with a mocked `db.engine.dialect.name` and a legacy short column length, and assert:
- Prod: a `RuntimeError` is raised for unsupported dialects.
- Non-prod: only a warning is logged and no exception is raised.
This will ensure the startup hardening behavior is explicitly covered and protected against regressions.
Suggested implementation:
```python
from app import _is_production_environment, _password_column_migration_sql, _ensure_password_column_capacity # noqa: E402
```
```python
def test_is_production_environment_false_for_development(self):
with patch.dict(os.environ, {'APP_ENV': 'development'}, clear=False):
self.assertFalse(_is_production_environment())
def test_ensure_password_column_capacity_unsupported_dialect_raises_in_production(self):
# Simulate production environment
with patch.dict(os.environ, {'APP_ENV': 'production'}, clear=False):
# Force an unsupported dialect name
with patch.object(db.engine, "dialect") as mock_dialect:
mock_dialect.name = "unsupported_dialect"
# In production, unsupported dialects should hard-fail
with self.assertRaises(RuntimeError):
_ensure_password_column_capacity()
def test_ensure_password_column_capacity_unsupported_dialect_warns_in_non_production(self):
# Simulate non-production environment
with patch.dict(os.environ, {'APP_ENV': 'development'}, clear=False):
# Force an unsupported dialect name
with patch.object(db.engine, "dialect") as mock_dialect:
mock_dialect.name = "unsupported_dialect"
# Patch logger to capture warnings
with patch("app.logger") as mock_logger:
# In non-production, unsupported dialects should only emit a warning
_ensure_password_column_capacity()
# No exception should be raised and a warning should be logged
mock_logger.warning.assert_called()
```
If `db.engine.dialect` is not patchable as an attribute in your SQLAlchemy setup, you may need to instead:
1. Patch the path actually used inside `_ensure_password_column_capacity`, for example:
- `with patch("app.db.engine.dialect") as mock_dialect:` or
- `with patch("models.db.engine.dialect") as mock_dialect:`
depending on how `db` is imported and referenced in `app.py`.
2. If `_ensure_password_column_capacity` uses a different logger (e.g., `current_app.logger` or `flask_app.logger`), adjust the `patch("app.logger")` line to target the exact logger path used there.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
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
Changes
app.py_is_production_environment()helper and reused it in config/runtime checks._password_column_migration_sql(dialect)helper to centralize supported SQL mapping._ensure_password_column_capacity()behavior:tests/test_app.pyAppHardeningHelpersTestcases for:Validation
pytest tests/ -qpassed (25 passed)Risk
Summary by Sourcery
Harden production runtime behavior around environment detection and legacy password column schema compatibility.
Enhancements:
Tests: