Add typed Version object
#821
Conversation
norkunas
added
enhancement
WalkthroughIntroduces a readonly Version value object with a fromArray factory; changes HandlesSystem::version() to return Version; adds explicit Changes
Sequence Diagram(s)sequenceDiagram
participant Test as ClientTest
participant Client as Meilisearch Client (HandlesSystem)
participant Delegate as Remote delegate (show())
participant Version as Meilisearch\Contracts\Version
Test->>Client: version()
Client->>Delegate: show()
Delegate-->>Client: {commitSha, commitDate, pkgVersion} (array)
Client->>Version: Version::fromArray(array)
Version->>Version: parse commitDate -> DateTimeImmutable
Version-->>Client: Version object
Client-->>Test: Version object
Note right of Test: Test calls getCommitSha(), getPkgVersion()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #821 +/- ##
==========================================
- Coverage 89.78% 88.01% -1.77%
==========================================
Files 59 81 +22
Lines 1449 1736 +287
==========================================
+ Hits 1301 1528 +227
- Misses 148 208 +60 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 (1)
tests/Endpoints/ClientTest.php (1)
202-208: Consider adding test coverage forgetCommitDate().The test validates
getCommitSha()andgetPkgVersion(), but thegetCommitDate()getter is not exercised. Since this is part of the new public API surface, consider adding an assertion to verify that it returns a validDateTimeImmutableinstance.Apply this diff to add validation for
getCommitDate():public function testVersion(): void { $response = $this->client->version(); self::assertMatchesRegularExpression('/^[0-9a-f]{40}$/i', $response->getCommitSha()); self::assertGreaterThanOrEqual(0, version_compare($response->getPkgVersion(), '1.26.0')); + self::assertInstanceOf(\DateTimeImmutable::class, $response->getCommitDate()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Contracts/Http.php(1 hunks)src/Contracts/Version.php(1 hunks)src/Endpoints/Delegates/HandlesSystem.php(1 hunks)src/Http/Client.php(5 hunks)tests/Endpoints/ClientTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/Contracts/Http.php (5)
src/Http/Client.php (5)
get(69-77)post(87-98)put(108-119)patch(121-129)delete(131-139)src/Endpoints/Tasks.php (1)
get(20-23)src/Endpoints/Batches.php (1)
get(13-16)src/Endpoints/Keys.php (2)
get(156-161)delete(208-211)src/Endpoints/Indexes.php (1)
delete(147-153)
src/Http/Client.php (4)
src/Contracts/Http.php (5)
get(16-16)post(24-24)put(32-32)patch(38-38)delete(44-44)src/Endpoints/Tasks.php (1)
get(20-23)src/Endpoints/Keys.php (2)
get(156-161)delete(208-211)src/Endpoints/Indexes.php (1)
delete(147-153)
tests/Endpoints/ClientTest.php (1)
src/Contracts/Version.php (2)
getCommitSha(23-26)getPkgVersion(33-36)
src/Endpoints/Delegates/HandlesSystem.php (2)
src/Contracts/Version.php (2)
Version(7-53)fromArray(45-52)src/Contracts/Endpoint.php (1)
show(19-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Upload coverage to Codecov
🔇 Additional comments (5)
src/Contracts/Http.php (1)
16-44: LGTM! Return type declarations improve type safety.The addition of
: mixedreturn types to the HTTP interface methods is appropriate given that different endpoints return different types (arrays, objects, null). This breaking change aligns with the broader refactoring to introduce typed objects likeVersion.src/Contracts/Version.php (3)
7-18: LGTM! Well-designed immutable value object.The use of
finalclass andreadonlyproperties follows best practices for value objects, ensuring immutability and preventing inheritance. The type declarations and PHPDoc annotations provide good static analysis support.
23-36: LGTM! Getters follow standard conventions.The getter methods provide clean access to the readonly properties with appropriate return types. Using
DateTimeImmutableensures that returned date objects cannot be modified externally.
45-52: Remove validation suggestion—it contradicts established SDK architecture.The review recommends adding validation to
fromArray(), but verification shows the codebase uses a two-layer error handling model:
- HTTP layer validation: Client.php handles response validation and throws ApiException or InvalidResponseBodyException before data reaches factory methods
- Factory assumption: All 15+
fromArray()implementations assume pre-validated input, including Task.php which also instantiates DateTimeImmutable without try-catchVersion::fromArray is called from HandlesSystem.php with data that has already passed HTTP validation (json_decode with JSON_THROW_ON_ERROR, status checks). Adding local validation would duplicate upstream work and create inconsistency across the SDK. This is deliberate SDK design, not an oversight.
Likely an incorrect or invalid review comment.
src/Http/Client.php (1)
69-131: LGTM! Implementation correctly matches interface contract.The addition of
: mixedreturn types to the HTTP method implementations is consistent with the updatedHttpinterface. No behavioral changes were made—only type declarations were added.
norkunas
force-pushed
the
version
branch
6 times, most recently
from
1614bd9 to
50c393b
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Endpoints/Delegates/HandlesSystem.php (2)
38-47: Partial fix: Missing keys and invalid date format still unhandled.The previous review concern was marked as addressed, but only the null response scenario is now handled (lines 42-44). The
Version::fromArray()implementation still lacks protection against:
- Missing array keys: Direct access to
$data['commitSha'],$data['commitDate'], and$data['pkgVersion']will cause undefined array key errors- Invalid date format:
new \DateTimeImmutable($data['commitDate'])can throw exceptions on malformed datesThe previous review recommended mirroring the error handling pattern in
Indexes::parseDate()with try-catch blocks and adding a@throwsannotation.Consider adding comprehensive validation:
public function version(): VersionContract { $version = $this->version->show(); if (!\is_array($version)) { throw new LogicException('Version did not respond with valid data.'); } + + // Validate required keys + $requiredKeys = ['commitSha', 'commitDate', 'pkgVersion']; + foreach ($requiredKeys as $key) { + if (!isset($version[$key]) || $version[$key] === '') { + throw new LogicException("Version response missing required key: {$key}"); + } + } + + // Validate date format + try { + return VersionContract::fromArray($version); + } catch (\Exception $e) { + throw new LogicException('Failed to parse version data: ' . $e->getMessage(), 0, $e); + } - - return VersionContract::fromArray($version); }Alternatively, move this validation logic into
Version::fromArray()itself.
38-38: Add@throwsdocumentation for theversion()method.The method can throw
LogicException(line 43) and potentially other exceptions fromVersion::fromArray(). Add a docblock with@throwsannotation to document this behavior for API consumers.+ /** + * @throws LogicException If the version endpoint returns invalid data + */ public function version(): VersionContract
🧹 Nitpick comments (1)
tests/Contracts/VersionTest.php (1)
25-36: Consider adding edge case tests for error scenarios.The current tests only validate the happy path. Consider adding test cases for:
- Malformed
commitDatestrings that cannot be parsed byDateTimeImmutable- Missing required array keys (
commitSha,commitDate,pkgVersion)- Empty or invalid values for the non-empty-string typed fields
These tests would ensure that
Version::fromArray()handles invalid input appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Contracts/Http.php(1 hunks)src/Contracts/Version.php(1 hunks)src/Endpoints/Delegates/HandlesSystem.php(2 hunks)src/Http/Client.php(5 hunks)tests/Contracts/VersionTest.php(1 hunks)tests/Endpoints/ClientTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/Endpoints/ClientTest.php
- src/Contracts/Version.php
- src/Http/Client.php
- src/Contracts/Http.php
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Contracts/VersionTest.php (1)
src/Contracts/Version.php (5)
Version(7-53)getCommitSha(23-26)getCommitDate(28-31)getPkgVersion(33-36)fromArray(45-52)
🔇 Additional comments (2)
tests/Contracts/VersionTest.php (1)
12-23: LGTM!The test correctly validates direct construction of the Version object and all getter methods. The use of
assertSamefor the date comparison is appropriate since it references the same object instance.src/Endpoints/Delegates/HandlesSystem.php (1)
8-8: LGTM!The import statements are appropriate. Aliasing
VersionasVersionContractavoids naming conflicts with theVersionendpoint class, andLogicExceptionis properly used for validation.Also applies to: 13-13
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/Contracts/VersionTest.php (1)
25-36: Consider adding edge case tests.The current tests cover happy paths well. Consider adding tests for error scenarios to improve robustness:
- Invalid date format in
fromArray(e.g.,'commitDate' => 'invalid')- Missing required keys in
fromArrayarray- Empty strings for
commitShaandpkgVersion(given thenon-empty-stringconstraint)Would you like me to generate additional test cases for these scenarios?
src/Contracts/Version.php (1)
33-36: Add @return annotation for consistency.The
getPkgVersionmethod should include a@return non-empty-stringannotation to match the consistency ofgetCommitShaand align with the constructor's parameter type hint.Apply this diff:
+ /** + * @return non-empty-string + */ public function getPkgVersion(): string
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Contracts/Http.php(1 hunks)src/Contracts/Version.php(1 hunks)src/Endpoints/Delegates/HandlesSystem.php(2 hunks)src/Http/Client.php(5 hunks)tests/Contracts/VersionTest.php(1 hunks)tests/Endpoints/ClientTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Endpoints/Delegates/HandlesSystem.php
- tests/Endpoints/ClientTest.php
🧰 Additional context used
🧬 Code graph analysis (4)
src/Contracts/Version.php (2)
src/Meilisearch.php (1)
Meilisearch(7-18)src/Http/Client.php (1)
__construct(43-62)
tests/Contracts/VersionTest.php (1)
src/Contracts/Version.php (5)
Version(7-53)getCommitSha(23-26)getCommitDate(28-31)getPkgVersion(33-36)fromArray(45-52)
src/Contracts/Http.php (1)
src/Http/Client.php (5)
get(69-77)post(87-98)put(108-119)patch(121-129)delete(131-139)
src/Http/Client.php (3)
src/Endpoints/Tasks.php (1)
get(20-23)src/Endpoints/Keys.php (2)
get(156-161)delete(208-211)src/Endpoints/Indexes.php (1)
delete(147-153)
🔇 Additional comments (3)
tests/Contracts/VersionTest.php (1)
5-5: Namespace correctly updated.The namespace now properly aligns with the PSR-4 file path. This resolves the previous review concern.
src/Contracts/Http.php (1)
16-16: LGTM! Return types properly specified.The explicit
: mixedreturn types appropriately reflect the varied response types (arrays, null, objects) returned by these HTTP methods and align with the implementation inClient.Also applies to: 24-24, 32-32, 38-38, 44-44
src/Http/Client.php (1)
69-69: LGTM! Implementation correctly matches interface.The explicit
: mixedreturn types properly implement the updatedHttpinterface contract and appropriately reflect the varied response types returned by these methods.Also applies to: 87-87, 108-108, 121-121, 131-131
| public static function fromArray(array $data): Version | ||
| { | ||
| return new self( | ||
| $data['commitSha'], | ||
| new \DateTimeImmutable($data['commitDate']), | ||
| $data['pkgVersion'], | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add validation and error handling in fromArray.
The fromArray method directly accesses array keys without validating their existence and constructs DateTimeImmutable without error handling. This can lead to runtime errors if:
- The API response structure changes or keys are missing
- The
commitDatestring is in an invalid format
Consider adding validation:
public static function fromArray(array $data): Version
{
+ if (!isset($data['commitSha'], $data['commitDate'], $data['pkgVersion'])) {
+ throw new \InvalidArgumentException('Missing required keys in version data');
+ }
+
+ try {
+ $commitDate = new \DateTimeImmutable($data['commitDate']);
+ } catch (\Exception $e) {
+ throw new \InvalidArgumentException('Invalid commitDate format: ' . $e->getMessage(), 0, $e);
+ }
+
return new self(
$data['commitSha'],
- new \DateTimeImmutable($data['commitDate']),
+ $commitDate,
$data['pkgVersion'],
);
}🤖 Prompt for AI Agents
In src/Contracts/Version.php around lines 45 to 52, the fromArray method assumes
keys exist and that commitDate is valid; add explicit validation for required
keys ('commitSha','commitDate','pkgVersion') and their types, throw a clear
InvalidArgumentException when a key is missing or has an unexpected type, and
wrap DateTimeImmutable construction in a try/catch to convert parsing errors
into a descriptive exception (or validate the date format first) so the method
fails fast with a meaningful message instead of producing uncaught runtime
errors.
There was a problem hiding this comment.
Static analysis is for that. It's a soft contract
There was a problem hiding this comment.
@norkunas, understood! I see this aligns with the project's established approach where docblock type annotations serve as contracts for static analysis tools. The method signature already documents the expected array structure with non-empty-string types, which is sufficient for static analysis to catch issues.
Thank you for the clarification!
🧠 Learnings used
Learnt from: norkunas
Repo: meilisearch/meilisearch-php PR: 735
File: src/Contracts/SimilarDocumentsQuery.php:10-12
Timestamp: 2025-11-17T06:57:43.914Z
Learning: In the meilisearch-php project, docblock type annotations (such as `non-empty-string`, `positive-int`, etc.) are treated as contracts for static analysis tools rather than requiring runtime validation. If users violate these documented constraints, it's their responsibility.
2 participants
Pull Request
Related issue
Fixes #745
What does this PR do?
mixedreturn type toHttpclient/interfacePR checklist
Please check if your PR fulfills the following requirements:
Summary by CodeRabbit
Breaking Changes
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.