RHCLOUD-45005: Use LockId and LockToken tiny types in tuple commands#1331
Merged
snehagunta merged 2 commits intoproject-kessel:mainfrom May 1, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe changes introduce strongly-typed lock identifiers and tokens throughout the locking system by replacing string fields with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
2 tasks
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Replace raw string with model.LockId and model.LockToken in AcquireLockCommand, FencingCheck, and AcquireLockResult. Validate at the service boundary via NewLockId/NewLockToken; remove redundant DeserializeLockId/DeserializeLockToken calls in the usecase layer. Made-with: Cursor
snehagunta
force-pushed
the
RHCLOUD-45005-tiny-types-lock-id-token
branch
from
31dbe28 to
202eb7c
Compare
Contributor
Author
Sanity Test ResultsBranch: Commit: Full test output |
Rajagopalan-Ranganathan
approved these changes
May 1, 2026
Contributor
|
/lgtm |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/service/tuples/tuples_test.go (1)
60-63: ⚡ Quick winAdd error-path coverage for the new lock/fencing validation.
These updates only verify successful conversion, but
toAcquireLockCommandandfencingCheckFromProtonow fail on empty or whitespace-only lock ids/tokens. A couple of negative cases here would lock in the main behavior change from this PR.Example test shape
func TestToAcquireLockCommand(t *testing.T) { - req := &pb.AcquireLockRequest{ - LockId: "lock-123", - } - - cmd, err := toAcquireLockCommand(req) - - require.NoError(t, err) - expectedLockId := model.DeserializeLockId("lock-123") - assert.Equal(t, expectedLockId, cmd.LockId) + t.Run("valid lock id", func(t *testing.T) { + req := &pb.AcquireLockRequest{LockId: "lock-123"} + cmd, err := toAcquireLockCommand(req) + require.NoError(t, err) + assert.Equal(t, model.DeserializeLockId("lock-123"), cmd.LockId) + }) + + t.Run("empty lock id", func(t *testing.T) { + _, err := toAcquireLockCommand(&pb.AcquireLockRequest{LockId: " "}) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid lock id") + }) }Also applies to: 132-135, 179-183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/tuples/tuples_test.go` around lines 60 - 63, Add negative test cases in tuples_test.go that exercise the new validation paths in toAcquireLockCommand and fencingCheckFromProto by passing empty string and whitespace-only values for lock IDs and tokens (e.g., call fencingCheckFromProto/toAcquireLockCommand with "" and " "). Assert that these calls return an error (and do not produce valid model.DeserializeLockId/model.DeserializeLockToken results) instead of succeeding; update the existing success checks around expectedLockId/expectedLockToken to remain as positive cases and add corresponding failure assertions for the empty/whitespace inputs (also add analogous negative assertions for the other test blocks covering the fencing conversion at the other locations).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/service/tuples/tuples_test.go`:
- Around line 60-63: Add negative test cases in tuples_test.go that exercise the
new validation paths in toAcquireLockCommand and fencingCheckFromProto by
passing empty string and whitespace-only values for lock IDs and tokens (e.g.,
call fencingCheckFromProto/toAcquireLockCommand with "" and " "). Assert that
these calls return an error (and do not produce valid
model.DeserializeLockId/model.DeserializeLockToken results) instead of
succeeding; update the existing success checks around
expectedLockId/expectedLockToken to remain as positive cases and add
corresponding failure assertions for the empty/whitespace inputs (also add
analogous negative assertions for the other test blocks covering the fencing
conversion at the other locations).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fe5f3b12-f5fa-4593-bf3e-ccc46a6ec5b0
📒 Files selected for processing (5)
internal/biz/usecase/tuples/commands.gointernal/biz/usecase/tuples/tuple_crud_usecase.gointernal/biz/usecase/tuples/tuple_crud_usecase_test.gointernal/service/tuples/tuples.gointernal/service/tuples/tuples_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
2 participants
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
stringwithmodel.LockIdandmodel.LockTokeninAcquireLockCommand,FencingCheck, andAcquireLockResultNewLockId/NewLockToken; remove redundantDeserializeLockId/DeserializeLockTokencalls in the usecase layerStack
3/4 in the tiny-types series. Depends on #1330 (merge that first).
Test plan
go build ./...passesgo test ./internal/...— all 36 packages passMade with Cursor
Summary by CodeRabbit
Refactor
Bug Fixes