feat: server repository crud - #25
Conversation
Add readonly data transfer object to represent server configuration with properties for name, host, port, username, and SSH private key path. Includes sensible defaults for optional properties (port: 22, username: root).
Replace server-specific terminology (servers, production, web1) with generic widget terminology (widgets, alpha, beta) in InventoryService documentation and tests to prevent confusion with the new ServerRepository implementation.
WalkthroughAdds a ServerRepository and ServerDTO; wires ServerRepository into BaseCommand and tests; initializes repository from InventoryService during command initialization. Updates InventoryService docs/examples and test fixtures to new example paths. Minor wording tweaks in internal .cursor review command docs. Introduces repository unit tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant C as BaseCommand
participant I as InventoryService
participant R as ServerRepository
U->>C: instantiate(command)
activate C
C->>I: load inventory (existing file/config)
C->>R: loadInventory(I)
note right of R: Repository bootstraps internal<br/>state from inventory (prefix 'servers')
deactivate C
sequenceDiagram
autonumber
participant R as ServerRepository
participant I as InventoryService
R->>R: create(ServerDTO)
alt name already exists
R-->>R: throw RuntimeException
else
R->>I: set servers.<name> = dehydrated array
R->>R: update in-memory cache
end
R->>R: findByName(name)
R-->>R: hydrate DTO or null
R->>R: all()
R-->>R: array<ServerDTO>
R->>R: delete(name)
R->>I: delete servers.<name>
R->>R: update cache
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (2)
app/Contracts/BaseCommand.php (1)
38-38: Consider the impact of mandatory ServerRepository dependency.All commands inheriting from BaseCommand now require a ServerRepository dependency, even if they don't use server management features. This is a reasonable trade-off for centralized initialization, but consider whether a more granular approach (e.g., traits or separate base classes) would better serve commands that don't need server management.
app/Repositories/ServerRepository.php (1)
65-76: Consider indexing servers by name for O(1) lookups.The current linear search is acceptable for small server counts but will degrade as the list grows. For better performance at scale, consider maintaining an internal associative array keyed by server name.
Example refactor:
final class ServerRepository { private const PREFIX = 'servers'; private ?InventoryService $inventory = null; - /** @var array<int, array<string, mixed>> */ - private array $servers = []; + /** @var array<string, ServerDTO> */ + private array $serversByName = []; public function loadInventory(InventoryService $inventory): void { $this->inventory = $inventory; $servers = $inventory->get(self::PREFIX); if (!is_array($servers)) { $servers = []; $inventory->set(self::PREFIX, $servers); } - /** @var array<int, array<string, mixed>> $servers */ - $this->servers = $servers; + $this->serversByName = []; + foreach ($servers as $server) { + if (is_array($server) && isset($server['name'])) { + $dto = $this->hydrateServerDTO($server); + $this->serversByName[$dto->name] = $dto; + } + } } public function findByName(string $name): ?ServerDTO { $this->assertInventoryLoaded(); - - foreach ($this->servers as $server) { - if (isset($server['name']) && $server['name'] === $name) { - return $this->hydrateServerDTO($server); - } - } - - return null; + return $this->serversByName[$name] ?? null; } public function all(): array { $this->assertInventoryLoaded(); - - $result = []; - foreach ($this->servers as $server) { - $result[] = $this->hydrateServerDTO($server); - } - - return $result; + return array_values($this->serversByName); } public function create(ServerDTO $server): void { $this->assertInventoryLoaded(); - $existing = $this->findByName($server->name); - if (null !== $existing) { + if (isset($this->serversByName[$server->name])) { throw new \RuntimeException("Server '{$server->name}' already exists"); } - $this->servers[] = $this->dehydrateServerDTO($server); - - $this->inventory->set(self::PREFIX, $this->servers); + $this->serversByName[$server->name] = $server; + $this->persistServers(); } public function delete(string $name): void { $this->assertInventoryLoaded(); - - $filtered = []; - foreach ($this->servers as $server) { - if (isset($server['name']) && $server['name'] !== $name) { - $filtered[] = $server; - } - } - - $this->servers = $filtered; - - $this->inventory->set(self::PREFIX, $this->servers); + unset($this->serversByName[$name]); + $this->persistServers(); + } + + private function persistServers(): void + { + $servers = []; + foreach ($this->serversByName as $dto) { + $servers[] = $this->dehydrateServerDTO($dto); + } + $this->inventory->set(self::PREFIX, $servers); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.cursor/commands/_review.md(1 hunks).cursor/commands/review-branch.md(1 hunks).cursor/commands/review-diff.md(1 hunks)app/Contracts/BaseCommand.php(3 hunks)app/DTOs/ServerDTO.php(1 hunks)app/Repositories/ServerRepository.php(1 hunks)app/Services/InventoryService.php(1 hunks)tests/Fixtures/TestConsoleCommand.php(2 hunks)tests/TestHelpers.php(3 hunks)tests/Unit/Contracts/BaseCommandTest.php(4 hunks)tests/Unit/DTOs/ServerDTOTest.php(1 hunks)tests/Unit/Repositories/ServerRepositoryTest.php(1 hunks)tests/Unit/Services/InventoryServiceTest.php(9 hunks)tests/Unit/TestHelpersTest.php(1 hunks)tests/Unit/Traits/ConsoleInputTraitTest.php(1 hunks)tests/Unit/Traits/ConsoleOutputTraitTest.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
**/*.php: Follow PSR-12 coding standard in all PHP files
Declare strict types in all PHP files (declare(strict_types=1);)
Use PHP 8.x features where appropriate (unions, match, attributes, readonly)
Use import statements (use ...) instead of fully qualified class names in code
All methods must declare explicit return types with proper generics (e.g., Collection<int, User>)
Prefer Dependency Injection over manual class resolution/instantiation
Use Symfony component classes (e.g., Filesystem, Process) instead of native PHP functions for easier mocking
All object creation must use $container->build(ClassName::class) except for value objects, DTOs, and pure data structures
Access the container via constructor injection in production code; tests may instantiate Container directly
Services must receive dependencies via constructor injection
Use SymfonyStyle consistently for all user-facing console output
Add DocBlock comments with minimalist descriptions, parameters, and return types for classes and functions
Use comments to separate sections and explain complex logic; avoid obvious or stale comments
Use the exact section header comment format with a single newline between headers/subheaders/paragraphs
**/*.php: Eliminate single-use private methods by inlining them directly
Cache expensive computed values by initializing them in the constructor instead of recomputing
Prefer direct property access over method calls when appropriate to avoid call overhead
Organize code into comment-separated sections; prefer alphabetical ordering when it does not conflict with logical grouping
Files:
tests/TestHelpers.phptests/Unit/TestHelpersTest.phpapp/DTOs/ServerDTO.phpapp/Services/InventoryService.phptests/Unit/DTOs/ServerDTOTest.phpapp/Contracts/BaseCommand.phptests/Fixtures/TestConsoleCommand.phptests/Unit/Traits/ConsoleOutputTraitTest.phptests/Unit/Services/InventoryServiceTest.phptests/Unit/Repositories/ServerRepositoryTest.phptests/Unit/Traits/ConsoleInputTraitTest.phpapp/Repositories/ServerRepository.phptests/Unit/Contracts/BaseCommandTest.php
{tests/**,test/**,**/*@(Test|Spec).php}
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
Do not run or edit tests unless explicitly instructed
Files:
tests/TestHelpers.phptests/Unit/TestHelpersTest.phptests/Unit/DTOs/ServerDTOTest.phptests/Fixtures/TestConsoleCommand.phptests/Unit/Traits/ConsoleOutputTraitTest.phptests/Unit/Services/InventoryServiceTest.phptests/Unit/Repositories/ServerRepositoryTest.phptests/Unit/Traits/ConsoleInputTraitTest.phptests/Unit/Contracts/BaseCommandTest.php
tests/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/02-tests.mdc)
tests/**/*.php: Use Pest exclusively with it() syntax for tests
In unit tests, instantiate classes manually with explicit mocks; use the container only for integration tests
Keep individual test files under ~1.8x the size of the source they cover
Test core business logic; avoid testing the framework itself
Prefer dataset-driven testing with ->with([...]) for multiple scenarios
Eliminate overlapping tests; avoid two tests covering the same functionality
Consolidate assertions with expect(...)->and(...) when appropriate
Mock external dependencies only; do not mock internal behavior
Avoid performance tests unless performance is the primary concern
Do not sacrifice readability to hit size/ratio targets
Follow the AAA pattern (Arrange, Act, Assert) in tests; use ACT & ASSERT for exception tests when act triggers assertion
Organize tests with describe() blocks, beforeEach() setup, and extract helpers/traits for DRY
Forbid meaningless assertions (e.g., toBeTrue, not->toBeNull, type-only checks, sleep(...)); use time mocking instead of sleep
Prefer meaningful assertions on behavior and results, and proper mocks (e.g., expect domain outputs; mock->shouldReceive(...))
Unit tests: mock all external dependencies; test single units in isolation; execute in milliseconds
Integration tests: perform real file operations/external processes; cover CLI commands and full workflows
Ignore PHPStan issues in tests; avoid excessive PHPDoc added only to appease types
Files:
tests/TestHelpers.phptests/Unit/TestHelpersTest.phptests/Unit/DTOs/ServerDTOTest.phptests/Fixtures/TestConsoleCommand.phptests/Unit/Traits/ConsoleOutputTraitTest.phptests/Unit/Services/InventoryServiceTest.phptests/Unit/Repositories/ServerRepositoryTest.phptests/Unit/Traits/ConsoleInputTraitTest.phptests/Unit/Contracts/BaseCommandTest.php
app/**/*Command.php
📄 CodeRabbit inference engine (.cursor/rules/03-commands.mdc)
app/**/*Command.php: Never use Symfony IO methods directly in commands; use BaseCommand custom methods (writeln, text, info, note, hr, h1, success, error, warning) exclusively
Use status helpers (success, error with optional tip, warning) for all status messages to ensure consistent formatting
Use laravel/prompts for all user interactions (text, password, confirm, select, multiselect, suggest, search, spin) instead of Symfony IO
Support both interactive prompts and CLI options; use getOptionOrPrompt for each option and showCommandHint to display the full non-interactive command
Use getOptionOrPrompt to detect provided options vs prompt interactively, leveraging Laravel Prompts parameters
Files:
app/Contracts/BaseCommand.php
app/Contracts/BaseCommand.php
📄 CodeRabbit inference engine (.cursor/rules/03-commands.mdc)
app/Contracts/BaseCommand.php: If missing output functionality, add a new, reusable, minimally-scoped, well-documented method to BaseCommand
Keep BaseCommand focused on shared initialization/configuration/orchestration; do not place individual I/O operations here
Files:
app/Contracts/BaseCommand.php
🧬 Code graph analysis (11)
tests/TestHelpers.php (2)
app/Repositories/ServerRepository.php (2)
ServerRepository(15-168)loadInventory(31-43)app/Services/InventoryService.php (1)
loadInventoryFile(97-111)
tests/Unit/TestHelpersTest.php (1)
app/Services/InventoryService.php (1)
get(67-73)
tests/Unit/DTOs/ServerDTOTest.php (1)
app/DTOs/ServerDTO.php (1)
ServerDTO(7-17)
app/Contracts/BaseCommand.php (1)
app/Repositories/ServerRepository.php (2)
ServerRepository(15-168)loadInventory(31-43)
tests/Fixtures/TestConsoleCommand.php (3)
app/Repositories/ServerRepository.php (1)
ServerRepository(15-168)app/Contracts/BaseCommand.php (1)
__construct(34-41)tests/Unit/Contracts/BaseCommandTest.php (1)
__construct(25-33)
tests/Unit/Traits/ConsoleOutputTraitTest.php (2)
tests/Fixtures/TestConsoleCommand.php (1)
TestConsoleCommand(22-85)tests/TestHelpers.php (2)
mockInventoryService(157-174)mockServerRepository(238-251)
tests/Unit/Services/InventoryServiceTest.php (1)
app/Services/InventoryService.php (2)
get(67-73)set(54-60)
tests/Unit/Repositories/ServerRepositoryTest.php (4)
app/DTOs/ServerDTO.php (1)
ServerDTO(7-17)app/Repositories/ServerRepository.php (6)
ServerRepository(15-168)all(83-93)loadInventory(31-43)create(48-60)findByName(65-76)delete(98-112)tests/TestHelpers.php (1)
mockInventoryService(157-174)app/Services/InventoryService.php (1)
loadInventoryFile(97-111)
tests/Unit/Traits/ConsoleInputTraitTest.php (2)
tests/Fixtures/TestConsoleCommand.php (1)
TestConsoleCommand(22-85)tests/TestHelpers.php (1)
mockServerRepository(238-251)
app/Repositories/ServerRepository.php (2)
app/DTOs/ServerDTO.php (1)
ServerDTO(7-17)app/Services/InventoryService.php (2)
InventoryService(33-264)set(54-60)
tests/Unit/Contracts/BaseCommandTest.php (5)
app/Repositories/ServerRepository.php (1)
ServerRepository(15-168)tests/TestHelpers.php (2)
__construct(49-61)mockServerRepository(238-251)app/Contracts/BaseCommand.php (1)
__construct(34-41)app/DTOs/ServerDTO.php (1)
__construct(9-16)tests/Fixtures/TestConsoleCommand.php (1)
__construct(28-35)
🔇 Additional comments (28)
app/Services/InventoryService.php (1)
14-31: LGTM! Documentation examples updated for consistency.The docblock examples have been updated to use
widgets.alphapaths instead ofservers.production, improving consistency with the test fixtures and avoiding confusion with the new ServerRepository which manages actual server data under aserversprefix.app/DTOs/ServerDTO.php (1)
9-15: Consider adding basic validation for required fields.The DTO accepts empty strings for
nameandhost, and any integer forport. While DTOs are typically pure data structures, consider whether validation logic should be added to prevent invalid server configurations from being created.For example, you might want to ensure:
nameandhostare non-empty stringsportis within valid range (1-65535)Should validation be added here or handled at the repository layer?
app/Contracts/BaseCommand.php (2)
8-8: LGTM! ServerRepository properly injected.The ServerRepository is correctly added as a constructor dependency following the dependency injection pattern per coding guidelines.
Also applies to: 38-38
95-99: LGTM! Repository initialization order is correct.The
servers->loadInventory()call is correctly placed afterinventory->loadInventoryFile()(line 94), ensuring the inventory service is loaded before the repository attempts to read from it.tests/Unit/Traits/ConsoleInputTraitTest.php (1)
16-16: LGTM! Test updated to match BaseCommand constructor.The TestConsoleCommand instantiation correctly adds the
mockServerRepository()dependency to align with the updated BaseCommand constructor signature.tests/Unit/TestHelpersTest.php (1)
147-153: LGTM! Test data aligned with documentation examples.The test expectations have been updated to use
widgets.alpha.colorpaths, maintaining consistency with the InventoryService docblock examples updated in this PR. The test continues to validate both array and string data formats correctly.tests/Unit/DTOs/ServerDTOTest.php (1)
1-36: LGTM! Clean DTO test coverage.The tests properly validate both the full constructor and default parameter behavior. The use of consolidated assertions with
expect()->and()is appropriate for testing multiple properties of the same object.tests/TestHelpers.php (2)
167-167: LGTM! Consistent test data update.The default test data path has been updated from
serverstowidgets, aligning with the broader test data restructuring across the test suite.
234-252: LGTM! Well-structured repository mock helper.The helper correctly:
- Creates and loads the inventory service
- Initializes the repository with loaded inventory
- Follows the existing helper patterns in the file
tests/Unit/Traits/ConsoleOutputTraitTest.php (1)
16-16: LGTM! Proper dependency injection.The ServerRepository is now correctly wired into the test command construction, aligning with the updated constructor signature.
tests/Unit/Services/InventoryServiceTest.php (1)
33-235: LGTM! Consistent test data path updates.All test data paths have been systematically updated from
servers.*towidgets.*throughout the test scenarios. The test logic and assertions remain sound, with only the example data paths changed.tests/Fixtures/TestConsoleCommand.php (2)
9-9: LGTM! Required import added.
28-35: LGTM! Constructor properly updated.The ServerRepository dependency is correctly added to the constructor and passed to the parent BaseCommand, maintaining consistency with the updated base class signature.
tests/Unit/Contracts/BaseCommandTest.php (3)
9-9: LGTM! Required import added.
25-33: LGTM! Test fixture properly updated.The TestableBaseCommand fixture now correctly accepts and passes the ServerRepository to the parent constructor, aligning with BaseCommand's updated signature.
54-99: LGTM! Tests properly updated with repository dependency.Both test cases correctly instantiate and wire the ServerRepository dependency using the
mockServerRepository()helper, maintaining test validity while accommodating the new constructor signature.tests/Unit/Repositories/ServerRepositoryTest.php (5)
15-22: LGTM! Proper state validation test.Correctly verifies that repository operations fail with a clear error when inventory is not loaded, enforcing the initialization contract.
28-68: LGTM! Comprehensive CRUD lifecycle coverage.This test thoroughly validates:
- Server creation with both full and minimal properties
- Retrieval by name with property verification
- Null return for missing servers
- Complete listing functionality
- Deletion behavior including graceful handling of non-existent entries
70-82: LGTM! Duplicate prevention properly enforced.Correctly validates that creating a server with an existing name throws a RuntimeException with a clear message.
88-120: LGTM! Robust malformed data handling.The parameterized test effectively validates graceful degradation when inventory data is malformed, covering:
- Missing required fields (name, host)
- Type mismatches (invalid port, wrong name type)
- Appropriate fallback to defaults
126-155: LGTM! Initialization edge cases covered.Both tests properly validate:
- Bootstrapping an empty server array when the key is missing
- Loading existing servers from inventory data
app/Repositories/ServerRepository.php (7)
31-43: LGTM! Proper inventory initialization.The method correctly:
- Stores the inventory reference
- Bootstraps an empty array if the 'servers' key doesn't exist
- Syncs internal state with persisted data
48-60: LGTM! Create with duplicate prevention.Properly enforces uniqueness by checking for existing servers before insertion, with a clear error message on conflict.
83-93: LGTM! Clean conversion to DTOs.Properly converts all stored servers to DTOs for public consumption.
98-112: LGTM! Delete with proper filtering.Correctly removes the server by filtering and persisting, gracefully handling non-existent names.
124-129: LGTM! Clear guard clause.Properly enforces initialization with a helpful error message and PHPStan assertion.
136-145: LGTM! Clean DTO serialization.Straightforward conversion of DTO properties to array for storage.
152-167: LGTM! Robust DTO hydration with type safety.Properly handles missing fields and type mismatches by:
- Providing sensible defaults
- Validating types before assignment
- Falling back to defaults on type mismatches
| readonly class ServerDTO | ||
| { | ||
| public function __construct( | ||
| public string $name, | ||
| public string $host, | ||
| public int $port = 22, | ||
| public string $username = 'root', | ||
| public ?string $privateKeyPath = null, | ||
| ) { | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add class-level DocBlock.
The ServerDTO class is missing a DocBlock comment. As per coding guidelines, all classes should have DocBlock comments with minimalist descriptions.
Apply this diff to add a DocBlock:
+/**
+ * Server configuration data transfer object.
+ */
readonly class ServerDTO
{📝 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.
| readonly class ServerDTO | |
| { | |
| public function __construct( | |
| public string $name, | |
| public string $host, | |
| public int $port = 22, | |
| public string $username = 'root', | |
| public ?string $privateKeyPath = null, | |
| ) { | |
| } | |
| } | |
| /** | |
| * Server configuration data transfer object. | |
| */ | |
| readonly class ServerDTO | |
| { | |
| public function __construct( | |
| public string $name, | |
| public string $host, | |
| public int $port = 22, | |
| public string $username = 'root', | |
| public ?string $privateKeyPath = null, | |
| ) { | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/DTOs/ServerDTO.php around lines 7 to 17, the readonly ServerDTO class
lacks a class-level DocBlock; add a minimalist DocBlock immediately above the
class declaration containing a one-line description of the DTO (e.g., "Data
transfer object representing an SSH server") and optional @package or @author
tags per project conventions, ensuring standard PHPDoc syntax (/** ... */).
1 participant
Summary by CodeRabbit
New Features
Documentation
Refactor
Tests
Chores