[2.8/3] Add limitToLast implementation and tests. #2296

wu-hui · 2019-10-24T19:32:57Z

Spec tests are not in this PR, as I need to basically implement the target:query split and mapping in spec tests builder/runner.

Figured it might be a good idea to have it reviewed without spec tests. Please let me know if you want them to be together.

…ryTargetSplit

schmidt-sebastian

Thank you for this! This looks pretty reasonable. I added some comments for cleanup, but I do think this is close. I am excited to see the multi-tab spec tests in the following PR, but the test coverage here makes me feel confident. Thank you also for cleaning up the comments/code in Index Free.

schmidt-sebastian · 2019-10-25T05:05:20Z

packages/firebase/index.d.ts

@@ -8079,14 +8079,23 @@ declare namespace firebase.firestore {
    ): Query;

    /**
-     * Creates and returns a new Query where the results are limited to the
-     * specified number of documents.
+     * Creates and returns a new Query where only the first matching documents


What do you think of making this slightly more concise?

"Creates and returns a new Query that only returns the first matching documents".

I tried to come up with a way to include the original "specified number of documents" phrasing, but I can't come up with a good idea short of: "the first n matching documents". However, this would require us to rename the argument. Maybe you have an idea on how we can combine "first" and "specified"?

I think your version is good enough.

schmidt-sebastian · 2019-10-25T05:06:36Z

packages/firebase/index.d.ts

-     * Creates and returns a new Query where the results are limited to the
-     * specified number of documents.
+     * Creates and returns a new Query where only the first matching documents
+     * are returned as results.
     *
     * @param limit The maximum number of items to return.
     * @return The created Query.
     */
    limit(limit: number): Query;


The original API proposal suggests to make this method as deprecated. Should we do that and introduce limitToFirst()? In that case, I would also leave the comments for limit as they were.

Actually the decision was to keep limit and add limitToLast only.

See the comments in go/limitToLast

Sounds good. Do you mind updating the proposed API so this is immediately apparent? The API proposal should be the source of truth and we should in general update the proposal to match the decision from the API council.

Made some edit suggestions there.

schmidt-sebastian · 2019-10-25T05:08:09Z

packages/firestore-types/index.d.ts

