-
Notifications
You must be signed in to change notification settings - Fork 0
reworked share endpoint #211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the share endpoint architecture to use RESTful conventions with a unified /inventory/share endpoint, and fixes a bug where write access could not be granted to users who already had read access.
Changes:
- Unified share endpoints: Changed from separate
/inventory/addShareand/inventory/removeShareendpoints to a single/inventory/shareendpoint using PATCH for adding and DELETE for removing - Added
ON CONFLICT DO NOTHINGclauses to SQL INSERT statements to prevent duplicate key errors when granting write access to existing readers - Refactored parameter structure from
reader_uuid/writer_uuidtomember_uuid+share_type('r' or 'w') for clearer semantics
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| frontend/src/store/DatabaseHandler.ts | Updated endpoint URLs and HTTP methods; refactored buildShareParams to use member_uuid and share_type parameters |
| backend/repositories/src/repos/inventory_repository.rs | Added ON CONFLICT DO NOTHING to add_reader and add_writer SQL queries to handle duplicate entries gracefully |
| backend/inventarwerk_api/src/routers/inventory_router.rs | Refactored share endpoints to use unified URL with PATCH/DELETE verbs; simplified parameter structure and authorization logic; added share_type validation |
Comments suppressed due to low confidence (1)
backend/inventarwerk_api/src/routers/inventory_router.rs:475
- The API documentation is missing the 400 Bad Request response status that is returned when member_uuid is not provided. The responses section should include this error case for completeness.
responses(
(status = 204, description = "Share permissions removed successfully")
),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c0derMo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we strictly need the DELETE share endpoint? Imo it'd also make sense to do it all via one (PATCH?) endpoint, and if no permission is set, the user is removed, if no user is supplied, everyone is removed
(Also, does Rust have Enums to use instead of the "r" and "w" string literals?)
This is now implemented
Rust has enums that could also be used in the parameters to filter out wrong requests, but doing so would result in request rejected with a 404 and not with a 400. And just using it only in the match struct would result in unnecessary code that is only used once. I would like to avoid that unless we would need it somewhere else. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This PR reworks the share endpoint, it also fixes an issue where write access could not be granted when already having read access.