feat: introduce EntityFilter class with support for and/or filters #1061

danieljbruce · 2023-01-19T14:55:30Z

This code will enable the composite OR filters (much like the same way the composite AND filters work).

danieljbruce · 2023-01-19T15:09:26Z

TODO: Add a unit test for OR in query and a unit test for AND in entity.

danieljbruce · 2023-01-26T15:00:57Z

system-test/datastore.ts

+            ])
+          );
+        const [entities] = await datastore.runQuery(q);
+        assert.strictEqual(entities!.length, 6);


This will change.

This reverts commit 37400a6.

Adds a new setAncestor method for ensuring only one ancestor is set for the query at a time. This will avoid errors that result because of setting multiple ancestors. Also deprecate hasAncestor because it will lead to warnings like this. Add parser logic to use the value provided in setAncestor for query sent to backend.

Added tests for the query proto, one to make sure the query structure is right when setting ancestor once and one to make sure the query structure is right when setting ancestor twice. Also added a unit test to make sure that ancestor is set the right way internally when using setAncestor.

This code change adjusts the expected result for running an OR query. Old result used to correspond with AND, but now corresponds to OR.

This reverts commit e84582f.

This commit is done to avoid a breaking typescript change which could have the potential to affect some users who read `filters` on a query as its type had been changed to be more flexible, but is now back to what it was.

AND and OR should not be static functions of the filter class because then the user has to type Filter.AND instead of AND for example.

Artifacts of having imports laying around and moving functionality between files

danieljbruce · 2023-03-01T15:04:22Z

src/filter.ts

+ * @see {@link https://cloud.google.com/datastore/docs/concepts/queries#filters| Filters Reference}
+ *
+ */
+export abstract class Filter {


Maybe this should be an interface.

This reverts commit bc15f32.

This reverts commit db02a50.

danieljbruce · 2023-03-01T15:29:00Z

system-test/datastore.ts

+          'Character',
+          'Eddard',
+        ]);
+        it('should run a query using hasAncestor first', async () => {


typo first/last should be switched around

Mariatta · 2023-03-06T18:29:42Z

system-test/datastore.ts

+            ])
+          );
+        const [entities] = await datastore.runQuery(q);
+        assert.strictEqual(entities!.length, 6);


Is it possible to also assert that the resulting entities indeed have the values family = Stark and apprearances >= 20?

I looked at the data and there is actually one entity that has family ['Stark', 'Tully']. I believe this is intended behaviour for the filter because this is also the result when we test a case where legacy .filter is used.

I just checked should filter queries with defined indexes on main. It seems this filter checks to see if Stark is present. I guess this matches what you said. We can check for its pressence.

Done in last commit

Mariatta · 2023-03-06T18:31:03Z

system-test/datastore.ts

+            ])
+          );
+        const [entities] = await datastore.runQuery(q);
+        assert.strictEqual(entities!.length, 8);


Is it possible to assert that in resulting query, there are entities that have value family = Stark and not appearances >=20?

Right now I am seeing that all the entities which are returned have a family name of Stark.

Still looking at this

Done in last commit

src/entity.ts

kolea2 · 2023-03-07T15:07:41Z

src/entity.ts

+        filter => new PropertyFilter(filter.name, filter.op, filter.val)
+      );
+      const newFilters = query.newFilters;
+      queryProto.filter = AND(newFilters.concat(filters)).toProto();


a little confused by this, what is this logic doing? Can you add some comments?

Added comments to the code here.

This is mainly just a refactor. Old code is being moved into the Filter classes, but this new code does the same thing as the removed code. The functionality had to be pulled into the Filter classes to avoid creating duplicate code.

kolea2 · 2023-03-07T15:10:10Z

src/query.ts

