fix: allow IN queries on key #1085

danieljbruce · 2023-03-14T21:35:15Z

The fix contains:

An update to the way that entities are encoded
An update to tests so that an instanceOf check passes
A cleanup of the PropertyFilter function
An additional test to check for IN with keys

This change matches the change shown in another PR, but the test cases won’t allow for the new change. There must be some edge cases that haven’t been considered here.

isDsKey currently does not check to see that the key is an entity.Key properly so we need to modify this function so that it does a proper check.

This reverts commit 4f0db50.

This reverts commit d279595.

For an instanceof check to pass, we need to import the entity property the right way

Make code a lot more succinct for the PropertyFilter class before adding the new test in.

There is no need for a private encodeValue function so we just inline this change.

This test supports using __key__ with IN and an array.

This change modifies the proto to be what we want it to be so that the deep strict equal check works.

This commit removes a comment that isn’t necessary anymore and renames a variable to demonstrate its proper use.

The new test doesn’t enter the DSKey check when the entity is used from the beforeEach hook. Use the global entity instead.

The specific query proto used should be moved into the test as it is only used by that specific test.

vicb

Looks nice 👍

There are a few "GitHub Actions / lint" that look worth addressing - IMO the typings of this lib could use some love but out of scope for this PR.

vicb · 2023-03-15T14:49:04Z

src/filter.ts

@@ -13,7 +13,7 @@
 // limitations under the License.

 import {Operator, Filter as IFilter} from './query';
-import {entity} from './entity';
+import {entity, ValueProto} from './entity';


Please fix (remove ValueProto)

vicb · 2023-03-15T14:50:13Z

src/filter.ts

-      this.op,
-      this.val
-    ).encodedValue();
+    const value = entity.encodeValue(this.val, this.name);


You can also value: entity.encodeValue(this.val, this.name) inline. Not sure what the preferred style is.

Yes. This can be even more compact.

vicb · 2023-03-15T14:51:21Z

test/entity.ts

@@ -1894,8 +1894,62 @@ describe('entity', () => {
      assert.deepStrictEqual(entity.queryToQueryProto(query), queryProto);
    });

