Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: make $refreshTtl optional and re-order arguments for list push and dictionary increment #77

Merged
merged 7 commits into from
Dec 2, 2022
Merged

chore: make $refreshTtl optional and re-order arguments for list push and dictionary increment #77

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6cc0d26
chore: make refreshTtl optional and default to true; re-order argumen…
poppoerika Nov 28, 2022
2512025
fix: tests
poppoerika Nov 28, 2022
1d5d4ae
chore: add CollectionTtl class; update tests accordingly
poppoerika Nov 28, 2022
9be7921
fix: use CollectionTtl in setAddElement and fix tests
poppoerika Nov 28, 2022
2e8af3a
fix: CollectionTtl::fromCacheTtl()->withNoRefreshTtlOnUpdates()
poppoerika Nov 29, 2022
d0dea01
chore: add type for returnCollectionTtl arg and update nate to includ…
poppoerika Nov 29, 2022
4f987c0
chore: make CollectionTtl param type explicitly nullable by updating…
poppoerika Dec 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 13 additions & 12 deletions src/Cache/SimpleCacheClient.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
use Momento\Cache\CacheOperationTypes\ListCachesResponse;
use Momento\Config\IConfiguration;
use Momento\Logging\ILoggerFactory;
use Momento\Requests\CollectionTtl;
use Psr\Log\LoggerAwareInterface;
use Psr\Log\LoggerInterface;

Expand Down Expand Up @@ -105,17 +106,17 @@ public function listFetch(string $cacheName, string $listName): CacheListFetchRe
}

public function listPushFront(
string $cacheName, string $listName, string $value, bool $refreshTtl, ?int $ttlSeconds = null, ?int $truncateBackToSize = null
string $cacheName, string $listName, string $value, ?int $truncateBackToSize = null, CollectionTtl $ttl = null
): CacheListPushFrontResponse
{
return $this->dataClient->listPushFront($cacheName, $listName, $value, $refreshTtl, $truncateBackToSize, $ttlSeconds);
return $this->dataClient->listPushFront($cacheName, $listName, $value, $truncateBackToSize, $ttl);
}

public function listPushBack(
string $cacheName, string $listName, string $value, bool $refreshTtl, ?int $ttlSeconds = null, ?int $truncateFrontToSize = null
string $cacheName, string $listName, string $value, ?int $truncateFrontToSize = null, CollectionTtl $ttl = null
): CacheListPushBackResponse
{
return $this->dataClient->listPushBack($cacheName, $listName, $value, $refreshTtl, $truncateFrontToSize, $ttlSeconds);
return $this->dataClient->listPushBack($cacheName, $listName, $value, $truncateFrontToSize, $ttl);
}

public function listPopFront(string $cacheName, string $listName): CacheListPopFrontResponse
Expand Down Expand Up @@ -143,9 +144,9 @@ public function listErase(string $cacheName, string $listName, ?int $beginIndex
return $this->dataClient->listErase($cacheName, $listName, $beginIndex, $count);
}

public function dictionarySetField(string $cacheName, string $dictionaryName, string $field, string $value, bool $refreshTtl, ?int $ttlSeconds = null): CacheDictionarySetFieldResponse
public function dictionarySetField(string $cacheName, string $dictionaryName, string $field, string $value, CollectionTtl $ttl = null): CacheDictionarySetFieldResponse
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer the CollectionTtl parameter be explicitly nullable in all of these functions:

Suggested change
public function dictionarySetField(string $cacheName, string $dictionaryName, string $field, string $value, CollectionTtl $ttl = null): CacheDictionarySetFieldResponse
public function dictionarySetField(string $cacheName, string $dictionaryName, string $field, string $value, ?CollectionTtl $ttl = null): CacheDictionarySetFieldResponse

It looks like the null assignment does that implicitly, but I think making it explicit and consistent with the rest of our nullable parameters will make it more readable and maintainable.

{
return $this->dataClient->dictionarySetField($cacheName, $dictionaryName, $field, $value, $refreshTtl, $ttlSeconds);
return $this->dataClient->dictionarySetField($cacheName, $dictionaryName, $field, $value, $ttl);
}

public function dictionaryGetField(string $cacheName, string $dictionaryName, string $field): CacheDictionaryGetFieldResponse
Expand All @@ -163,9 +164,9 @@ public function dictionaryFetch(string $cacheName, string $dictionaryName): Cach
return $this->dataClient->dictionaryFetch($cacheName, $dictionaryName);
}

