Open
Conversation
Contributor
|
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 Use the checkbox below for a quick retry:
WalkthroughThis PR introduces a complete deployment management system with deployment provider accounts, featuring SQLModel-based data models, database migrations, comprehensive CRUD operations with API key encryption, validation utilities, and extensive test coverage for the new entities and their interactions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 3❌ Failed checks (3 warnings)
✅ 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 |
Contributor
|
✅ Migration Validation Passed All migrations follow the Expand-Contract pattern correctly. |
Contributor
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12011 +/- ##
==========================================
- Coverage 37.31% 37.31% -0.01%
==========================================
Files 1592 1596 +4
Lines 78280 78594 +314
Branches 11824 11824
==========================================
+ Hits 29212 29328 +116
- Misses 47448 47646 +198
Partials 1620 1620
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/backend/base/langflow/tests/services/database/models/deployment_provider_account/test_crud.py (1)
22-43: Consider adding a small DB-backed test layer alongside these unit mocks.Most assertions are mock-interaction based, so SQL/ORM integration regressions can still slip through.
As per coding guidelines: "Warn when mocks are used instead of testing real behavior and interactions, and suggest using real objects or test doubles when mocks become excessive."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/base/langflow/tests/services/database/models/deployment_provider_account/test_crud.py` around lines 22 - 43, The tests rely solely on mocks (_make_db and _make_provider_account) which can miss ORM/integration regressions; add a small DB-backed test layer by creating an AsyncSession test fixture (e.g., in-memory SQLite or the project's test database) and a helper that persists a real DeploymentProviderAccount model instance instead of returning a MagicMock from _make_provider_account, and use that session in at least one CRUD test to assert real DB behavior; update or add a helper to produce a real AsyncSession (replacing _make_db or adding a new fixture) and ensure the test cleans up transactions after each run.src/backend/base/langflow/alembic/versions/2a5defa5ddc0_add_deployment_table.py (1)
62-75: Consider adding a composite index for the paginated listing query.Current single-column indexes may degrade for large per-user/provider datasets when filtering by
(user_id, deployment_provider_account_id)and ordering by(created_at, id).📈 Suggested migration diff
@@ with op.batch_alter_table("deployment", schema=None) as batch_op: + batch_op.create_index( + batch_op.f("ix_deployment_user_provider_created_id"), + ["user_id", "deployment_provider_account_id", "created_at", "id"], + unique=False, + ) batch_op.create_index(batch_op.f("ix_deployment_name"), ["name"], unique=False) @@ with op.batch_alter_table("deployment", schema=None) as batch_op: + batch_op.drop_index(batch_op.f("ix_deployment_user_provider_created_id")) batch_op.drop_constraint(RESOURCE_KEY_UNIQUE_CONSTRAINT, type_="unique")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/base/langflow/alembic/versions/2a5defa5ddc0_add_deployment_table.py` around lines 62 - 75, Add a composite index to speed paginated listing queries: inside the op.batch_alter_table("deployment", schema=None) block (the same place where batch_op.create_index and create_unique_constraint are called), add a non-unique composite index on ["user_id", "deployment_provider_account_id", "created_at", "id"] (use batch_op.f(...) to name it, e.g., ix_deployment_user_provider_created_id) so queries filtering by user_id and deployment_provider_account_id and ordering by created_at,id are covered; keep the existing unique constraints NAME_UNIQUE_CONSTRAINT and RESOURCE_KEY_UNIQUE_CONSTRAINT unchanged.src/backend/base/langflow/tests/services/database/models/deployment/test_crud.py (1)
31-31: Remove 20 redundant@pytest.mark.asynciodecorators throughout this module.With
asyncio_mode = "auto"enabled in pyproject.toml, async tests are automatically detected and these decorators add unnecessary noise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/base/langflow/tests/services/database/models/deployment/test_crud.py` at line 31, The test module contains 20 redundant uses of the pytest.mark.asyncio decorator (e.g., decorators applied to async test functions in src/backend/base/langflow/tests/services/database/models/deployment/test_crud.py); since asyncio_mode = "auto" is enabled, remove the unnecessary `@pytest.mark.asyncio` decorators from the async test functions (leave the async def test_* functions intact) so tests rely on automatic detection rather than the explicit decorator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/backend/base/langflow/services/database/models/deployment_provider_account/crud.py`:
- Around line 98-103: The create path currently sets provider_tenant_id to
provider_tenant_id.strip() which preserves empty string; change the creation
logic that builds the DeploymentProviderAccount (where provider_tenant_id is
assigned) to normalize blank values to None (e.g., if provider_tenant_id is not
None after strip and != "" else None) so it matches the update logic's
normalization; update the assignment that currently reads
provider_tenant_id=provider_tenant_id.strip() if provider_tenant_id is not None
else None to return None for blank strings.
- Around line 110-113: The IntegrityError handlers currently pass the raw
exception object to logger.aerror (logger.aerror("IntegrityError creating
provider account: %s", exc)), which can leak SQL and sensitive parameters;
update each handler (the except IntegrityError as exc blocks that call
logger.aerror and compute err_detail) to not log exc directly—instead log a
generic, non-sensitive message and include only the sanitized err_detail string
(or a redacted/safe error token) along with context such as the operation
("creating provider account") and ensure db.rollback() remains; replace all uses
of logger.aerror(..., exc) in these IntegrityError handlers (the blocks that set
err_detail) with logger.aerror(..., err_detail) or a redacted constant.
In `@src/backend/base/langflow/services/database/models/deployment/crud.py`:
- Around line 89-106: In list_deployments_page validate the pagination params
before building the query: check the offset and limit parameters (used in
list_deployments_page signature) and guard against negative offset and
non-positive limit (e.g., raise a ValueError or return an empty list) so the DB
call using AsyncSession and the select(Deployment)...offset(offset).limit(limit)
chain never receives invalid values; put the checks at the top of
list_deployments_page to fail fast and document the behavior.
In `@src/backend/base/langflow/services/database/models/folder/model.py`:
- Around line 9-13: Move the Flow symbol out of the runtime import and into the
TYPE_CHECKING block: keep FlowRead imported at top-level from
langflow.services.database.models.flow.model, remove Flow from that runtime
import, and add "from langflow.services.database.models.flow.model import Flow"
inside the existing TYPE_CHECKING block (alongside Deployment). Update any type
annotations if needed to use Flow directly (no stringization required if
TYPE_CHECKING is present).
In `@src/backend/base/langflow/services/database/utils.py`:
- Around line 100-117: The parse_uuid function currently lets non-str/non-UUID
values pass through; update parse_uuid to explicitly handle UUID instances and
raise a TypeError for any unsupported runtime types: if isinstance(value, UUID)
return it, if isinstance(value, str) keep the existing logic, otherwise raise a
TypeError that includes the field_name and the actual type (e.g., f"{field_name}
must be a UUID or string, got {type(value).__name__}"), so callers get a clear,
immediate error instead of downstream failures.
In
`@src/backend/base/langflow/tests/services/database/models/deployment_provider_account/test_crud.py`:
- Around line 379-381: Update the failing multiline docstrings so they include a
blank line between the one-line summary and the rest of the description to
satisfy Ruff D205: locate the docstring that begins "When provider_tenant_id is
not passed at all (_UNSET), the existing ..." in test_crud.py (and the other
similar docstring around the second occurrence) and insert an empty line after
the summary line so the docstring has a single-line summary, a blank line, then
the detailed description.
---
Nitpick comments:
In
`@src/backend/base/langflow/alembic/versions/2a5defa5ddc0_add_deployment_table.py`:
- Around line 62-75: Add a composite index to speed paginated listing queries:
inside the op.batch_alter_table("deployment", schema=None) block (the same place
where batch_op.create_index and create_unique_constraint are called), add a
non-unique composite index on ["user_id", "deployment_provider_account_id",
"created_at", "id"] (use batch_op.f(...) to name it, e.g.,
ix_deployment_user_provider_created_id) so queries filtering by user_id and
deployment_provider_account_id and ordering by created_at,id are covered; keep
the existing unique constraints NAME_UNIQUE_CONSTRAINT and
RESOURCE_KEY_UNIQUE_CONSTRAINT unchanged.
In
`@src/backend/base/langflow/tests/services/database/models/deployment_provider_account/test_crud.py`:
- Around line 22-43: The tests rely solely on mocks (_make_db and
_make_provider_account) which can miss ORM/integration regressions; add a small
DB-backed test layer by creating an AsyncSession test fixture (e.g., in-memory
SQLite or the project's test database) and a helper that persists a real
DeploymentProviderAccount model instance instead of returning a MagicMock from
_make_provider_account, and use that session in at least one CRUD test to assert
real DB behavior; update or add a helper to produce a real AsyncSession
(replacing _make_db or adding a new fixture) and ensure the test cleans up
transactions after each run.
In
`@src/backend/base/langflow/tests/services/database/models/deployment/test_crud.py`:
- Line 31: The test module contains 20 redundant uses of the pytest.mark.asyncio
decorator (e.g., decorators applied to async test functions in
src/backend/base/langflow/tests/services/database/models/deployment/test_crud.py);
since asyncio_mode = "auto" is enabled, remove the unnecessary
`@pytest.mark.asyncio` decorators from the async test functions (leave the async
def test_* functions intact) so tests rely on automatic detection rather than
the explicit decorator.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/backend/base/langflow/alembic/versions/2a5defa5ddc0_add_deployment_table.pysrc/backend/base/langflow/alembic/versions/8106300be7aa_add_deployment_provider_account_table.pysrc/backend/base/langflow/services/database/models/__init__.pysrc/backend/base/langflow/services/database/models/deployment/__init__.pysrc/backend/base/langflow/services/database/models/deployment/crud.pysrc/backend/base/langflow/services/database/models/deployment/model.pysrc/backend/base/langflow/services/database/models/deployment_provider_account/__init__.pysrc/backend/base/langflow/services/database/models/deployment_provider_account/crud.pysrc/backend/base/langflow/services/database/models/deployment_provider_account/model.pysrc/backend/base/langflow/services/database/models/folder/model.pysrc/backend/base/langflow/services/database/models/folder/pagination_model.pysrc/backend/base/langflow/services/database/models/user/model.pysrc/backend/base/langflow/services/database/utils.pysrc/backend/base/langflow/tests/services/database/models/deployment/__init__.pysrc/backend/base/langflow/tests/services/database/models/deployment/test_crud.pysrc/backend/base/langflow/tests/services/database/models/deployment/test_model.pysrc/backend/base/langflow/tests/services/database/models/deployment_provider_account/__init__.pysrc/backend/base/langflow/tests/services/database/models/deployment_provider_account/test_crud.pysrc/backend/base/langflow/tests/services/database/models/deployment_provider_account/test_model.pysrc/backend/base/langflow/tests/services/database/models/test_parse_uuid.py
src/backend/base/langflow/services/database/models/deployment_provider_account/crud.py
Show resolved
Hide resolved
src/backend/base/langflow/services/database/models/deployment_provider_account/crud.py
Outdated
Show resolved
Hide resolved
src/backend/base/langflow/services/database/models/folder/model.py
Outdated
Show resolved
Hide resolved
...ackend/base/langflow/tests/services/database/models/deployment_provider_account/test_crud.py
Outdated
Show resolved
Hide resolved
Critical Issues Fixed 1. is_encrypted removed — update_provider_account now always encrypts, matching create_provider_account. No more heuristic that could store plaintext keys. 2. Proper Alembic revision IDs — a1b2c3d4e5f6 → 8106300be7aa, c3d4e5f6a7b8 → 2a5defa5ddc0 (randomly generated). 3. Folder ↔ Deployment relationship — Added folder on Deployment and deployments on Folder with "all, delete, delete-orphan" cascade. Important Issues Fixed 4. Schema layering — Added DeploymentRead, DeploymentProviderAccountCreate, DeploymentProviderAccountRead (no api_key!), DeploymentProviderAccountUpdate. 5. Cascade config — DeploymentProviderAccount.deployments now uses "all, delete, delete-orphan" (matching Folder.flows pattern). 6. UUID validation standardized — Both CRUDs now use _parse_uuid() that raises ValueError with context (field name + value). No moent return None/return 0 on bad input. 7. encrypt_api_key error context — Wrapped in try/except raising RuntimeError with clear message about encryption config. Suggestions Fixed 8. Field validators — name, resource_key (Deployment) and provider_key, provider_url (DeploymentProviderAccount) now validated non-empty with .strip(). 9. (provider_account_id, resource_key) uniqueness — Added to both model and migration. 10. account_id NULL documented — Comment explaining unique constraint behavior with NULLs. 11. or 0 removed from count_deployment_rows.
…persistence - Extract shared validators (validate_non_empty_string) to database/utils.py - Add DeploymentCreate schema with field validators - Add _UNSET sentinel to update_provider_account for nullable field handling - Extract _encrypt_api_key helper with broadened exception handling - Add empty-string validation in CRUD create functions before DB round-trip - Escalate IntegrityError and None rowcount logging to aerror with rollback - Fix parse_uuid to chain exceptions with `from exc` - Fix folder relationship nullability (Folder, not Folder | None) - Add tests for 5 previously untested CRUD functions and new validation paths
- Rename count_deployments to count_deployments_by_provider - Add update_deployment CRUD function and DeploymentUpdate schema - Extract _strip_or_raise helper to deduplicate input validation - Clarify IntegrityError messages to describe conflicts generically - Document cascade semantics on User model relationships, double- validation rationale in CRUD, and model_fields_set usage on DeploymentProviderAccountUpdate - Add missing get_deployment and update_deployment tests
HzaRashid
force-pushed
the
wxo-persistence
branch
from
3293607 to
fc421c2
Compare
…atibility - Validate pagination bounds (offset >= 0, limit > 0) in list_deployments_page - Normalize blank provider_tenant_id to None on create, matching update behavior - Centralize normalization via normalize_string_or_none utility and model validators - Remove raw exception objects from IntegrityError log messages to avoid leaking SQL - Reject unsupported types in parse_uuid with a clear TypeError - Remove `from __future__ import annotations` from table models to fix SQLAlchemy relationship mapper errors at runtime
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
2 participants
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 by CodeRabbit
New Features
Tests