Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough新しい所有権管理機能（Ownership）を導入します。ドメインモデル、リポジトリ層、ユースケース層、HTTPハンドラーを追加し、所有権のCRUD操作と認可制御を実装します。 Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as Handler<br/>(Ownership)
participant UseCase as UseCase<br/>(Ownership)
participant Repo as Repository<br/>(Ownership)
participant DB as Database
rect rgba(100, 150, 200, 0.5)
Note over Client,DB: Create Ownership
Client->>Handler: POST /items/{itemId}/owners
Handler->>Handler: Bind request body
Handler->>UseCase: CreateOwnership(ownership)
UseCase->>Repo: Create(ownership)
Repo->>DB: INSERT ownership
DB-->>Repo: Created record
Repo-->>UseCase: *domain.Ownership
UseCase-->>Handler: *domain.Ownership
Handler-->>Client: 201 Created
end
rect rgba(150, 150, 100, 0.5)
Note over Client,DB: Update Ownership (with auth)
Client->>Handler: PATCH /items/{itemId}/owners/{ownershipId}
Handler->>Handler: Get userID from context
Handler->>UseCase: UpdateOwnership(ownership, userID)
UseCase->>Repo: GetByID(ownershipId)
Repo->>DB: SELECT ownership
DB-->>Repo: ownership record
Repo-->>UseCase: *domain.Ownership
UseCase->>UseCase: Check: ownership.UserID == userID?
alt Owner Match
UseCase->>Repo: Update(ownership)
Repo->>DB: UPDATE ownership
DB-->>Repo: Updated record
Repo-->>UseCase: *domain.Ownership
UseCase-->>Handler: *domain.Ownership
Handler-->>Client: 200 OK
else Owner Mismatch
UseCase-->>Handler: ErrForbidden
Handler-->>Client: 403 Forbidden
end
end
rect rgba(200, 100, 100, 0.5)
Note over Client,DB: Delete Ownership (with auth)
Client->>Handler: DELETE /items/{itemId}/owners/{ownershipId}
Handler->>Handler: Get userID from context
Handler->>UseCase: DeleteOwnership(ownershipId, itemId, userID)
UseCase->>Repo: GetByID(ownershipId)
Repo->>DB: SELECT ownership
DB-->>Repo: ownership record
Repo-->>UseCase: *domain.Ownership
UseCase->>UseCase: Check: ownership.UserID == userID<br/>and ownership.ItemID == itemId?
alt Authorization & ItemID Match
UseCase->>Repo: Delete(ownershipId)
Repo->>DB: DELETE ownership
DB-->>Repo: Deletion confirmed
Repo-->>UseCase: nil error
UseCase-->>Handler: nil error
Handler-->>Client: 200 OK
else Authorization or ItemID Mismatch
UseCase-->>Handler: ErrForbidden or ErrNotFound
Handler-->>Client: 403/404
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
repository/ownership.go (1)
13-16: 主要検索列にインデックスがありません。この PR で
GetByItemID/GetByUserIDが追加されていますが、item_idとuser_idにインデックスがないので、件数が増えると全表走査になりやすいです。少なくともこの 2 列はインデックス対象にしたいです。user_idは等価検索しかしていないので、TEXTよりインデックスを張りやすい型に寄せておくと後で扱いやすいです。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@repository/ownership.go` around lines 13 - 16, GetByItemID/GetByUserID will do full table scans because ItemID and UserID lack indexes; update the Ownership model fields (ItemID and UserID) to add GORM index tags (e.g., `index` on ItemID and UserID) and change UserID from TEXT to an index-friendly type (e.g., varchar with length or UUID string) instead of `type:text`, then generate/apply a DB migration so the new indexes are created; locate the fields ItemID and UserID in ownership.go to make these changes.handler/converter.go (1)
87-94:nilを受けるとここで panic します。同じファイルの
toOpenAPIItemはnilを明示的に弾いていますが、こちらはdを即参照しています。コンバータの契約を揃えてnilをエラーにするか、少なくとも呼び出し側で non-nil を保証したいです。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@handler/converter.go` around lines 87 - 94, toOpenAPIOwnership dereferences d without a nil check and can panic (toOpenAPIItem explicitly guards nil); add an explicit nil check at the start of toOpenAPIOwnership and handle it consistently (e.g. if d == nil return an empty openapi.Ownership or propagate an error) so callers won't panic; update callers or tests if you choose to change the contract so non-nil is enforced, and keep behavior consistent with toOpenAPIItem.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@handler/handler.go`:
- Around line 12-19: EditItemOwners handler currently calls UpdateOwnership
without passing the route {itemId} so ownership-item boundary isn’t verified;
modify the handler method (EditItemOwners in handler/ownership.go) to extract
and pass the itemId to the usecase call (UpdateOwnership in
usecase/ownership.go), and update UpdateOwnership signature to accept itemId and
verify the retrieved Ownership.ItemID matches that itemId before applying
changes (return an error if they differ) to prevent updating an ownership via a
different itemId path.
- Around line 12-19: The EditItemOwners authorization currently rejects any
request where o.UserID != userID by returning ErrForbidden; update the check in
usecase/ownership.go (the function called from handler/ownership.go invoked by
EditItemOwners) to permit the action when the requester is either the owner
(o.UserID == userID) OR an administrator. Implement this by adding an admin-role
check (e.g., call an existing auth/user usecase method such as IsAdmin(userID)
or accept a boolean isAdmin parameter) and only return ErrForbidden when neither
ownership nor admin status is true; keep the ErrForbidden semantics otherwise.
In `@handler/ownership.go`:
- Around line 15-30: In PostItemOwners, do not trust req.UserId: extract the
authenticated user ID from the request context (the value your auth middleware
stores on ctx) and set ownership.UserID to that value before calling
h.ou.CreateOwnership; if you still accept a userId in the body, validate it
matches the authenticated ID and return a 403/400 when it does not. Update the
ownership construction (domain.Ownership.UserID) to use the context-derived ID
(not req.UserId) and ensure error responses reflect validation failures vs
creation errors when calling h.ou.CreateOwnership.
In `@repository/ownership_test.go`:
- Around line 81-84: The test is asserting slice equality with a fixed order for
results from GetByItemID / GetByUserID which makes tests flaky if repository/DB
order changes; either make ordering part of the repository contract (add ORDER
BY in the repository methods) or make the test order-independent by comparing as
unordered collections (e.g. use assert.ElementsMatch or sort both slices by a
stable key such as ID before comparing) for the expected []*domain.Ownership
values used in these tests (references: GetByItemID, GetByUserID,
domain.Ownership, the expected variable).
- Around line 228-240: The test currently expects Update to insert a new
Ownership when the ID doesn't exist (see test case name "success: create new
ownership if not exist"); instead change the test to expect domain.ErrNotFound
and update the repository Update method to detect gorm.RowsAffected == 0 and
return domain.ErrNotFound (do not perform an upsert inside Update); if upsert
behavior is required, add a separate Upsert/CreateOrUpdate method rather than
treating Update as an insert.
In `@repository/ownership.go`:
- Around line 103-113: The Update method in ownershipRepository currently uses
tx.Save(model) which can INSERT when UPDATE affects 0 rows; change it to perform
an UPDATE with an explicit WHERE (use tx.Model(&OwnershipModel{}).Where("id =
?", model.ID).Updates(model) or equivalent) then check the result.RowsAffected
and if 0 return gorm.ErrRecordNotFound; wrap this in the existing transaction
pattern and return the domain object via model.toDomain() on success (see the
Delete() method in the same file for the expected RowsAffected/error-check
pattern).
In `@usecase/ownership.go`:
- Line 12: 引数の ownership の ItemID/UserID
をそのまま保存してしまい既存レコードの所有者・アイテムを差し替えられる問題があるため、UpdateOwnership
を修正してください：現在のレコードを取得する処理（参照されている o.UserID）で認可を行い、保存時には引数 ownership
全体を使わず既存レコードから固定する itemID と userID を保持し、更新可能なフィールド（Rentable と Memo
のみ）だけを既存レコードにコピーして保存するように変更します；また DeleteOwnership と同様に itemID
を別途受け取って整合性チェックを行うパターンに合わせて実装してください。
- Around line 54-60: The ownership deletion currently checks o.UserID before
validating o.ItemID, leaking nested-resource existence; switch the two checks so
you first validate that o.ItemID == itemID and return domain.ErrNotFound (using
the existing o.ItemID and itemID symbols) if it doesn't match, and only after
that check o.UserID vs userID and return ErrForbidden when the user is not the
owner.
---
Nitpick comments:
In `@handler/converter.go`:
- Around line 87-94: toOpenAPIOwnership dereferences d without a nil check and
can panic (toOpenAPIItem explicitly guards nil); add an explicit nil check at
the start of toOpenAPIOwnership and handle it consistently (e.g. if d == nil
return an empty openapi.Ownership or propagate an error) so callers won't panic;
update callers or tests if you choose to change the contract so non-nil is
enforced, and keep behavior consistent with toOpenAPIItem.
In `@repository/ownership.go`:
- Around line 13-16: GetByItemID/GetByUserID will do full table scans because
ItemID and UserID lack indexes; update the Ownership model fields (ItemID and
UserID) to add GORM index tags (e.g., `index` on ItemID and UserID) and change
UserID from TEXT to an index-friendly type (e.g., varchar with length or UUID
string) instead of `type:text`, then generate/apply a DB migration so the new
indexes are created; locate the fields ItemID and UserID in ownership.go to make
these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4d0fded-39a4-470e-b4d5-e71a3a8fd4a0
📒 Files selected for processing (18)
.gitignoredomain/mock/mock_ownership_repository.godomain/ownership.gohandler/converter.gohandler/file_test.gohandler/handler.gohandler/item_test.gohandler/ownership.gohandler/ownership_test.gomain.gorepository/db.gorepository/ownership.gorepository/ownership_test.gousecase/errors.gousecase/mock/mock_ownership_usecase.gousecase/mock_generate.gousecase/ownership.gousecase/ownership_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
リリースノート
新機能
テスト