Feature/app factory pattern #27
Conversation
WalkthroughReplaces a global Flask app with a factory pattern ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Vercel as Vercel Function
participant Factory as create_app()
participant Config as ProductionConfig
participant Ext as extensions (db/migrate/jwt/api)
participant App as Flask App
Vercel->>Factory: create_app(Config)
Factory->>Config: load settings
Factory->>Ext: ext.init_app(app)
Ext-->>Factory: extensions configured
Factory->>App: register blueprints/routes
Factory-->>Vercel: Flask app instance
sequenceDiagram
autonumber
participant Client
participant App as Flask App
participant JWT as JWTManager
participant CB as JWT callbacks
participant Resp as Response
Client->>App: HTTP request with token
App->>JWT: validate token
alt token invalid/expired/missing
JWT->>CB: invoke callback (invalid/expired/missing)
CB-->>App: JSON error (401)
App-->>Client: 401 JSON error
else valid token
App-->>Client: 2xx response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code graph analysis (3)tests/test_subcategory.py (3)
tests/test_category.py (3)
tests/test_product.py (3)
🔇 Additional comments (6)
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: 2
🧹 Nitpick comments (4)
config.py (1)
30-47: Annotate mutable class attribute withClassVar.The mutable dictionary
API_SPEC_OPTIONSshould be annotated withtyping.ClassVarto clarify it's a class-level constant shared across instances.Apply this diff:
+from typing import ClassVar + class Config: # sqlalchemy SQLALCHEMY_TRACK_MODIFICATIONS = False # jwt JWT_SECRET_KEY = os.getenv("JWT_SECRET_KEY") JWT_ACCESS_TOKEN_EXPIRES = timedelta(hours=3) JWT_REFRESH_TOKEN_EXPIRES = timedelta(days=3) # flask-smorest API_TITLE = "Ecommerce REST API" API_VERSION = "v1" OPENAPI_VERSION = "3.0.2" # flask-smorest openapi swagger OPENAPI_URL_PREFIX = "/" OPENAPI_SWAGGER_UI_PATH = "/" OPENAPI_SWAGGER_UI_URL = "https://cdn.jsdelivr.net/npm/swagger-ui-dist/" # flask-smorest Swagger UI top level authorize dialog box - API_SPEC_OPTIONS = { + API_SPEC_OPTIONS: ClassVar[dict] = { "components": { "securitySchemes": {tests/test_category.py (1)
127-134: Verify passing app_context() into token helperYou pass self.client.application.app_context() to utils.get_expired_token_headers. Ensure utils expects a context manager and handles pushing/popping it. Prefer passing the app (self.client.application) and letting the helper manage app_context internally to avoid leaks.
Apply consistently across the three parametrizations:
- (lambda self: utils.get_expired_token_headers(self.client.application.app_context()), "token_expired"), + (lambda self: utils.get_expired_token_headers(self.client.application), "token_expired"),Also applies to: 143-150, 167-174
app/__init__.py (1)
11-11: Typo in comment“extenstions” -> “extensions”.
- # initialize extenstions + # initialize extensionstests/conftest.py (1)
49-61: Minor: simplify closure by defining headers before nested functionCurrent nonlocal usage is fine; moving headers assignment above improves readability.
-@pytest.fixture -def create_authenticated_headers(register_user, login_user): - def _get_headers(email="[email protected]", password="testpassword"): - nonlocal headers - if not headers: - register_user(email, password) - resp = login_user(email, password) - tokens = resp.get_json() - headers = utils.get_auth_header(tokens["access_token"]) - - return headers - - headers = None - return _get_headers +@pytest.fixture +def create_authenticated_headers(register_user, login_user): + headers = None + + def _get_headers(email="[email protected]", password="testpassword"): + nonlocal headers + if not headers: + register_user(email, password) + resp = login_user(email, password) + tokens = resp.get_json() + headers = utils.get_auth_header(tokens["access_token"]) + return headers + + return _get_headers
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
api/vercel_function.py(1 hunks)app/__init__.py(1 hunks)app/extensions.py(1 hunks)config.py(1 hunks)populate_db.py(1 hunks)tests/conftest.py(1 hunks)tests/test_auth.py(2 hunks)tests/test_category.py(1 hunks)tests/test_product.py(1 hunks)tests/test_relationships.py(1 hunks)tests/test_subcategory.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
tests/test_product.py (1)
app/models.py (1)
Product(113-131)
tests/test_subcategory.py (1)
app/models.py (1)
Subcategory(93-110)
tests/test_auth.py (1)
app/models.py (1)
User(18-59)
app/__init__.py (2)
tests/conftest.py (1)
app(9-22)config.py (1)
DevelopmentConfig(50-52)
tests/test_category.py (1)
app/models.py (1)
Category(74-90)
api/vercel_function.py (3)
tests/conftest.py (1)
app(9-22)app/__init__.py (1)
create_app(7-28)config.py (1)
ProductionConfig(61-62)
tests/conftest.py (2)
app/__init__.py (1)
create_app(7-28)config.py (1)
TestingConfig(55-58)
tests/test_relationships.py (1)
app/models.py (3)
Category(74-90)Subcategory(93-110)Product(113-131)
populate_db.py (2)
tests/conftest.py (1)
app(9-22)app/__init__.py (1)
create_app(7-28)
🪛 Ruff (0.14.0)
config.py
30-47: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
app/extensions.py
26-26: Unused function argument: jwt_header
(ARG001)
34-34: Unused function argument: error
(ARG001)
39-39: Unused function argument: error
(ARG001)
🔇 Additional comments (11)
config.py (2)
58-58: LGTM: Random JWT secret for testing.Generating a random JWT secret key for each test run is a good security practice that prevents test tokens from being reused.
15-15: Verify JWT_SECRET_KEY is set in production.
JWT_SECRET_KEYwill beNoneif the environment variable is not set, which would cause JWT operations to fail. This is a critical configuration for production.Consider adding validation to ensure the secret key is set in production environments:
class Config: # sqlalchemy SQLALCHEMY_TRACK_MODIFICATIONS = False # jwt JWT_SECRET_KEY = os.getenv("JWT_SECRET_KEY") + if not JWT_SECRET_KEY: + raise ValueError("JWT_SECRET_KEY environment variable must be set") JWT_ACCESS_TOKEN_EXPIRES = timedelta(hours=3) JWT_REFRESH_TOKEN_EXPIRES = timedelta(days=3)Alternatively, apply validation only in
ProductionConfig:class ProductionConfig(Config): SQLALCHEMY_DATABASE_URI = os.getenv("SQLALCHEMY_DATABASE_URI") def __init__(self): if not self.JWT_SECRET_KEY: raise ValueError("JWT_SECRET_KEY must be set in production")app/extensions.py (2)
9-22: LGTM: Well-structured extension initialization.The PostgreSQL naming convention is properly documented, and the centralized extension initialization supports the factory pattern cleanly.
25-43: LGTM: JWT error handlers provide clear feedback.The JWT callback functions provide context-aware error messages. The static analysis warnings about unused parameters (
jwt_header,error) are false positives—these parameters are required by the Flask-JWT-Extended decorator signatures even when not used in the implementation.tests/test_subcategory.py (1)
16-28: LGTM: Simplified test helpers.The removal of explicit
app_contextusage is correct—the factory-basedappfixture now manages the application context lifecycle, allowing direct ORM queries in test helpers.api/vercel_function.py (1)
1-5: LGTM: Factory-based app creation for deployment.The change to factory-based initialization with
ProductionConfigis correct and maintains the module-levelappvariable required by Vercel's deployment model.tests/test_auth.py (1)
19-47: LGTM: Consistent app_context removal.The removal of explicit
app_contextusage from test helpers aligns with the factory-based app fixture that now manages the application context lifecycle.tests/test_relationships.py (1)
13-40: LGTM: Comprehensive app_context cleanup.The removal of explicit
app_contextusage from setup and all relationship helper methods is consistent with the factory-based app fixture pattern.populate_db.py (1)
2-7: LGTM: Factory-based app for database population.The switch to factory-based app creation via
create_app()is correct and maintains the properapp.app_context()usage in themain()function.tests/test_product.py (1)
17-29: LGTM: Simplified test helpers.The removal of explicit
app_contextusage is correct and consistent with the factory-based app fixture that manages the application context lifecycle.tests/test_category.py (1)
16-28: DB access without explicit app_context: LGTMUsing Category.query under the test app context provided by the fixture is correct.
| app = create_app(TestingConfig) | ||
|
|
||
| # setup | ||
| app_context = app.app_context() | ||
| app_context.push() | ||
| db.create_all() | ||
|
|
There was a problem hiding this comment.
In‑memory SQLite can reset across connections
Using sqlite:///:memory: without StaticPool may create a fresh DB per connection, causing flaky tests (tables “disappear” between create_all and requests). Use StaticPool and disable same thread checks, or use a file DB.
Update TestingConfig (in config.py), and import StaticPool:
+from sqlalchemy.pool import StaticPool
+
class TestingConfig(Config):
TESTING = True
- SQLALCHEMY_DATABASE_URI = "sqlite:///:memory:"
+ SQLALCHEMY_DATABASE_URI = "sqlite:///:memory:"
+ SQLALCHEMY_ENGINE_OPTIONS = {
+ "poolclass": StaticPool,
+ "connect_args": {"check_same_thread": False},
+ }Alternatively: use a temp file URI (e.g., sqlite:///test.db) and remove StaticPool.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| app = create_app(TestingConfig) | |
| # setup | |
| app_context = app.app_context() | |
| app_context.push() | |
| db.create_all() | |
| from sqlalchemy.pool import StaticPool | |
| class TestingConfig(Config): | |
| TESTING = True | |
| SQLALCHEMY_DATABASE_URI = "sqlite:///:memory:" | |
| SQLALCHEMY_ENGINE_OPTIONS = { | |
| "poolclass": StaticPool, | |
| "connect_args": {"check_same_thread": False}, | |
| } |
🤖 Prompt for AI Agents
In tests/conftest.py around lines 10-16 the app uses an in-memory SQLite DB
which can reset across connections; update TestingConfig in config.py to use
StaticPool and disable same-thread checks by importing
sqlalchemy.pool.StaticPool and setting SQLALCHEMY_DATABASE_URI =
"sqlite:///:memory:" plus SQLALCHEMY_ENGINE_OPTIONS = {'poolclass': StaticPool,
'connect_args': {'check_same_thread': False}} so the same in-memory DB is shared
across connections; alternatively change TestingConfig to use a temp file DB URI
like sqlite:///test.db and remove StaticPool.
There was a problem hiding this comment.
SqlAlchemy with Sqlite by default uses SingletonThreadPool, which maintains a single connection per thread. My tests run in a single thread sequentially and each test creates and drops all tables and data as its setup to ensure test isolation. All my tests pass. So, I don't think StaticPool with "check_same_thread": False is needed currently.
With the type of tests that i am writing won't using a single connection across multiple threads be a problem, as i am checking the number of categories created, and i assume i am the only one that is creating it. what if another thread also creates categories and my assertion of number of categories created in the table fails?
What do you think?
There was a problem hiding this comment.
@piyush-jaiswal, you're absolutely correct! Your analysis is spot-on:
Your current setup is appropriate for sequential single-threaded tests:
-
✅ SingletonThreadPool is sufficient: Since pytest runs tests sequentially by default (single thread), the SingletonThreadPool maintains one connection throughout the test session, keeping the in-memory database intact.
-
✅ Test isolation: Your setup/teardown with
db.create_all()anddb.drop_all()in theappfixture ensures each test starts with a clean slate, which is the right approach. -
✅ Your threading concern is valid: You're right that with StaticPool +
check_same_thread: False, if tests were running in parallel threads, you could have race conditions where multiple threads create categories simultaneously, breaking your count assertions.
When StaticPool would be needed:
- Running tests in parallel with
pytest-xdist(pytest -n auto) - If your Flask app code opens multiple connections internally (rare with typical Flask patterns)
- If you were using a worker pool or background threads in tests
Since all your tests pass and you're running sequentially, no changes are needed. Your current configuration is correct for your use case.
There was a problem hiding this comment.
for reference later: https://docs.sqlalchemy.org/en/20/dialects/sqlite.html#threading-pooling-behavior
There was a problem hiding this comment.
@piyush-jaiswal, thanks for sharing that reference! It's a great resource to have on hand for understanding SQLite's pooling behavior in different scenarios. I've noted it for future reference.
✏️ Learnings added
Learnt from: piyush-jaiswal
PR: piyush-jaiswal/ecommerce-rest-api#27
File: tests/conftest.py:10-16
Timestamp: 2025-10-16T08:06:43.379Z
Learning: For SQLite in-memory database testing with Flask/SQLAlchemy in `tests/conftest.py`: SingletonThreadPool (SQLAlchemy's default for SQLite) is sufficient for sequential single-threaded tests. StaticPool with `check_same_thread: False` is only needed when running tests in parallel (e.g., with pytest-xdist) or when the app opens multiple connections. Reference: https://docs.sqlalchemy.org/en/20/dialects/sqlite.html#threading-pooling-behavior
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/__init__.py (1)
11-11: Fix typo in comment."extenstions" should be "extensions".
Apply this diff:
- # initialize extenstions + # initialize extensions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/__init__.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/__init__.py (2)
tests/conftest.py (1)
app(9-22)config.py (1)
DevelopmentConfig(50-52)
🔇 Additional comments (1)
app/__init__.py (1)
7-28: LGTM! Clean factory pattern implementation.The factory pattern is correctly implemented with proper extension initialization order and blueprint registration. The past concern about
migrate.init_app(app, db)on line 13 has been properly addressed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/test_subcategory.py (1)
129-129: Remove unused lambda parameter.The
selfparameter is captured but never used in these lambda functions within theparametrizedecorators.Apply this diff to simplify the lambdas:
- (lambda self: utils.get_expired_token_headers(), "token_expired"), + (lambda: utils.get_expired_token_headers(), "token_expired"),Repeat for lines 145 and 169.
Also applies to: 145-145, 169-169
tests/test_category.py (1)
130-130: Remove unused lambda parameter.The
selfparameter is declared but never referenced in these parametrized lambda expressions.Apply this diff:
- (lambda self: utils.get_expired_token_headers(), "token_expired"), + (lambda: utils.get_expired_token_headers(), "token_expired"),Repeat for lines 146 and 170.
Also applies to: 146-146, 170-170
tests/test_product.py (1)
157-157: Remove unused lambda parameter.The
selfparameter in these parametrized lambdas is never used.Apply this diff:
- (lambda self: utils.get_expired_token_headers(), "token_expired"), + (lambda: utils.get_expired_token_headers(), "token_expired"),Repeat for lines 173 and 197.
Also applies to: 173-173, 197-197
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/__init__.py(1 hunks)tests/test_auth.py(3 hunks)tests/test_category.py(4 hunks)tests/test_product.py(4 hunks)tests/test_subcategory.py(4 hunks)tests/utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
tests/test_product.py (2)
app/models.py (1)
Product(113-131)tests/utils.py (1)
get_expired_token_headers(17-21)
app/__init__.py (2)
tests/conftest.py (1)
app(9-22)config.py (1)
DevelopmentConfig(50-52)
tests/test_auth.py (2)
app/models.py (1)
User(18-59)tests/utils.py (1)
get_expired_token_headers(17-21)
tests/test_subcategory.py (2)
app/models.py (1)
Subcategory(93-110)tests/utils.py (1)
get_expired_token_headers(17-21)
tests/test_category.py (1)
tests/utils.py (1)
get_expired_token_headers(17-21)
🪛 Ruff (0.14.0)
tests/test_product.py
157-157: Unused lambda argument: self
(ARG005)
173-173: Unused lambda argument: self
(ARG005)
197-197: Unused lambda argument: self
(ARG005)
tests/test_subcategory.py
129-129: Unused lambda argument: self
(ARG005)
145-145: Unused lambda argument: self
(ARG005)
169-169: Unused lambda argument: self
(ARG005)
tests/test_category.py
130-130: Unused lambda argument: self
(ARG005)
146-146: Unused lambda argument: self
(ARG005)
170-170: Unused lambda argument: self
(ARG005)
🔇 Additional comments (6)
tests/test_subcategory.py (1)
16-28: LGTM: Application context management eliminated.Direct ORM queries now work without explicit
app_context()blocks because the test fixture (fromconftest.py) pushes an application context for the entire test lifecycle. This aligns with the factory pattern migration.tests/test_auth.py (1)
19-47: LGTM: Application context usage removed.ORM queries and token decoding now execute directly without
app_context()wrappers, consistent with the factory-based test setup.tests/utils.py (1)
17-21: LGTM: Signature simplified for factory pattern.Removing the
app_contextparameter aligns with the refactored test fixtures that now manage the application context lifecycle automatically.tests/test_category.py (1)
16-28: LGTM: Application context blocks removed.Direct ORM access now leverages the application context pushed by the test fixture, eliminating the need for explicit context management.
tests/test_product.py (1)
17-29: LGTM: Application context management removed.ORM queries execute directly without
app_context()wrappers, consistent with the factory pattern test fixtures.app/__init__.py (1)
7-28: LGTM: Factory pattern implemented correctly.The
create_appfactory properly initializes extensions using theirinit_appmethods and registers blueprints within the factory function. This enables per-environment configuration and improves testability by eliminating global application state.
Summary by CodeRabbit
New Features
Refactor
Tests
Chores