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.
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
feat!: [Datastore] Upgrade to PHP V2 #7179
base: main
Are you sure you want to change the base?
Conversation
1f14746
to
237531f
Compare
2eab49d
to
cee1539
Compare
…e-cloud-php into datastore-php-v2
chore(Datastore): Enhance readability of RunQuery and RunAggregationQuery APIs
I'll give this a more thorough review later today, but don't forget we need to remove all V1 GAPIC client classes. For Datastore V1, that would be Also the PR title should be renamed to better describe what the PR is doing. For PubSub, we said "remove connection classes". I don't mind using V2 in the title (since we are bumping to a V2 version), but we do not need to include PHP in the title. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! A few suggestions with testing and some other minor things. The bigger question I have is with the Operations
class - it looks like we are doing a LOT of converting of parameters. Why is so much of this necessary? Since we are doing a major version break, we have an opportunity to clean this up. Parameters which need to be changed should fall in two categories:
- We think the current (non-standard) interface is better. This would include things like accepting the string values of constants instead of their int versions. And since we know which parameters are enums, we could even automate this for all parameters (something to consider, but maybe not a good idea)
- We think the GAPIC interface is better. These should then be deprecated and either removed entirely (if they are not heavily used parameters) or renamed/converted to their new parameter/type.
* Non zero positive integer $x => `shouldBeCalledTimes($x)` | ||
* ] | ||
*/ | ||
private function mockSendRequest($methodName, $params, $returnValue, $shouldBeCalledTimes = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a fan of abstracting out the test logic to the trait like this - it makes the tests harder to read. You may end up saving a few lines of code, but you sacrifice readability. If I have to maintain these tests later, I may have to make changes to your abstracted functions, and take time to learn them, and even fix bugs in them.
I'd rather see the tests mock requests to the requestHandler
directly. See below for an example.
); | ||
|
||
if (!is_null($shouldBeCalledTimes)) { | ||
if ($shouldBeCalledTimes == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifying $shouldBeCalledTimes
as 0
to mean shouldBeCalled()
is intuitive, especially since shouldNotBeCalled()
exists.
Also, generally speaking, we should be able to specify the exact number of times we expect the function to be called (e.g. using shouldBeCalledOnce()
and shouldBeCalledTimes()
instead of just shouldBeCalled()
).
$this->mockSendRequest( | ||
'commit', | ||
[], | ||
['mutationResults' => [['version' => 1]]], | ||
0 | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following reads cleaner than using an abstraction function, and accomplishes the same thing
$this->requestHandler->sendRequest(
DatastoreGapicClient::class,
'commit',
Argument::type(CommitRequest::class),
[]
)
->shouldBeCalled()
->willReturn(['mutationResults' => [['version' => 1]]]);
]); | ||
$this->mockSendRequest( | ||
'beginTransaction', | ||
[], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the vast majority of these are providing an empty second argument, which means this can simply check the Request
class instead of the more complicated logic in mockSendRequest
(e.g. what I have above)
'beginTransaction', | ||
[], | ||
['transaction' => 'foo'], | ||
0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've noticed we are using 0
here almost always, but since we expect the call to only happen once, this should be 1
, which is a tighter assertion than simply ensuring the method is called.
So when these are refactored out of that function (which I think they should be), you would want to use shouldBeCalledOnce()
instead of shouldBeCalled()
'project_id' => isset($key['partitionId']['projectId']) | ||
? $key['partitionId']['projectId'] | ||
: null, | ||
'namespace_id' => isset($key['partitionId']['namespaceId']) | ||
? $key['partitionId']['namespaceId'] | ||
: null, | ||
'database_id' => isset($key['partitionId']['databaseId']) | ||
? $key['partitionId']['databaseId'] | ||
: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the null coalescing operator for readability
'project_id' => isset($key['partitionId']['projectId']) | |
? $key['partitionId']['projectId'] | |
: null, | |
'namespace_id' => isset($key['partitionId']['namespaceId']) | |
? $key['partitionId']['namespaceId'] | |
: null, | |
'database_id' => isset($key['partitionId']['databaseId']) | |
? $key['partitionId']['databaseId'] | |
: null, | |
'project_id' => $key['partitionId']['projectId'] ?? null, | |
'namespace_id' => $key['partitionId']['namespaceId'] ?? null, | |
'database_id' => $key['partitionId']['databaseId'] ?? null, |
$local = []; | ||
|
||
if (isset($key['partitionId'])) { | ||
$p = $this->arrayFilterRemoveNull([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit but it's more readable to just use short functions:
$p = array_filter($array, fn ($v) => !is_null($v));
} | ||
|
||
return $filter; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this logic be in a Serializer
class? This makes me a bit nervous, because changes to these parameters will break the API...
Is there a generic way we can do this logic? For instance, Message::serializeToJsonString
will handle string enums (e.g. "AND"
and "OR"
)
// verify that the operator, given as enum value, exists. | ||
PropertyFilterOperator::name($operator); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really need to do this? The API will validate this.
$filter['compositeFilter']['op'] = CompositeFilterOperator::PBOR; | ||
} else { | ||
$filter['compositeFilter']['op'] = CompositeFilterOperator::OPERATOR_UNSPECIFIED; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we not doing the same thing here as above and just call CompositeFilterOperator::value()
? The way we have it here, if the "op"
is anything but AND
or OR
, it gets overwritten by OPERATOR_UNSPECIFIED
.
Main PR for all the Datastore PHP v2 upgrade changes as mentioned below:
Google\Cloud\Datastore\DatastoreClient
7215b2e$requestHandler
prophecy andserializer
be2533c^1.55
bf2c50bOperation
class as internal d6e2823BREAKING_CHANGE_REASON=RC-1 for the version 2 of the Datastore library.