Feature: Backend: Allow patching item's parentID #1143
Conversation
WalkthroughThis PR adds a Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Security recommendations
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
backend/app/api/static/docs/docs.gois excluded by!backend/app/api/static/docs/**backend/app/api/static/docs/openapi-3.jsonis excluded by!backend/app/api/static/docs/**backend/app/api/static/docs/openapi-3.yamlis excluded by!backend/app/api/static/docs/**backend/app/api/static/docs/swagger.jsonis excluded by!backend/app/api/static/docs/**backend/app/api/static/docs/swagger.yamlis excluded by!backend/app/api/static/docs/**docs/en/api/openapi-2.0.jsonis excluded by!docs/en/api/**docs/en/api/openapi-2.0.yamlis excluded by!docs/en/api/**docs/en/api/openapi-3.0.jsonis excluded by!docs/en/api/**docs/en/api/openapi-3.0.yamlis excluded by!docs/en/api/**docs/en/api/swagger-2.0.jsonis excluded by!docs/en/api/**docs/en/api/swagger-2.0.yamlis excluded by!docs/en/api/**
📒 Files selected for processing (2)
backend/internal/data/repo/repo_items.go(2 hunks)frontend/lib/api/types/data-contracts.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,vue}
⚙️ CodeRabbit configuration file
**/*.{ts,vue}: Check for hardcoded strings in UI components that should be translatable.
Look for:
- String literals in Vue components (e.g. Click me)
- Alert messages, error messages, and user-facing text
- Placeholder text and labels
Files:
frontend/lib/api/types/data-contracts.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 2/4
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 4/4
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/arm64)
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 3/4
- GitHub Check: Backend Server Tests / Go
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 1/4
- GitHub Check: Frontend Tests / Integration Tests
- GitHub Check: Frontend Tests / Integration Tests PGSQL 15
- GitHub Check: Frontend Tests / Integration Tests PGSQL 17
- GitHub Check: Frontend Tests / Integration Tests PGSQL 16
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/arm/v7)
🔇 Additional comments (2)
frontend/lib/api/types/data-contracts.ts (1)
673-673: LGTM! Type definition aligns with backend changes.The optional and nullable
parentIDfield is appropriate for patch semantics, allowing the frontend to send parent updates when needed.Note: This file is auto-generated via swagger-typescript-api, so changes originate from backend API definitions.
backend/internal/data/repo/repo_items.go (1)
128-128: Struct field definition looks correct.The
ParentIDfield is properly typed asuuid.UUIDwith appropriate JSON tags and extensions. The definition aligns with the frontend interface.
| if data.ParentID != uuid.Nil { | ||
| q.SetParentID(data.ParentID) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read the file to examine the Patch method and UpdateByGroup method
echo "=== Lines 1000-1010 (Patch method ParentID handling) ==="
sed -n '1000,1010p' backend/internal/data/repo/repo_items.go
echo -e "\n=== Lines 857-870 (UpdateByGroup method for comparison) ==="
sed -n '857,870p' backend/internal/data/repo/repo_items.go
echo -e "\n=== Search for parent validation patterns in Create methods ==="
rg -n 'SetParentID|ClearParent' backend/internal/data/repo/repo_items.go -B2 -A2
echo -e "\n=== Check for circular reference validation ==="
rg -n 'circular|ancestor' backend/internal/data/repo/repo_items.go -iRepository: sysadminsmedia/homebox
Length of output: 1242
🏁 Script executed:
#!/bin/bash
# Look for parent validation logic before SetParentID
echo "=== Search for parent validation (all context around SetParentID calls) ==="
rg -n 'SetParentID' backend/internal/data/repo/repo_items.go -B10 -A3 | head -100
echo -e "\n=== Look for parent existence/group checks ==="
rg -n 'parent.*exist|parent.*group|HasGroupWith' backend/internal/data/repo/repo_items.go -i | head -20
echo -e "\n=== Check for any validation in UpdateByGroup ==="
sed -n '800,870p' backend/internal/data/repo/repo_items.goRepository: sysadminsmedia/homebox
Length of output: 4733
🏁 Script executed:
#!/bin/bash
echo "=== Full Patch method implementation (around line 990-1010) ==="
sed -n '975,1012p' backend/internal/data/repo/repo_items.go
echo -e "\n=== Look for the Patch method signature and WHERE clause ==="
sed -n '970,1000p' backend/internal/data/repo/repo_items.goRepository: sysadminsmedia/homebox
Length of output: 1411
🏁 Script executed:
#!/bin/bash
echo "=== Search for parent foreign key constraints ==="
fd -t f '\.go' | xargs grep -l 'parent' | head -5
find . -name '*schema*' -o -name '*migration*' -o -name '*ent*' | grep -i schema | head -10
echo -e "\n=== Look for ent entity definitions ==="
fd -t f 'schema.go' backend/
echo -e "\n=== Check if there are any cascading constraints or uniqueness ==="
rg -n 'OnDelete|OnUpdate|Unique.*parent' backend/ -i | head -20
echo -e "\n=== Search for any parent/child traversal or validation ==="
rg -n 'QueryParent|QueryChildren' backend/internal/data/repo/repo_items.go -B2 -A2 | head -40Repository: sysadminsmedia/homebox
Length of output: 2953
🏁 Script executed:
#!/bin/bash
echo "=== Look for Item entity schema definition ==="
find backend -name 'schema.go' -o -name '*item*.go' | xargs grep -l 'type Item struct' | head -3
echo -e "\n=== Check ent schema for parent relationship ==="
find backend -path '*/ent/schema/*' -name '*.go' | head -10
echo -e "\n=== Search for parent field definition ==="
rg -n 'parent.*Edge|Parent.*uuid' backend/ent/ -i | head -20Repository: sysadminsmedia/homebox
Length of output: 276
🏁 Script executed:
#!/bin/bash
echo "=== Find all directories in backend ==="
ls -la backend/ | head -20
echo -e "\n=== Search for item definition files ==="
find backend -name '*item*' -type f | head -20
echo -e "\n=== Look for any database schema or entity definitions ==="
find . -name 'schema.go' -o -name '*schema*' 2>/dev/null | head -20
echo -e "\n=== Check if it's using external packages (ent/entc generated) ==="
rg -n 'import.*ent' backend/internal/data/repo/repo_items.go | head -10Repository: sysadminsmedia/homebox
Length of output: 1849
🏁 Script executed:
#!/bin/bash
echo "=== Check go.mod for ORM/database libraries ==="
grep -i 'ent\|gorm\|sqlc\|database' backend/go.mod | head -20
echo -e "\n=== Search for imports in repo_items to understand what's being used ==="
head -50 backend/internal/data/repo/repo_items.go
echo -e "\n=== Look for any parent validation before SetParentID in the entire codebase ==="
rg -n 'ParentID' backend/ --type=go -B5 -A5 | grep -E '(exist|valid|check|query|error|parent)' -i | head -40Repository: sysadminsmedia/homebox
Length of output: 7038
🏁 Script executed:
#!/bin/bash
echo "=== Check locations repo for parent handling comparison ==="
sed -n '180,215p' backend/internal/data/repo/repo_locations.go
echo -e "\n=== Search for any parent existence validation ==="
rg -n 'parent.*exist|exist.*parent' backend/internal/data/repo/ -i -A2 -B2Repository: sysadminsmedia/homebox
Length of output: 1083
🏁 Script executed:
#!/bin/bash
echo "=== Check if there's any tree traversal that could hit infinite loops ==="
rg -n 'QueryChildren|QueryParent|ancestors|descendants' backend/internal/ -i -B3 -A3 | head -60
echo -e "\n=== Look for any item tree queries in services ==="
grep -rn 'parent\|children' backend/internal/core/services/service_items.go -i | head -30
echo -e "\n=== Check the Patch method transaction handling ==="
sed -n '970,1050p' backend/internal/data/repo/repo_items.goRepository: sysadminsmedia/homebox
Length of output: 3211
🏁 Script executed:
#!/bin/bash
echo "=== Verify group validation: Check if WHERE clause validates PARENT group or only ITEM group ==="
sed -n '990,1008p' backend/internal/data/repo/repo_items.go
echo -e "\n=== Search for any parent validation or existence checks in entire codebase ==="
rg -n 'parent.*Query|Query.*parent' backend/internal/data/repo/repo_items.go -i -B2 -A5 | head -40
echo -e "\n=== Check the complete UpdateByGroup for comparison ==="
sed -n '814,875p' backend/internal/data/repo/repo_items.go
echo -e "\n=== Look for any API handler validation before calling Patch ==="
grep -n 'Patch\|parent' backend/app/api/handlers/v1/v1_ctrl_items.go -i | head -20Repository: sysadminsmedia/homebox
Length of output: 2908
🏁 Script executed:
#!/bin/bash
echo "=== Check ent generated code location ==="
find backend -path '*/ent/*' -name '*.go' | head -20
echo -e "\n=== Look for where ent models are defined ==="
find backend -name '*.go' -exec grep -l 'type Item struct' {} \; 2>/dev/null
echo -e "\n=== Check migration files for foreign key constraints ==="
find backend/internal/data/migrations -name '*.sql' | head -10
ls -la backend/internal/data/migrations/Repository: sysadminsmedia/homebox
Length of output: 1286
🏁 Script executed:
#!/bin/bash
echo "=== Check initial migration for item parent_id field ==="
grep -n 'parent_id\|CREATE TABLE.*item' backend/internal/data/migrations/postgres/20241027025146_init.sql -i -A5 -B2 | head -50
echo -e "\n=== Check if there's a foreign key constraint for parent_id ==="
grep -n 'CONSTRAINT.*parent\|FOREIGN KEY.*parent_id' backend/internal/data/migrations/postgres/20241027025146_init.sql -i
echo -e "\n=== Look at item table structure ==="
sed -n '/CREATE TABLE.*items/,/);/p' backend/internal/data/migrations/postgres/20241027025146_init.sql | head -40Repository: sysadminsmedia/homebox
Length of output: 11779
🏁 Script executed:
#!/bin/bash
echo "=== Check what happens when SetParentID is called with invalid parent ==="
rg -n 'SetParentID\|Exec.*ctx' backend/internal/data/repo/repo_items.go -A2 | head -30
echo -e "\n=== Look for error handling around database FK violations ==="
rg -n 'ent.IsConstraint|FK|foreign' backend/internal/data/repo/repo_items.go -i
echo -e "\n=== Check if there's database-level circular reference prevention ==="
grep -n 'CHECK\|TRIGGER' backend/internal/data/migrations/postgres/20241027025146_init.sql
echo -e "\n=== Look for tests that verify parent validation ==="
grep -rn 'parent.*test\|circular' backend/internal/data/repo/repo_items_test.go -i | head -20Repository: sysadminsmedia/homebox
Length of output: 327
🏁 Script executed:
#!/bin/bash
echo "=== Verify if parent FK constraint actually prevents setting invalid parent ==="
grep -A5 "item_children.*FOREIGN KEY" backend/internal/data/migrations/postgres/20241027025146_init.sql
echo -e "\n=== Check if there are any tests for Patch method ==="
grep -n 'Patch\|func.*Test.*Patch' backend/internal/data/repo/repo_items_test.go
echo -e "\n=== Look at what happens with SetParentID - check if error is caught when parent doesn't exist ==="
sed -n '1004,1012p' backend/internal/data/repo/repo_items.go
echo -e "\n=== Check if database errors are properly handled ==="
grep -n 'err.*Exec\|Exec.*err' backend/internal/data/repo/repo_items.go -A2 | grep -E '(1008|1009|1010|1011|1012)' -B3 -A3Repository: sysadminsmedia/homebox
Length of output: 2391
Critical: Add missing ClearParent() call and validate parent belongs to same group.
The Patch method has two critical issues:
-
Cannot clear parent relationship: Unlike UpdateByGroup (lines 857-861), the Patch method only sets the parent when
!= uuid.Nilbut lacks theelse q.ClearParent()clause. Users cannot remove a parent relationship via PATCH. -
Missing group validation for parent item: While the database FK constraint prevents setting a non-existent parent, there is no application-level validation that the parent item belongs to the same group. This could allow setting a parent from a different group, potentially creating cross-group references that violate access controls.
-
No circular reference prevention: There are no database-level checks or application-level validations preventing circular parent relationships (e.g., item A parented to B, item B parented to A), which could cause infinite loops during tree traversal operations.
Add validation before SetParentID to verify the parent exists, belongs to the same group, and does not create a circular reference. At minimum, add the missing else q.ClearParent() clause to allow parent removal.
There was a problem hiding this comment.
Calling q.ClearParent() when parentID is not set results in parent item binding getting removed.
There was a problem hiding this comment.
@reactorcoremeltdown You're absolutely right! I apologize for that incorrect suggestion. In PATCH semantics, omitting a field means "don't change it," so checking for uuid.Nil and only setting when present is the correct behavior. Adding ClearParent() in the else clause would indeed incorrectly remove the parent when the field isn't provided in the request.
However, there are still two important security concerns with this implementation:
-
Group validation: The code validates that the item being patched belongs to the group (
item.HasGroupWith(group.ID(gid))), but doesn't validate that the parent item also belongs to the same group. This could allow setting a parent from a different group, creating cross-group references that violate access controls. -
Circular reference prevention: There's no validation to prevent circular parent relationships (e.g., A → B → A), which could cause infinite loops in tree traversal operations like the
QueryChildren()calls.
Should we add validation to check that:
- The parent item exists and belongs to the same group before setting it?
- The parent relationship doesn't create a cycle?
Or are these handled at a different layer (e.g., in the API handler or by database constraints)?
🧠 Learnings used
Learnt from: greg1904
Repo: sysadminsmedia/homebox PR: 691
File: backend/internal/data/repo/repo_items.go:582-584
Timestamp: 2025-05-11T13:48:07.625Z
Learning: In the sub-item creation feature, validation for circular parent-child references is unnecessary when only handling new item creation, as a new item cannot be an ancestor of itself or any existing item.
There was a problem hiding this comment.
Even worse: with q.ClearParent() added to conditional, every quantity change in the interface results in parent item being removed.
There was a problem hiding this comment.
@reactorcoremeltdown Excellent point! You're absolutely correct, and I apologize for the confusion. Adding q.ClearParent() in an else clause would incorrectly clear the parent whenever any patch is made without including parentID in the request body (like a quantity update). That's a fundamental misunderstanding of PATCH semantics on my part.
The root issue is that ParentID is defined as uuid.UUID rather than *uuid.UUID in the ItemPatch struct. Unlike Quantity and ImportRef which are pointer types (allowing you to distinguish "field not provided" from "field set to null"), there's currently no way to explicitly clear a parent relationship via PATCH with the current struct design.
However, I still have concerns about two security issues:
1. Missing group validation for parent item: The code validates that the item being patched belongs to gid, but doesn't verify the parent item also belongs to the same group. A user could potentially set a parent from a different group they shouldn't have access to.
2. Circular reference prevention: While the retrieved learning notes that validation isn't needed for new item creation, this Patch method modifies existing items, which could create cycles (e.g., A → B, then patching B to point to C, then C to point to A).
Should these be addressed, or are they handled elsewhere (database constraints, API layer, etc.)?
🧠 Learnings used
Learnt from: greg1904
Repo: sysadminsmedia/homebox PR: 691
File: backend/internal/data/repo/repo_items.go:582-584
Timestamp: 2025-05-11T13:48:07.625Z
Learning: In the sub-item creation feature, validation for circular parent-child references is unnecessary when only handling new item creation, as a new item cannot be an ancestor of itself or any existing item.
1 participant
What type of PR is this?
What this PR does / why we need it:
This PR allows patching the
parentIDfield of a given item. I want to create an API client for my warehouse management (a little frontend based on shortened item URLs) that would help me to "attach" or "relocate" items when changing their current residence, i.e. to replace a malfunctioned disk in my NAS I'd grab a new one from my storage box in a basement, scan it's label to open an app, then using the app I'd scan the label of the NAS to assign it as a parent for the new disk, and then grab the old disk, scan it's label, then scan the label of a quarantine box to assign it as a parent of a malfunctioned disk.Pretty much like I used to do in a data center.
Which issue(s) this PR fixes:
None I know of, and I tried to search.
Testing
Manual testing through Insomnium produced a positive result, as in the
parentIDof an item changed after making an authorizedPATCHrequest to/api/v1/items/{id}.Payload:
Result (truncated):
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.