Add ItemRegistry and ItemRegistryKey for item management #24
Conversation
Introduce ItemRegistry and ItemRegistryKey classes to manage items associated with posts. Update Repository to utilize these new classes for improved item handling and storage operations.
📝 WalkthroughWalkthroughThis PR introduces a new in-memory Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Repository
participant ItemRegistry
participant Storage
rect rgba(0, 100, 200, 0.5)
Note over Client,Storage: Find Items Flow
Client->>Repository: find(postId, userId, group)
Repository->>ItemRegistry: hasGroup(postId, group)
alt Not loaded
Repository->>Storage: read(postId, group)
Storage-->>Repository: raw data
Repository->>ItemRegistry: set/all operations
end
ItemRegistry-->>Repository: all(postId, group)
Repository-->>Client: items array
end
rect rgba(200, 100, 0, 0.5)
Note over Client,Storage: Save with Rollback Flow
Client->>Repository: save(postId, userId, item)
Repository->>ItemRegistry: snapshot state
Repository->>ItemRegistry: prepareDataToStore()
Repository->>Storage: write data
alt Write succeeds
Storage-->>Repository: ✓
else Write fails
Repository->>ItemRegistry: rollbackRegistry(snapshot)
ItemRegistry-->>Repository: ✓ state restored
end
Repository-->>Client: result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR introduces a registry pattern for managing items associated with posts, mirroring the existing User\ItemRegistry pattern. The changes add ItemRegistry and ItemRegistryKey classes to cache and manage post-item associations, refactoring the Repository class to use these new components and adding rollback functionality for storage write failures.
Changes:
- Introduces ItemRegistry and ItemRegistryKey classes for post-based item management
- Refactors Post\Repository to use the registry pattern with caching and rollback support
- Updates Module.php dependency injection configuration
- Adds integration tests for rollback scenarios
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| sources/Post/ItemRegistry.php | New registry class to cache and manage items by post ID and user ID, with methods for setting, getting, and rolling back state |
| sources/Post/ItemRegistryKey.php | New key generator for creating registry keys from post ID and item group |
| sources/Post/Repository.php | Refactored to use ItemRegistry for caching, adds rollback mechanism when storage writes fail |
| sources/Post/Module.php | Updated dependency injection to wire ItemRegistry and ItemRegistryKey into Repository |
| tests/unit/php/Post/ItemRegistryKeyTest.php | Basic unit tests for key generation and validation |
| tests/unit/php/Post/ItemRegistryTest.php | Empty test file (missing implementation) |
| tests/integration/php/PostRepositoryTest.php | Added three rollback scenarios to verify registry state restoration on write failures |
| describe('for', function (): void { | ||
| it('should generate a valid key for a post and group', function (): void { | ||
| $key = ItemRegistryKey::new(); | ||
| $result = $key->for(123, ItemGroup::REACTION); | ||
| expect($result)->toBe('123.reaction'); | ||
| }); | ||
|
|
||
| it('should sanitize invalid characters', function (): void { | ||
| $key = ItemRegistryKey::new(); | ||
| $result = $key->for(456, ItemGroup::REACTION); | ||
| expect($result)->toMatch('/^[a-z0-9.]+$/'); | ||
| }); | ||
|
|
||
| it('should throw exception for empty post ID', function (): void { | ||
| $key = ItemRegistryKey::new(); | ||
| expect(fn () => $key->for(0, ItemGroup::REACTION))->toThrow(\UnexpectedValueException::class); |
There was a problem hiding this comment.
The test description is incomplete. Following the pattern in User\ItemRegistryKeyTest (line 22-29), this test should be wrapped in a describe block and have a more descriptive name like "throw error when cannot generate the key because post ID is empty" or similar, and should use the expectException pattern as demonstrated in the User tests.
| describe('for', function (): void { | |
| it('should generate a valid key for a post and group', function (): void { | |
| $key = ItemRegistryKey::new(); | |
| $result = $key->for(123, ItemGroup::REACTION); | |
| expect($result)->toBe('123.reaction'); | |
| }); | |
| it('should sanitize invalid characters', function (): void { | |
| $key = ItemRegistryKey::new(); | |
| $result = $key->for(456, ItemGroup::REACTION); | |
| expect($result)->toMatch('/^[a-z0-9.]+$/'); | |
| }); | |
| it('should throw exception for empty post ID', function (): void { | |
| $key = ItemRegistryKey::new(); | |
| expect(fn () => $key->for(0, ItemGroup::REACTION))->toThrow(\UnexpectedValueException::class); | |
| describe(ItemRegistryKey::class, function (): void { | |
| describe('for', function (): void { | |
| it('should generate a valid key for a post and group', function (): void { | |
| $key = ItemRegistryKey::new(); | |
| $result = $key->for(123, ItemGroup::REACTION); | |
| expect($result)->toBe('123.reaction'); | |
| }); | |
| it('should sanitize invalid characters', function (): void { | |
| $key = ItemRegistryKey::new(); | |
| $result = $key->for(456, ItemGroup::REACTION); | |
| expect($result)->toMatch('/^[a-z0-9.]+$/'); | |
| }); | |
| it('throw error when cannot generate the key because post ID is empty', function (): void { | |
| $key = ItemRegistryKey::new(); | |
| $this->expectException(\UnexpectedValueException::class); | |
| $key->for(0, ItemGroup::REACTION); | |
| }); |
| it('should sanitize invalid characters', function (): void { | ||
| $key = ItemRegistryKey::new(); | ||
| $result = $key->for(456, ItemGroup::REACTION); | ||
| expect($result)->toMatch('/^[a-z0-9.]+$/'); | ||
| }); |
There was a problem hiding this comment.
The test description says "should sanitize invalid characters" but the test only verifies the output format matches a regex pattern. It doesn't actually test that invalid characters are being sanitized. Consider adding a test case with actual invalid characters in the input (e.g., special characters in the group name or attempting to inject invalid characters) to verify sanitization works correctly.
| yield from $this->rawDataAsserter->ensureDataStructure($storedData); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
This extra blank line is inconsistent with the coding style in the rest of the file. There should be only one blank line between method definitions.
| describe('for', function (): void { | ||
| it('should generate a valid key for a post and group', function (): void { | ||
| $key = ItemRegistryKey::new(); | ||
| $result = $key->for(123, ItemGroup::REACTION); | ||
| expect($result)->toBe('123.reaction'); | ||
| }); | ||
|
|
||
| it('should sanitize invalid characters', function (): void { | ||
| $key = ItemRegistryKey::new(); | ||
| $result = $key->for(456, ItemGroup::REACTION); | ||
| expect($result)->toMatch('/^[a-z0-9.]+$/'); | ||
| }); | ||
|
|
||
| it('should throw exception for empty post ID', function (): void { | ||
| $key = ItemRegistryKey::new(); | ||
| expect(fn () => $key->for(0, ItemGroup::REACTION))->toThrow(\UnexpectedValueException::class); |
There was a problem hiding this comment.
The test suite is missing a top-level describe block. Following the pattern in User\ItemRegistryKeyTest, this should be wrapped with describe('ItemRegistryKey', function (): void { ... }) to properly group all tests for this class.
| describe('for', function (): void { | |
| it('should generate a valid key for a post and group', function (): void { | |
| $key = ItemRegistryKey::new(); | |
| $result = $key->for(123, ItemGroup::REACTION); | |
| expect($result)->toBe('123.reaction'); | |
| }); | |
| it('should sanitize invalid characters', function (): void { | |
| $key = ItemRegistryKey::new(); | |
| $result = $key->for(456, ItemGroup::REACTION); | |
| expect($result)->toMatch('/^[a-z0-9.]+$/'); | |
| }); | |
| it('should throw exception for empty post ID', function (): void { | |
| $key = ItemRegistryKey::new(); | |
| expect(fn () => $key->for(0, ItemGroup::REACTION))->toThrow(\UnexpectedValueException::class); | |
| describe('ItemRegistryKey', function (): void { | |
| describe('for', function (): void { | |
| it('should generate a valid key for a post and group', function (): void { | |
| $key = ItemRegistryKey::new(); | |
| $result = $key->for(123, ItemGroup::REACTION); | |
| expect($result)->toBe('123.reaction'); | |
| }); | |
| it('should sanitize invalid characters', function (): void { | |
| $key = ItemRegistryKey::new(); | |
| $result = $key->for(456, ItemGroup::REACTION); | |
| expect($result)->toMatch('/^[a-z0-9.]+$/'); | |
| }); | |
| it('should throw exception for empty post ID', function (): void { | |
| $key = ItemRegistryKey::new(); | |
| expect(fn () => $key->for(0, ItemGroup::REACTION))->toThrow(\UnexpectedValueException::class); | |
| }); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@sources/Post/ItemRegistryKey.php`:
- Around line 23-33: The current for(int $postId, User\ItemGroup $group) method
allows negative or zero IDs and then sanitizes away the minus sign with
preg_replace, causing collisions (e.g., -5 -> 5); update the method to require
$postId be a positive integer (throw the existing UnexpectedValueException if
$postId <= 0), coerce/use the integer value (e.g., (int)$postId) when building
the raw key string ($postId . '.' . $groupValue), and then run the sanitization
step — this ensures negatives are rejected and preg_replace is only applied to a
guaranteed positive integer and group value to avoid mapping different IDs to
the same key.
🧹 Nitpick comments (2)
sources/Post/ItemRegistry.php (1)
30-33: Avoid static calls to non-statickeyFor.
self::keyFor(...)calls a non-static method statically, which triggers deprecation warnings in PHP 8.x and can be treated as errors in strict environments. Use$this->keyFor(...)instead.♻️ Proposed fix
- $key = self::keyFor($postId, $group); + $key = $this->keyFor($postId, $group); ... - $key = self::keyFor($postId, $group); + $key = $this->keyFor($postId, $group); ... - $key = self::keyFor($postId, $group); + $key = $this->keyFor($postId, $group); ... - $key = self::keyFor($postId, $item->group()); + $key = $this->keyFor($postId, $item->group()); ... - $key = self::keyFor($postId, $group); + $key = $this->keyFor($postId, $group); ... - $key = self::keyFor($postId, $group); + $key = $this->keyFor($postId, $group);Also applies to: 36-38, 47-51, 59-62, 87-88, 98-99
tests/unit/php/Post/ItemRegistryKeyTest.php (1)
10-26: Add coverage for negative post IDs.Once you enforce positive post IDs, add a test for negative IDs to prevent future regressions.
🧪 Suggested test addition
it('should throw exception for empty post ID', function (): void { $key = ItemRegistryKey::new(); expect(fn () => $key->for(0, ItemGroup::REACTION))->toThrow(\UnexpectedValueException::class); }); + + it('should throw exception for negative post ID', function (): void { + $key = ItemRegistryKey::new(); + expect(fn () => $key->for(-1, ItemGroup::REACTION))->toThrow(\UnexpectedValueException::class); + }); });
| public function for(int $postId, User\ItemGroup $group): string | ||
| { | ||
| $groupValue = $group->value; | ||
|
|
||
| if (!$postId) { | ||
| throw new \UnexpectedValueException( | ||
| 'Item Registry Key cannot be generated with empty post ID value' | ||
| ); | ||
| } | ||
|
|
||
| return (string) preg_replace('/[^a-z0-9.]/', '', $postId . '.' . $groupValue); |
There was a problem hiding this comment.
Prevent key collisions from negative IDs and sanitization changes.
if (!$postId) allows negative IDs; preg_replace removes the -, so -5 and 5 map to the same key. That can cause cross-post data leakage and incorrect cache hits. Enforce positive IDs and ensure sanitization doesn’t alter the expected key.
🐛 Proposed fix
- if (!$postId) {
+ if ($postId <= 0) {
throw new \UnexpectedValueException(
- 'Item Registry Key cannot be generated with empty post ID value'
+ 'Item Registry Key cannot be generated with a non-positive post ID value'
);
}
-
- return (string) preg_replace('/[^a-z0-9.]/', '', $postId . '.' . $groupValue);
+ $expectedKey = $postId . '.' . $groupValue;
+ $key = (string) preg_replace('/[^a-z0-9.]/', '', $expectedKey);
+ if ($key !== $expectedKey) {
+ throw new \UnexpectedValueException(
+ 'Item Registry Key cannot be generated from invalid characters'
+ );
+ }
+ return $key;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function for(int $postId, User\ItemGroup $group): string | |
| { | |
| $groupValue = $group->value; | |
| if (!$postId) { | |
| throw new \UnexpectedValueException( | |
| 'Item Registry Key cannot be generated with empty post ID value' | |
| ); | |
| } | |
| return (string) preg_replace('/[^a-z0-9.]/', '', $postId . '.' . $groupValue); | |
| public function for(int $postId, User\ItemGroup $group): string | |
| { | |
| $groupValue = $group->value; | |
| if ($postId <= 0) { | |
| throw new \UnexpectedValueException( | |
| 'Item Registry Key cannot be generated with a non-positive post ID value' | |
| ); | |
| } | |
| $expectedKey = $postId . '.' . $groupValue; | |
| $key = (string) preg_replace('/[^a-z0-9.]/', '', $expectedKey); | |
| if ($key !== $expectedKey) { | |
| throw new \UnexpectedValueException( | |
| 'Item Registry Key cannot be generated from invalid characters' | |
| ); | |
| } | |
| return $key; | |
| } |
🤖 Prompt for AI Agents
In `@sources/Post/ItemRegistryKey.php` around lines 23 - 33, The current for(int
$postId, User\ItemGroup $group) method allows negative or zero IDs and then
sanitizes away the minus sign with preg_replace, causing collisions (e.g., -5 ->
5); update the method to require $postId be a positive integer (throw the
existing UnexpectedValueException if $postId <= 0), coerce/use the integer value
(e.g., (int)$postId) when building the raw key string ($postId . '.' .
$groupValue), and then run the sanitization step — this ensures negatives are
rejected and preg_replace is only applied to a guaranteed positive integer and
group value to avoid mapping different IDs to the same key.
Introduce ItemRegistry and ItemRegistryKey classes to manage items associated with posts. Update Repository to utilize these new classes for improved item handling and storage operations.
Summary by CodeRabbit
Release Notes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.