Skip to content

[Breaking Change] Add support for Document Snapshots in Firestore Query Cursors - #1162

Merged
jdpedrie merged 4 commits into
googleapis:masterfrom
jdpedrie:firestore-snapshot-cursors
Jul 18, 2018
Merged

[Breaking Change] Add support for Document Snapshots in Firestore Query Cursors#1162
jdpedrie merged 4 commits into
googleapis:masterfrom
jdpedrie:firestore-snapshot-cursors

Conversation

@jdpedrie

@jdpedrie jdpedrie commented Jul 6, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

cc @schmidt-sebastian

Extracted from #923 and updated to address pull request comments.

Breaking change is the standardization in Query of using InvalidArgumentException, replacing various throws of BadMethodCallException.

Closes #851.

@jdpedrie jdpedrie added the api: firestore Issues related to the Firestore API. label Jul 6, 2018
@jdpedrie
jdpedrie requested a review from dwsupplee July 6, 2018 20:16
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 6, 2018
@jdpedrie jdpedrie changed the title Add support for Document Snapshots in Firestore Query Cursors [Breaking Change] Add support for Document Snapshots in Firestore Query Cursors Jul 6, 2018
tmatsuo
tmatsuo reviewed Jul 9, 2018
View reviewed changes
Comment thread Firestore/src/Query.php Outdated
{
if (FieldValue::isSentinelValue($value)) {
throw new \InvalidArgumentException(
'Value cannot be a `Google\Cloud\Firestore\FieldType` value.'

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread Firestore/src/Query.php Outdated
}
if (FieldValue::isSentinelValue($value)) {
throw new \InvalidArgumentException(
'Value cannot be a `Google\Cloud\Firestore\FieldType` value.'

This comment was marked as spam.

Sign in to view

dwsupplee
dwsupplee suggested changes Jul 17, 2018
View reviewed changes

@dwsupplee dwsupplee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking good, mostly minor feedback

Comment thread Firestore/src/CollectionReference.php Outdated
$this->valueMapper = $valueMapper;
$this->name = $name;

$parent = $this->parentPath($this->name);

This comment was marked as spam.

Sign in to view

Comment thread Firestore/src/Query.php Outdated
throw new \BadMethodCallException(
$orderBy = $this->queryKey('orderBy') ?: [];
if ($fieldValues instanceof DocumentSnapshot) {
list ($fieldValues, $orderBy) = $this->snapshotPosition($fieldValues, $orderBy);

This comment was marked as spam.

Sign in to view

Comment thread Firestore/src/Query.php
* @param DocumentSnapshot $snapshot The document snapshot
* @return array[] A list, where position 0 is fieldValues and position 1 is orderBy.
*/
private function snapshotPosition(DocumentSnapshot $snapshot, array $orderBy)

This comment was marked as spam.

Sign in to view

Comment thread Firestore/src/Query.php Outdated
* Build cursors for document snapshots.
*
* @param DocumentSnapshot $snapshot The document snapshot
* @return array[] A list, where position 0 is fieldValues and position 1 is orderBy.

This comment was marked as spam.

Sign in to view

Comment thread Firestore/src/Query.php Outdated
$direction = self::DIR_ASCENDING;
$fieldValues = [];

// If the list of orderBy clauses already contains __name__, use it unchanged.

This comment was marked as spam.

Sign in to view

Comment thread Firestore/src/Query.php Outdated
private function snapshotPosition(DocumentSnapshot $snapshot, array $orderBy)
{
$appendName = true;
$direction = self::DIR_ASCENDING;

This comment was marked as spam.

Sign in to view

Comment thread Firestore/src/Query.php
*/
private function snapshotPosition(DocumentSnapshot $snapshot, array $orderBy)
{
$appendName = true;

This comment was marked as spam.

Sign in to view

Comment thread Firestore/src/Query.php Outdated
{
$appendName = true;
$direction = self::DIR_ASCENDING;
$fieldValues = [];

This comment was marked as spam.

Sign in to view

Comment thread Firestore/src/Query.php
if (isset($query['where']['compositeFilter']) && count($query['where']['compositeFilter']['filters']) === 1) {
$filter = $query['where']['compositeFilter']['filters'][0];
unset($query['where']);
$query['where'] = $filter;

This comment was marked as spam.

Sign in to view

Comment thread Firestore/tests/Unit/QueryTest.php
}

public function testPositionWithDocumentSnapshot()
{

This comment was marked as spam.

Sign in to view

schmidt-sebastian
schmidt-sebastian reviewed Jul 17, 2018
View reviewed changes

@schmidt-sebastian schmidt-sebastian left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Logic looks fine to me.

@jdpedrie
jdpedrie merged commit 0488b2d into googleapis:master Jul 18, 2018
@jdpedrie
jdpedrie deleted the firestore-snapshot-cursors branch July 18, 2018 13:18
@tmatsuo tmatsuo mentioned this pull request Jul 18, 2018
Merged
sduskis pushed a commit that referenced this pull request Jul 23, 2018
@dwsupplee @sduskis
Update Bigtable branch with latest from master (#1190)
c2f4028 
* Add isRacy() to SafeSearch (#1166)

The SafeSearch class included `is` functions for each SafeSearch feature (adult, spoof, medical, violence), but was missing Racy. This Pull Request adds `isRacy()` to the SafeSearch class.

* Exclude component vendor folder from snippet coverage (#1168)

cc @tmatsuo

* Allow context to handle Throwable interface (#1164)

* Add getServiceAccount to the BigQueryClient (#1167)

* Add getServiceAccount to the BigQueryClient

See:
https://cloud.google.com/bigquery/docs/reference/rest/v2/projects/getServiceAccount

* Added $options to getServiceAccount and fixed the indent

* Use single quote

* Add snippet test

* Comment update, use self instead of $this

* Comment update

* Prepare v0.71.0 (#1169)

* Prepare v0.71.0

* patch release for Logging

* Correct version for Logging

* [Kms] Regenerate with the new gapic config (#1165)

* Update for the new gapic configuration

* Docs update for the new and nicer namespace

* Removed the files with the old namespace

* Use the new namespace in the code sample

Add back the deprecated files

* Changed the wording for the deprecation warning

* Fix firestore queries (#1161)

* Bump gax to 0.35 (#1170)

* Add an interactive release builder. (#1160)

* Add an interactive release builder.

* Create build directory if it doesn't exist

* Add getServiceAccount method to StorageClient (#1173)

* Re-generate library using Tasks/synth.py (#1174)

* Re-generate library using Tasks/synth.py

* Use new namespace in user visible area, tweak the deprecation wording

* Add support for Numeric type (#1172)

* Add support for Numeric type

* Added a cast in Numeric's constructor, added system tests

* Allow '123.' and '.123', update tests

* [Breaking Change] Add support for Document Snapshots in Firestore Query Cursors (#1162)

cc @schmidt-sebastian

Extracted from #923 and updated to address pull request comments.

Breaking change is the standardization in Query of using `InvalidArgumentException`, replacing various throws of `BadMethodCallException`.

Closes #851.

* Fix Storage Requesterpays system tests (#1180)

The requester pays system tests have been broken for some time. This change fixes them.

* Bandaid for protobuf 4761 (#1176)

* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase

* Prepare v0.72.0 (#1181)

* Added a document for the time filter on BigQueryClient->jobs() (#1183)

* Narrowed the time filter in the system test (#1185)

* Re-generate library using BigQueryDataTransfer/synth.py (#1184)

* Re-generate library using BigQueryDataTransfer/synth.py

* Tweak the wording on the deprecation warning

Also use the new namespace in the sample code

* pin auth version until we have a way to silence warnings (#1189)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers
@schmidt-sebastian schmidt-sebastian schmidt-sebastian left review comments
@tmatsuo tmatsuo tmatsuo approved these changes
@dwsupplee dwsupplee dwsupplee approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: firestore Issues related to the Firestore API. cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jdpedrie @tmatsuo @dwsupplee @schmidt-sebastian @googlebot