+    it('should support using key with IN', () => {


nit: I'm not sure if "with IN" is relevant here?

Good point.

I think more specifically, it is addressing the case of using an array as a value as that was what was causing the error in the first place.

Remove an unused import, make value more inline and change name of test case to reflect new functionality.

danieljbruce · 2023-03-17T15:37:18Z

@vicb There is just one more thing we should confirm. I notice that val in Property Filter is of type any so even though filtering by __key__ ought to use only values that are keys and arrays of keys, the compiler doesn't guarantee it. Are these definitely the only two cases with which we can expect users to filter with __key__?

Since the new test simply introduces a new proto and checks it against __key__ with an array, we still have no way to verify if this encoding will actually work. In this system test we verify that using __key__ with an array actually works in a query.

vicb · 2023-03-17T16:41:55Z

val in Property Filter is of type any

As discussed it should probably be unknown instead.

Are these definitely the only two cases with which we can expect users to filter with key?

Probably.
You should be able to leverage Typescript conditional types to enforce that.

Modify the type to use `unknown` instead of `any`.

Leverage typescript to let the user know at compile time that Keys and arrays of keys are to be used when __key__ is used.

The restricted value type should be used on filter as well as the property filter.

vicb · 2023-03-17T22:48:23Z

src/filter.ts

@@ -56,42 +57,34 @@ export abstract class EntityFilter {
  abstract toProto(): any;
 }

+export type restrictedValueType<T> = T extends '__key__'


RestrictedValueType

Changed the name of this variable to more specifically describe what this does

vicb · 2023-03-17T22:57:00Z

src/query.ts

+    value?: restrictedValueType<T> | null
+  ): Query;
+  filter<T extends string>(
+    propertyOrFilter: T,


T | EntityFilter

Signature has been updated

vicb · 2023-03-17T22:57:56Z

src/query.ts

+    value: restrictedValueType<T> | null
+  ): Query;
+  filter<T extends string>(
+    propertyOrFilter: T | EntityFilter,
    operatorOrValue?: Operator,


This should be restrictedValueType<T> | null | Operator

Signature has been updated

danieljbruce · 2023-03-20T14:20:30Z

See 8d269d2 for a case where the compiler doesn't seem to complain even though it should.

danieljbruce · 2023-03-20T14:21:02Z

I mean see 8d269d2

danieljbruce · 2023-03-20T14:22:09Z

I mean #1089

Checking for nulls is now included in the restricted value type so we don’t need to declare it in the signature of the function.

rename a value type variable to allowed filter value type to more accurately describe the type’s purpose.

The old variable name doesn’t follow the naming convention.

prettier and change the typeguard so that it checks to see if the input is an instance of EntityFilter.

kolea2 · 2023-03-22T14:43:31Z

src/filter.ts

@@ -156,5 +144,5 @@ class CompositeFilter extends EntityFilter {
 }

 export function isFilter(filter: any): filter is EntityFilter {
-  return (filter as EntityFilter).toProto !== undefined;
+  return filter instanceof EntityFilter;


why does this have to change?

I suggested the fix so I'll answer.

filter.toProto !== undefined only checks if the object has a toProto properties (not set to undefined)
It is the case that EntityFilters have a toProto methods but it is not necessarily the case that an object with a toProto property is an EntityFilter.

filter instanceof EntityFilter is the correct check for filter is EntityFilter

I hope that makes sense.

thanks @vicb, both using instanceof and avoiding type assertions (filter as EntityFilter) are recommended in the Google TypeScript Style Guide. LGTM 👍

danieljbruce · 2023-03-22T17:40:21Z

src/query.ts

-    propertyOrFilter: string | EntityFilter,
-    operatorOrValue?: Operator,
-    value?: {} | null
+  filter(filter: EntityFilter): Query;


This signature change makes the input parameters more restrictive. Do we really want to go through with this? If so would this be a major version bump?

@vicb @bcoe

My take is that the old signature was buggy and wouldn't catch unsupported calls. So I see this as a bug fix.

Also I think the impl could be improved by switching on arguments.length. 1, 2, 3 corresponding to the first 3 signatures in order.

I will do this in a separate issue:

#1096

+1 to go with a patch version bump

ruyadorno

LGTM 👍

Let's just make sure to keep an eye and monitor if the internal change from src/filter.ts encodeValue() is going to have any impact in the end users of the library. Releasing this as a patch version release is going to help us having the flexibility to rollback if needed, so I'd advise to proceed with a fix: commit.

Great work @danieljbruce!

ruyadorno · 2023-03-27T16:17:10Z

src/query.ts

-    propertyOrFilter: string | EntityFilter,
-    operatorOrValue?: Operator,
-    value?: {} | null
+  filter(filter: EntityFilter): Query;


+1 to go with a patch version bump

ruyadorno · 2023-03-27T16:23:08Z

src/filter.ts

@@ -156,5 +144,5 @@ class CompositeFilter extends EntityFilter {
 }

 export function isFilter(filter: any): filter is EntityFilter {
-  return (filter as EntityFilter).toProto !== undefined;
+  return filter instanceof EntityFilter;


thanks @vicb, both using instanceof and avoiding type assertions (filter as EntityFilter) are recommended in the Google TypeScript Style Guide. LGTM 👍

danieljbruce added 11 commits March 10, 2023 16:56

Change to try to ignore __key__ as it is redundant

ad3d2ea

This change matches the change shown in another PR, but the test cases won’t allow for the new change. There must be some edge cases that haven’t been considered here.

Add a check for isDsKey

4f0db50

isDsKey currently does not check to see that the key is an entity.Key properly so we need to modify this function so that it does a proper check.

Revert "Add a check for isDsKey"

23c3200

This reverts commit 4f0db50.

Add comments so that key is never reassigned

d279595

Revert "Add comments so that key is never reassigned"

ee08623

This reverts commit d279595.

Use global entity to test

72acbf1

For an instanceof check to pass, we need to import the entity property the right way

clean up the property class

258c478

Make code a lot more succinct for the PropertyFilter class before adding the new test in.

Inline encodeValue

f3ff52f

There is no need for a private encodeValue function so we just inline this change.

Add a test for supporting key with IN

d91b8bd

This test supports using __key__ with IN and an array.

Modifies the query proto to correct the test

6337f63

This change modifies the proto to be what we want it to be so that the deep strict equal check works.

Remove comments and clean up test

a9bcc0a

This commit removes a comment that isn’t necessary anymore and renames a variable to demonstrate its proper use.

danieljbruce requested review from a team as code owners March 14, 2023 21:35

product-auto-label bot added size: m Pull request size is medium. api: datastore Issues related to the googleapis/nodejs-datastore API. labels Mar 14, 2023

Use global entity in the new test instead of entit

7910b43

The new test doesn’t enter the DSKey check when the entity is used from the beforeEach hook. Use the global entity instead.

danieljbruce changed the title ~~1065 query filter for in with keys test~~ fix: 1065 query filter for in with keys test Mar 15, 2023

Move the query proto into the test

f186ee2

The specific query proto used should be moved into the test as it is only used by that specific test.

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

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

vicb approved these changes Mar 15, 2023

View reviewed changes

PR updates

3e7f057

Remove an unused import, make value more inline and change name of test case to reflect new functionality.

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

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

danieljbruce added 3 commits March 17, 2023 13:03

use unknown instead of any for value

1dcc17f

Modify the type to use `unknown` instead of `any`.

Introduce restricted value when key is used

de1f070

Leverage typescript to let the user know at compile time that Keys and arrays of keys are to be used when __key__ is used.

Add the restricted value type to query too

25575ba

The restricted value type should be used on filter as well as the property filter.

vicb reviewed Mar 17, 2023

View reviewed changes

danieljbruce added 4 commits March 20, 2023 16:22

Modify the interface for filter

8b07e55

Checking for nulls is now included in the restricted value type so we don’t need to declare it in the signature of the function.

rename to allowedFilterValueType

40a5e19

rename a value type variable to allowed filter value type to more accurately describe the type’s purpose.

Use capital letter to follow name conventions

cf9105c

The old variable name doesn’t follow the naming convention.

type guard should check instance of

a551ebe

prettier and change the typeguard so that it checks to see if the input is an instance of EntityFilter.

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

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

Merge branch 'main' into 1065-query-filter-for-IN-with-keys-test

8bbc2dc

danieljbruce changed the title ~~fix: 1065 query filter for in with keys test~~ fix: 1065 eliminate unnecessary logic when using __key__ for a filter Mar 21, 2023

danieljbruce changed the title ~~fix: 1065 eliminate unnecessary logic when using __key__ for a filter~~ fix: allow IN queries on __key__ Mar 21, 2023

kolea2 reviewed Mar 22, 2023

View reviewed changes

danieljbruce commented Mar 22, 2023

View reviewed changes

danieljbruce mentioned this pull request Mar 27, 2023

Change filter in query implementation to use a switch #1096

Closed

ruyadorno approved these changes Mar 27, 2023

View reviewed changes

danieljbruce added 2 commits March 27, 2023 13:05

Merge branch 'main' into 1065-query-filter-for-IN-with-keys-test

d68506e

Merge branch 'main' into 1065-query-filter-for-IN-with-keys-test

fb9bdf7

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

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

danieljbruce merged commit dd2d5f4 into googleapis:main Mar 29, 2023

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

chore(main): release 7.5.0 #1095

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: allow IN queries on key #1085

fix: allow IN queries on key #1085

danieljbruce commented Mar 14, 2023

vicb left a comment

vicb Mar 15, 2023

danieljbruce Mar 15, 2023

vicb Mar 15, 2023

danieljbruce Mar 15, 2023

vicb Mar 15, 2023

danieljbruce Mar 15, 2023

danieljbruce commented Mar 17, 2023

vicb commented Mar 17, 2023

vicb Mar 17, 2023

danieljbruce Mar 20, 2023

vicb Mar 17, 2023

danieljbruce Mar 20, 2023

vicb Mar 17, 2023

danieljbruce Mar 20, 2023

danieljbruce commented Mar 20, 2023

danieljbruce commented Mar 20, 2023

danieljbruce commented Mar 20, 2023

kolea2 Mar 22, 2023

vicb Mar 22, 2023

ruyadorno Mar 27, 2023

danieljbruce Mar 22, 2023

vicb Mar 22, 2023

danieljbruce Mar 27, 2023

ruyadorno Mar 27, 2023

ruyadorno left a comment

ruyadorno Mar 27, 2023

ruyadorno Mar 27, 2023

fix: allow IN queries on __key__ #1085

fix: allow IN queries on __key__ #1085

Conversation

danieljbruce commented Mar 14, 2023

vicb 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

danieljbruce commented Mar 17, 2023

vicb commented Mar 17, 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

danieljbruce commented Mar 20, 2023

danieljbruce commented Mar 20, 2023

danieljbruce commented Mar 20, 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

ruyadorno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fix: allow IN queries on key #1085

fix: allow IN queries on key #1085