Remove documents table #618
Conversation
|
Warning Rate limit exceeded@tankerkiller125 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis change removes the entire Document entity and all related logic, schema, migrations, and API references from the codebase. All document-related repositories, schema definitions, queries, mutations, and tests are deleted. The Attachment entity is expanded with new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant API
participant ItemService
participant AttachmentRepo
participant FileSystem
User->>Frontend: Upload attachment (title, file, primary)
Frontend->>API: POST /items/{id}/attachments (title, file, primary)
API->>ItemService: AttachmentAdd(itemID, title, type, primary, file)
ItemService->>AttachmentRepo: Create(itemID, {title, content}, type, primary)
AttachmentRepo->>FileSystem: Save file (hashed path)
FileSystem-->>AttachmentRepo: Success/Failure
AttachmentRepo-->>ItemService: Attachment entity (with title, path)
ItemService-->>API: ItemOut (attachments with title, path)
API-->>Frontend: Response (attachments with title, path)
Frontend-->>User: Show attachment (title, download link)
Possibly related PRs
Suggested reviewers
Poem
Security Recommendations:
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Copilot reviewed 36 out of 36 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (3)
backend/internal/data/ent/group.go:51
- Ensure that the new loadedTypes array size of 6 correctly reflects the removal of the "documents" edge and that all index references in GroupEdges accessor methods have been updated to prevent off-by-one errors.
loadedTypes [6]bool
backend/internal/data/ent/attachment_query.go:376
- Verify that the updated loadedTypes array for attachments (now size 1) correctly accounts for the removal of the document edge and that related eager-loading logic is properly adjusted.
loadedTypes = [1]bool{
backend/internal/data/ent/client.go:42
- Confirm that removal of the Document client initialization does not leave any downstream dependencies unresolved and update any affected modules accordingly.
c.Document = NewDocumentClient(c.config)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Deploying homebox-docs with
|
| Latest commit: |
6847fdd
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ca6030db.homebox-docs.pages.dev |
| Branch Preview URL: | https://mk-merge-attachments-and-doc.homebox-docs.pages.dev |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Copilot reviewed 52 out of 55 changed files in this pull request and generated 1 comment.
Files not reviewed (3)
- backend/app/api/static/docs/swagger.json: Language not supported
- backend/app/api/static/docs/swagger.yaml: Language not supported
- backend/go.mod: Language not supported
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Switching the branch for my dev workspace to this and running it results in all of my prev images not loading correctly |
|
Hmmm, seems that the migration isn't working quite right |
|
Seems to be working better but seems to loose the attachment title |
|
This PR is on hold until I can get the migrations stuff sorted. It seems that we're in desperate need of a total migrations system replacement as the ent migration system frankly sucks when it's embedded into an application. Will be creating a different PR for that work. |
…nto mk/merge-attachments-and-docs
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
backend/app/api/main.go (1)
164-166: Enhanced migration settings improve security and reliabilityThe addition of these three migration options strengthens the database schema creation process:
- Explicitly setting the dialect ensures compatibility between different database systems
- Enabling foreign keys helps maintain data integrity during the migration
- Using replay mode ensures consistent migration application
These settings will help address the migration issues reported in GitHub Actions. From a security perspective, enabling foreign key constraints is particularly valuable as it prevents orphaned records that could potentially lead to data integrity issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/internal/data/migrations/postgres/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
backend/app/api/main.go(1 hunks)backend/internal/data/migrations/postgres/20250419184104_merge_docs_attachments.sql(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/api/main.go (1)
backend/internal/sys/config/conf_database.go (1)
Database(12-21)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Frontend Tests / Integration Tests PGSQL 16
- GitHub Check: Frontend Tests / Integration Tests
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 3/4
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 1/4
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 4/4
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 2/4
- GitHub Check: Frontend Tests / Integration Tests PGSQL 17
- GitHub Check: Frontend Tests / Integration Tests PGSQL 15
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/amd64)
- GitHub Check: Analyze (go)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
backend/internal/data/migrations/postgres/20250419184104_merge_docs_attachments.sql (1)
11-12:✅ Verification successful
Verify all foreign key constraints are handled
The migration drops one constraint, but there might be other foreign keys or indexes related to the documents table.
🏁 Script executed:
#!/bin/bash # Check for any other constraints or indexes referencing the documents table echo "Checking for other potential constraints or references to documents table:" rg -A 3 "CONSTRAINT.*documents|REFERENCES.*documents|INDEX.*documents" --glob "*.sql" # Look for any code that might still expect the document relationship echo "Searching for code that might still reference documents:" rg -A 3 "document_attachments|documents" --type go --type sqlLength of output: 19664
All PostgreSQL foreign keys to “documents” are accounted for and removed
Our search confirmed that the only constraint in Postgres referencing the
documentstable is
attachments_documents_attachmentsin the init migration, and it’s dropped in step 3 of your merge migration. No other SQL migrations or Go code rely on a DB-leveldocumentsrelationship.• SQL migrations:
– No remainingREFERENCES "documents"orCONSTRAINT … documentsin the Postgres folder.
• Application code:
– The hits inrepo_item_attachments.goand tests refer to a filesystem directory named “documents,” not the DB table.Security recommendation:
Wrap the entire migration in a transaction and ensure you have a recent backup—this avoids exposing intermediate states or orphaned data if something goes wrong.
backend/internal/data/migrations/postgres/20250419184104_merge_docs_attachments.sql
Show resolved
Hide resolved
backend/internal/data/migrations/postgres/20250419184104_merge_docs_attachments.sql
Show resolved
Hide resolved
backend/internal/data/migrations/postgres/20250419184104_merge_docs_attachments.sql
Show resolved
Hide resolved
…and-docs # Conflicts: # backend/app/api/main.go # backend/app/tools/migrations/main.go # backend/go.mod # backend/go.sum # backend/internal/data/migrations/postgres/atlas.sum # backend/internal/data/migrations/sqlite3/atlas.sum # backend/internal/data/repo/repo_maintenance.go
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
coderabbitai
bot
added
the
📖 documentation
|
This one is now ready for some solid testing with the new migration system @katosdev @tonyaellie everything seems to work correctly in my testing including existing documents moving over with the correct path and name data. |
|
When switching from main to this with images worked perfectly. Copying my data from a docker version of the hay-kot to this branch i had to replace /data/xxx/documents/yyy.png with .data/xxx/documents/yyy.png for the path but i assume that was due to the switch from docker? Otherwise worked fine |
|
@tonyaellie Yeah switching between docker and IDE code will do that, I keep a copy of the hay-kot binary version for this kind of testing. |
What type of PR is this?
What this PR does / why we need it:
This PR removes the documents table after merging the important bits into the attachments table. This significantly simplifies the overall design of the database and also makes it simpler for further work with attachements.
Which issue(s) this PR fixes:
Part of #265
Summary by CodeRabbit
New Features
Improvements
Migrations
Bug Fixes
Chores