Conversation
norkunas
added
enhancement
📝 WalkthroughWalkthroughReplaces mutable, array-based key handling with immutable value objects and typed query contracts; converts the Keys endpoint into a thin HTTP wrapper returning Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #903 +/- ##
==========================================
- Coverage 89.78% 87.89% -1.89%
==========================================
Files 59 85 +26
Lines 1449 1793 +344
==========================================
+ Hits 1301 1576 +275
- Misses 148 217 +69 ☔ 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: 3
🧹 Nitpick comments (2)
tests/Endpoints/KeysAndPermissionsTest.php (1)
189-192: Redundant assertions flagged by PHPStan.These
assertArrayHasKeycalls are flagged becausegetRawKeys()already declares a return type that guarantees these keys exist. While harmless, they add no value to test coverage.Consider removing them or keeping only the value assertions.
Proposed simplification
$raw = $this->client->getRawKeys(); - self::assertArrayHasKey('offset', $raw); - self::assertArrayHasKey('limit', $raw); - self::assertArrayHasKey('total', $raw); - self::assertArrayHasKey('results', $raw); self::assertSame(0, $raw['offset']); self::assertSame(20, $raw['limit']); self::assertSame(2, $raw['total']);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Endpoints/KeysAndPermissionsTest.php` around lines 189 - 192, Remove the redundant assertArrayHasKey assertions for 'offset', 'limit', 'total', and 'results' in KeysAndPermissionsTest since getRawKeys()'s return type guarantees those keys; instead keep or add assertions that verify the actual values in $raw (e.g., value/content checks) or only assert on behaviour, referencing the getRawKeys() call and the $raw variable to locate the assertions to delete.src/Contracts/Key.php (1)
40-48: Consider adding PHPDoc return types to array getters.
getActions()andgetIndexes()return untyped arrays. Adding@returnannotations would improve IDE support and static analysis.Proposed improvement
+ /** + * `@return` list<KeyAction> + */ public function getActions(): array { return $this->actions; } + /** + * `@return` list<non-empty-string> + */ public function getIndexes(): array { return $this->indexes; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Contracts/Key.php` around lines 40 - 48, The getters getActions() and getIndexes() return untyped arrays—add PHPDoc `@return` annotations above each method to improve IDE/static analysis; if you know the element types, use a precise annotation like `@return` Action[] or `@return` Index[] respectively, otherwise use `@return` mixed[] (or array<string,mixed>) to indicate an array is returned; update the PHPDoc for the methods getActions() and getIndexes() in class Key to include these annotations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Contracts/CreateKeyRequest.php`:
- Around line 30-54: The PHPDoc for CreateKeyRequest::toArray() incorrectly
marks description, name, expiresAt, and uid as always non-empty strings; update
the `@return` annotation to reflect that description and name may be null
(nullable non-empty-string), expiresAt may be null (nullable non-empty-string)
and uid is optional/nullable (either absent or nullable non-empty-string) so
static analyzers like PHPStan see the real shape returned by toArray(); locate
the toArray() method and adjust the array shape types accordingly to match the
payload generation logic (actions: list<non-empty-string>, indexes:
list<non-empty-string>, description: ?non-empty-string, name: ?non-empty-string,
uid?: non-empty-string|null, expiresAt: ?non-empty-string).
In `@src/Contracts/UpdateKeyRequest.php`:
- Around line 24-36: The PHPDoc for UpdateKeyRequest::toArray incorrectly
declares both 'name' and 'description' as non-empty-string while the method
returns $this->name and $this->description which are nullable (?string); update
the `@return` annotation to reflect their nullable types (e.g., array{name:
?string, description: ?string}) or alternatively ensure the method
normalizes/validates and returns non-empty strings before keeping the current
PHPDoc — modify either the PHPDoc or the toArray logic for consistency with
$this->name and $this->description.
In `@src/Endpoints/Keys.php`:
- Around line 43-57: The Key::fromArray() path currently uses KeyAction::from()
which will throw ValueError for unknown action strings; update Key::fromArray()
(the code invoked by Keys::get and Keys::all) to use
KeyAction::tryFrom($actionString) and provide a safe fallback when tryFrom
returns null (e.g., preserve the original string in a neutral enum-like value,
set a default 'unknown' marker, or store the raw action string) so new/unknown
Meilisearch actions don't throw at runtime; adjust any downstream consumers that
expect a KeyAction instance to handle the fallback case.
---
Nitpick comments:
In `@src/Contracts/Key.php`:
- Around line 40-48: The getters getActions() and getIndexes() return untyped
arrays—add PHPDoc `@return` annotations above each method to improve IDE/static
analysis; if you know the element types, use a precise annotation like `@return`
Action[] or `@return` Index[] respectively, otherwise use `@return` mixed[] (or
array<string,mixed>) to indicate an array is returned; update the PHPDoc for the
methods getActions() and getIndexes() in class Key to include these annotations.
In `@tests/Endpoints/KeysAndPermissionsTest.php`:
- Around line 189-192: Remove the redundant assertArrayHasKey assertions for
'offset', 'limit', 'total', and 'results' in KeysAndPermissionsTest since
getRawKeys()'s return type guarantees those keys; instead keep or add assertions
that verify the actual values in $raw (e.g., value/content checks) or only
assert on behaviour, referencing the getRawKeys() call and the $raw variable to
locate the assertions to delete.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e35bbfdc-0bdb-4ffa-92a1-72a29caf27ce
📒 Files selected for processing (10)
src/Contracts/CreateKeyRequest.phpsrc/Contracts/Key.phpsrc/Contracts/KeyAction.phpsrc/Contracts/KeysResults.phpsrc/Contracts/UpdateKeyRequest.phpsrc/Endpoints/Delegates/HandlesKeys.phpsrc/Endpoints/Keys.phptests/Endpoints/KeysAndPermissionsTest.phptests/Endpoints/TenantTokenTest.phptests/TestCase.php
💤 Files with no reviewable changes (1)
- tests/TestCase.php
norkunas
force-pushed
the
typed-keys
branch
3 times, most recently
from
f6a2586 to
2105e98
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Endpoints/Keys.php`:
- Around line 60-71: Update the PHPDoc for the allRaw method so the options
array uses non-negative-int for offset instead of positive-int to allow 0 and
match the response type; locate the allRaw(array $options = []) method in
Keys.php and change the `@param` declaration's offset type from positive-int to
non-negative-int so the input and response types are consistent.
- Around line 15-33: The PHPStan type RawKey's expiresAt currently uses
"expiresAt?: non-empty-string" which omits the documented null case and
incorrectly marks the field optional; update the RawKey phpdoc (the
`@phpstan-type` RawKey block) to declare "expiresAt: non-empty-string|null" (i.e.,
a required field that can be a non-empty RFC3339 string or null) so it matches
Meilisearch responses; ensure any references to RawKey/RawKeys remain
consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 08b762f4-fedd-41aa-ba93-a428d94194c4
📒 Files selected for processing (10)
src/Contracts/CreateKeyRequest.phpsrc/Contracts/Key.phpsrc/Contracts/KeyAction.phpsrc/Contracts/KeysResults.phpsrc/Contracts/UpdateKeyRequest.phpsrc/Endpoints/Delegates/HandlesKeys.phpsrc/Endpoints/Keys.phptests/Endpoints/KeysAndPermissionsTest.phptests/Endpoints/TenantTokenTest.phptests/TestCase.php
💤 Files with no reviewable changes (1)
- tests/TestCase.php
✅ Files skipped from review due to trivial changes (2)
- src/Contracts/UpdateKeyRequest.php
- src/Contracts/KeyAction.php
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/Endpoints/TenantTokenTest.php
- src/Contracts/CreateKeyRequest.php
- src/Endpoints/Delegates/HandlesKeys.php
- src/Contracts/KeysResults.php
- src/Contracts/Key.php
- tests/Endpoints/KeysAndPermissionsTest.php
norkunas
force-pushed
the
typed-keys
branch
2 times, most recently
from
26ea7cb to
f57a074
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/Contracts/CreateKeyRequest.php (1)
30-38:⚠️ Potential issue | 🟡 Minor
toArray()'s return shape still treats nullable fields as strings.
description,name, andexpiresAtare always present in the payload, but each can benullhere. The current@returnshape overstates the contract and will give static analysis the wrong signal.Possible fix
/** * `@return` array{ * actions: list<non-empty-string>, * indexes: list<non-empty-string>, - * description: non-empty-string, - * name: non-empty-string, - * expiresAt: non-empty-string, + * description: non-empty-string|null, + * name: non-empty-string|null, + * expiresAt: non-empty-string|null, * uid?: non-empty-string * } */Based on learnings, these PHPDoc shapes are treated as static-analysis contracts in this repo.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Contracts/CreateKeyRequest.php` around lines 30 - 38, The PHPDoc return shape on CreateKeyRequest::toArray currently declares description, name, and expiresAt as non-empty-string but those fields can be null; update the docblock for toArray in src/Contracts/CreateKeyRequest.php so description, name, and expiresAt are typed as non-empty-string|null (e.g. description: non-empty-string|null, name: non-empty-string|null, expiresAt: non-empty-string|null) while keeping actions and indexes as list<non-empty-string> and uid as an optional non-empty-string to accurately reflect the nullable contract for static analysis.src/Endpoints/Keys.php (1)
15-25:⚠️ Potential issue | 🟡 Minor
RawKeyis still too strict for nullable response fields.The alias says
name,description, andexpiresAtare strings-only, but the key endpoints expose them as nullable fields, andKey::fromArray()already normalizes them that way. KeepingRawKeystricter than the deserializer leaks a false contract intoget()andallRaw(). (meilisearch.com)Possible fix
* `@phpstan-type` RawKey array{ * uid: non-empty-string, * key: non-empty-string, * actions: list<non-empty-string>, * indexes: list<non-empty-string>, - * name?: non-empty-string, - * description?: non-empty-string, - * expiresAt: non-empty-string, + * name?: non-empty-string|null, + * description?: non-empty-string|null, + * expiresAt: non-empty-string|null, * createdAt: non-empty-string, * updatedAt: non-empty-string * }Based on learnings, these PHPDoc shapes are treated as static-analysis contracts in this repo.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Endpoints/Keys.php` around lines 15 - 25, The RawKey phpstan-type is too strict: change the types for the nullable fields to allow null to match API responses and the Key::fromArray() normalizer—update the phpstan-type alias (RawKey) so name and description and expiresAt are typed as non-empty-string|null (or ?non-empty-string) instead of only non-empty-string; ensure get() and allRaw() now reflect this loosened contract so static analysis aligns with the deserializer behavior in Key::fromArray().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Contracts/CreateKeyRequest.php`:
- Line 47: The expiresAt formatting in CreateKeyRequest is using the erroneous
format string 'Y-m-d\TH:i:s.vu\Z' which concatenates milliseconds and
microseconds; update the expiresAt->format call in CreateKeyRequest (the
expiresAt serialization line) to use a correct RFC3339-safe format: prefer
'Y-m-d\TH:i:s.uP' to emit microseconds with an RFC3339 timezone offset, or if
the datetime is guaranteed UTC use 'Y-m-d\TH:i:s.u\Z' instead.
In `@src/Endpoints/Keys.php`:
- Around line 61-64: Update the PHPDoc for the allRaw() method so the
$options['limit'] type reflects the Meilisearch API (allowing 0); change the
`limit` annotation from `positive-int` to `non-negative-int` in the docblock for
allRaw() to match other pagination methods and the API contract, ensuring the
array shape remains the same and only the type token is replaced.
---
Duplicate comments:
In `@src/Contracts/CreateKeyRequest.php`:
- Around line 30-38: The PHPDoc return shape on CreateKeyRequest::toArray
currently declares description, name, and expiresAt as non-empty-string but
those fields can be null; update the docblock for toArray in
src/Contracts/CreateKeyRequest.php so description, name, and expiresAt are typed
as non-empty-string|null (e.g. description: non-empty-string|null, name:
non-empty-string|null, expiresAt: non-empty-string|null) while keeping actions
and indexes as list<non-empty-string> and uid as an optional non-empty-string to
accurately reflect the nullable contract for static analysis.
In `@src/Endpoints/Keys.php`:
- Around line 15-25: The RawKey phpstan-type is too strict: change the types for
the nullable fields to allow null to match API responses and the
Key::fromArray() normalizer—update the phpstan-type alias (RawKey) so name and
description and expiresAt are typed as non-empty-string|null (or
?non-empty-string) instead of only non-empty-string; ensure get() and allRaw()
now reflect this loosened contract so static analysis aligns with the
deserializer behavior in Key::fromArray().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 59bbec37-d1b8-4207-b3b4-8884f622db3a
📒 Files selected for processing (10)
src/Contracts/CreateKeyRequest.phpsrc/Contracts/Key.phpsrc/Contracts/KeyAction.phpsrc/Contracts/KeysResults.phpsrc/Contracts/UpdateKeyRequest.phpsrc/Endpoints/Delegates/HandlesKeys.phpsrc/Endpoints/Keys.phptests/Endpoints/KeysAndPermissionsTest.phptests/Endpoints/TenantTokenTest.phptests/TestCase.php
💤 Files with no reviewable changes (1)
- tests/TestCase.php
✅ Files skipped from review due to trivial changes (4)
- src/Contracts/Key.php
- src/Contracts/UpdateKeyRequest.php
- tests/Endpoints/KeysAndPermissionsTest.php
- src/Contracts/KeysResults.php
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Endpoints/Delegates/HandlesKeys.php
- src/Contracts/KeyAction.php
| 'indexes' => $this->indexes, | ||
| 'description' => $this->description, | ||
| 'name' => $this->name, | ||
| 'expiresAt' => $this->expiresAt?->format('Y-m-d\TH:i:s.vu\Z'), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the file
find . -name "CreateKeyRequest.php" -type fRepository: meilisearch/meilisearch-php
Length of output: 107
🏁 Script executed:
# Also search for any related test files
find . -path "*/test*" -name "*CreateKeyRequest*" -o -path "*/spec*" -name "*CreateKeyRequest*"Repository: meilisearch/meilisearch-php
Length of output: 53
🏁 Script executed:
# Let's verify the PHP datetime format documentation claims
# We'll check what 'v', 'u', and 'Z' actually do
cat <<'EOF' > /tmp/php_datetime_test.php
<?php
// Test PHP datetime format characters
// Create a sample datetime with microseconds
$dt = new DateTimeImmutable('2024-04-15 10:30:45.123456', new DateTimeZone('UTC'));
echo "Original datetime: " . $dt->format('Y-m-d H:i:s.u') . "\n";
echo "\nFormat character meanings:\n";
echo "'v' (milliseconds): " . $dt->format('v') . "\n";
echo "'u' (microseconds): " . $dt->format('u') . "\n";
echo "'Z' (timezone offset in seconds): " . $dt->format('Z') . "\n";
echo "\nFull formats:\n";
echo "Current (v+u): " . $dt->format('Y-m-d\TH:i:s.vu\Z') . "\n";
echo "Proposed (u only): " . $dt->format('Y-m-d\TH:i:s.u\Z') . "\n";
echo "Proposed (p timezone): " . $dt->format('Y-m-d\TH:i:s.up') . "\n";
// Test with non-UTC timezone
$dtNonUtc = new DateTimeImmutable('2024-04-15 10:30:45.123456', new DateTimeZone('America/New_York'));
echo "\n\nNon-UTC timezone (America/New_York):\n";
echo "Current (v+u): " . $dtNonUtc->format('Y-m-d\TH:i:s.vu\Z') . "\n";
echo "Proposed (u+p): " . $dtNonUtc->format('Y-m-d\TH:i:s.up') . "\n";
EOF
# Run it through PHP if available
php /tmp/php_datetime_test.php 2>/dev/null || echo "PHP not available, checking documentation instead"Repository: meilisearch/meilisearch-php
Length of output: 440
🌐 Web query:
PHP DateTime format 'v' 'u' 'Z' characters meaning documentation
💡 Result:
In PHP DateTimeInterface::format() (and date_format()), these format characters mean:
u: Microseconds (6 digits). Note:date()always outputs000000because it only accepts an integer timestamp;DateTimeInterface::format()can output real microseconds if theDateTimewas created with them. [1]v: Milliseconds (3 digits). Same caveat asu. [1]Z: Timezone offset in seconds from UTC (west of UTC is negative; east is positive), range-43200to50400. [1]
Documentation: PHP manual, DateTimeInterface::format [1].
Source
[1] PHP Manual: DateTimeInterface::format (datetime.format.php) (php.net)
Citations:
🏁 Script executed:
cat -n ./src/Contracts/CreateKeyRequest.php | head -60Repository: meilisearch/meilisearch-php
Length of output: 2658
🏁 Script executed:
# Search for tests related to CreateKeyRequest
find . -type f \( -name "*.php" \) -exec grep -l "CreateKeyRequest\|expiresAt" {} \;Repository: meilisearch/meilisearch-php
Length of output: 359
🏁 Script executed:
# Search for API documentation or specifications about the format
find . -type f \( -name "*.md" -o -name "*.json" -o -name "*.yaml" \) -exec grep -l "expiresAt\|key.*format" {} \; | head -10Repository: meilisearch/meilisearch-php
Length of output: 103
🏁 Script executed:
# Check the test file for expected format
grep -A 10 -B 5 "expiresAt\|RFC 3339" ./tests/Endpoints/KeysAndPermissionsTest.php | head -50Repository: meilisearch/meilisearch-php
Length of output: 1893
🏁 Script executed:
# Check what 'p' and 'c' format characters produce in PHP
cat <<'EOF' > /tmp/php_format_chars.php
<?php
// Test different format characters for RFC 3339 compliance
$dt = new DateTimeImmutable('2024-04-15 10:30:45.123456', new DateTimeZone('UTC'));
$dtNonUtc = new DateTimeImmutable('2024-04-15 10:30:45.123456', new DateTimeZone('America/New_York'));
echo "UTC timezone:\n";
echo "'Y-m-d\TH:i:s.u\Z' (just u+Z): " . $dt->format('Y-m-d\TH:i:s.u\Z') . "\n";
echo "'Y-m-d\TH:i:s.up' (u+p): " . $dt->format('Y-m-d\TH:i:s.up') . "\n";
echo "'Y-m-d\TH:i:s.uP' (u+P): " . $dt->format('Y-m-d\TH:i:s.uP') . "\n";
echo "'c' (ISO 8601): " . $dt->format('c') . "\n";
echo "\nNon-UTC timezone (America/New_York):\n";
echo "'Y-m-d\TH:i:s.u\Z' (u+Z): " . $dtNonUtc->format('Y-m-d\TH:i:s.u\Z') . "\n";
echo "'Y-m-d\TH:i:s.up' (u+p): " . $dtNonUtc->format('Y-m-d\TH:i:s.up') . "\n";
echo "'Y-m-d\TH:i:s.uP' (u+P): " . $dtNonUtc->format('Y-m-d\TH:i:s.uP') . "\n";
echo "'c' (ISO 8601): " . $dtNonUtc->format('c') . "\n";
EOF
php /tmp/php_format_chars.phpRepository: meilisearch/meilisearch-php
Length of output: 364
🏁 Script executed:
# Check the tests for expected format patterns
cat ./tests/Endpoints/KeysAndPermissionsTest.php | grep -A 20 "expiresAt"Repository: meilisearch/meilisearch-php
Length of output: 2336
🏁 Script executed:
# Fix the PHP test to work in the sandbox
cat <<'EOF' > /tmp/php_format_test.php
<?php
// Test different format characters for RFC 3339 compliance
// Use UTC only since timezone DB may not be available
$dt = new DateTimeImmutable('2024-04-15 10:30:45.123456', new DateTimeZone('UTC'));
echo "UTC timezone output:\n";
echo "'Y-m-d\TH:i:s.vu\Z' (current, buggy): " . $dt->format('Y-m-d\TH:i:s.vu\Z') . "\n";
echo "'Y-m-d\TH:i:s.u\Z' (just u+Z): " . $dt->format('Y-m-d\TH:i:s.u\Z') . "\n";
echo "'Y-m-d\TH:i:s.up' (u+p): " . $dt->format('Y-m-d\TH:i:s.up') . "\n";
echo "'Y-m-d\TH:i:s.uP' (u+P): " . $dt->format('Y-m-d\TH:i:s.uP') . "\n";
echo "'c' (ISO 8601): " . $dt->format('c') . "\n";
echo "\n\nFormat character meanings:\n";
echo "'p' = timezone abbreviation\n";
echo "'P' = timezone offset (+00:00)\n";
echo "'Z' = timezone offset in seconds (literal Z for UTC)\n";
// RFC 3339 format should be: 2024-04-15T10:30:45.123456Z or 2024-04-15T10:30:45.123456+00:00
EOF
php /tmp/php_format_test.phpRepository: meilisearch/meilisearch-php
Length of output: 513
🌐 Web query:
RFC 3339 date time format microseconds timezone
💡 Result:
In RFC 3339, an Internet timestamp is:
date-time = YYYY-MM-DD "T" HH:MM:SS [ "." 1*DIGIT ] ( "Z" / (("+" / "-") HH:MM) )(ABNF in §5.6). Fractional seconds are optional and, if present, are a dot plus 1 or more digits—so microseconds are represented by using 6 digits. [1]
Examples (microseconds + timezone):
- UTC (“Zulu”):
2026-04-07T13:45:12.123456Z[1] - Numeric offset:
2026-04-07T13:45:12.123456-04:00[1]
Notes:
- RFC 3339 also defines
-00:00as “UTC time is known, but the local offset is unknown” (different meaning thanZ/+00:00). [1]
Sources
- [1] RFC 3339 (IETF Datatracker), §4.3 and §5.6 — ABNF for fractional seconds and time offsets, and
-00:00convention.
🏁 Script executed:
# Check the Meilisearch API documentation to see expected format
cat ./.code-samples.meilisearch.yaml | grep -A 5 -B 5 "expiresAt\|keys"Repository: meilisearch/meilisearch-php
Length of output: 961
🏁 Script executed:
# Check how expiresAt is created and if it's always UTC
rg "expiresAt" -B 3 -A 3 --type php | head -60Repository: meilisearch/meilisearch-php
Length of output: 4056
🏁 Script executed:
# Check if there are any examples or docs about timezone handling
find . -type f \( -name "*.md" -o -name "*.rst" \) -exec grep -l "expiresAt\|timezone" {} \;Repository: meilisearch/meilisearch-php
Length of output: 53
🏁 Script executed:
# Verify what 'p' actually outputs for UTC timezone
cat <<'EOF' > /tmp/verify_p.php
<?php
$dt = new DateTimeImmutable('2024-04-15 10:30:45.123456', new DateTimeZone('UTC'));
echo "Format 'p' for UTC: '" . $dt->format('p') . "'\n";
EOF
php /tmp/verify_p.phpRepository: meilisearch/meilisearch-php
Length of output: 94
The format string .vu\Z produces malformed fractional seconds and should be corrected.
The v (milliseconds, 3 digits) and u (microseconds, 6 digits) format codes overlap and when concatenated produce .123123456 instead of the intended fractional seconds. For RFC 3339 compliance, use .uP instead, which outputs microseconds with a proper timezone offset (+00:00 or -05:00), or .u\Z if the datetime is guaranteed to be in UTC.
The proposed fix using .up works for UTC times since p outputs Z for UTC, but it is implicit and would output timezone abbreviations (e.g., EDT, PST) instead of RFC 3339 offsets for non-UTC input.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Contracts/CreateKeyRequest.php` at line 47, The expiresAt formatting in
CreateKeyRequest is using the erroneous format string 'Y-m-d\TH:i:s.vu\Z' which
concatenates milliseconds and microseconds; update the expiresAt->format call in
CreateKeyRequest (the expiresAt serialization line) to use a correct
RFC3339-safe format: prefer 'Y-m-d\TH:i:s.uP' to emit microseconds with an
RFC3339 timezone offset, or if the datetime is guaranteed UTC use
'Y-m-d\TH:i:s.u\Z' instead.
There was a problem hiding this comment.
I've used the same format as it was done previously
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check git history for the old expiresAt format string
git log --oneline --all | head -10
git log --all --oneline -- src/Endpoints/Keys.php | head -5
Length of output: 527
🏁 Script executed:
#!/bin/bash
# Search for expiresAt format in git history
git log -p --all -- src/Endpoints/Keys.php | grep -A2 -B2 "expiresAt\|format\|Y-m-d" | head -60
Length of output: 59
🏁 Script executed:
#!/bin/bash
git show 5a8459c:src/Endpoints/Keys.php | grep -n "expiresAt\|format\|Y-m-d" | head -20
Length of output: 878
@norkunas, confirmed — the old Keys.php (line 171) used the exact same format string 'Y-m-d\TH:i:s.vu\Z', so you are correctly preserving the pre-existing behavior. I'll defer to your judgment on keeping it consistent.
✏️ Learnings added
Learnt from: norkunas
Repo: meilisearch/meilisearch-php PR: 903
File: src/Contracts/CreateKeyQuery.php:47-47
Timestamp: 2026-04-07T07:41:41.598Z
Learning: In meilisearch/meilisearch-php, the `expiresAt` datetime format string `'Y-m-d\TH:i:s.vu\Z'` (combining PHP's `v` milliseconds and `u` microseconds) is intentionally kept as a pre-existing pattern carried over from the old `src/Endpoints/Keys.php` implementation. Do not flag this as a bug introduced by new code.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
We don't need to handle this here. I opened #906 to track it
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
norkunas
force-pushed
the
typed-keys
branch
2 times, most recently
from
c61e846 to
ae2d412
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Endpoints/Delegates/HandlesKeys.php (1)
57-60: Consider adding return type annotation fordeleteKey.The
deleteKeymethod returnsarraybut lacks PHPDoc describing the expected shape. For consistency with the other typed methods, consider documenting the return structure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Endpoints/Delegates/HandlesKeys.php` around lines 57 - 60, The deleteKey method currently returns array but lacks a PHPDoc describing the expected shape; update the HandlesKeys::deleteKey method to include a PHPDoc block above the method that documents the return type and structure (e.g. keys of the returned array, expected boolean/status fields, error message or deleted id if applicable) consistent with other endpoint methods, and reference that it forwards to $this->keys->delete so the documented shape matches that method's contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Endpoints/Delegates/HandlesKeys.php`:
- Around line 57-60: The deleteKey method currently returns array but lacks a
PHPDoc describing the expected shape; update the HandlesKeys::deleteKey method
to include a PHPDoc block above the method that documents the return type and
structure (e.g. keys of the returned array, expected boolean/status fields,
error message or deleted id if applicable) consistent with other endpoint
methods, and reference that it forwards to $this->keys->delete so the documented
shape matches that method's contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ee073da-1f65-4e35-831a-ccfdd8f12b05
📒 Files selected for processing (10)
src/Contracts/CreateKeyQuery.phpsrc/Contracts/Key.phpsrc/Contracts/KeyAction.phpsrc/Contracts/KeysResults.phpsrc/Contracts/UpdateKeyQuery.phpsrc/Endpoints/Delegates/HandlesKeys.phpsrc/Endpoints/Keys.phptests/Endpoints/KeysAndPermissionsTest.phptests/Endpoints/TenantTokenTest.phptests/TestCase.php
💤 Files with no reviewable changes (1)
- tests/TestCase.php
✅ Files skipped from review due to trivial changes (1)
- src/Contracts/KeyAction.php
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Contracts/Key.php
- src/Endpoints/Keys.php
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/Endpoints/KeysAndPermissionsTest.php (1)
18-25: Consider scoping tearDown to test-created keys only.The blanket deletion of all keys via
getKeys()could impact test isolation if tests run in parallel or if future tests in other classes create keys that persist across test methods. Consider tracking keys created during each test and deleting only those, similar to howTenantTokenTestdeletes only its specific key by UID.That said, this may be acceptable if the test suite runs serially and no other tests depend on keys persisting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Endpoints/KeysAndPermissionsTest.php` around lines 18 - 25, The tearDown currently deletes every key returned by $this->client->getKeys(), which can break isolation; change it to only delete keys created by the current test by tracking created key UIDs in a property (e.g., $this->createdKeys) whenever tests call the client to create a key, and then in tearDown() iterate over $this->createdKeys calling $this->client->deleteKey($uid); ensure any helper that creates keys (used in KeysAndPermissionsTest) pushes the returned key UID into $this->createdKeys so only those UIDs are removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Contracts/Key.php`:
- Around line 94-107: The Key::fromArray method currently maps actions using
KeyAction::from($action) which will throw on unknown enum values; update this to
use KeyAction::tryFrom($action) and filter out (or map to a safe default) null
results so unknown actions do not cause a ValueError, or alternatively wrap the
mapping in a try/catch for ValueError and handle unrecognized actions
gracefully; modify the array_map call in fromArray to use tryFrom and exclude
nulls (or preserve raw strings) before constructing the Key instance.
---
Nitpick comments:
In `@tests/Endpoints/KeysAndPermissionsTest.php`:
- Around line 18-25: The tearDown currently deletes every key returned by
$this->client->getKeys(), which can break isolation; change it to only delete
keys created by the current test by tracking created key UIDs in a property
(e.g., $this->createdKeys) whenever tests call the client to create a key, and
then in tearDown() iterate over $this->createdKeys calling
$this->client->deleteKey($uid); ensure any helper that creates keys (used in
KeysAndPermissionsTest) pushes the returned key UID into $this->createdKeys so
only those UIDs are removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 400600fa-d67d-45df-b800-4c273a7544fc
📒 Files selected for processing (10)
src/Contracts/CreateKeyQuery.phpsrc/Contracts/Key.phpsrc/Contracts/KeyAction.phpsrc/Contracts/KeysResults.phpsrc/Contracts/UpdateKeyQuery.phpsrc/Endpoints/Delegates/HandlesKeys.phpsrc/Endpoints/Keys.phptests/Endpoints/KeysAndPermissionsTest.phptests/Endpoints/TenantTokenTest.phptests/TestCase.php
💤 Files with no reviewable changes (1)
- tests/TestCase.php
✅ Files skipped from review due to trivial changes (2)
- src/Contracts/UpdateKeyQuery.php
- src/Contracts/KeyAction.php
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/Endpoints/TenantTokenTest.php
- src/Contracts/CreateKeyQuery.php
- src/Endpoints/Keys.php
| public static function fromArray(array $data): self | ||
| { | ||
| return new self( | ||
| $data['uid'], | ||
| $data['key'], | ||
| array_map(static fn (string $action) => KeyAction::from($action), $data['actions']), | ||
| $data['indexes'], | ||
| $data['name'] ?? null, | ||
| $data['description'] ?? null, | ||
| isset($data['expiresAt']) ? new \DateTimeImmutable($data['expiresAt']) : null, | ||
| new \DateTimeImmutable($data['createdAt']), | ||
| new \DateTimeImmutable($data['updatedAt']), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Meilisearch API keys actions list latest version
💡 Result:
In the latest Meilisearch version (v1.38+ as of early 2026, per GitHub releases), API keys are defined by a list of actions (permissions). The complete list of actions from official OpenAPI documentation includes: - * - search - documents.* - documents.add - documents.get - documents.delete - indexes.* - indexes.create - indexes.get - indexes.update - indexes.delete - indexes.swap - indexes.compact - tasks.* - tasks.cancel - tasks.delete - tasks.get - tasks.compact - settings.* - settings.get - settings.update - stats.* - stats.get - metrics.* - metrics.get - dumps.* - dumps.create - snapshots.* - snapshots.create - version - keys.create - keys.get - keys.update - keys.delete - experimental.get - experimental.update - export - network.get - network.update - chatCompletions - chats.* - chats.get - chats.delete - chatsSettings.* - chatsSettings.get - chatsSettings.update - .get - webhooks.get - webhooks.update - webhooks.delete - webhooks.create - webhooks. - fields.post - dynamicSearchRules.get - dynamicSearchRules.create - dynamicSearchRules.update - dynamicSearchRules.delete - dynamicSearchRules.* Wildcard patterns like documents., indexes., etc., grant all sub-actions under that category. The "" action grants all permissions. Actions are scoped to specific indexes via the "indexes" array when creating keys (e.g., [""] for all indexes).
Citations:
- 1: https://meilisearch.com/docs/reference/api/keys/list-api-keys
- 2: https://meilisearch.dev/docs/reference/api/keys
- 3: https://www.meilisearch.com/docs/capabilities/security/how_to/manage_api_keys
- 4: https://meilisearch.com/docs/reference/api/keys/create-api-key
- 5: https://meilisearch.com/docs/reference/api/keys/get-api-key
- 6: https://github.com/meilisearch/meilisearch/releases
- 7: https://meilisearch.com/docs/reference/api/openapi
🏁 Script executed:
# Find the KeyAction enum definition
fd -t f -i 'keyaction' src/Repository: meilisearch/meilisearch-php
Length of output: 98
🏁 Script executed:
# Search for KeyAction definition using rg
rg -t php 'enum KeyAction|class KeyAction' src/Repository: meilisearch/meilisearch-php
Length of output: 121
🏁 Script executed:
# Check the Key.php file to see the full context around lines 94-107
cat -n src/Contracts/Key.php | head -120Repository: meilisearch/meilisearch-php
Length of output: 3535
🏁 Script executed:
cat -n src/Contracts/KeyAction.phpRepository: meilisearch/meilisearch-php
Length of output: 2950
Forward-compatibility risk with KeyAction::from() for unrecognized actions.
The KeyAction enum is incomplete compared to the official Meilisearch API specification. When the server introduces new action types (e.g., additional wildcard patterns or experimental features), KeyAction::from($action) will throw a ValueError for unrecognized values, causing key retrieval to fail entirely.
Use tryFrom() with filtering to gracefully handle unknown actions, or catch ValueError to provide better error context.
Proposed fix using tryFrom() with filtering
- array_map(static fn (string $action) => KeyAction::from($action), $data['actions']),
+ array_values(array_filter(
+ array_map(static fn (string $action) => KeyAction::tryFrom($action), $data['actions']),
+ static fn (?KeyAction $action) => $action !== null
+ )),Alternatively, catch ValueError to retain unknown actions as-is or throw a more descriptive domain exception.
📝 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 static function fromArray(array $data): self | |
| { | |
| return new self( | |
| $data['uid'], | |
| $data['key'], | |
| array_map(static fn (string $action) => KeyAction::from($action), $data['actions']), | |
| $data['indexes'], | |
| $data['name'] ?? null, | |
| $data['description'] ?? null, | |
| isset($data['expiresAt']) ? new \DateTimeImmutable($data['expiresAt']) : null, | |
| new \DateTimeImmutable($data['createdAt']), | |
| new \DateTimeImmutable($data['updatedAt']), | |
| ); | |
| } | |
| public static function fromArray(array $data): self | |
| { | |
| return new self( | |
| $data['uid'], | |
| $data['key'], | |
| array_values(array_filter( | |
| array_map(static fn (string $action) => KeyAction::tryFrom($action), $data['actions']), | |
| static fn (?KeyAction $action) => $action !== null | |
| )), | |
| $data['indexes'], | |
| $data['name'] ?? null, | |
| $data['description'] ?? null, | |
| isset($data['expiresAt']) ? new \DateTimeImmutable($data['expiresAt']) : null, | |
| new \DateTimeImmutable($data['createdAt']), | |
| new \DateTimeImmutable($data['updatedAt']), | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Contracts/Key.php` around lines 94 - 107, The Key::fromArray method
currently maps actions using KeyAction::from($action) which will throw on
unknown enum values; update this to use KeyAction::tryFrom($action) and filter
out (or map to a safe default) null results so unknown actions do not cause a
ValueError, or alternatively wrap the mapping in a try/catch for ValueError and
handle unrecognized actions gracefully; modify the array_map call in fromArray
to use tryFrom and exclude nulls (or preserve raw strings) before constructing
the Key instance.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/Endpoints/KeysAndPermissionsTest.php (1)
52-61: Assert the supplied UID verbatim.These cases pass a known UID, so a UUID regex will still pass if the request UID is ignored or rewritten.
assertSame()on the exact UID would make the round-trip contract explicit here.Also applies to: 112-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Endpoints/KeysAndPermissionsTest.php` around lines 52 - 61, Replace the loose UUID regex assertion with a strict equality check so the test verifies the server returned the exact supplied UID; specifically, change the assertion that calls $key->getUid() using assertMatchesRegularExpression(...) to assertSame('a6524df1-fe31-4019-8578-0e0ee2302104', $key->getUid()) and do the same for the other case referenced (the assertions around lines 112-123) to ensure both tests assert the exact UID value rather than just matching a UUID pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Endpoints/Keys.php`:
- Around line 15-25: The RawKey phpstan type currently marks name and
description as optional; change the type definition for RawKey so that name and
description are nullable instead (name: non-empty-string|null, description:
non-empty-string|null) to match Meilisearch returning null when unset; update
the phpdoc block that defines RawKey in the Keys.php endpoint so consumers and
static analysis recognize null values for the name and description fields.
In `@tests/Endpoints/KeysAndPermissionsTest.php`:
- Around line 18-25: The teardown currently deletes all keys from the shared
Meilisearch instance making the test order-dependent and removing keys not
created by this class; fix it by scoping cleanup to only keys this test class
creates (or by clearing state in setUp). Add a property like $createdKeyUids
initialized in setUp(), push each created key UID to that list when calling the
client in tests (where keys are created), and change tearDown() to iterate only
over $createdKeyUids and call $this->client->deleteKey($uid); alternatively, in
setUp() delete any existing keys that match a class-specific UID prefix to
guarantee a clean slate before tests run; adjust references to
getKeys()/deleteKey() only within those controlled filters.
- Around line 47-60: The test constructs the CreateKeyQuery with expiresAt using
new \DateTimeImmutable('tomorrow 08:00:00') twice which can produce
timezone/race issues; fix by creating one UTC DateTimeImmutable instance (e.g.
$expires = (new \DateTimeImmutable('tomorrow 08:00:00', new
\DateTimeZone('UTC'))) before calling createKey and pass $expires into
CreateKeyQuery.expiresAt, then assertEquals($expires, $key->getExpiresAt());
apply the same change in testGetKey() to reuse the single $expires variable for
both request and expected value.
---
Nitpick comments:
In `@tests/Endpoints/KeysAndPermissionsTest.php`:
- Around line 52-61: Replace the loose UUID regex assertion with a strict
equality check so the test verifies the server returned the exact supplied UID;
specifically, change the assertion that calls $key->getUid() using
assertMatchesRegularExpression(...) to
assertSame('a6524df1-fe31-4019-8578-0e0ee2302104', $key->getUid()) and do the
same for the other case referenced (the assertions around lines 112-123) to
ensure both tests assert the exact UID value rather than just matching a UUID
pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e5455c8f-f796-43b5-8d4a-6d3eb1052715
📒 Files selected for processing (10)
src/Contracts/CreateKeyQuery.phpsrc/Contracts/Key.phpsrc/Contracts/KeyAction.phpsrc/Contracts/KeysResults.phpsrc/Contracts/UpdateKeyQuery.phpsrc/Endpoints/Delegates/HandlesKeys.phpsrc/Endpoints/Keys.phptests/Endpoints/KeysAndPermissionsTest.phptests/Endpoints/TenantTokenTest.phptests/TestCase.php
💤 Files with no reviewable changes (1)
- tests/TestCase.php
✅ Files skipped from review due to trivial changes (1)
- src/Contracts/KeyAction.php
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/Endpoints/TenantTokenTest.php
- src/Contracts/CreateKeyQuery.php
- src/Contracts/Key.php
- src/Endpoints/Delegates/HandlesKeys.php
- src/Contracts/UpdateKeyQuery.php
| /** | ||
| * @phpstan-type RawKey array{ | ||
| * uid: non-empty-string, | ||
| * key: non-empty-string, | ||
| * actions: list<non-empty-string>, | ||
| * indexes: list<non-empty-string>, | ||
| * name?: non-empty-string, | ||
| * description?: non-empty-string, | ||
| * expiresAt: non-empty-string|null, | ||
| * createdAt: non-empty-string, | ||
| * updatedAt: non-empty-string |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the Meilisearch API responses for GET /keys and GET /keys/{keyOrUid}, are nameanddescription omitted when unset, or returned as null?
💡 Result:
In Meilisearch API responses for both GET /keys (list) and GET /keys/{keyOrUid} (single key), the name and description fields are included and returned as null when unset. They are not omitted. The OpenAPI schemas for KeyView (single key) and PaginationView_KeyView (list) define both fields as type: ["string", "null"], explicitly allowing null values. Examples in the documentation show them as description: null even when no value is set, confirming they appear as null rather than being absent.
Citations:
- 1: https://meilisearch.dev/docs/reference/api/keys
- 2: https://meilisearch.com/docs/reference/api/keys/list-api-keys
- 3: https://meilisearch.com/docs/reference/api/keys/get-api-key
- 4: https://meilisearch.com/docs/reference/api/keys/create-api-key
- 5: https://meilisearch.com/docs/reference/api/keys
Update RawKey type to allow null for name and description fields.
The Meilisearch API returns name and description as null when unset, not omitted. Update the type definition from optional to nullable:
name: non-empty-string|null,
description: non-empty-string|null,instead of:
name?: non-empty-string,
description?: non-empty-string,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Endpoints/Keys.php` around lines 15 - 25, The RawKey phpstan type
currently marks name and description as optional; change the type definition for
RawKey so that name and description are nullable instead (name:
non-empty-string|null, description: non-empty-string|null) to match Meilisearch
returning null when unset; update the phpdoc block that defines RawKey in the
Keys.php endpoint so consumers and static analysis recognize null values for the
name and description fields.
| protected function tearDown(): void | ||
| { | ||
| /* @phpstan-ignore-next-line */ | ||
| self::assertIsArray($this->client->getRawKeys()); | ||
| } | ||
| foreach ($this->client->getKeys() as $key) { | ||
| $this->client->deleteKey($key->getUid()); | ||
| } | ||
|
|
||
| public function testGetKeysAlwaysReturnsArray(): void | ||
| { | ||
| /* @phpstan-ignore-next-line */ | ||
| self::assertIsIterable($this->client->getKeys()); | ||
| parent::tearDown(); | ||
| } |
There was a problem hiding this comment.
This teardown still leaves the class order-dependent.
Line 20 only cleans up after the test, so the exact count/total/ordering assertions below will still fail if earlier tests left keys behind on the shared Meilisearch instance. It also deletes keys this class did not create. Please scope cleanup to UIDs created by this class, or clear fixture state in setUp() as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Endpoints/KeysAndPermissionsTest.php` around lines 18 - 25, The
teardown currently deletes all keys from the shared Meilisearch instance making
the test order-dependent and removing keys not created by this class; fix it by
scoping cleanup to only keys this test class creates (or by clearing state in
setUp). Add a property like $createdKeyUids initialized in setUp(), push each
created key UID to that list when calling the client in tests (where keys are
created), and change tearDown() to iterate only over $createdKeyUids and call
$this->client->deleteKey($uid); alternatively, in setUp() delete any existing
keys that match a class-specific UID prefix to guarantee a clean slate before
tests run; adjust references to getKeys()/deleteKey() only within those
controlled filters.
| $key = $this->client->createKey(new CreateKeyQuery( | ||
| actions: [KeyAction::TasksAny, KeyAction::IndexesGet], | ||
| indexes: ['movies*'], | ||
| name: 'task_manager', | ||
| description: 'manages tasks', | ||
| uid: 'a6524df1-fe31-4019-8578-0e0ee2302104', | ||
| expiresAt: new \DateTimeImmutable('tomorrow 08:00:00'), | ||
| )); | ||
|
|
||
| self::assertSame([KeyAction::TasksAny, KeyAction::IndexesGet], $key->getActions()); | ||
| self::assertSame(['movies*'], $key->getIndexes()); | ||
| self::assertSame('task_manager', $key->getName()); | ||
| self::assertSame('manages tasks', $key->getDescription()); | ||
| self::assertEquals(new \DateTimeImmutable('tomorrow 08:00:00'), $key->getExpiresAt()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the test file mentioned in the review
find . -name "KeysAndPermissionsTest.php" -type fRepository: meilisearch/meilisearch-php
Length of output: 115
🏁 Script executed:
#!/bin/bash
# List repository structure to understand the codebase
git ls-files | grep -E "(test|spec)" | head -20Repository: meilisearch/meilisearch-php
Length of output: 807
🏁 Script executed:
#!/bin/bash
# Read the test file to examine the code and structure
wc -l ./tests/Endpoints/KeysAndPermissionsTest.phpRepository: meilisearch/meilisearch-php
Length of output: 119
🏁 Script executed:
#!/bin/bash
# Read the test file focusing on the relevant sections
head -150 ./tests/Endpoints/KeysAndPermissionsTest.php | cat -nRepository: meilisearch/meilisearch-php
Length of output: 6847
🏁 Script executed:
#!/bin/bash
# Check the rest of the test file for any setUp or configuration
tail -106 ./tests/Endpoints/KeysAndPermissionsTest.php | cat -nRepository: meilisearch/meilisearch-php
Length of output: 4236
🏁 Script executed:
#!/bin/bash
# Check the parent TestCase class
find . -name "TestCase.php" -type f | head -5Repository: meilisearch/meilisearch-php
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Read the TestCase base class
cat -n ./tests/TestCase.phpRepository: meilisearch/meilisearch-php
Length of output: 7369
🏁 Script executed:
#!/bin/bash
# Check for any timezone configuration in test setup or CI configuration
grep -r "timezone\|UTC\|date_default_timezone" ./tests/ ./scripts/ ./.github/ 2>/dev/null | head -20Repository: meilisearch/meilisearch-php
Length of output: 53
🏁 Script executed:
#!/antml:bash
# Check the CreateKeyQuery to understand how expiresAt is handled
cat -n ./src/Contracts/CreateKeyQuery.php | head -80Repository: meilisearch/meilisearch-php
Length of output: 2671
Reuse one explicit expiresAt value in these assertions.
Each test reparses tomorrow 08:00:00, which makes the comparison sensitive to the default timezone and to the test crossing midnight between request construction and assertion. Capture one UTC DateTimeImmutable and reuse it for both the request and the expected value.
Suggested change
- $key = $this->client->createKey(new CreateKeyQuery(
+ $expiresAt = new \DateTimeImmutable('tomorrow 08:00:00', new \DateTimeZone('UTC'));
+ $key = $this->client->createKey(new CreateKeyQuery(
actions: [KeyAction::TasksAny, KeyAction::IndexesGet],
indexes: ['movies*'],
name: 'task_manager',
description: 'manages tasks',
uid: 'a6524df1-fe31-4019-8578-0e0ee2302104',
- expiresAt: new \DateTimeImmutable('tomorrow 08:00:00'),
+ expiresAt: $expiresAt,
));
@@
- self::assertEquals(new \DateTimeImmutable('tomorrow 08:00:00'), $key->getExpiresAt());
+ self::assertEquals($expiresAt, $key->getExpiresAt());Apply the same pattern in testGetKey() (lines 107–122).
📝 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.
| $key = $this->client->createKey(new CreateKeyQuery( | |
| actions: [KeyAction::TasksAny, KeyAction::IndexesGet], | |
| indexes: ['movies*'], | |
| name: 'task_manager', | |
| description: 'manages tasks', | |
| uid: 'a6524df1-fe31-4019-8578-0e0ee2302104', | |
| expiresAt: new \DateTimeImmutable('tomorrow 08:00:00'), | |
| )); | |
| self::assertSame([KeyAction::TasksAny, KeyAction::IndexesGet], $key->getActions()); | |
| self::assertSame(['movies*'], $key->getIndexes()); | |
| self::assertSame('task_manager', $key->getName()); | |
| self::assertSame('manages tasks', $key->getDescription()); | |
| self::assertEquals(new \DateTimeImmutable('tomorrow 08:00:00'), $key->getExpiresAt()); | |
| $expiresAt = new \DateTimeImmutable('tomorrow 08:00:00', new \DateTimeZone('UTC')); | |
| $key = $this->client->createKey(new CreateKeyQuery( | |
| actions: [KeyAction::TasksAny, KeyAction::IndexesGet], | |
| indexes: ['movies*'], | |
| name: 'task_manager', | |
| description: 'manages tasks', | |
| uid: 'a6524df1-fe31-4019-8578-0e0ee2302104', | |
| expiresAt: $expiresAt, | |
| )); | |
| self::assertSame([KeyAction::TasksAny, KeyAction::IndexesGet], $key->getActions()); | |
| self::assertSame(['movies*'], $key->getIndexes()); | |
| self::assertSame('task_manager', $key->getName()); | |
| self::assertSame('manages tasks', $key->getDescription()); | |
| self::assertEquals($expiresAt, $key->getExpiresAt()); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Endpoints/KeysAndPermissionsTest.php` around lines 47 - 60, The test
constructs the CreateKeyQuery with expiresAt using new
\DateTimeImmutable('tomorrow 08:00:00') twice which can produce timezone/race
issues; fix by creating one UTC DateTimeImmutable instance (e.g. $expires = (new
\DateTimeImmutable('tomorrow 08:00:00', new \DateTimeZone('UTC'))) before
calling createKey and pass $expires into CreateKeyQuery.expiresAt, then
assertEquals($expires, $key->getExpiresAt()); apply the same change in
testGetKey() to reuse the single $expires variable for both request and expected
value.
Migration guideThis PR turns the keys API from an array-based, loosely typed interface into a typed, object-based one:
Before vs after usage Before (v1.x) // Create key
$key = $client->createKey([
'description' => 'tenant token key',
'actions' => ['*'],
'indexes' => ['tenant*'],
'expiresAt' => '2055-10-02T00:00:00Z',
]);
// Update key
$updated = $client->updateKey($key->getKey(), [
'description' => 'new description',
]);
// List keys (array of Keys endpoint objects)
$keys = $client->getKeys();
foreach ($keys as $k) {
echo $k->getUid();
}After (v2.x) use Meilisearch\Contracts\CreateKeyQuery;
use Meilisearch\Contracts\UpdateKeyQuery;
use Meilisearch\Contracts\KeyAction;
// Create key
$key = $client->createKey(new CreateKeyQuery(
actions: [KeyAction::Any],
indexes: ['tenant*'],
description: 'tenant token key',
expiresAt: new DateTimeImmutable('2055-10-02T00:00:00Z'),
));
// Update (also used to clear name/description by passing null)
$updated = $client->updateKey(new UpdateKeyQuery(
keyOrUid: $key->getKey(), // or $key->getUid()
name: 'task_manager',
description: 'manages tasks',
));
// List keys (KeysResults of Key objects)
$keysResults = $client->getKeys();
foreach ($keysResults->getResults() as $k) {
echo $k->getUid();
} |
2 participants
Pull Request
Note
See below for migration guide
Related issue
Fixes #841
What does this PR do?
PR checklist
Please check if your PR fulfills the following requirements:
Summary by CodeRabbit
New Features
Refactor
Tests