fix: ValidationError reduction on OSS and Desktop#12920
Open
olayinkaadelakun wants to merge 4 commits intorelease-1.10.0from
Open
fix: ValidationError reduction on OSS and Desktop#12920olayinkaadelakun wants to merge 4 commits intorelease-1.10.0from
olayinkaadelakun wants to merge 4 commits intorelease-1.10.0from
Conversation
Contributor
|
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 PR introduces validation error handling in the flow upload endpoint to return HTTP 422 for Pydantic validation failures, and expands the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-1.10.0 #12920 +/- ##
==================================================
- Coverage 53.82% 53.82% -0.01%
==================================================
Files 2045 2051 +6
Lines 185870 187236 +1366
Branches 27938 26628 -1310
==================================================
+ Hits 100041 100771 +730
- Misses 84728 85355 +627
- Partials 1101 1110 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
|
Build successful! ✅ |
Contributor
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
Collaborator
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
![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: 3
🤖 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/flows.py`:
- Around line 441-448: The handler assumes 'data' is a mapping which causes a
TypeError when uploaded JSON is a list or scalar; update the block around
FlowListCreate / FlowCreate and normalize_code_for_import to first validate that
data is a dict (e.g., isinstance(data, dict)) and if not return/raise an
HTTPException(status_code=422) with a clear message, otherwise proceed with the
existing "flows" in data branching and construction of FlowListCreate/FlowCreate
so that non-object payloads are handled before any ** unpacking occurs and the
existing ValidationError catch still applies.
In `@src/backend/tests/unit/api/v1/test_flows.py`:
- Around line 676-694: The test test_upload_flow_accepts_valid_endpoint_name
currently only asserts the response is not 500 or 422 which can hide other
failures; change it to assert the explicit expected success status (e.g.,
response.status_code == status.HTTP_200_OK or HTTP_201_CREATED depending on the
API) and validate basic response shape by checking the JSON payload contains
expected keys (parse response.json() and assert presence/values for keys like
"folder_name" or a list of uploaded flows or a success flag). Locate the
client.post call and replace the negative assertions with a positive status
assertion and one or two lightweight JSON checks to confirm the upload
succeeded.
In
`@src/backend/tests/unit/services/database/models/traces/test_enum_serialization.py`:
- Around line 165-167: Add a symmetric test to cover arbitrary-string values for
the output field: create a new test (e.g.,
test_should_accept_arbitrary_string_as_output) that constructs
TraceSummaryRead(**{**_TRACE_SUMMARY_DEFAULTS, "output": "plain string"}) and
asserts summary.output == "plain string"; this mirrors the existing
test_should_accept_arbitrary_string_as_input and ensures the widened type for
output (str | dict | None) accepts plain strings without regression.
🪄 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: 3a239f74-f39c-4293-b062-b1a1638bfae9
📒 Files selected for processing (4)
src/backend/base/langflow/api/v1/flows.pysrc/backend/base/langflow/services/database/models/traces/model.pysrc/backend/tests/unit/api/v1/test_flows.pysrc/backend/tests/unit/services/database/models/traces/test_enum_serialization.py
Comment on lines
+441
to
+448
| try: | ||
| if "flows" in data: | ||
| data = {**data, "flows": [normalize_code_for_import(f) for f in data["flows"]]} | ||
| flow_list = FlowListCreate(**data) | ||
| else: | ||
| flow_list = FlowListCreate(flows=[FlowCreate(**normalize_code_for_import(data))]) | ||
| except ValidationError as e: | ||
| raise HTTPException(status_code=422, detail=str(e)) from e |
Contributor
There was a problem hiding this comment.
Handle non-object upload payloads to avoid fallback 500s
At Line 442, "flows" in data assumes data is a mapping. If the uploaded JSON is a list/scalar, the else branch can raise TypeError (** on non-mapping), which bypasses this ValidationError handler and can still return 500.
Proposed fix
# Normalise code fields: if exported with code-as-lines format, rejoin to
# strings before creating the Pydantic models so the DB always stores strings.
try:
+ if not isinstance(data, dict):
+ raise HTTPException(status_code=422, detail="Invalid payload: expected a JSON object")
if "flows" in data:
+ if not isinstance(data["flows"], list):
+ raise HTTPException(status_code=422, detail="Invalid payload: 'flows' must be a list")
data = {**data, "flows": [normalize_code_for_import(f) for f in data["flows"]]}
flow_list = FlowListCreate(**data)
else:
flow_list = FlowListCreate(flows=[FlowCreate(**normalize_code_for_import(data))])
except ValidationError as e:
raise HTTPException(status_code=422, detail=str(e)) from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/base/langflow/api/v1/flows.py` around lines 441 - 448, The
handler assumes 'data' is a mapping which causes a TypeError when uploaded JSON
is a list or scalar; update the block around FlowListCreate / FlowCreate and
normalize_code_for_import to first validate that data is a dict (e.g.,
isinstance(data, dict)) and if not return/raise an
HTTPException(status_code=422) with a clear message, otherwise proceed with the
existing "flows" in data branching and construction of FlowListCreate/FlowCreate
so that non-object payloads are handled before any ** unpacking occurs and the
existing ValidationError catch still applies.
Comment on lines
+676
to
+694
| async def test_upload_flow_accepts_valid_endpoint_name(client: AsyncClient, logged_in_headers): | ||
| """Endpoint names with only letters, numbers, hyphens, and underscores are accepted.""" | ||
| import json | ||
|
|
||
| flow_data = { | ||
| "name": "neuro-vision", | ||
| "data": {}, | ||
| "endpoint_name": "neuro-vision-planning_phase1", | ||
| } | ||
| file_content = json.dumps({"folder_name": "proj", "flows": [flow_data]}) | ||
|
|
||
| response = await client.post( | ||
| "api/v1/flows/upload/", | ||
| files={"file": ("flows.json", file_content, "application/json")}, | ||
| headers=logged_in_headers, | ||
| ) | ||
| assert response.status_code != status.HTTP_500_INTERNAL_SERVER_ERROR | ||
| assert response.status_code != status.HTTP_422_UNPROCESSABLE_ENTITY | ||
|
|
Contributor
There was a problem hiding this comment.
Strengthen the success assertion to prevent false positives
At Lines 692-693, != 500 and != 422 can still pass for other failures. Please assert the expected success code (and ideally basic response shape) for a valid payload.
Proposed fix
response = await client.post(
"api/v1/flows/upload/",
files={"file": ("flows.json", file_content, "application/json")},
headers=logged_in_headers,
)
- assert response.status_code != status.HTTP_500_INTERNAL_SERVER_ERROR
- assert response.status_code != status.HTTP_422_UNPROCESSABLE_ENTITY
+ assert response.status_code == status.HTTP_201_CREATED
+ result = response.json()
+ assert isinstance(result, list)
+ assert len(result) == 1
+ assert result[0]["endpoint_name"] == "neuro-vision-planning_phase1"Based on learnings: Applies to **/test_*.py : For API endpoints, verify both success and error response testing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/tests/unit/api/v1/test_flows.py` around lines 676 - 694, The test
test_upload_flow_accepts_valid_endpoint_name currently only asserts the response
is not 500 or 422 which can hide other failures; change it to assert the
explicit expected success status (e.g., response.status_code ==
status.HTTP_200_OK or HTTP_201_CREATED depending on the API) and validate basic
response shape by checking the JSON payload contains expected keys (parse
response.json() and assert presence/values for keys like "folder_name" or a list
of uploaded flows or a success flag). Locate the client.post call and replace
the negative assertions with a positive status assertion and one or two
lightweight JSON checks to confirm the upload succeeded.
Comment on lines
+165
to
+167
| def test_should_accept_arbitrary_string_as_input(self): | ||
| summary = TraceSummaryRead(**{**_TRACE_SUMMARY_DEFAULTS, "input": "plain string"}) | ||
| assert summary.input == "plain string" |
Contributor
There was a problem hiding this comment.
Add the symmetric arbitrary-string case for output.
Line 165 adds arbitrary string coverage for input, but output (also widened to str | dict | None) doesn’t have the same regression guard.
Suggested test addition
class TestTraceSummaryReadIoFields:
@@
def test_should_accept_arbitrary_string_as_input(self):
summary = TraceSummaryRead(**{**_TRACE_SUMMARY_DEFAULTS, "input": "plain string"})
assert summary.input == "plain string"
+ def test_should_accept_arbitrary_string_as_output(self):
+ summary = TraceSummaryRead(**{**_TRACE_SUMMARY_DEFAULTS, "output": "plain string"})
+ assert summary.output == "plain string"
+
def test_should_default_input_and_output_to_none(self):
summary = TraceSummaryRead(**_TRACE_SUMMARY_DEFAULTS)
assert summary.input is None
assert summary.output is NoneAs per coding guidelines: “Verify tests cover both positive and negative scenarios where appropriate.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/backend/tests/unit/services/database/models/traces/test_enum_serialization.py`
around lines 165 - 167, Add a symmetric test to cover arbitrary-string values
for the output field: create a new test (e.g.,
test_should_accept_arbitrary_string_as_output) that constructs
TraceSummaryRead(**{**_TRACE_SUMMARY_DEFAULTS, "output": "plain string"}) and
asserts summary.output == "plain string"; this mirrors the existing
test_should_accept_arbitrary_string_as_input and ensures the widened type for
output (str | dict | None) accepts plain strings without regression.
github-actions
Bot
added
bug
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
1 participant
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
Release Notes
Bug Fixes
Tests