public function dictionarySetFields(string $cacheName, string $dictionaryName, array $items, bool $refreshTtl, ?int $ttlSeconds = null): CacheDictionarySetFieldsResponse
public function dictionarySetFields(string $cacheName, string $dictionaryName, array $items, CollectionTtl $ttl = null): CacheDictionarySetFieldsResponse
{
return $this->dataClient->dictionarySetFields($cacheName, $dictionaryName, $items, $refreshTtl, $ttlSeconds);
return $this->dataClient->dictionarySetFields($cacheName, $dictionaryName, $items, $ttl);
}

public function dictionaryGetFields(string $cacheName, string $dictionaryName, array $fields): CacheDictionaryGetFieldsResponse
Expand All @@ -174,10 +175,10 @@ public function dictionaryGetFields(string $cacheName, string $dictionaryName, a
}

public function dictionaryIncrement(
string $cacheName, string $dictionaryName, string $field, bool $refreshTtl, int $amount = 1, ?int $ttlSeconds = null
string $cacheName, string $dictionaryName, string $field, int $amount = 1, CollectionTtl $ttl = null
): CacheDictionaryIncrementResponse
{
return $this->dataClient->dictionaryIncrement($cacheName, $dictionaryName, $field, $refreshTtl, $amount, $ttlSeconds);
return $this->dataClient->dictionaryIncrement($cacheName, $dictionaryName, $field, $amount, $ttl);
}

public function dictionaryRemoveField(string $cacheName, string $dictionaryName, string $field): CacheDictionaryRemoveFieldResponse
Expand All @@ -190,9 +191,9 @@ public function dictionaryRemoveFields(string $cacheName, string $dictionaryName
return $this->dataClient->dictionaryRemoveFields($cacheName, $dictionaryName, $fields);
}

public function setAddElement(string $cacheName, string $setName, string $element, bool $refreshTtl, ?int $ttlSeconds = null): CacheSetAddElementResponse
public function setAddElement(string $cacheName, string $setName, string $element, CollectionTtl $ttl = null): CacheSetAddElementResponse
{
return $this->dataClient->setAddElement($cacheName, $setName, $element, $refreshTtl, $ttlSeconds);
return $this->dataClient->setAddElement($cacheName, $setName, $element, $ttl);
}

public function setFetch(string $cacheName, string $setName): CacheSetFetchResponse
Expand Down
52 changes: 34 additions & 18 deletions src/Cache/_ScsDataClient.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,8 @@
use Momento\Cache\Errors\SdkError;
use Momento\Cache\Errors\UnknownError;
use Momento\Config\IConfiguration;
use Momento\Requests\CollectionTtl;
use Momento\Requests\CollectionTtlFactory;
use Momento\Utilities\_ErrorConverter;
use Psr\Log\LoggerAwareInterface;
use Psr\Log\LoggerInterface;
Expand Down Expand Up @@ -175,6 +177,14 @@ private function ttlToMillis(?int $ttl = null): int
return $ttl * 1000;
}

private function returnCollectionTtl(?CollectionTtl $ttl): CollectionTtl
{
if (!$ttl) {
return CollectionTtl::fromCacheTtl();
}
return $ttl;
}

private function processCall(UnaryCall $call): mixed
{
[$response, $status] = $call->wait();
Expand Down Expand Up @@ -271,18 +281,19 @@ public function listFetch(string $cacheName, string $listName): CacheListFetchRe
}

