Skip to content

[Breaking Change] Update Firestore Queries and fix Conformance tests#923

Closed
jdpedrie wants to merge 9 commits intogoogleapis:masterfrom
jdpedrie:firestore-conformance
Closed

[Breaking Change] Update Firestore Queries and fix Conformance tests#923
jdpedrie wants to merge 9 commits intogoogleapis:masterfrom
jdpedrie:firestore-conformance

Conversation

@jdpedrie
Copy link
Copy Markdown
Contributor

@jdpedrie jdpedrie commented Mar 2, 2018 •
edited
Loading

cc @schmidt-sebastian

This change should bring PHP into line with the latest requirements for Queries.

  • [Feature] Query position methods (i.e. startAt, startAfter, endBefore, endAt) now accept either an array of values or a single DocumentSnapshot.

Closes #851.

@jdpedrie jdpedrie added the api: firestore Issues related to the Firestore API. label Mar 2, 2018
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 2, 2018
@jdpedrie jdpedrie changed the title Update Firestore Queries and fix Conformance tests [Breaking Change] Update Firestore Queries and fix Conformance tests Mar 2, 2018
schmidt-sebastian
schmidt-sebastian suggested changes Mar 15, 2018
View reviewed changes
Copy link
Copy Markdown

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

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

Thanks for this. Some comments on the API code itself. I didn't look at the tests.

Comment thread src/Firestore/CollectionReference.php
[
'collectionId' => $this->pathId($this->name)
'collectionId' => $this->pathId($this->name),
'allDescendants' => false,

This comment was marked as spam.

Sign in to view

Comment thread src/Firestore/Query.php Outdated
public function where($fieldPath, $operator, $value)
{
if (FieldValue::isSentinelValue($value)) {
throw new \InvalidArgumentException('Value cannot be a sentinel value.');

This comment was marked as spam.

Sign in to view

Comment thread src/Firestore/Query.php Outdated
}

if (FieldValue::isSentinelValue($value)) {
throw new \InvalidArgumentException('Value cannot be a sentinel value.');

This comment was marked as spam.

Sign in to view

Comment thread src/Firestore/Query.php Outdated
$this->childPath($collection, $value)
);
} elseif ($value instanceof DocumentReference) {
} elseif ($value instanceof DocumentReference || $value instanceof DocumentSnapshot) {

This comment was marked as spam.

Sign in to view

Comment thread src/Firestore/Query.php Outdated
$fieldValues = [];

// If the list of orderBy clauses already contains __name__, use it unchanged.
$append = !(bool) array_filter($orderBy, function ($order) {

This comment was marked as spam.

Sign in to view

Comment thread src/Firestore/Query.php Outdated
// If the query has existing orderBy constraints
if ($orderBy) {
// If the list of orderBy clauses already contains __name__, use it unchanged.
$append = !(bool) array_filter($orderBy, function ($order) {

This comment was marked as spam.

Sign in to view

Comment thread src/Firestore/Query.php
$lastOrderDirection = end($orderBy)['direction'];
$fieldValues[] = $snapshot;
$orderBy[] = [
'field' => [

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread src/Firestore/Query.php Outdated
'direction' => self::DIR_ASCENDING
];

$fieldValues[] = $snapshot;

This comment was marked as spam.

Sign in to view

Comment thread src/Firestore/Query.php
foreach ($orderBy as $order) {
$path = $order['field']['fieldPath'];
if ($path === self::DOCUMENT_ID) {
continue;

This comment was marked as spam.

Sign in to view

Comment thread src/Firestore/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

This comment was marked as spam.

Sign in to view

@jdpedrie jdpedrie mentioned this pull request Mar 30, 2018
Merged
@dwsupplee
Copy link
Copy Markdown
Contributor

dwsupplee commented Apr 26, 2018

Friendly ping here. Would like for the first review to be touched on before I delve in too deep.

@jdpedrie
Copy link
Copy Markdown
Contributor Author

jdpedrie commented Apr 26, 2018

I'll be moving back to this once the other priorities are out the door. There are several other things beyond the restructure and review which need to be resolved. I'll give you a heads up when it's ready for your review!

@jdpedrie jdpedrie added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label May 9, 2018
@jdpedrie
Copy link
Copy Markdown
Contributor Author

jdpedrie commented Jul 6, 2018 •
edited
Loading

I'm working to decompose these into several pull requests. The first for bug fixes can be found at #1161. Another is forthcoming for snapshot cursors, and another in a few days for conformance.

@jdpedrie jdpedrie closed this Jul 6, 2018
@jdpedrie jdpedrie mentioned this pull request Jul 6, 2018
Merged
jdpedrie added a commit that referenced this pull request Jul 18, 2018
@jdpedrie
[Breaking Change] Add support for Document Snapshots in Firestore Que…
0488b2d 
…ry 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.
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

1 more reviewer

@schmidt-sebastian schmidt-sebastian schmidt-sebastian requested 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. do not merge Indicates a pull request not ready for merge, due to either quality or timing.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jdpedrie @dwsupplee @schmidt-sebastian @googlebot