fix: fix ID type in permission#2006
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27d46d3510
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Ok(note_id.to_string()) | ||
| let note = state | ||
| .note_stg() | ||
| .get_note_by_public_id(normalized_resource_id) |
There was a problem hiding this comment.
This change makes note permissions canonicalize resource_id to note.public_id, but existing permission rows created before this commit were keyed by numeric note.id (the previous parser normalized to i64), and permission queries/deletes in storage still match resource_id by exact string. In environments with pre-existing data, lookups for the same note will stop finding old rows and updates can create a second permission set under the new key, effectively dropping current permissions until a data migration or compatibility lookup is added.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Updates the permission APIs to resolve resource IDs correctly (notably for note), aligning request path IDs with the backing storage format and improving API documentation around missing resources.
Changes:
- Replace
parse_resource_contextwith asyncresolve_resource_contextthat validates/normalizes resource IDs via storage lookup. - Add
NoteStorage::get_note_by_public_idand use it to return 404 when a referenced note resource doesn’t exist. - Update OpenAPI (
utoipa) response docs to include/clarify 404 “Resource not found” cases.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| mono/src/api/router/permission_router.rs | Switches to async resource context resolution; documents 404 for missing resources. |
| mono/src/api/router/group_router.rs | Switches to async resource context resolution across group permission endpoints; updates 404 descriptions. |
| mono/src/api/api_common/group_permission.rs | Introduces resolve_resource_context and resource ID resolution via note public_id lookup. |
| jupiter/src/storage/note_storage.rs | Adds get_note_by_public_id query helper used by the API layer. |
| let note = state | ||
| .note_stg() | ||
| .get_note_by_public_id(normalized_resource_id) | ||
| .await? | ||
| .ok_or_else(|| { |
There was a problem hiding this comment.
resolve_resource_id loads the full notes::Model (including large text columns like description_html) just to validate existence/normalize the ID. This adds avoidable DB/IO overhead on every permission request. Consider switching to an existence check or a query that selects only a small column (e.g., id/public_id) and then return normalized_resource_id.to_owned() once the row is confirmed to exist.
| pub async fn get_note_by_public_id( | ||
| &self, | ||
| public_id: &str, | ||
| ) -> Result<Option<notes::Model>, MegaError> { | ||
| let model = notes::Entity::find() | ||
| .filter(notes::Column::PublicId.eq(public_id)) | ||
| .one(self.get_connection()) | ||
| .await?; | ||
| Ok(model) |
There was a problem hiding this comment.
get_note_by_public_id is newly added but has no unit tests. Other storage modules in this repo use #[cfg(test)] + crate::tests::test_storage to cover query behavior; adding a small test here would help prevent regressions (e.g., ensuring it returns Some for an inserted note and None for a missing public_id).
No description provided.