public function listPushFront(
string $cacheName, string $listName, string $value, bool $refreshTtl, ?int $truncateBackToSize = null, ?int $ttlSeconds = null
string $cacheName, string $listName, string $value, ?int $truncateBackToSize = null, CollectionTtl $ttl = null
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above: please make all the CollectionTtl parameters in this file explicitly nullable ?CollectionTtl instead.

): CacheListPushFrontResponse
{
try {
$collectionTtl = $this->returnCollectionTtl($ttl);
validateCacheName($cacheName);
validateListName($listName);
validateTruncateSize($truncateBackToSize);
$ttlMillis = $this->ttlToMillis($ttlSeconds);
$ttlMillis = $this->ttlToMillis($collectionTtl->getTtl());
$listPushFrontRequest = new _ListPushFrontRequest();
$listPushFrontRequest->setListName($listName);
$listPushFrontRequest->setValue($value);
$listPushFrontRequest->setRefreshTtl($refreshTtl);
$listPushFrontRequest->setRefreshTtl($collectionTtl->getRefreshTtl());
$listPushFrontRequest->setTtlMilliseconds($ttlMillis);
if (!is_null($truncateBackToSize)) {
$listPushFrontRequest->setTruncateBackToSize($truncateBackToSize);
Expand All @@ -300,18 +311,19 @@ public function listPushFront(
}

public function listPushBack(
string $cacheName, string $listName, string $value, bool $refreshTtl, ?int $truncateFrontToSize = null, ?int $ttlSeconds = null
string $cacheName, string $listName, string $value, ?int $truncateFrontToSize = null, CollectionTtl $ttl = null
): CacheListPushBackResponse
{
try {
$collectionTtl = $this->returnCollectionTtl($ttl);
validateCacheName($cacheName);
validateListName($listName);
validateTruncateSize($truncateFrontToSize);
$ttlMillis = $this->ttlToMillis($ttlSeconds);
$ttlMillis = $this->ttlToMillis($collectionTtl->getTtl());
$listPushBackRequest = new _ListPushBackRequest();
$listPushBackRequest->setListName($listName);
$listPushBackRequest->setValue($value);
$listPushBackRequest->setRefreshTtl($refreshTtl);
$listPushBackRequest->setRefreshTtl($collectionTtl->getRefreshTtl());
$listPushBackRequest->setTtlMilliseconds($ttlMillis);
if (!is_null($truncateFrontToSize)) {
$listPushBackRequest->setTruncateFrontToSize($truncateFrontToSize);
Expand Down Expand Up @@ -442,19 +454,20 @@ public function listErase(string $cacheName, string $listName, ?int $beginIndex
return new CacheListEraseResponseSuccess();
}

public function dictionarySetField(string $cacheName, string $dictionaryName, string $field, string $value, bool $refreshTtl, ?int $ttlSeconds = null): CacheDictionarySetFieldResponse
public function dictionarySetField(string $cacheName, string $dictionaryName, string $field, string $value, CollectionTtl $ttl = null): CacheDictionarySetFieldResponse
{
try {
$collectionTtl = $this->returnCollectionTtl($ttl);
validateCacheName($cacheName);
validateDictionaryName($dictionaryName);
validateFieldName($field);
validateValueName($value);
$ttlMillis = $this->ttlToMillis($ttlSeconds);
$ttlMillis = $this->ttlToMillis($collectionTtl->getTtl());
validateTtl($ttlMillis);
$dictionarySetFieldRequest = new _DictionarySetRequest();
$dictionarySetFieldRequest->setDictionaryName($dictionaryName);
$dictionarySetFieldRequest->setItems([$this->toSingletonFieldValuePair($field, $value)]);
$dictionarySetFieldRequest->setRefreshTtl($refreshTtl);
$dictionarySetFieldRequest->setRefreshTtl($collectionTtl->getRefreshTtl());
$dictionarySetFieldRequest->setTtlMilliseconds($ttlMillis);
$call = $this->grpcManager->client->DictionarySet($dictionarySetFieldRequest, ["cache" => [$cacheName]], ["timeout" => $this->timeout]);
$this->processCall($call);
Expand Down Expand Up @@ -541,14 +554,15 @@ public function dictionaryFetch(string $cacheName, string $dictionaryName): Cach
return new CacheDictionaryFetchResponseMiss();
}

public function dictionarySetFields(string $cacheName, string $dictionaryName, array $items, bool $refreshTtl, ?int $ttlSeconds = null): CacheDictionarySetFieldsResponse
public function dictionarySetFields(string $cacheName, string $dictionaryName, array $items, CollectionTtl $ttl = null): CacheDictionarySetFieldsResponse
{
try {
$collectionTtl = $this->returnCollectionTtl($ttl);
validateCacheName($cacheName);
validateDictionaryName($dictionaryName);
validateItems($items);
validateFieldsKeys($items);
$ttlMillis = $this->ttlToMillis($ttlSeconds);
$ttlMillis = $this->ttlToMillis($collectionTtl->getTtl());
$protoItems = [];
foreach ($items as $field => $value) {
$fieldValuePair = new _DictionaryFieldValuePair();
Expand All @@ -558,7 +572,7 @@ public function dictionarySetFields(string $cacheName, string $dictionaryName, a
}
$dictionarySetFieldsRequest = new _DictionarySetRequest();
$dictionarySetFieldsRequest->setDictionaryName($dictionaryName);
$dictionarySetFieldsRequest->setRefreshTtl($refreshTtl);
$dictionarySetFieldsRequest->setRefreshTtl($collectionTtl->getRefreshTtl());
$dictionarySetFieldsRequest->setItems($protoItems);
$dictionarySetFieldsRequest->setTtlMilliseconds($ttlMillis);
$call = $this->grpcManager->client->DictionarySet($dictionarySetFieldsRequest, ["cache" => [$cacheName]], ["timeout" => $this->timeout]);
Expand Down Expand Up @@ -595,21 +609,22 @@ public function dictionaryGetFields(string $cacheName, string $dictionaryName, a
}

public function dictionaryIncrement(
string $cacheName, string $dictionaryName, string $field, bool $refreshTtl, int $amount = 1, ?int $ttlSeconds = null
string $cacheName, string $dictionaryName, string $field, int $amount = 1, CollectionTtl $ttl = null
): CacheDictionaryIncrementResponse
{
try {
$collectionTtl = $this->returnCollectionTtl($ttl);
validateCacheName($cacheName);
validateDictionaryName($dictionaryName);
validateFieldName($field);
$ttlMillis = $this->ttlToMillis($ttlSeconds);
$ttlMillis = $this->ttlToMillis($collectionTtl->getTtl());
validateTtl($ttlMillis);
$dictionaryIncrementRequest = new _DictionaryIncrementRequest();
$dictionaryIncrementRequest
->setDictionaryName($dictionaryName)
->setField($field)
->setAmount($amount)
->setRefreshTtl($refreshTtl)
->setRefreshTtl($collectionTtl->getRefreshTtl())
->setTtlMilliseconds($ttlMillis);
$call = $this->grpcManager->client->DictionaryIncrement(
$dictionaryIncrementRequest, ["cache" => [$cacheName]], ["timeout" => $this->timeout]
Expand Down Expand Up @@ -669,17 +684,18 @@ public function dictionaryRemoveFields(string $cacheName, string $dictionaryName
return new CacheDictionaryRemoveFieldsResponseSuccess();
}

public function setAddElement(string $cacheName, string $setName, string $element, bool $refreshTt, ?int $ttlSeconds = null): CacheSetAddElementResponse
public function setAddElement(string $cacheName, string $setName, string $element, CollectionTtl $ttl = null): CacheSetAddElementResponse
{
try {
$collectionTtl = $this->returnCollectionTtl($ttl);
validateCacheName($cacheName);
validateSetName($setName);
validateElement($element);
$ttlMillis = $this->ttlToMillis($ttlSeconds);
$ttlMillis = $this->ttlToMillis($collectionTtl->getTtl());
validateTtl($ttlMillis);
$setAddElementRequest = new _SetUnionRequest();
$setAddElementRequest->setSetName($setName);
$setAddElementRequest->setRefreshTtl($refreshTt);
$setAddElementRequest->setRefreshTtl($collectionTtl->getRefreshTtl());
$setAddElementRequest->setTtlMilliseconds($ttlMillis);
$setAddElementRequest->setElements([$element]);
$call = $this->grpcManager->client->SetUnion($setAddElementRequest, ["cache" => [$cacheName]], ["timeout" => $this->timeout]);
Expand Down
46 changes: 46 additions & 0 deletions src/Requests/CollectionTtl.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
<?php
declare(strict_types=1);

namespace Momento\Requests;

class CollectionTtl
{
private ?int $ttlSeconds;
private ?bool $refreshTtl;

public function __construct(?int $ttlSeconds = null, ?bool $refreshTtl = true)
{
$this->ttlSeconds = $ttlSeconds;
$this->refreshTtl = $refreshTtl;
}

public static function fromCacheTtl(): CollectionTtl
{
return new CollectionTtl(null, true);
}

public static function of(int $ttl): CollectionTtl
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd prefer this parameter to match the constructor and be called $ttlSeconds instead, so the unit is explicit in the parameter name (and in the named parameter label). I know I personally get confused about when we're using seconds vs. millis.

{
return new CollectionTtl($ttl);
}

public function getTtl(): int|null
{
return $this->ttlSeconds;
}

public function getRefreshTtl(): bool|null
{
return $this->refreshTtl;
}

public function withRefreshTtlOnUpdates(): CollectionTtl
{
return new CollectionTtl($this->ttlSeconds, $this->refreshTtl);
}

public function withNoRefreshTtlOnUpdates(): CollectionTtl
{
return new CollectionTtl($this->ttlSeconds, false);
}
}
Loading