feat: Add PUT endpoint for projects upsert#11391
Open
ogabrielluiz wants to merge 2 commits into
Open
Conversation
Contributor
WalkthroughAdded a PUT endpoint ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (5 passed)
✨ 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
Additional details and impacted files@@ Coverage Diff @@
## main #11391 +/- ##
==========================================
+ Coverage 34.54% 34.56% +0.02%
==========================================
Files 1415 1415
Lines 67401 67450 +49
Branches 9937 9937
==========================================
+ Hits 23282 23313 +31
- Misses 42889 42908 +19
+ Partials 1230 1229 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
github-actions
Bot
added
enhancement
![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: 2
🤖 Fix all issues with AI agents
In `@src/backend/base/langflow/api/v1/projects.py`:
- Around line 531-549: The PUT-create path in _create_new_project currently only
constructs and persists the Folder model, so projects created via PUT skip the
POST path's auth defaults and MCP registration; update _create_new_project to
apply the same initialization logic used in create_project: after validating
uniqueness and before session.add, set auth defaults (e.g., enable API-key auth
when AUTO_LOGIN is false) and invoke the same MCP auto-registration routine used
by create_project (or call a new shared helper that encapsulates both the
API-key enabling logic and MCP registration), ensuring the code references the
same Folder model handling (Folder.model_validate) and reuses/create a helper to
avoid duplication.
- Around line 551-556: The code currently updates flows by IDs from
project.flows_list and project.components_list without checking ownership;
change the logic to first fetch/count Flow rows matching Flow.id.in_(flow_ids)
AND Flow.user_id == current_user.id (or your request user variable), compare the
count/IDs returned to the requested flow_ids and if any requested ID is missing
raise HTTPException(status_code=404), and only then perform the update
constrained to those same ownership-filtered rows (e.g.,
update(Flow).where(and_(Flow.id.in_(flow_ids), Flow.user_id ==
current_user.id)).values(folder_id=new_project.id)); ensure you reference
project.flows_list, project.components_list, Flow, new_project.id and the
authenticated user variable in the fix.
🧹 Nitpick comments (1)
src/backend/tests/unit/api/v1/test_projects.py (1)
221-265: Make other‑user cleanup resilient to test failures.If an assertion fails before the cleanup block, the fixed username can remain in the DB and cause collisions in later runs. Wrap setup/teardown in
try/finallyand use a unique username.♻️ Suggested tweak
async def test_upsert_project_returns_404_for_other_users_project(client: AsyncClient, logged_in_headers): """Test that PUT returns 404 when trying to upsert another user's project (avoids leaking existence).""" from langflow.services.auth.utils import get_password_hash from langflow.services.database.models.user.model import User # Create another user other_user_id = uuid.uuid4() - async with session_scope() as session: - other_user = User( - id=other_user_id, - username="other_user_for_project_upsert_test", - password=get_password_hash("testpassword"), - is_active=True, - is_superuser=False, - ) - session.add(other_user) - await session.commit() - - # Login as other user and create a project - login_data = { - "username": "other_user_for_project_upsert_test", - "password": "testpassword", # pragma: allowlist secret - } - login_response = await client.post("api/v1/login", data=login_data) - assert login_response.status_code == status.HTTP_200_OK, f"Login failed: {login_response.text}" - other_user_headers = {"Authorization": f"Bearer {login_response.json()['access_token']}"} - - project_data = {"name": "other_user_project", "description": ""} - create_response = await client.post("api/v1/projects/", json=project_data, headers=other_user_headers) - other_user_project_id = create_response.json()["id"] - - # Try to upsert other user's project with original user's credentials - update_data = {"name": "trying_to_steal", "description": ""} - response = await client.put(f"api/v1/projects/{other_user_project_id}", json=update_data, headers=logged_in_headers) - - assert response.status_code == status.HTTP_404_NOT_FOUND - assert "not found" in response.json()["detail"].lower() - - # Cleanup - async with session_scope() as session: - user = await session.get(User, other_user_id) - if user: - await session.delete(user) - await session.commit() + other_username = f"other_user_for_project_upsert_test_{other_user_id}" + try: + async with session_scope() as session: + other_user = User( + id=other_user_id, + username=other_username, + password=get_password_hash("testpassword"), + is_active=True, + is_superuser=False, + ) + session.add(other_user) + await session.commit() + + # Login as other user and create a project + login_data = { + "username": other_username, + "password": "testpassword", # pragma: allowlist secret + } + login_response = await client.post("api/v1/login", data=login_data) + assert login_response.status_code == status.HTTP_200_OK, f"Login failed: {login_response.text}" + other_user_headers = {"Authorization": f"Bearer {login_response.json()['access_token']}"} + + project_data = {"name": "other_user_project", "description": ""} + create_response = await client.post("api/v1/projects/", json=project_data, headers=other_user_headers) + other_user_project_id = create_response.json()["id"] + + # Try to upsert other user's project with original user's credentials + update_data = {"name": "trying_to_steal", "description": ""} + response = await client.put( + f"api/v1/projects/{other_user_project_id}", json=update_data, headers=logged_in_headers + ) + + assert response.status_code == status.HTTP_404_NOT_FOUND + assert "not found" in response.json()["detail"].lower() + finally: + async with session_scope() as session: + user = await session.get(User, other_user_id) + if user: + await session.delete(user) + await session.commit()As per coding guidelines, ensure async test resources are cleaned up with try/finally blocks.
Comment on lines
+531
to
+549
| async def _create_new_project( | ||
| session: DbSession, | ||
| project: FolderCreate, | ||
| project_id: UUID, | ||
| user_id: UUID, | ||
| ) -> FolderRead: | ||
| """Create a new project with specified ID (PUT create path). | ||
|
|
||
| Unlike POST, this fails on name conflict instead of auto-renaming. | ||
| """ | ||
| await _check_name_uniqueness(session, project.name, user_id) | ||
|
|
||
| new_project = Folder.model_validate(project, from_attributes=True) | ||
| new_project.id = project_id | ||
| new_project.user_id = user_id | ||
|
|
||
| session.add(new_project) | ||
| await session.flush() | ||
| await session.refresh(new_project) |
Contributor
There was a problem hiding this comment.
PUT‑create bypasses POST defaults (auth + MCP) and can drift behavior.
create_project auto‑enables API‑key auth when AUTO_LOGIN is false and performs MCP auto‑registration; _create_new_project doesn’t, so PUT‑created projects can end up with different auth/MCP behavior. This is user‑visible and may weaken the intended auth posture. Please mirror the POST initialization here (or extract shared helpers).
🔒 Proposed fix (mirror POST auth defaults)
new_project = Folder.model_validate(project, from_attributes=True)
new_project.id = project_id
new_project.user_id = user_id
+
+ # Mirror POST defaults for auth_settings when AUTO_LOGIN is disabled
+ settings_service = get_settings_service()
+ if not settings_service.auth_settings.AUTO_LOGIN and not new_project.auth_settings:
+ new_project.auth_settings = encrypt_auth_settings({"auth_type": "apikey"})
+ await logger.adebug(
+ f"Auto-enabled API key authentication for project {new_project.name} ({new_project.id}) "
+ "due to AUTO_LOGIN=false"
+ )Also consider extracting the MCP auto‑registration block from create_project and reusing it here for consistency.
🤖 Prompt for AI Agents
In `@src/backend/base/langflow/api/v1/projects.py` around lines 531 - 549, The
PUT-create path in _create_new_project currently only constructs and persists
the Folder model, so projects created via PUT skip the POST path's auth defaults
and MCP registration; update _create_new_project to apply the same
initialization logic used in create_project: after validating uniqueness and
before session.add, set auth defaults (e.g., enable API-key auth when AUTO_LOGIN
is false) and invoke the same MCP auto-registration routine used by
create_project (or call a new shared helper that encapsulates both the API-key
enabling logic and MCP registration), ensuring the code references the same
Folder model handling (Folder.model_validate) and reuses/create a helper to
avoid duplication.
Comment on lines
+551
to
+556
| # Associate flows with the new project | ||
| flow_ids = (project.flows_list or []) + (project.components_list or []) | ||
| if flow_ids: | ||
| await session.exec( | ||
| update(Flow).where(Flow.id.in_(flow_ids)).values(folder_id=new_project.id) # type: ignore[attr-defined] | ||
| ) |
Contributor
There was a problem hiding this comment.
Validate flow/component ownership before reassignment.
The PUT‑create path updates flows by ID without a user_id check, so a caller could reassign flows they don’t own. Please constrain updates to the current user and return 404 on any non‑owned IDs to avoid leakage.
🔐 Proposed fix
flow_ids = (project.flows_list or []) + (project.components_list or [])
if flow_ids:
- await session.exec(
- update(Flow).where(Flow.id.in_(flow_ids)).values(folder_id=new_project.id) # type: ignore[attr-defined]
- )
+ owned_ids = (
+ await session.exec(select(Flow.id).where(Flow.id.in_(flow_ids), Flow.user_id == user_id))
+ ).all()
+ owned_ids = set(owned_ids)
+ if set(flow_ids) - owned_ids:
+ raise HTTPException(status_code=404, detail="Flow not found")
+ await session.exec(
+ update(Flow).where(Flow.id.in_(owned_ids)).values(folder_id=new_project.id) # type: ignore[attr-defined]
+ )📝 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.
Suggested change
| # Associate flows with the new project | |
| flow_ids = (project.flows_list or []) + (project.components_list or []) | |
| if flow_ids: | |
| await session.exec( | |
| update(Flow).where(Flow.id.in_(flow_ids)).values(folder_id=new_project.id) # type: ignore[attr-defined] | |
| ) | |
| # Associate flows with the new project | |
| flow_ids = (project.flows_list or []) + (project.components_list or []) | |
| if flow_ids: | |
| owned_ids = ( | |
| await session.exec(select(Flow.id).where(Flow.id.in_(flow_ids), Flow.user_id == user_id)) | |
| ).all() | |
| owned_ids = set(owned_ids) | |
| if set(flow_ids) - owned_ids: | |
| raise HTTPException(status_code=404, detail="Flow not found") | |
| await session.exec( | |
| update(Flow).where(Flow.id.in_(owned_ids)).values(folder_id=new_project.id) # type: ignore[attr-defined] | |
| ) |
🤖 Prompt for AI Agents
In `@src/backend/base/langflow/api/v1/projects.py` around lines 551 - 556, The
code currently updates flows by IDs from project.flows_list and
project.components_list without checking ownership; change the logic to first
fetch/count Flow rows matching Flow.id.in_(flow_ids) AND Flow.user_id ==
current_user.id (or your request user variable), compare the count/IDs returned
to the requested flow_ids and if any requested ID is missing raise
HTTPException(status_code=404), and only then perform the update constrained to
those same ownership-filtered rows (e.g.,
update(Flow).where(and_(Flow.id.in_(flow_ids), Flow.user_id ==
current_user.id)).values(folder_id=new_project.id)); ensure you reference
project.flows_list, project.components_list, Flow, new_project.id and the
authenticated user variable in the fix.
Member
CRITICAL: Security & PII
CRITICAL: DRY
CRITICAL: File Structure
IMPORTANT: Architecture & Structure
IMPORTANT: Code Quality
TESTING
Detailed Findings🔴 CRITICAL #1 — File Structure Violation (Blocker)File: The file already has 691 lines. This PR adds ~105 more lines, bringing it to ~796 lines.
Required action: The new functions ( 🔴 CRITICAL #2 — Mixed Responsibilities in Same FileThe file now contains functions with different responsibility prefixes:
Per the rules, functions with different prefixes (check*, update*, create*, download*, upload*) MUST NOT coexist in the same file. Required action: Separate validation helpers, persistence logic, and handlers into distinct files. 🔴 CRITICAL #3 — Internal Error Details Exposed to Users (Security)# projects.py, upsert_project endpoint
except Exception as e:
if "UNIQUE constraint failed" in str(e):
raise HTTPException(status_code=409, detail="Name must be unique") from e
raise HTTPException(status_code=500, detail=str(e)) from e # ❌ DANGERProblems:
Break risk: In production with PostgreSQL, a name conflict would return 500 with internal DB details instead of 409. Suggested fix: except IntegrityError as e:
raise HTTPException(status_code=409, detail="Name must be unique") from e
except Exception:
raise HTTPException(status_code=500, detail="Internal server error")🟠 IMPORTANT #1 — Incomplete Functionality: MCP Server Not HandledThe existing
The existing
The new Break risk:
🟠 IMPORTANT #2 — auth_settings Ignored in Update Path
async def _update_existing_project(...) -> FolderRead:
if project.name:
existing_project.name = project.name
if project.description is not None:
existing_project.description = project.description
if project.parent_id is not None:
existing_project.parent_id = project.parent_id
# ❌ auth_settings is not handledBut Risk: Updates via PUT silently lose authentication configuration. 🟠 IMPORTANT #3 — Inconsistency: flows_list/components_list Ignored in Update PathIn the create path (PUT create), # _create_new_project
flow_ids = (project.flows_list or []) + (project.components_list or [])
if flow_ids:
await session.exec(update(Flow).where(Flow.id.in_(flow_ids)).values(folder_id=new_project.id))In the update path (PUT update), they are completely ignored. This is inconsistent with the existing PATCH Risk: Clients using PUT to update projects with flow reassociation will get unexpected behavior — flows silently remain unchanged. 🟠 IMPORTANT #4 — Falsy Check Fails with Empty Stringasync def _update_existing_project(...):
if project.name: # ❌ Empty string "" is falsy — silently skipped
...
if project.description is not None: # ✅ Correct
...
if project.parent_id is not None: # ✅ Correct
...
Risk: Sending 🟠 IMPORTANT #5 — DRY Violation: Partial Duplication with Existing update_projectThe update logic in
This creates two update paths with different behaviors, which is a source of future bugs. 🟠 IMPORTANT #6 — Race Condition Between Check and Createexisting_project = (await session.exec(select(Folder).where(Folder.id == project_id))).first()
if existing_project is not None:
# update path
else:
# create path — but another request could have created between the check and the createThe protection via 🟡 RECOMMENDED #1 — No Logging at Key Decision PointsThe endpoint has no structured logging for:
Per observability rules, decision points should have structured logging. 🟡 RECOMMENDED #2 — DB Error String Matching is Fragileif "UNIQUE constraint failed" in str(e):
This code works only with SQLite. In production with another database, the 409 will never be returned for DB-level name conflicts. 🟡 RECOMMENDED #3 — parent_id Without Existence Validationif project.parent_id is not None:
existing_project.parent_id = project.parent_idThere is no check whether 🟢 TESTING — Missing Adversarial TestsThe 8 tests cover happy path scenarios and some conflicts, but are missing:
Per the rule: "Happy path tests are the foundation — but they are NOT enough by themselves." 🟢 TESTING — Coverage Not ValidatedThe rules require:
The PR shows no evidence of coverage execution. Break Risk Summary
Recommendations
|
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
PUT /api/v1/projects/{project_id}endpoint with upsert semantics (create or update)201 Createdfor new projects,200 OKfor updates404for other user's projects (avoids leaking resource existence)409 Conflicton name uniqueness violationsparent_idfield toFolderCreateschema for consistency with the modelChanges
API endpoint (
src/backend/base/langflow/api/v1/projects.py):_check_name_uniqueness()- reusable helper for name validation_update_existing_project()- handles UPDATE path_create_new_project()- handles CREATE path with specified IDupsert_project()- main PUT endpointModel (
src/backend/base/langflow/services/database/models/folder/model.py):parent_idtoFolderCreateschemaTests (
src/backend/tests/unit/api/v1/test_projects.py):Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.