@@ -1091,6 +1091,15 @@ export class Query {
   */
  limit(limit: number): Query;


Please make sure that both API definitions/comments are in sync (they seemed to be slightly different even before this PR).

schmidt-sebastian · 2019-10-25T05:10:53Z

packages/firestore/src/api/database.ts

@@ -1539,14 +1539,24 @@ export class Query implements firestore.Query {
  limit(n: number): firestore.Query {
    validateExactNumberOfArgs('Query.limit', arguments, 1);
    validateArgType('Query.limit', 'number', 1, n);
+    this.verifyPositiveLimit(n);


What do you think of moving this to util/input_validation, which means that we could make this generic and include the callsite (Query.limitToFirst, Query.limitToLast)?

schmidt-sebastian · 2019-10-25T05:18:06Z

packages/firestore/src/api/database.ts

  get(options?: firestore.GetOptions): Promise<firestore.QuerySnapshot> {
    validateBetweenNumberOfArgs('Query.get', arguments, 0, 1);
    validateGetOptions('Query.get', options);
    return new Promise(
      (resolve: Resolver<firestore.QuerySnapshot>, reject: Rejecter) => {
+        this.validateHasExplicitOrderByForLimitToLast(this._query);


This validation should happen outside of the Promise. Validation errors should throw, but if you throw inside of a Promise this is converted to a rejected Promise (which would indicate a backend error).

schmidt-sebastian · 2019-10-25T06:33:52Z

packages/firestore/test/integration/api/query.test.ts

+    });
+  });
+
+  it('can unlisten to mirror queries', () => {


schmidt-sebastian · 2019-10-25T06:35:42Z

packages/firestore/test/integration/api/query.test.ts

+        { k: 'a', sort: 0 },
+        { k: 'e', sort: -1 }
+      ]);
+


This test might be easier to reason about if you remove the expect(toDataArray(snapshot)) asserts that you have already verified in the test above.

I think this assertion is beneficial because it checks unlistening from one of the mirror queries will not stop others from functioning.

Added a comment to make the intention more clear.

Is there maybe another way to reduce the test case size? 40 lines of shared setup seems a little much in my personal opinion.

I have removed some unnecessary asserts that are repeating what other tests already covered, this should help a little bit?

I understand you concern, but IMHO it's not worth to make them sharing setup in this case, the added abstraction would probably make these tests harder to read.

schmidt-sebastian · 2019-10-25T06:37:35Z

packages/firestore/test/integration/api/query.test.ts

+      snapshot = await storeLimitToLastEvent.awaitEvent();
+      expect(toDataArray(snapshot)).to.deep.equal([
+        { k: 'e', sort: -1 },
+        { k: 'a', sort: -2 }


Same comment as above - the test would be more concise if it focused on relistening, rather than verifying each individual result. If you can keep the same test coverage, it might be worth removing some of the duplicated asserts.

Added comment like above.

schmidt-sebastian · 2019-10-25T06:41:19Z

packages/firestore/test/unit/local/index_free_query_engine.test.ts

@@ -264,7 +264,28 @@ describe('IndexFreeQueryEngine', () => {
  it('does not use initial results for limit query with document removal', async () => {
    const query = Query.atPath(path('coll'))
      .addFilter(filter('matches', '==', true))
-      .withLimit(1);
+      .withLimitToFirst(1);


Instead of duplicating the limit() tests, what do you think about making one of them use limitToLast() and keeping just limitToFirst() for the rest of them?

schmidt-sebastian · 2019-10-25T06:48:02Z

packages/firestore/test/unit/core/query.test.ts

+      fromLimit.toTargetQuery().canonicalId()
+    );
+
+    expect(limitQuery.isEqual(fromLimitToLast.toTargetQuery())).to.be.true;


I am still concerned about this method. The contract just doesn't feel right and seeing this spelled out in a test makes this obvious. I understand that there may be a need for this conversion, but since this is limited to Multi-Tab and Web-only, what to you think of handling this conversion in a private method in Sync Engine (or a free-standing function):

function convertTargetToMultiTabQuery(...)

Good point that this is multi-tab only. Moved it to SyncEngine.

wu-hui

Thanks for make some time to review this!

wu-hui · 2019-10-27T17:44:08Z

packages/firebase/index.d.ts

@@ -8079,14 +8079,23 @@ declare namespace firebase.firestore {
    ): Query;

    /**
-     * Creates and returns a new Query where the results are limited to the
-     * specified number of documents.
+     * Creates and returns a new Query where only the first matching documents


I think your version is good enough.

wu-hui · 2019-10-27T17:46:47Z

packages/firebase/index.d.ts

-     * Creates and returns a new Query where the results are limited to the
-     * specified number of documents.
+     * Creates and returns a new Query where only the first matching documents
+     * are returned as results.
     *
     * @param limit The maximum number of items to return.
     * @return The created Query.
     */
    limit(limit: number): Query;


Actually the decision was to keep limit and add limitToLast only.

See the comments in go/limitToLast

wu-hui · 2019-10-27T17:50:29Z

packages/firebase/index.d.ts

+     * Creates and returns a new Query where only the last matching documents
+     * are returned as results.
+     *
+     * @param limit The maximum number of items to return.


wu-hui · 2019-10-27T17:54:35Z

packages/firestore-types/index.d.ts

@@ -1091,6 +1091,15 @@ export class Query {
   */
  limit(limit: number): Query;


wu-hui · 2019-10-27T18:05:17Z

packages/firestore/src/api/database.ts

@@ -1539,14 +1539,24 @@ export class Query implements firestore.Query {
  limit(n: number): firestore.Query {
    validateExactNumberOfArgs('Query.limit', arguments, 1);
    validateArgType('Query.limit', 'number', 1, n);
+    this.verifyPositiveLimit(n);


wu-hui · 2019-10-27T18:29:50Z

packages/firestore/test/integration/api/query.test.ts

+    });
+  });
+
+  it('can unlisten to mirror queries', () => {


wu-hui · 2019-10-27T18:31:56Z

packages/firestore/test/integration/api/query.test.ts

+        { k: 'a', sort: 0 },
+        { k: 'e', sort: -1 }
+      ]);
+


I think this assertion is beneficial because it checks unlistening from one of the mirror queries will not stop others from functioning.

Added a comment to make the intention more clear.

wu-hui · 2019-10-27T18:34:30Z

packages/firestore/test/integration/api/query.test.ts

+      snapshot = await storeLimitToLastEvent.awaitEvent();
+      expect(toDataArray(snapshot)).to.deep.equal([
+        { k: 'e', sort: -1 },
+        { k: 'a', sort: -2 }


Added comment like above.

wu-hui · 2019-10-27T18:44:06Z

packages/firestore/test/unit/local/index_free_query_engine.test.ts

@@ -264,7 +264,28 @@ describe('IndexFreeQueryEngine', () => {
  it('does not use initial results for limit query with document removal', async () => {
    const query = Query.atPath(path('coll'))
      .addFilter(filter('matches', '==', true))
-      .withLimit(1);
+      .withLimitToFirst(1);


wu-hui · 2019-10-27T18:46:13Z

packages/firestore/test/unit/core/query.test.ts

+      fromLimit.toTargetQuery().canonicalId()
+    );
+
+    expect(limitQuery.isEqual(fromLimitToLast.toTargetQuery())).to.be.true;


Good point that this is multi-tab only. Moved it to SyncEngine.

schmidt-sebastian

Thanks for the update. Can you see if you can come up with a strategy to remove the test duplication? Ideally, our GZIP compression rate for our tests would be pretty low :-P

schmidt-sebastian · 2019-10-28T17:11:41Z

packages/firebase/index.d.ts

-     * Creates and returns a new Query where the results are limited to the
-     * specified number of documents.
+     * Creates and returns a new Query where only the first matching documents
+     * are returned as results.
     *
     * @param limit The maximum number of items to return.
     * @return The created Query.
     */
    limit(limit: number): Query;


Sounds good. Do you mind updating the proposed API so this is immediately apparent? The API proposal should be the source of truth and we should in general update the proposal to match the decision from the API council.

schmidt-sebastian · 2019-10-28T17:17:21Z

packages/firebase/index.d.ts

+     * Creates and returns a new Query where only the last matching documents
+     * are returned as results.
+     *
+     * @param limit The maximum number of items to return.


How about:

You must specify at least one `orderBy` clause for `limitToLast` queries, otherwise an exception will be thrown thrown during execution.

If you like your version better, we should still make it "an exception will be thrown".

schmidt-sebastian · 2019-10-28T17:23:22Z

packages/firestore/src/api/database.ts

@@ -1861,11 +1872,21 @@ export class Query implements firestore.Query {
    };
  }

+  private validateHasExplicitOrderByForLimitToLast(query: InternalQuery): void {


As per our standup discussion, let's stay consistent with the rest of the Query methods (and explore the option to allow startAt().order()). Let's figure out what is feasible and hopefully we can leave this PR as is.

schmidt-sebastian · 2019-10-28T17:25:59Z

packages/firestore/src/core/sync_engine.ts

+   * different from the original `Query` from which we obtained this instance.
+   */
+  // PORTING NOTE: Multi-tab only
+  private targetToQuery(target: Target): Query {


Is there a way to indicate that this conversion is lossy? We use "synthesized" in other parts of the API already 9

firebase-js-sdk/packages/firestore/src/remote/remote_event.ts

Line 68 in 6b53e00

static createSynthesizedRemoteEventForCurrentChange(

), maybe this could be targetToSynthesizedQuery or synthesizeTargetToQuery?

schmidt-sebastian · 2019-10-28T17:26:39Z

packages/firestore/src/remote/remote_store.ts

-      !objUtils.contains(this.listenTargets, queryData.targetId),
-      'listen called with duplicate targetId!'
-    );
+    if (objUtils.contains(this.listenTargets, queryData.targetId)) {


Sounds good. As per standup discussion, let's leave this as is.

schmidt-sebastian · 2019-10-28T17:30:45Z

packages/firestore/src/util/input_validation.ts

+  if (n <= 0) {
+    throw new FirestoreError(
+      Code.INVALID_ARGUMENT,
+      `Function "${functionName}" created invalid Query. Query limit (${n}) is invalid. Limit must be positive.`


This method should now be generic:

export function validatePositiveNumber(functionName: string, position: number, n: number): void { if (n <= 0) { const description = valueDescription(argument); throw new FirestoreError( Code.INVALID_ARGUMENT, `Function "${functionName}()" requires its ${ordinal(position)} argument to be a positive number, but it was: ${description}`); } }

schmidt-sebastian · 2019-10-28T17:56:05Z

packages/firestore/test/integration/api/query.test.ts

+  });
+
+  it('can listen to mirror queries', () => {
+    const testDocs = {


How about something like:

Queries that mapped to the same target ID are referred to as "mirror queries". An example for a mirror query is a limitToLast() query and a limit() query that share the same backend Target ID. Since limitToLast() queries are sent to the backend with a modified orderBy() clause, they can map to the same target representation as limit() query, even if both queries appear separate to the user.

schmidt-sebastian · 2019-10-28T17:59:15Z

packages/firestore/test/integration/api/query.test.ts

+        { k: 'a', sort: 0 },
+        { k: 'e', sort: -1 }
+      ]);
+


Is there maybe another way to reduce the test case size? 40 lines of shared setup seems a little much in my personal opinion.

schmidt-sebastian

Thanks for addressing the feedback. Some small cleanup left, but ship it :)

schmidt-sebastian · 2019-10-29T04:42:37Z

packages/firestore/src/api/database.ts

@@ -1540,14 +1540,14 @@ export class Query implements firestore.Query {
  limit(n: number): firestore.Query {
    validateExactNumberOfArgs('Query.limit', arguments, 1);
    validateArgType('Query.limit', 'number', 1, n);
-    validatePositiveLimit('limit', n);
+    validatePositiveNumber('limit', 1, n);


The function name here should be "Query.limit" (here and below).

schmidt-sebastian · 2019-10-29T04:43:21Z

packages/firestore/src/core/sync_engine.ts

@@ -1008,11 +1008,12 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
    return activeQueries;
  }
  /**
-   * Creates a `Query` object from this `Target`. Note the result might be
-   * different from the original `Query` from which we obtained this instance.
+   * Creates a `Query` object from this `Target`. There is no way to obtain


s/this/the specified

schmidt-sebastian · 2019-10-29T04:44:01Z

packages/firestore/src/core/sync_engine.ts

-   * different from the original `Query` from which we obtained this instance.
+   * Creates a `Query` object from this `Target`. There is no way to obtain
+   * the original `Query`, so we synthesize a `Query` from the `Target` object.
+   * The result might be different from the original `Query`,


Replace comma with period. Maybe also mention that is used for multi-tab and hence the limitation described here does not alter the behavior?

schmidt-sebastian · 2019-10-29T04:44:20Z

packages/firestore/test/integration/api/query.test.ts

@@ -140,8 +140,12 @@ apiDescribe('Queries', (persistence: boolean) => {
    });
  });

-  // Mirror queries are `Query`s mapped to the same `Target`, meaning
-  // they are supported by the same backend Query.
+  // Queries that mapped to the same target ID are referred to as


s/mapped/map (I take credit for that one)

Also maybe start with "Two queries...".

wu-hui

Thanks for the help!

wu-hui · 2019-10-29T13:45:30Z

packages/firestore/src/api/database.ts

@@ -1540,14 +1540,14 @@ export class Query implements firestore.Query {
  limit(n: number): firestore.Query {
    validateExactNumberOfArgs('Query.limit', arguments, 1);
    validateArgType('Query.limit', 'number', 1, n);
-    validatePositiveLimit('limit', n);
+    validatePositiveNumber('limit', 1, n);


wu-hui · 2019-10-29T13:45:53Z

packages/firestore/src/api/database.ts

@@ -1861,11 +1872,21 @@ export class Query implements firestore.Query {
    };
  }

+  private validateHasExplicitOrderByForLimitToLast(query: InternalQuery): void {


wu-hui · 2019-10-29T13:53:56Z

packages/firestore/src/core/sync_engine.ts

@@ -1008,11 +1008,12 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
    return activeQueries;
  }
  /**
-   * Creates a `Query` object from this `Target`. Note the result might be
-   * different from the original `Query` from which we obtained this instance.
+   * Creates a `Query` object from this `Target`. There is no way to obtain


wu-hui · 2019-10-29T13:54:01Z

packages/firestore/src/core/sync_engine.ts

-   * different from the original `Query` from which we obtained this instance.
+   * Creates a `Query` object from this `Target`. There is no way to obtain
+   * the original `Query`, so we synthesize a `Query` from the `Target` object.
+   * The result might be different from the original `Query`,


wu-hui · 2019-10-29T13:55:13Z

packages/firestore/test/integration/api/query.test.ts

@@ -140,8 +140,12 @@ apiDescribe('Queries', (persistence: boolean) => {
    });
  });

-  // Mirror queries are `Query`s mapped to the same `Target`, meaning
-  // they are supported by the same backend Query.
+  // Queries that mapped to the same target ID are referred to as


* [1/3] Query/Target split -- create Target and make remote/ and local/ work with Target. (#2254) * [2/3] Query/Target split: make SyncEngine able to handle the mapping between Target and Query. (#2281) * [2.8/3] Add limitToLast implementation and tests. (#2296) * [3/3] Spec tests for limitToLast+multi-tab (#2300) * Add spec test for single client (#2313) * add change log for limitToLast * Address comments

wu-hui added 11 commits September 23, 2019 15:11

Sketch commit

06c79f5

added toQueryTarget and more integraion tests.

0059aec

Addressing remaining comments

8dee3b1

Addressing comments #2

cc77b72

Merge branch 'wuandy/limitToLast' into wuandy/MergeLimitToLastWithQue…

56b76ba

…ryTargetSplit

add limitToLast to Query object

407a2a5

All!Tests!Pass!!

782e08b

More integration tests.

8c8026b

spec tests not working yet

2da1cb7

[AUTOMATED]: Prettier Code Styling

0ccfb75

Ready for review, no spec tests yet.

06b1162

wu-hui requested review from Feiyang1, hiranya911 and hsubox76 as code owners October 24, 2019 19:32

wu-hui requested a review from schmidt-sebastian October 24, 2019 19:35

wu-hui assigned schmidt-sebastian Oct 24, 2019

wu-hui added the api: firestore label Oct 24, 2019

wu-hui added 2 commits October 24, 2019 15:36

remove only

aec8064

Fix integration test

72bf7d2

schmidt-sebastian reviewed Oct 25, 2019

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Oct 25, 2019

wu-hui added 2 commits October 27, 2019 14:49

Address comments 1

9a00cac

[AUTOMATED]: Prettier Code Styling

eec17b0

wu-hui commented Oct 27, 2019

View reviewed changes

fix imports

dd99e3c

wu-hui assigned schmidt-sebastian and unassigned wu-hui Oct 27, 2019

schmidt-sebastian reviewed Oct 28, 2019

View reviewed changes

schmidt-sebastian removed their assignment Oct 28, 2019

schmidt-sebastian assigned wu-hui Oct 28, 2019

address comments 2

71e38f7

wu-hui assigned schmidt-sebastian and unassigned wu-hui Oct 29, 2019

schmidt-sebastian approved these changes Oct 29, 2019

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Oct 29, 2019

Address comments 3

1608d11

wu-hui commented Oct 29, 2019

View reviewed changes

wu-hui merged commit aa9390e into wuandy/QueryTargetAndLimitToLast Oct 29, 2019

wu-hui added a commit that referenced this pull request Nov 1, 2019

[2.8/3] Add limitToLast implementation and tests. (#2296)

a7e96c2

firebase locked and limited conversation to collaborators Nov 29, 2019

		@@ -1091,6 +1091,15 @@ export class Query {
		*/
		limit(limit: number): Query;

[2.8/3] Add limitToLast implementation and tests. #2296

[2.8/3] Add limitToLast implementation and tests. #2296

Uh oh!

Conversation

wu-hui commented Oct 24, 2019

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wu-hui left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!