fix(database): use issubclass instead of isinstance for pool class validation#10536
fix(database): use issubclass instead of isinstance for pool class validation#10536yashwantbezawada wants to merge 8 commits into
Conversation
…lidation Fixes langflow-ai#10231 The pool class validation was incorrectly using isinstance(pool_class(), ...) which attempts to instantiate the pool class without required arguments. SQLAlchemy pool classes like NullPool require a 'creator' argument during instantiation, causing crashes when users configure custom pool classes. Changed to use issubclass(pool_class, ...) which validates class inheritance without instantiation. This is the correct way to check if a class is a subclass of sa.pool.Pool. Also added comprehensive test coverage for pool class validation including NullPool, StaticPool, QueuePool, and invalid pool class scenarios.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe change modifies SQLAlchemy pool class validation logic in DatabaseService initialization from attempting instantiation via Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (7 passed)
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 |
github-actions
Bot
added
bug
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/base/langflow/services/database/service.py (1)
131-135: Remove invalid poolclass from kwargs when validation fails.When poolclass validation fails (line 135), the error is logged but the invalid string value remains in
kwargsand gets passed tocreate_async_engineon line 137. SQLAlchemy expectspoolclassto be a class object, not a string, so this will cause a downstream error.Apply this diff to remove the invalid poolclass:
if pool_class and isinstance(pool_class, type) and issubclass(pool_class, sa.pool.Pool): logger.debug(f"Using poolclass: {poolclass_key}.") kwargs["poolclass"] = pool_class else: logger.error(f"Invalid poolclass '{poolclass_key}' specified. Using default pool class.") + kwargs.pop("poolclass", None)
🧹 Nitpick comments (1)
src/backend/tests/unit/services/database/test_database_service.py (1)
28-28: Suppress or avoid unused variable warnings.The
db_servicevariable is assigned in all four tests to trigger the__init__validation logic but is never referenced afterward, causing linter warnings (F841).Consider one of these approaches:
Option 1: Use underscore to indicate intentional non-use:
- db_service = DatabaseService(mock_settings_service) + _ = DatabaseService(mock_settings_service)Option 2: Add noqa comment:
- db_service = DatabaseService(mock_settings_service) + db_service = DatabaseService(mock_settings_service) # noqa: F841Also applies to: 49-49, 71-71, 98-98
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/backend/base/langflow/services/database/service.py(1 hunks)src/backend/tests/unit/services/database/test_database_service.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{src/backend/**/*.py,tests/**/*.py,Makefile}
📄 CodeRabbit inference engine (.cursor/rules/backend_development.mdc)
{src/backend/**/*.py,tests/**/*.py,Makefile}: Run make format_backend to format Python code before linting or committing changes
Run make lint to perform linting checks on backend Python code
Files:
src/backend/base/langflow/services/database/service.pysrc/backend/tests/unit/services/database/test_database_service.py
src/backend/tests/unit/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/backend_development.mdc)
Test component integration within flows using create_flow, build_flow, and get_build_events utilities
Files:
src/backend/tests/unit/services/database/test_database_service.py
src/backend/tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/testing.mdc)
src/backend/tests/**/*.py: Unit tests for backend code must be located in the 'src/backend/tests/' directory, with component tests organized by component subdirectory under 'src/backend/tests/unit/components/'.
Test files should use the same filename as the component under test, with an appropriate test prefix or suffix (e.g., 'my_component.py' → 'test_my_component.py').
Use the 'client' fixture (an async httpx.AsyncClient) for API tests in backend Python tests, as defined in 'src/backend/tests/conftest.py'.
When writing component tests, inherit from the appropriate base class in 'src/backend/tests/base.py' (ComponentTestBase, ComponentTestBaseWithClient, or ComponentTestBaseWithoutClient) and provide the required fixtures: 'component_class', 'default_kwargs', and 'file_names_mapping'.
Each test in backend Python test files should have a clear docstring explaining its purpose, and complex setups or mocks should be well-commented.
Test both sync and async code paths in backend Python tests, using '@pytest.mark.asyncio' for async tests.
Mock external dependencies appropriately in backend Python tests to isolate unit tests from external services.
Test error handling and edge cases in backend Python tests, including using 'pytest.raises' and asserting error messages.
Validate input/output behavior and test component initialization and configuration in backend Python tests.
Use the 'no_blockbuster' pytest marker to skip the blockbuster plugin in tests when necessary.
Be aware of ContextVar propagation in async tests; test both direct event loop execution and 'asyncio.to_thread' scenarios to ensure proper context isolation.
Test error handling by mocking internal functions using monkeypatch in backend Python tests.
Test resource cleanup in backend Python tests by using fixtures that ensure proper initialization and cleanup of resources.
Test timeout and performance constraints in backend Python tests using 'asyncio.wait_for' and timing assertions.
Test Langflow's Messag...
Files:
src/backend/tests/unit/services/database/test_database_service.py
🧠 Learnings (3)
📚 Learning: 2025-07-21T14:16:14.125Z
Learnt from: CR
Repo: langflow-ai/langflow PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-07-21T14:16:14.125Z
Learning: Applies to src/backend/tests/**/*.py : Mock external dependencies appropriately in backend Python tests to isolate unit tests from external services.
Applied to files:
src/backend/tests/unit/services/database/test_database_service.py
📚 Learning: 2025-07-21T14:16:14.125Z
Learnt from: CR
Repo: langflow-ai/langflow PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-07-21T14:16:14.125Z
Learning: Applies to src/backend/tests/unit/test_database.py : For database-related tests that may fail in batch runs but pass individually, consider running them sequentially and be aware of this behavior when writing such tests.
Applied to files:
src/backend/tests/unit/services/database/test_database_service.py
📚 Learning: 2025-07-21T14:16:14.125Z
Learnt from: CR
Repo: langflow-ai/langflow PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-07-21T14:16:14.125Z
Learning: Applies to src/backend/tests/**/*.py : Test error handling by mocking internal functions using monkeypatch in backend Python tests.
Applied to files:
src/backend/tests/unit/services/database/test_database_service.py
🧬 Code graph analysis (1)
src/backend/tests/unit/services/database/test_database_service.py (1)
src/backend/base/langflow/services/database/service.py (1)
DatabaseService(39-485)
🪛 GitHub Actions: Ruff Style Check
src/backend/tests/unit/services/database/test_database_service.py
[error] 5-5: Ruff: F401 'pytest' imported but unused.
🪛 GitHub Check: Ruff Style Check (3.13)
src/backend/tests/unit/services/database/test_database_service.py
[failure] 98-98: Ruff (F841)
src/backend/tests/unit/services/database/test_database_service.py:98:9: F841 Local variable db_service is assigned to but never used
[failure] 71-71: Ruff (F841)
src/backend/tests/unit/services/database/test_database_service.py:71:9: F841 Local variable db_service is assigned to but never used
[failure] 49-49: Ruff (F841)
src/backend/tests/unit/services/database/test_database_service.py:49:9: F841 Local variable db_service is assigned to but never used
[failure] 28-28: Ruff (F841)
src/backend/tests/unit/services/database/test_database_service.py:28:9: F841 Local variable db_service is assigned to but never used
[failure] 5-5: Ruff (F401)
src/backend/tests/unit/services/database/test_database_service.py:5:8: F401 pytest imported but unused
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Lint Backend / Run Mypy (3.12)
- GitHub Check: Run Backend Tests / LFX Tests - Python 3.10
- GitHub Check: Run Frontend Tests / Determine Test Suites and Shard Distribution
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 3
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 5
- GitHub Check: Run Backend Tests / Integration Tests - Python 3.10
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 2
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 4
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 1
- GitHub Check: Test Starter Templates
- GitHub Check: Optimize new Python code in this PR
- GitHub Check: Update Component Index
🔇 Additional comments (1)
src/backend/tests/unit/services/database/test_database_service.py (1)
79-83: Test should verify that invalid poolclass is removed from kwargs.Lines 82-83 check whether the invalid poolclass string is still present in
kwargs, which documents the current buggy behavior where invalid strings are passed tocreate_async_engine. Once the bug inservice.pyis fixed (by removing invalid poolclass from kwargs), this test assertion should be updated to verify thatpoolclassis not incall_kwargs.After applying the fix in
service.pyto remove invalid poolclass from kwargs, update this test:# Verify that poolclass was NOT passed to create_async_engine call_kwargs = mock_create_engine.call_args[1] - # The invalid poolclass should not be in the kwargs, or if it is, it should be the string not a class - if "poolclass" in call_kwargs: - assert call_kwargs["poolclass"] == "InvalidPoolClass" + # The invalid poolclass should have been removed from kwargs + assert "poolclass" not in call_kwargs
| if poolclass_key is not None: | ||
| pool_class = getattr(sa, poolclass_key, None) | ||
| if pool_class and isinstance(pool_class(), sa.pool.Pool): | ||
| if pool_class and issubclass(pool_class, sa.pool.Pool): |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Good fix, but add TypeError protection.
The change from isinstance(pool_class(), sa.pool.Pool) to issubclass(pool_class, sa.pool.Pool) correctly fixes the instantiation bug without requiring pool constructor arguments.
However, issubclass() raises TypeError if the first argument is not a class. Since getattr(sa, poolclass_key, None) on line 130 could return non-class attributes from the sa module (functions, constants, etc.), you should add a type check.
Apply this diff to prevent TypeError:
- if pool_class and issubclass(pool_class, sa.pool.Pool):
+ if pool_class and isinstance(pool_class, type) and issubclass(pool_class, sa.pool.Pool):📝 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.
| if pool_class and issubclass(pool_class, sa.pool.Pool): | |
| if pool_class and isinstance(pool_class, type) and issubclass(pool_class, sa.pool.Pool): |
🤖 Prompt for AI Agents
In src/backend/base/langflow/services/database/service.py around line 131,
issubclass(pool_class, sa.pool.Pool) can raise TypeError if pool_class is not a
class; check that pool_class is a class (e.g., using isinstance(pool_class,
type) or inspect.isclass(pool_class)) before calling issubclass, and only call
issubclass when that check passes so non-class attributes from getattr(sa,
poolclass_key, None) are ignored.
|
|
||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| import pytest |
There was a problem hiding this comment.
Remove unused pytest import.
The pytest import is not used anywhere in this test file.
Apply this diff:
-import pytest
import sqlalchemy as sa📝 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.
| import pytest | |
| import sqlalchemy as sa |
🧰 Tools
🪛 GitHub Actions: Ruff Style Check
[error] 5-5: Ruff: F401 'pytest' imported but unused.
🪛 GitHub Check: Ruff Style Check (3.13)
[failure] 5-5: Ruff (F401)
src/backend/tests/unit/services/database/test_database_service.py:5:8: F401 pytest imported but unused
🤖 Prompt for AI Agents
In src/backend/tests/unit/services/database/test_database_service.py around line
5, the file imports pytest but never uses it; remove the unused "import pytest"
line to clean up imports and avoid linter warnings.
…strings - Add pytest fixture for common mock setup - Add edge case tests for None, missing, and non-string poolclass values - Add comprehensive docstrings explaining test purpose and context - Add explicit verification that classes (not instances) are passed to engine - Improve test organization and readability
github-actions
Bot
added
bug
- Add isinstance(pool_class, type) check before issubclass to prevent TypeError - Restore pytest import needed for @pytest.fixture decorator - Addresses CodeRabbit feedback on line 131 and test file imports
github-actions
Bot
added
bug
- Add assertions for db_service to verify successful initialization - Use mock_logger and mock_create_engine in test_non_string_pool_class_value - Fixes F841 (unused variable) and ARG002 (unused argument) errors
github-actions
Bot
added
bug
- Add kwargs.pop("poolclass", None) when validation fails
- Update test to verify invalid poolclass is removed from kwargs
- Addresses CodeRabbit feedback about downstream SQLAlchemy errors
github-actions
Bot
added
bug
github-actions
Bot
added
bug
Fixes TypeError when non-string values are passed as poolclass. The test test_non_string_pool_class_value was failing because getattr() requires a string attribute name, but the code didn't validate the type before calling getattr(). This adds an isinstance() check to ensure poolclass_key is a string before attempting to use it. This addresses the CI failure in Unit Tests - Python 3.10 - Group 2.
github-actions
Bot
added
bug
|
Closing this as the fix was already merged in #10232. The main branch now uses issubclass for pool class validation. |
1 participant
Description
Fixes #10231
This PR fixes a bug where database initialization crashes when users configure custom SQLAlchemy pool classes like
NullPoolviaLANGFLOW_DB_CONNECTION_SETTINGS.Problem
The code was using
isinstance(pool_class(), sa.pool.Pool)to validate pool classes, which attempts to instantiate the pool class without required arguments. SQLAlchemy pool classes likeNullPool,StaticPool, etc. require acreatorargument during instantiation, causing this error:Solution
Changed to use
issubclass(pool_class, sa.pool.Pool)which validates class inheritance without instantiation. This is the correct and standard way to check if a class is a subclass of another class in Python.Changes
File:
src/backend/base/langflow/services/database/service.pyisinstance(pool_class(), ...)toissubclass(pool_class, ...)New File:
src/backend/tests/unit/services/database/test_database_service.pyTesting
The new test file includes 4 test cases:
test_null_pool_class_validation- Verifies NullPool can be validated without errorstest_static_pool_class_validation- Verifies StaticPool can be validated without errorstest_invalid_pool_class_logs_error- Ensures invalid pool classes log errors gracefullytest_queue_pool_class_validation- Verifies QueuePool (default) works correctlyImpact
Summary by CodeRabbit
Bug Fixes
Tests