Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis change introduces a new deployment API module with a FastAPI router and comprehensive Pydantic schemas. A new router is created at Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 4❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/backend/base/langflow/api/v1/deployments.py (1)
15-15: Prefer read-only DB dependency for GET routes.All read handlers currently inject
DbSession. Switching them toDbSessionReadOnlywill reduce unnecessary write-scoped session overhead on read paths.♻️ Proposed refactor
-from langflow.api.utils import CurrentActiveUser, DbSession +from langflow.api.utils import CurrentActiveUser, DbSession, DbSessionReadOnly @@ async def list_provider_accounts( user: CurrentActiveUser, - db: DbSession, + db: DbSessionReadOnly, ): @@ async def get_provider_account( provider_id: UUID, user: CurrentActiveUser, - db: DbSession, + db: DbSessionReadOnly, ): @@ async def list_deployment_types( provider_id: ProviderIdQuery, user: CurrentActiveUser, - db: DbSession, + db: DbSessionReadOnly, ): @@ async def list_deployments( provider_id: ProviderIdQuery, user: CurrentActiveUser, - db: DbSession, + db: DbSessionReadOnly, @@ async def get_deployment_execution( @@ - db: DbSession, + db: DbSessionReadOnly, @@ async def get_deployment( deployment_id: str, user: CurrentActiveUser, - db: DbSession, + db: DbSessionReadOnly, ): @@ async def get_deployment_status( deployment_id: str, - db: DbSession, + db: DbSessionReadOnly, user: CurrentActiveUser, ):Also applies to: 67-72, 75-85, 130-137, 140-168, 185-195, 203-210, 266-276
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/base/langflow/api/v1/deployments.py` at line 15, Import and use the read-only DB dependency instead of the write-scoped one: replace DbSession with DbSessionReadOnly in the import line (alongside CurrentActiveUser) and update all GET route handlers that currently depend on DbSession to depend on DbSessionReadOnly (e.g., any function signatures using the DbSession param). Ensure any type hints, dependency injection calls, and usages that expect a read-only session are adjusted accordingly for functions referenced in the file that serve GET endpoints.
🤖 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/api/v1/schemas/deployments.py`:
- Around line 241-248: DeploymentUpdateRequest currently permits no-op payloads
(e.g., {} or {"flow_versions": {}}); add a model-level validation to reject
empty updates by implementing a root_validator (prefer pre=True) on
DeploymentUpdateRequest that inspects incoming data and ensures at least one of
spec, flow_versions, or config is present and non-empty (treat empty
mappings/iterables as absent), and raise a ValueError with a clear message if
none are provided; reference the class name DeploymentUpdateRequest and its
fields spec, flow_versions, and config when locating where to add this
validator.
---
Nitpick comments:
In `@src/backend/base/langflow/api/v1/deployments.py`:
- Line 15: Import and use the read-only DB dependency instead of the
write-scoped one: replace DbSession with DbSessionReadOnly in the import line
(alongside CurrentActiveUser) and update all GET route handlers that currently
depend on DbSession to depend on DbSessionReadOnly (e.g., any function
signatures using the DbSession param). Ensure any type hints, dependency
injection calls, and usages that expect a read-only session are adjusted
accordingly for functions referenced in the file that serve GET endpoints.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 61cf9d0a-eb24-484a-b52b-d199f825a708
📒 Files selected for processing (5)
src/backend/base/langflow/api/router.pysrc/backend/base/langflow/api/v1/__init__.pysrc/backend/base/langflow/api/v1/deployments.pysrc/backend/base/langflow/api/v1/schemas/__init__.pysrc/backend/base/langflow/api/v1/schemas/deployments.py
| class DeploymentUpdateRequest(BaseModel): | ||
| spec: BaseDeploymentDataUpdate | None = Field(default=None, description="Deployment metadata updates.") | ||
| flow_versions: FlowVersionsPatch | None = Field( | ||
| default=None, | ||
| description="Flow version attach/detach patch payload.", | ||
| ) | ||
| config: ConfigDeploymentBindingUpdate | None = Field(default=None, description="Deployment config binding patch.") | ||
|
|
There was a problem hiding this comment.
Reject no-op deployment update payloads.
DeploymentUpdateRequest currently accepts payloads that perform no effective change (e.g. {} or {"flow_versions": {}}). This should fail validation to keep PATCH semantics explicit.
🔧 Proposed fix
class DeploymentUpdateRequest(BaseModel):
spec: BaseDeploymentDataUpdate | None = Field(default=None, description="Deployment metadata updates.")
flow_versions: FlowVersionsPatch | None = Field(
default=None,
description="Flow version attach/detach patch payload.",
)
config: ConfigDeploymentBindingUpdate | None = Field(default=None, description="Deployment config binding patch.")
+
+ `@model_validator`(mode="after")
+ def ensure_any_field_provided(self) -> DeploymentUpdateRequest:
+ has_flow_version_changes = bool(
+ self.flow_versions and (self.flow_versions.add or self.flow_versions.remove)
+ )
+ if self.spec is None and self.config is None and not has_flow_version_changes:
+ msg = "At least one update field must be provided."
+ raise ValueError(msg)
+ return self🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/base/langflow/api/v1/schemas/deployments.py` around lines 241 -
248, DeploymentUpdateRequest currently permits no-op payloads (e.g., {} or
{"flow_versions": {}}); add a model-level validation to reject empty updates by
implementing a root_validator (prefer pre=True) on DeploymentUpdateRequest that
inspects incoming data and ensures at least one of spec, flow_versions, or
config is present and non-empty (treat empty mappings/iterables as absent), and
raise a ValueError with a clear message if none are provided; reference the
class name DeploymentUpdateRequest and its fields spec, flow_versions, and
config when locating where to add this validator.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12041 +/- ##
==========================================
- Coverage 37.45% 37.36% -0.10%
==========================================
Files 1616 1617 +1
Lines 79060 79067 +7
Branches 11946 11946
==========================================
- Hits 29614 29540 -74
- Misses 47788 47870 +82
+ Partials 1658 1657 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ebd3f7b to
962f04f
Compare
- Strip internal implementation details from public-facing Field descriptions and endpoint docstrings (adapter routing, provider snapshots, binding payloads, lazy sync) - Rename flow_versions to flow_version_ids in DeploymentCreateRequest and DeploymentUpdateRequest for clarity - Remove redundant match_limit query parameter from list_deployments; page size already caps results
The API schema file inherited from and directly embedded service-layer types whose identifier fields use IdLike (UUID | str) for provider-owned IDs. This conflated provider-owned opaque identifiers with Langflow DB-managed UUIDs at the API boundary. - Rewrite RedeployResponse as standalone BaseModel instead of inheriting from DeploymentOperationResult (which carried id: IdLike) - Replace ConfigItem with API-local DeploymentConfigCreate (reference_id: str, explicitly provider-owned) - Replace ConfigDeploymentBindingUpdate with API-local DeploymentConfigBindingUpdate (config_id: str, not IdLike union) - Add module docstring documenting the two identifier domains (Langflow DB UUIDs vs provider-owned opaque strings) - Annotate all id/provider_* fields with their ownership domain - Keep BaseDeploymentData/BaseDeploymentDataUpdate imports (no ID fields, stable shapes)
d5b948e to
bff15e2
Compare
introduce the deployments api and schemas. No endpoints are implemented.
Moves all schemas into a dedicated folder. Creates a dedicated schemas file for the deployments api.
Summary by CodeRabbit
Release Notes