chore: add dictionary set, get, delete #19
Conversation
| private function ttlToMillis(?int $ttl = null): int | ||
| { | ||
| if (!$ttl) { | ||
| $ttl = $this->defaultTtlSeconds; | ||
| } | ||
| validateTtl($ttl); | ||
| return $ttl * 1000; | ||
| } | ||
|
|
||
| private function processCall(UnaryCall $call): mixed | ||
| { | ||
| [$response, $status] = $call->wait(); | ||
| if ($status->code !== 0) { | ||
| throw _ErrorConverter::convert($status->code, $status->details, $call->getMetadata()); | ||
| } | ||
| return $response; | ||
| } |
There was a problem hiding this comment.
Can you move these private function definitions up so they live between __construct() and set() please?
| return new CacheDictionaryGetResponseMiss(); | ||
| } | ||
| if ($dictionaryGetResponse->getFound()->getItems()->count() == 0) { | ||
| return new CacheDictionaryGetResponseError(new Exception("_DictionaryGetResponseResponse contained no data but was found")); |
There was a problem hiding this comment.
The exception used to construct the Error response class needs to be an SdkError like the one above this on line 428. Please use UnknownError like you did above unless you think one of the other SdkError-derived classes works better.
| private function getBadAuthTokenClient(): SimpleCacheClient | ||
| { | ||
| $badEnvName = "_MOMENTO_BAD_AUTH_TOKEN"; | ||
| putenv("{$badEnvName}={$this->BAD_AUTH_TOKEN}"); | ||
| $authProvider = new EnvMomentoTokenProvider($badEnvName); | ||
| putenv($badEnvName); | ||
| return new SimpleCacheClient($authProvider, $this->DEFAULT_TTL_SECONDS); | ||
| } | ||
|
|
There was a problem hiding this comment.
Can you move this to the top of the class, please, either before setUp() or after tearDown()?
tests/Cache/CacheClientTest.php
Outdated
| @@ -77,13 +67,18 @@ public function testCreateSetGetDelete() { | |||
| $this->assertNotNull($response->asSuccess()); | |||
| } | |||
|
|
|||
| // Client initialization tests | |||
| public function testNegativeDefaultTtl() { | |||
| // Happy path test | |||
There was a problem hiding this comment.
Please move this comment up so it appears before the testCreateSetGetDelete test.
tests/Cache/CacheClientTest.php
Outdated
| $response = $this->client->createCache($this->TEST_CACHE_NAME); | ||
| $this->assertNotNull($response->asAlreadyExists()); | ||
| } | ||
|
|
||
| // Create cache tests |
There was a problem hiding this comment.
This comment should come before the testCreateCacheAlreadyExists test please.
|
@pgautier404 Apologize for removing the list tests you've added. |
| $dictionarySetRequest->setItems([$this->toSingletonFieldValuePair($field, $value)]); | ||
| $dictionarySetRequest->setRefreshTtl($refreshTtl); | ||
| $dictionarySetRequest->setTtlMilliseconds($ttlMillis); | ||
| $call = $this->grpcManager->client->DictionarySet($dictionarySetRequest, ["cache" => [$cacheName]], ["timeout" => $this->deadline_seconds * self::$TIMEOUT_MULTIPLIER]); |
There was a problem hiding this comment.
I recommend pre-computing this so we don't multiply this out in all the functions, ie in the constructor store deadline_microseconds = $this->deadline_seconds * self::$TIMEOUT_MULTIPLIER and use that here
There was a problem hiding this comment.
I'd rather avoid using deadline_microseconds because they're technically not microseconds. I think this is a great idea, but I'd like to think about it a bit . . . I'll make this change in an upcoming PR.
| $this->assertEquals($value, $response->asHit()->value()); | ||
| } | ||
|
|
||
| public function testDictionaryDeleteThrowExceptionForEmptyCacheName() |
There was a problem hiding this comment.
function name is now out of date -- returns error instead of throws exception :)
| $this->assertEquals(MomentoErrorCode::INVALID_ARGUMENT_ERROR, $response->asError()->errorCode()); | ||
| } | ||
|
|
||
| public function testDictionaryDeleteThrowExceptionForEmptyDictionaryName() |
There was a problem hiding this comment.
Also out of date function name
This reverts commit 901618f.
|
@malandis We will address test names in a follow-up pr to get this pr merge first. |
Added dictionary set, get, and delete APIs and tests to check those operations.
Ticket: #17