feat: revise deployment schemas#12150
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 pull request introduces deployment type routing hints, restructures deployment binding schemas to support raw payload creation workflows, enhances UUID handling in flow version attachments, and adds provider-specific update data support across deployment service interfaces and schemas. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 |
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (44.25%) is below the target coverage (60.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #12150 +/- ##
=======================================
Coverage 38.22% 38.23%
=======================================
Files 1630 1630
Lines 80158 80188 +30
Branches 12112 12114 +2
=======================================
+ Hits 30638 30656 +18
- Misses 47784 47795 +11
- Partials 1736 1737 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
- Add exactly-one validation to ConfigDeploymentBindingUpdate (both layers) using model_fields_set XOR for config_id vs raw_payload - Add raw_payload field to service-layer ConfigDeploymentBindingUpdate for symmetry with the snapshot passthrough pattern - Add deployment_type routing hint to execution methods (protocol, ABC, service) - Unify duplicate handling: silent dedup everywhere (dict.fromkeys) - Fix broken API tests after FlowVersionsAttach/Patch str→UUID migration - Restore deleted test coverage (blank ids, order-preserving dedup) and add new tests for raw_payload, mutual exclusion, snapshot_ids, and noop rejection - Add clarifying comments: overlap check ID domains, post-validation types, deployment_type scope in protocol docstring
a6020a1 to
99ac50c
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lfx/src/lfx/services/adapters/deployment/schema.py (1)
96-145:⚠️ Potential issue | 🟠 MajorAdd
model_config = ConfigDict(extra="forbid")to prevent silent field drops.This class currently accepts Pydantic's default
extra='ignore'behavior, which silently drops unknown fields. With the field rename from older names toadd_ids/remove_ids, mixed payloads with typos or stale field names will partially apply without error—e.g.,{"add_ids": ["snap_1"], "remove": ["snap_2"]}validates and only performs the attach, silently ignoring the malformedremovefield.Adding the config option (as is already done for
SnapshotItemsin this file) will fail fast on extra fields:🛠️ Proposed fix
class SnapshotDeploymentBindingUpdate(BaseModel): """Snapshot deployment binding patch payload. Supports three operations: bind existing snapshots by ID, create new snapshots from raw payloads, or unbind snapshots by ID. At least one of the three fields must be provided. """ + model_config = ConfigDict(extra="forbid") add_ids: list[IdLike] | None = Field(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lfx/src/lfx/services/adapters/deployment/schema.py` around lines 96 - 145, The model currently allows unknown fields to be silently ignored; add a Pydantic config to forbid extras by defining model_config = ConfigDict(extra="forbid") on this model class (the one that declares add_ids, add_raw_payloads, remove_ids and contains the `@field_validator` validate_id_lists and `@model_validator` validate_operations methods) so that malformed or stale fields (e.g., "remove") raise validation errors instead of being dropped; mirror the pattern used on SnapshotItems in this file.
🤖 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 478-483: The current ensure_any_field_provided model_validator
only checks model_fields_set, so payloads like {"spec": null} still pass; update
it to ensure at least one of the tracked fields ('spec', 'flow_version_ids',
'config', 'provider_data') is both present and not None/empty. In
ensure_any_field_provided, iterate the specific attributes (self.spec,
self.flow_version_ids, self.config, self.provider_data) or check their presence
in self.model_fields_set and that their value is not None (and not empty where
applicable), and raise the same ValueError if none of these contain a meaningful
value; return self otherwise.
In `@src/backend/tests/unit/api/v1/test_deployment_schemas.py`:
- Around line 103-111: The tests test_flow_versions_patch_deduplicates_add and
test_flow_versions_patch_deduplicates_remove currently only use a single UUID so
they don't detect reordering (a set-based dedup would pass). Update both tests
to use two distinct UUIDs with interleaved duplicates (e.g. u1, u2, u1) when
constructing FlowVersionsPatch so we assert that duplicates are removed but
original order of first occurrences is preserved (expect result.add == [u1, u2]
and result.remove == [u1, u2] respectively). Ensure you reference
FlowVersionsPatch in the updated assertions.
- Around line 130-133: The test test_accepts_raw_payload_only currently only
checks non-nullity; change it to assert exact passthrough by constructing a
representative nested dict (e.g., nested lists/dicts, extra keys) and creating
DeploymentConfigBindingUpdate(raw_payload=that_dict), then assert
update.raw_payload == that_dict and update.config_id is None so the schema truly
preserves the payload unchanged; update the test name or docstring if needed to
reflect exact equality check.
In `@src/lfx/src/lfx/services/adapters/deployment/schema.py`:
- Around line 410-414: The validator validate_has_changes currently relies on
model_fields_set and can allow fields explicitly set to None (e.g., {"spec":
null}) to pass; update it to perform explicit None checks against the instance
attributes (e.g., self.spec, self.snapshot, self.config, self.provider_data)
using "is not None" so only genuinely provided non-null values count as changes;
if all four fields are None (or absent) raise the same ValueError. Follow the
same explicit None-check pattern used in BaseDeploymentDataUpdate,
SnapshotDeploymentBindingUpdate, and ConfigDeploymentBindingUpdate when
implementing this logic in validate_has_changes.
---
Outside diff comments:
In `@src/lfx/src/lfx/services/adapters/deployment/schema.py`:
- Around line 96-145: The model currently allows unknown fields to be silently
ignored; add a Pydantic config to forbid extras by defining model_config =
ConfigDict(extra="forbid") on this model class (the one that declares add_ids,
add_raw_payloads, remove_ids and contains the `@field_validator` validate_id_lists
and `@model_validator` validate_operations methods) so that malformed or stale
fields (e.g., "remove") raise validation errors instead of being dropped; mirror
the pattern used on SnapshotItems in this file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2b556f1f-30fb-4585-b0df-3904663687a0
📒 Files selected for processing (8)
.secrets.baselinesrc/backend/base/langflow/api/v1/schemas/deployments.pysrc/backend/tests/unit/api/v1/test_deployment_schemas.pysrc/lfx/src/lfx/services/adapters/deployment/base.pysrc/lfx/src/lfx/services/adapters/deployment/schema.pysrc/lfx/src/lfx/services/adapters/deployment/service.pysrc/lfx/src/lfx/services/interfaces.pysrc/lfx/tests/unit/services/deployment/test_deployment_schema.py
HimavarshaVS
left a comment
There was a problem hiding this comment.
minor comment , but not a blocker
adjusts the deployment schemas in the api and deployment adapter to allow raw payload passthrough of Deployment Configurations and Flow Artifacts in the e2e patch operation path. Introduces a breaking rename of the fields in the
SnapshotDeploymentBindingUpdateschema.Summary by CodeRabbit
New Features
Improvements