Make attachment storage paths relative in database with cross-platform support#967
Conversation
Deploying homebox-docs with
|
| Latest commit: |
b237cbb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://096ccda0.homebox-docs.pages.dev |
| Branch Preview URL: | https://copilot-fix-4fec873a-6f0f-43.homebox-docs.pages.dev |
Co-authored-by: tankerkiller125 <3457368+tankerkiller125@users.noreply.github.com>
Co-authored-by: tankerkiller125 <3457368+tankerkiller125@users.noreply.github.com>
|
@copilot some older installs may have a |
Yes, the migration logic handles the I tested this with examples:
The migration works by finding "/documents/" and extracting from 36 characters before it (the UUID start) to the end, which preserves both hash-based and guid.extension filename formats. Commit: 3a2cf53 |
WalkthroughPaths for item attachments are shifted from absolute to relative storage paths. The repo now stores relative paths in DB and computes full paths for bucket operations. API GET handler uses the repo to resolve full paths. New Postgres and SQLite migrations convert existing absolute paths to relative. Tests updated to assert relative paths. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant API as API Handler
participant Repo as AttachmentRepo
participant Bucket as Object Storage
User->>API: GET /items/{id}/attachments/{hash}
API->>Repo: Fetch doc (relative path)
API->>Repo: GetFullPath(relativePath)
Repo-->>API: fullPath
API->>Bucket: NewReader(fullPath)
Bucket-->>API: Stream bytes
API-->>User: ServeContent (attachment)
API->>Bucket: Close reader
sequenceDiagram
autonumber
participant Svc as Service
participant Repo as AttachmentRepo
participant Bucket as Object Storage
note over Svc,Repo: Upload flow
Svc->>Repo: UploadFile(gid, hash, data)
Repo->>Repo: path(gid, hash) => relative
Repo->>Bucket: WriteAll(fullPath(relative), data)
Bucket-->>Repo: OK
Repo-->>Svc: Return relative path (store in DB)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Security recommendations
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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)
backend/internal/data/repo/repo_item_attachments.go (1)
391-399: Resource leak:thumbBucketis never closed.The thumbnail deletion branch opens a bucket and never closes it.
Apply:
thumbBucket, err := blob.OpenBucket(ctx, r.GetConnString()) if err != nil { log.Err(err).Msg("failed to open bucket for thumbnail deletion") return err } +defer func(b *blob.Bucket) { + if cerr := b.Close(); cerr != nil { + log.Err(cerr).Msg("failed to close thumbnail bucket") + } +}(thumbBucket) err = thumbBucket.Delete(ctx, r.fullPath(thumb.Path))
🧹 Nitpick comments (5)
backend/app/api/handlers/v1/v1_ctrl_items_attachments.go (1)
189-211: Use GetFullPath defensively and harden response headers.
- Defensive keying: If any legacy rows still carry an already-prefixed or absolute path,
GetFullPath(doc.Path)could double-prefix. Either makeGetFullPathidempotent (preferred; see repo comment) or add a quick guard here before passing the key toNewReader. Also consider normalizing to forward slashes for non-file drivers.- Security: Set an explicit Content-Type (from
doc.MimeTypewhen present) and addX-Content-Type-Options: nosniffto avoid browser sniffing.Apply:
- file, err := bucket.NewReader(ctx, ctrl.repo.Attachments.GetFullPath(doc.Path), nil) + // NB: GetFullPath should be idempotent; ensure it won't double-prefix if legacy absolute paths slip through. + file, err := bucket.NewReader(ctx, ctrl.repo.Attachments.GetFullPath(doc.Path), nil) @@ - // Set the Content-Disposition header for RFC6266 compliance + // Set headers disposition := "inline; filename*=UTF-8''" + url.QueryEscape(doc.Title) w.Header().Set("Content-Disposition", disposition) + if doc.MimeType != "" { + w.Header().Set("Content-Type", doc.MimeType) + } + w.Header().Set("X-Content-Type-Options", "nosniff") http.ServeContent(w, r, doc.Title, doc.CreatedAt, file)Verification to run locally: verify that for a legacy record whose
attachments.pathstarts with the configured prefix,GetFullPathdoes not repeat that prefix; validate GET works for:
- relative:
uuid/documents/hash- legacy absolute:
/var/lib/homebox/uuid/documents/hash- legacy dotted:
.data/uuid/documents/hashbackend/internal/core/services/service_items_attachments_test.go (1)
55-61: Assert looks good; prefer repo-derived full path and OS-aware joins.
- The expectation of a relative DB path is correct.
- Minor: reading the file via
os.TempDir()hard-codes the storage root from the test setup; prefer using the repo to compute the full path, and usefilepath.Joinoverpath.Joinfor filesystem paths.Apply:
- // path should now be relative: {group}/{documents} - assert.Equal(t, path.Join(tGroup.ID.String(), "documents"), path.Dir(storedPath)) + // path should now be relative: {group}/{documents} + assert.Equal(t, path.Join(tGroup.ID.String(), "documents"), path.Dir(storedPath)) - bts, err := os.ReadFile(path.Join(os.TempDir(), storedPath)) + // Read via repo-computed full path to avoid assumptions about storage root + bts, err := os.ReadFile(tRepos.Attachments.GetFullPath(storedPath))backend/internal/data/migrations/postgres/20250826000000_make_attachment_paths_relative.sql (1)
5-11: Tighten the WHERE clause to truly target UUID/documents paths and fix the comment.
- The explanatory comment says “go back 37 characters” but the SQL uses 36 (which is correct for a 36-char UUID). Update the comment for clarity.
- Add a UUID-shape predicate to avoid mis-rewriting paths that happen to contain
/documents/but aren’t of the expected form.Apply:
- -- The approach: find the "/documents/" substring, go back 37 characters (UUID + slash), + -- The approach: find the "/documents/" substring, go back 36 characters (UUID), -- and extract from there to get "uuid/documents/hash" UPDATE attachments SET path = SUBSTRING(path FROM POSITION('/documents/' IN path) - 36) WHERE path LIKE '%/documents/%' - AND POSITION('/documents/' IN path) > 36; + AND POSITION('/documents/' IN path) > 36 + AND SUBSTRING(path FROM POSITION('/documents/' IN path) - 36 FOR 36) ~* '^[0-9a-f-]{36}$';Note: This keeps legacy
{uuid}/documents/{uuid}.{suffix}intact—only the prefix is removed, which is what we want.backend/internal/data/migrations/sqlite3/20250826000000_make_attachment_paths_relative.sql (1)
5-11: Mirror the Postgres safeguards in SQLite.SQLite lacks built-in REGEXP, but we can still gate on the 36-char UUID shape using GLOB.
Apply:
- -- The approach: find the "/documents/" substring, go back 37 characters (UUID + slash), + -- The approach: find the "/documents/" substring, go back 36 characters (UUID), -- and extract from there to get "uuid/documents/hash" UPDATE attachments SET path = SUBSTR(path, INSTR(path, '/documents/') - 36) WHERE path LIKE '%/documents/%' - AND INSTR(path, '/documents/') > 36; + AND INSTR(path, '/documents/') > 36 + AND SUBSTR(path, INSTR(path, '/documents/') - 36, 36) GLOB '????????-????-????-????-????????????';This reduces the chance of accidentally rewriting non-UUID paths that happen to contain
/documents/.backend/internal/data/repo/repo_item_attachments.go (1)
796-805: Ensure object key semantics are consistent across drivers.Writing with
fullPathassumesPrefixPathis a key prefix inside the bucket (not the bucket root). That’s correct for the “file:///./” case (root is CWD; prefix is the data dir), but may be wrong if the conn string already points at the data dir (prefix should be empty in that case). Please verify configuration contracts so that:
GetConnString()sets the bucket root.fullPath()returns a bucket-relative key: “//documents/”, never an OS-absolute path.If the contract can’t be guaranteed at config time, add a guard in
fullPath()that strips any absolute root matching the bucket directory to avoid absolute keys for fileblob.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
backend/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
backend/app/api/handlers/v1/v1_ctrl_items_attachments.go(1 hunks)backend/internal/core/services/service_items_attachments_test.go(1 hunks)backend/internal/data/migrations/postgres/20250826000000_make_attachment_paths_relative.sql(1 hunks)backend/internal/data/migrations/sqlite3/20250826000000_make_attachment_paths_relative.sql(1 hunks)backend/internal/data/repo/repo_item_attachments.go(5 hunks)
| func (r *AttachmentRepo) path(gid uuid.UUID, hash string) string { | ||
| return filepath.Join(r.storage.PrefixPath, gid.String(), "documents", hash) | ||
| return filepath.Join(gid.String(), "documents", hash) | ||
| } | ||
|
|
||
| func (r *AttachmentRepo) fullPath(relativePath string) string { | ||
| return filepath.Join(r.storage.PrefixPath, relativePath) | ||
| } | ||
|
|
||
| func (r *AttachmentRepo) GetFullPath(relativePath string) string { | ||
| return r.fullPath(relativePath) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make object key construction idempotent and cross-driver safe.
fullPath currently OS-joins PrefixPath and the (supposedly) relative key. Risks:
- Double-prefixing if a legacy absolute/over-prefixed key slips through.
- OS-specific separators (“\” on Windows) in blob keys can be problematic for non-file drivers.
- Ambiguity: “full path” suggests an OS path; for blob keys this is a bucket-relative object key.
Apply:
-func (r *AttachmentRepo) path(gid uuid.UUID, hash string) string {
- return filepath.Join(gid.String(), "documents", hash)
-}
+func (r *AttachmentRepo) path(gid uuid.UUID, hash string) string {
+ // Always produce bucket-style keys with forward slashes
+ return filepath.ToSlash(filepath.Join(gid.String(), "documents", hash))
+}
-func (r *AttachmentRepo) fullPath(relativePath string) string {
- return filepath.Join(r.storage.PrefixPath, relativePath)
-}
+func (r *AttachmentRepo) fullPath(p string) string {
+ if p == "" {
+ return filepath.ToSlash(filepath.Clean(r.storage.PrefixPath))
+ }
+ // Strip any accidental leading prefix to avoid double-prefixing
+ cleanedPrefix := filepath.Clean(r.storage.PrefixPath)
+ cleanedKey := filepath.Clean(p)
+ if strings.HasPrefix(cleanedKey, cleanedPrefix+string(filepath.Separator)) {
+ cleanedKey = strings.TrimPrefix(cleanedKey, cleanedPrefix+string(filepath.Separator))
+ }
+ if filepath.IsAbs(cleanedKey) {
+ // Treat as already full; return normalized slashes
+ return filepath.ToSlash(cleanedKey)
+ }
+ return filepath.ToSlash(filepath.Join(cleanedPrefix, cleanedKey))
+}
func (r *AttachmentRepo) GetFullPath(relativePath string) string {
return r.fullPath(relativePath)
}Follow-up: consider renaming to objectKey/GetObjectKey to reduce confusion between filesystem paths and blob keys.
📝 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.
| func (r *AttachmentRepo) path(gid uuid.UUID, hash string) string { | |
| return filepath.Join(r.storage.PrefixPath, gid.String(), "documents", hash) | |
| return filepath.Join(gid.String(), "documents", hash) | |
| } | |
| func (r *AttachmentRepo) fullPath(relativePath string) string { | |
| return filepath.Join(r.storage.PrefixPath, relativePath) | |
| } | |
| func (r *AttachmentRepo) GetFullPath(relativePath string) string { | |
| return r.fullPath(relativePath) | |
| } | |
| func (r *AttachmentRepo) path(gid uuid.UUID, hash string) string { | |
| // Always produce bucket-style keys with forward slashes | |
| return filepath.ToSlash(filepath.Join(gid.String(), "documents", hash)) | |
| } | |
| func (r *AttachmentRepo) fullPath(p string) string { | |
| if p == "" { | |
| return filepath.ToSlash(filepath.Clean(r.storage.PrefixPath)) | |
| } | |
| // Strip any accidental leading prefix to avoid double-prefixing | |
| cleanedPrefix := filepath.Clean(r.storage.PrefixPath) | |
| cleanedKey := filepath.Clean(p) | |
| if strings.HasPrefix(cleanedKey, cleanedPrefix+string(filepath.Separator)) { | |
| cleanedKey = strings.TrimPrefix(cleanedKey, cleanedPrefix+string(filepath.Separator)) | |
| } | |
| if filepath.IsAbs(cleanedKey) { | |
| // Treat as already full; return normalized slashes | |
| return filepath.ToSlash(cleanedKey) | |
| } | |
| return filepath.ToSlash(filepath.Join(cleanedPrefix, cleanedKey)) | |
| } | |
| func (r *AttachmentRepo) GetFullPath(relativePath string) string { | |
| return r.fullPath(relativePath) | |
| } |
🤖 Prompt for AI Agents
In backend/internal/data/repo/repo_item_attachments.go around lines 100 to 110,
the current fullPath uses filepath.Join which can double-prefix keys, introduce
OS-specific separators, and conflates filesystem paths with blob object keys;
change fullPath to build an idempotent, bucket-relative object key by: treat
storage.PrefixPath as a prefix token (not an OS path), strip any
leading/trailing slashes or any existing PrefixPath from relativePath to avoid
double-prefixing, join using '/' as the separator regardless of OS (e.g., if
prefix is empty return cleaned relative, else return prefix + "/" + cleaned
relative), and ensure the exported getter name reflects object keys (consider
renaming to objectKey/GetObjectKey as follow-up).
|
@copilot Some of the legacy Windows binary installs used the |
Co-authored-by: tankerkiller125 <3457368+tankerkiller125@users.noreply.github.com>
Updated the migration scripts to handle both Unix (
Windows paths are converted to use forward slashes for consistency. Tested with SQLite to verify the logic works correctly. Commit: 36ad775 |
tankerkiller125
deleted the
copilot/fix-4fec873a-6f0f-4318-92ef-23d0e2a67682
branch
This PR addresses the issue where attachment file paths were being stored as absolute paths in the database, including the full storage prefix path. This made the paths dependent on the specific storage configuration and caused issues when moving or reconfiguring storage locations.
Changes Made
Database Schema Changes
{group_id}/documents/{hash}instead of{prefix_path}/{group_id}/documents/{hash}/) and Windows (\) path separators for cross-platform compatibilityCode Changes
AttachmentRepo.path(): Now returns relative paths onlyAttachmentRepo.GetFullPath(): Public method to combine storage prefix with relative paths for file operationsMigration Strategy
The migration safely handles various existing path formats from different platforms:
/app/data/uuid/documents/hash→uuid/documents/hash\app\data\uuid\documents\hash→uuid/documents/hashC:\ProgramData\homebox\uuid\documents\hash→uuid/documents/hash.data/uuid/documents/hash→uuid/documents/hashuuid/documents/hash→uuid/documents/hash(unchanged)Windows paths are automatically converted to use forward slashes for consistency across platforms.
Benefits
Testing
The implementation maintains full backward compatibility while making the attachment storage system more flexible and portable across different operating systems and storage configurations.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
Summary by CodeRabbit
Bug Fixes
Chores