@@ -76,6 +76,7 @@ class Query {
  namespace?: string | null;
  kinds: string[];
  filters: Filter[];
+  newFilters: NewFilter[];


why do we need another variable for this?

At first I put all the filters into the filters object whether they were the old type of filter or the new filter class. This caused a compiler error however caught by https://github.com/googleapis/nodejs-datastore/pull/1067/files#diff-a9fb5a76e455ff1f3d63199cae37a32e289867ec02b1ec205b79468d074a211eR881.

Currently, users may be popping off the filters variable and expecting that variable to have the val property, but it won't if we allow filter to store the new Filter class.

kolea2 · 2023-03-07T15:10:44Z

src/query.ts

@@ -201,24 +207,29 @@ class Query {
   * const keyQuery = query.filter('__key__', key);
   * ```
   */
-  filter(property: string, value: {} | null): Query;
-  filter(property: string, operator: Operator, value: {} | null): Query;
+  filter(propertyOrFilter: string | NewFilter, value?: {} | null): Query;


why can't this just be string | Filter?

In this module, NewFilter is just what we call the imported Filter class. We cannot call it Filter because the name Filter is already taken in this module.

https://github.com/googleapis/nodejs-datastore/pull/1061/files#diff-ad0978c0145d3620d2a97bdff4307f9496529c0e3011a845e2d5bf1a169d2538R21

Filter has been renamed entityFilter so as not to cause confusion.

We add additional asserts to the data in order to capture the nuances of the composite operator. For example, for the OR test we make sure the filter doesn’t always require both conditions to be true.

Code for building the `filter` property of the query proto was pulled into the `Filter` object. Comments indicate how legacy functionality was maintained and which lines of code perform which task.

rename new filter to entity filter to eliminate the need for an internal rename that causes confusion

These test cases have mistakes in their names. We should change them to reflect the position of `hasAncestor`.

kolea2 · 2023-03-08T15:48:26Z

src/query.ts

@@ -170,7 +176,7 @@ class Query {
   *
   * @see {@link https://cloud.google.com/datastore/docs/concepts/queries#datastore-property-filter-nodejs| Datastore Filters}
   *
-   * @param {string} property The field name.
+   * @param {string | NewFilter} propertyOrFilter The field name.


shouldn't this be EntityFilter now? (side note - how is this compiling?)

You are right. This needs to change. This is compiling because this is actually just a documentation comment.

The name of the base class is now EntityFilter. Adjust this comment so that it correctly matches the parameter type.

bcoe

I left a few minor suggestions, this is looking really good IMO:

A comprehensive suite of tests.
Code reads well.

bcoe · 2023-03-08T16:39:19Z

src/entity.ts

@@ -21,6 +21,7 @@ import {PathType} from '.';
 import {protobuf as Protobuf} from 'google-gax';
 import * as path from 'path';
 import {google} from '../protos/protos';
+import {AND, PropertyFilter} from './filter';


nit, upper case is usually reserved for variable constants const FOO = 9; BAR = 99, to make it clear that this is an exported method, I might go with:

and/or, or applyAnd, applyOr.

Choosing the former

bcoe · 2023-03-08T16:53:47Z

src/filter.ts

+   */
+  // eslint-disable-next-line
+  toProto(): any {
+    const OP_TO_OPERATOR = new Map([


nit: I would probably hoist this to the top of the file, rather than declaring in toProto, at scale this will speed things up since you're allocating less Maps.

bcoe · 2023-03-08T16:56:03Z

src/query.ts

-      value = operatorOrValue as {};
-      operator = '=';
-    }
+    if (isFilter(propertyOrFilter)) {


This is good use of TypeScript's is functionality 👍

bcoe · 2023-03-08T17:01:09Z

@danieljbruce the tile of this PR seems a bit off, I believe you're actually introducing the ability to filter using a class rather than just a string?

Perhaps a better title:

feat: introduce EntityFilter class with support for and/or filters

Or something like this.

rename composite filter functions so that they don’t look like constants and move a map up so that it doesn’t have to be initialized every time.

kolea2 · 2023-03-08T18:20:12Z

src/entity.ts

+      const filters = query.filters.map(
+        filter => new PropertyFilter(filter.name, filter.op, filter.val)
+      );
+      const newFilters = query.entityFilters;


nit - rename variable

This was done

danieljbruce added 10 commits January 13, 2023 17:28

or filters interface

d370b54

implementation of new filter

3291df8

Remove duplicate code

69390d4

Parse with type guard

c90e932

Remove the newFilter variable

c2b29fc

Add filter and enable chaining

69f1cbf

test add filter

3f720c0

Add another unit test

20f616b

Add system-test stubs

5dea24a

Add system-tests for the OR filter

bb931e7

danieljbruce requested review from a team as code owners January 19, 2023 14:55

product-auto-label bot added size: l Pull request size is large. api: datastore Issues related to the googleapis/nodejs-datastore API. labels Jan 19, 2023

danieljbruce and others added 2 commits January 19, 2023 10:05

Move things around

44be8a9

Merge branch 'main' into or-filters

0055a09

danieljbruce added 3 commits January 19, 2023 13:30

Added a unit test

b6bb3b3

Merge branch 'or-filters' of https://github.com/danieljbruce/nodejs-d…

83668a1

…atastore into or-filters

Add a unit test for OR filters

c4fe304

kolea2 added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jan 19, 2023

Just use filter method

c397608

danieljbruce commented Jan 26, 2023

View reviewed changes

danieljbruce added 7 commits January 26, 2023 17:17

new warning test

37400a6

Revert "new warning test"

8ab6b80

This reverts commit 37400a6.

Now removes deprecation warning properly

db02a50

Add a test for new warning

bc15f32

change expected result for OR query

fe50d56

This code change adjusts the expected result for running an OR query. Old result used to correspond with AND, but now corresponds to OR.

danieljbruce added 4 commits February 27, 2023 15:53

Revert "Add setAncestor"

5f38179

This reverts commit e84582f.

Separate filters and new filters internally

059d4dc

This commit is done to avoid a breaking typescript change which could have the potential to affect some users who read `filters` on a query as its type had been changed to be more flexible, but is now back to what it was.

Move AND/OR into their own separate function

ec752e8

AND and OR should not be static functions of the filter class because then the user has to type Filter.AND instead of AND for example.

Eliminate unused imports

a5c41ab

Artifacts of having imports laying around and moving functionality between files

danieljbruce commented Mar 1, 2023

View reviewed changes

danieljbruce added 2 commits March 1, 2023 10:09

Revert "Add a test for new warning"

1ddeea6

This reverts commit bc15f32.

Revert "Now removes deprecation warning properly"

8e96bf5

This reverts commit db02a50.

danieljbruce commented Mar 1, 2023

View reviewed changes

Mariatta reviewed Mar 6, 2023

View reviewed changes

kolea2 reviewed Mar 7, 2023

View reviewed changes

danieljbruce added 4 commits March 7, 2023 10:12

Modify test cases to capture nuances in data

5883096

We add additional asserts to the data in order to capture the nuances of the composite operator. For example, for the OR test we make sure the filter doesn’t always require both conditions to be true.

Added comments to code that was refactored

0a68ed1

Code for building the `filter` property of the query proto was pulled into the `Filter` object. Comments indicate how legacy functionality was maintained and which lines of code perform which task.

rename NewFilter to entity filter

35bae06

rename new filter to entity filter to eliminate the need for an internal rename that causes confusion

Switch around last and first

262894a

These test cases have mistakes in their names. We should change them to reflect the position of `hasAncestor`.

kolea2 suggested changes Mar 8, 2023

View reviewed changes

kolea2 removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 8, 2023

Change the comment to reflect the new name

30c3832

The name of the base class is now EntityFilter. Adjust this comment so that it correctly matches the parameter type.

bcoe approved these changes Mar 8, 2023

View reviewed changes

danieljbruce changed the title ~~feat: OR filters~~ feat: introduce EntityFilter class with support for and/or filters Mar 8, 2023

rename and move constant map up

fa2235d

rename composite filter functions so that they don’t look like constants and move a map up so that it doesn’t have to be initialized every time.

kolea2 approved these changes Mar 8, 2023

View reviewed changes

danieljbruce and others added 2 commits March 8, 2023 13:36

Rename newFilters to entityFilters

4e7f9d5

Merge branch 'main' into or-filters

c72fcc9

danieljbruce added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 8, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 8, 2023

Mariatta approved these changes Mar 8, 2023

View reviewed changes

danieljbruce merged commit 8fc58c0 into googleapis:main Mar 8, 2023

release-please bot mentioned this pull request Mar 8, 2023

chore(main): release 7.4.0 #1077

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: introduce EntityFilter class with support for and/or filters #1061

feat: introduce EntityFilter class with support for and/or filters #1061

danieljbruce commented Jan 19, 2023

danieljbruce commented Jan 19, 2023

danieljbruce Jan 26, 2023

danieljbruce Mar 1, 2023

danieljbruce Mar 1, 2023

danieljbruce Mar 7, 2023

Mariatta Mar 6, 2023

danieljbruce Mar 6, 2023

danieljbruce Mar 6, 2023

danieljbruce Mar 7, 2023

Mariatta Mar 6, 2023

danieljbruce Mar 7, 2023

danieljbruce Mar 7, 2023

danieljbruce Mar 7, 2023

kolea2 Mar 7, 2023

danieljbruce Mar 7, 2023

kolea2 Mar 7, 2023

danieljbruce Mar 7, 2023

kolea2 Mar 7, 2023

danieljbruce Mar 7, 2023

danieljbruce Mar 7, 2023

kolea2 Mar 8, 2023

danieljbruce Mar 8, 2023

danieljbruce Mar 8, 2023

bcoe left a comment

bcoe Mar 8, 2023

danieljbruce Mar 8, 2023

bcoe Mar 8, 2023

danieljbruce Mar 8, 2023

bcoe Mar 8, 2023

danieljbruce Mar 8, 2023

bcoe commented Mar 8, 2023

kolea2 Mar 8, 2023

danieljbruce Mar 8, 2023

feat: introduce EntityFilter class with support for and/or filters #1061

feat: introduce EntityFilter class with support for and/or filters #1061

Conversation

danieljbruce commented Jan 19, 2023

danieljbruce commented Jan 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcoe commented Mar 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment