fix: various bugs#1596
Conversation
Summary by CodeRabbit
WalkthroughThis PR fixes several bugs: multipart form parsing now returns 413 for oversized uploads instead of generic errors; export/import Pub/Sub topics are cached and reused instead of shut down per publish (fixing repeat backup failures); entity parent changes no longer reparent children, and a derived ChangesMultipart Upload Size Handling
Pub/Sub Topic Reuse
Entity Location Derivation & Reparent Fix
Label Generator Fix
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Security recommendation: Since this PR introduces a proper 413 response for oversized multipart uploads, double-check that Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
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
added
go
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/components/Entity/CreateModal.vue (1)
545-569: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear template state for barcode imports When
params.productis present, any priorselectedTemplate/templateDatastill survives, socreate()can take the template endpoint and dropmanufacturer/modelNumber. CallclearTemplate()in that branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/Entity/CreateModal.vue` around lines 545 - 569, When CreateModal.vue handles a barcode product via the params.product branch, any previously selected template state can still remain and cause create() to use the template endpoint instead of the product payload. Update the logic around the params.product handling in CreateModal.vue to clear the template state there by calling clearTemplate(), so selectedTemplate/templateData do not override the looked-up manufacturer/modelNumber before submission.
🧹 Nitpick comments (3)
backend/app/api/handlers/v1/controller.go (1)
29-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a unit test for
multipartFormError.This function is the crux of the
#1538fix. Sincehttp.MaxBytesErroris trivial to construct (&http.MaxBytesError{Limit: n}), a small table test asserting 413 for that case and 400 otherwise would lock in the behavior cheaply.🧪 Example test sketch
func TestMultipartFormError(t *testing.T) { err := multipartFormError(&http.MaxBytesError{Limit: 1024}) var re *validate.RequestError require.ErrorAs(t, err, &re) require.Equal(t, http.StatusRequestEntityTooLarge, re.Status) err = multipartFormError(errors.New("boundary not found")) require.ErrorAs(t, err, &re) require.Equal(t, http.StatusBadRequest, re.Status) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/api/handlers/v1/controller.go` around lines 29 - 37, Add a small table-driven unit test for multipartFormError to lock in the `#1538` behavior: verify that when the input is an http.MaxBytesError the function returns a validate.RequestError with HTTP 413, and that a non-MaxBytes error path returns a validate.RequestError with HTTP 400. Use multipartFormError and validate.RequestError as the key symbols so the test stays focused on this helper.backend/internal/data/repo/repo_entities.go (1)
494-511: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAncestor walk issues one query per level.
The derivation runs a separate DB round-trip for each ancestor above the loaded parent. For typical depths (2–3) this is negligible, but since
GetOneis invoked after every create/update/patch and on item detail views, deep hierarchies pay a linear query cost. ThemaxAncestorDepth = 64guard against cycles is a nice touch. If deep nesting becomes common, a recursive CTE or a single walk query would collapse this to one round-trip.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/data/repo/repo_entities.go` around lines 494 - 511, The ancestor lookup in the entity derivation loop is doing one database round-trip per parent hop, which makes deep hierarchies expensive. Update the logic around the walk in repo_entities.go, especially the loop using cur, next, and maxAncestorDepth, to fetch ancestors in a single query path if possible, such as a recursive CTE or equivalent batched walk, while preserving the existing not-found handling and cycle guard behavior.frontend/components/Entity/CreateModal.vue (1)
548-551: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider validating/sanitizing third-party barcode API data before it flows into persisted records.
params.product.manufacturer/modelNumberoriginate from an external barcode lookup service and are copied straight into form state and eventually the create payload with no length/content validation on the client. Since the backend schema caps these at 255 chars (@maxLength 255indata-contracts.ts), an oversized or malformed value from the third-party API would just fail server-side, but it's worth confirming the backend enforces this consistently and that these values are HTML-escaped everywhere they're later rendered (e.g. label generator, PDF/print views) to avoid any stored-content issues from an untrusted upstream source.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/Entity/CreateModal.vue` around lines 548 - 551, The barcode lookup values copied in CreateModal.vue into form.manufacturer and form.modelNumber come from an untrusted third-party source and can exceed or contain malformed content before reaching persisted records. Add client-side sanitization/length guards at the point where params.product is mapped into the form, and verify the backend’s data-contracts.ts maxLength 255 enforcement is consistently applied so oversized values cannot be stored. Also ensure any later rendering paths that consume these fields, such as the label generator and PDF/print views, HTML-escape the values before display.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/internal/core/services/service_exports.go`:
- Around line 550-573: The cached topics created by ExportService.openTopic are
never shut down, so buffered messages/resources may remain unflushed at service
exit. Add a single service-teardown shutdown path that iterates over
ExportService.topics and calls Shutdown() on each cached pubsub.Topic during the
app’s graceful shutdown, while keeping openTopic free of per-publish Shutdown
for mem://.
---
Outside diff comments:
In `@frontend/components/Entity/CreateModal.vue`:
- Around line 545-569: When CreateModal.vue handles a barcode product via the
params.product branch, any previously selected template state can still remain
and cause create() to use the template endpoint instead of the product payload.
Update the logic around the params.product handling in CreateModal.vue to clear
the template state there by calling clearTemplate(), so
selectedTemplate/templateData do not override the looked-up
manufacturer/modelNumber before submission.
---
Nitpick comments:
In `@backend/app/api/handlers/v1/controller.go`:
- Around line 29-37: Add a small table-driven unit test for multipartFormError
to lock in the `#1538` behavior: verify that when the input is an
http.MaxBytesError the function returns a validate.RequestError with HTTP 413,
and that a non-MaxBytes error path returns a validate.RequestError with HTTP
400. Use multipartFormError and validate.RequestError as the key symbols so the
test stays focused on this helper.
In `@backend/internal/data/repo/repo_entities.go`:
- Around line 494-511: The ancestor lookup in the entity derivation loop is
doing one database round-trip per parent hop, which makes deep hierarchies
expensive. Update the logic around the walk in repo_entities.go, especially the
loop using cur, next, and maxAncestorDepth, to fetch ancestors in a single query
path if possible, such as a recursive CTE or equivalent batched walk, while
preserving the existing not-found handling and cycle guard behavior.
In `@frontend/components/Entity/CreateModal.vue`:
- Around line 548-551: The barcode lookup values copied in CreateModal.vue into
form.manufacturer and form.modelNumber come from an untrusted third-party source
and can exceed or contain malformed content before reaching persisted records.
Add client-side sanitization/length guards at the point where params.product is
mapped into the form, and verify the backend’s data-contracts.ts maxLength 255
enforcement is consistently applied so oversized values cannot be stored. Also
ensure any later rendering paths that consume these fields, such as the label
generator and PDF/print views, HTML-escape the values before display.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3014419b-2d63-4732-8195-1fae7d59cbc6
⛔ Files ignored due to path filters (9)
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/public/api/openapi-3.0.jsonis excluded by!docs/public/api/**docs/public/api/openapi-3.0.yamlis excluded by!docs/public/api/**docs/public/api/swagger-2.0.jsonis excluded by!docs/public/api/**docs/public/api/swagger-2.0.yamlis excluded by!docs/public/api/**
📒 Files selected for processing (14)
backend/app/api/handlers/v1/controller.gobackend/app/api/handlers/v1/v1_ctrl_entities.gobackend/app/api/handlers/v1/v1_ctrl_entities_attachments.gobackend/app/api/handlers/v1/v1_ctrl_exports.gobackend/internal/core/services/service_exports.gobackend/internal/core/services/service_exports_test.gobackend/internal/data/repo/repo_entities.gobackend/internal/data/repo/repo_entities_test.gofrontend/components/Entity/CreateModal.vuefrontend/lib/api/types/data-contracts.tsfrontend/locales/en.jsonfrontend/pages/item/[id]/index/edit.vuefrontend/pages/location/[id]/index/edit.vuefrontend/pages/reports/label-generator.vue
| // openTopic returns the long-lived publisher topic for name, opening and | ||
| // caching it on first use. The topic is reused across publishes and stays | ||
| // open for the process lifetime — see the topics field doc for why a | ||
| // per-publish Shutdown must not be reintroduced here. | ||
| func (s *ExportService) openTopic(ctx context.Context, name string) (*pubsub.Topic, error) { | ||
| s.topicsMu.Lock() | ||
| defer s.topicsMu.Unlock() | ||
| if topic, ok := s.topics[name]; ok { | ||
| return topic, nil | ||
| } | ||
| conn, err := utils.GenerateSubPubConn(s.pubSubConn, name) | ||
| if err != nil { | ||
| return err | ||
| return nil, err | ||
| } | ||
| topic, err := pubsub.OpenTopic(ctx, conn) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if s.topics == nil { | ||
| s.topics = make(map[string]*pubsub.Topic) | ||
| } | ||
| s.topics[name] = topic | ||
| return topic, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how ExportService is stopped and whether topics are ever shut down/flushed.
rg -nP 'func\s+\(s\s+\*ExportService\)' backend/internal/core/services/service_exports.go
rg -nP '\.Shutdown\(' backend/internal/core/services
rg -nP 'GenerateSubPubConn' backend/internal -A3Repository: sysadminsmedia/homebox
Length of output: 2988
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the ExportService shape and surrounding lifecycle hooks.
sed -n '1,220p' backend/internal/core/services/service_exports.go
printf '\n---\n'
sed -n '540,640p' backend/internal/core/services/service_exports.go
printf '\n---\n'
sed -n '1,220p' backend/internal/core/services/service.go 2>/dev/null || true
# Find any close/shutdown hooks for ExportService or its owning services.
rg -n '\b(Close|Shutdown|Stop|Dispose)\b' backend/internal/core/services backend/internal -g'*.go'
# Inspect the pubsub wrapper used by openTopic.
rg -n 'func\s+OpenTopic|type\s+Topic|func\s+\(.*Topic.*\)\s+Shutdown|type\s+Conn|GenerateSubPubConn' backend/internal -g'*.go' -A6 -B4Repository: sysadminsmedia/homebox
Length of output: 27868
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate ExportService construction and whether any service lifecycle hook closes cached topics.
rg -n 'ExportService|AllServices|New.*Service|Close\(\) error|Shutdown\(' backend/internal -g'*.go' -A4 -B4
printf '\n--- utils.GenerateSubPubConn ---\n'
sed -n '1,240p' backend/internal/sys/config/* 2>/dev/null || true
rg -n 'func\s+GenerateSubPubConn|GenerateSubPubConn\(' backend/internal backend/pkgs -g'*.go' -A8 -B4
printf '\n--- pubsub topic tests/docs in repo ---\n'
rg -n 'OpenTopic|Shutdown.*pubsub|shared singleton topic|mem://|collection_export|collection_import' backend/internal -g'*.go' -A4 -B4Repository: sysadminsmedia/homebox
Length of output: 50378
🌐 Web query:
gocloud.dev/pubsub OpenTopic context shutdown topic Send docs
💡 Result:
In the Go Cloud Development Kit (Go CDK) pubsub package, opening a topic and managing its lifecycle involves the following standard patterns: Opening a Topic To interact with a publish/subscribe system, you open a topic using the pubsub.OpenTopic function [1][2]. This function takes a context and a URL string that identifies the specific topic, with the URL scheme determining the driver (e.g., gcppubsub://) [3][1]. Example: ctx:= context.Background topic, err:= pubsub.OpenTopic(ctx, "gcppubsub://projects/myproject/topics/mytopic") if err!= nil { log.Fatal(err) } defer topic.Shutdown(ctx) Sending Messages Messages are sent using the Send method on the *pubsub.Topic object [1][4]. Unlike some other Pub/Sub client libraries, the Go CDK's Send method is synchronous: it only returns after the message has been sent to the service or has failed to be sent [4][5]. err:= topic.Send(ctx, &pubsub.Message{ Body: []byte("Hello, World!"), }) Shutdown The Shutdown method is used to gracefully disconnect the topic [1][6]. It flushes any pending message sends and performs necessary cleanup [1][6]. It is important to call Shutdown when you are finished with the topic to ensure all outstanding messages are processed [1][6]. The context passed to Shutdown is respected during the flushing process; if the context is cancelled, the shutdown procedure may be interrupted [6]. Note: The Go CDK pubsub API (gocloud.dev/pubsub) is distinct from the raw Google Cloud Pub/Sub Go client library (cloud.google.com/go/pubsub) [3][6]. The latter uses Stop instead of Shutdown to manage background resources [7][8]. Always ensure you are using the methods appropriate for the specific library you have imported [1][6].
Citations:
- 1: https://pkg.go.dev/gocloud.dev/pubsub
- 2: https://gocloud.dev/howto/pubsub/publish/
- 3: https://pkg.go.dev/gocloud.dev/pubsub/gcppubsub
- 4: https://pkg.go.dev/gocloud.dev@v0.46.0/pubsub
- 5: https://github.com/google/go-cloud/blob/v0.46.0/pubsub/pubsub.go
- 6: https://github.com/google/go-cloud/blob/master/pubsub/pubsub.go
- 7: https://code.googlesource.com/gocloud/+/HEAD/pubsub/topic.go
- 8: https://code.googlesource.com/gocloud/+/HEAD/pubsub/doc.go
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact pubsub connection-string helper and any service close methods.
sed -n '1,80p' backend/pkgs/utils/generator.go
printf '\n---\n'
rg -n 'func\s+\(s\s+\*ExportService\)\s+(Close|Shutdown)|func\s+\(s\s+\*AllServices\)\s+(Close|Shutdown)|func\s+\(.*\)\s+(Close|Shutdown)\(' backend/internal/core/services backend/internal/core -g'*.go' -A4 -B4Repository: sysadminsmedia/homebox
Length of output: 916
Add a service shutdown path for cached topics
openTopic should stay free of per-publish Shutdown for mem://, but the cached *pubsub.Topic values still need a single Shutdown() on service teardown so buffered sends/resources are flushed before exit. Wire that into the app’s graceful shutdown path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/internal/core/services/service_exports.go` around lines 550 - 573,
The cached topics created by ExportService.openTopic are never shut down, so
buffered messages/resources may remain unflushed at service exit. Add a single
service-teardown shutdown path that iterates over ExportService.topics and calls
Shutdown() on each cached pubsub.Topic during the app’s graceful shutdown, while
keeping openTopic free of per-publish Shutdown for mem://.
1 participant
What type of PR is this?
What this PR does / why we need it:
Fixes various bugs with 0.26.x
Which issue(s) this PR fixes:
Fixes: #1592
Fixes: #1591
Fixes: #1589
Fixes: #1578
Fixes: #1574
Fixes: #1538