From 8fc58c0f18e5333f996099e09ec673e9bf3460af Mon Sep 17 00:00:00 2001 From: danieljbruce Date: Wed, 8 Mar 2023 17:28:25 -0500 Subject: [PATCH] feat: introduce EntityFilter class with support for and/or filters (#1061) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * or filters interface * implementation of new filter * Remove duplicate code * Parse with type guard * Remove the newFilter variable * Add filter and enable chaining * test add filter * Add another unit test * Add system-test stubs * Add system-tests for the OR filter * Move things around * Added a unit test * Add a unit test for OR filters * Just use filter method * new warning test * Revert "new warning test" This reverts commit 37400a6d3a9d7385a0203db70949f037e4c35de4. * Now removes deprecation warning properly * Add a test for new warning * Add setAncestor 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. * Basic unit tests for setAncestor 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. * change expected result for OR query This code change adjusts the expected result for running an OR query. Old result used to correspond with AND, but now corresponds to OR. * Fix a test by not requiring the done callback A test is timing out because we are waiting for done to be called. This fix does not require done to be called. * Revert "Fix a test by not requiring the done callback" This reverts commit 1159b374f483d107736a765b5b4f1ade80116ce3. * Revert "Basic unit tests for setAncestor" This reverts commit 86841d6e0a3d26e2df647fa4a42ed849b61b38a6. * Revert "Add setAncestor" This reverts commit e84582faa6041429a4e81d47bd21dbc338a639e7. * Separate filters and new filters internally 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 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 Artifacts of having imports laying around and moving functionality between files * Revert "Add a test for new warning" This reverts commit bc15f325d87b4ffe46ae8727293c5cefa2300501. * Revert "Now removes deprecation warning properly" This reverts commit db02a5089242b25b4bebae4304a4778f0bf6d17f. * Modify test cases to capture nuances in data 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 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 rename new filter to entity filter to eliminate the need for an internal rename that causes confusion * Switch around last and first These test cases have mistakes in their names. We should change them to reflect the position of `hasAncestor`. * Change the comment to reflect the new name The name of the base class is now EntityFilter. Adjust this comment so that it correctly matches the parameter type. * rename and move constant map up 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. * Rename newFilters to entityFilters --- src/entity.ts | 55 ++++--------- src/filter.ts | 160 ++++++++++++++++++++++++++++++++++++ src/query.ts | 43 ++++++---- system-test/data/index.yaml | 6 ++ system-test/datastore.ts | 105 +++++++++++++++++++++++ test/entity.ts | 50 +++++++++++ test/query.ts | 35 ++++++++ 7 files changed, 398 insertions(+), 56 deletions(-) create mode 100644 src/filter.ts diff --git a/src/entity.ts b/src/entity.ts index 599f0073e..2023d511d 100644 --- a/src/entity.ts +++ b/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'; // eslint-disable-next-line @typescript-eslint/no-namespace export namespace entity { @@ -1183,18 +1184,6 @@ export namespace entity { * ``` */ export function queryToQueryProto(query: Query): QueryProto { - const OP_TO_OPERATOR = { - '=': 'EQUAL', - '>': 'GREATER_THAN', - '>=': 'GREATER_THAN_OR_EQUAL', - '<': 'LESS_THAN', - '<=': 'LESS_THAN_OR_EQUAL', - HAS_ANCESTOR: 'HAS_ANCESTOR', - '!=': 'NOT_EQUAL', - IN: 'IN', - NOT_IN: 'NOT_IN', - }; - const SIGN_TO_ORDER = { '-': 'DESCENDING', '+': 'ASCENDING', @@ -1249,34 +1238,20 @@ export namespace entity { queryProto.startCursor = query.startVal; } - if (query.filters.length > 0) { - const filters = query.filters.map(filter => { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - let value: any = {}; - - if (filter.name === '__key__') { - value.keyValue = entity.keyToKeyProto(filter.val); - } else { - value = entity.encodeValue(filter.val, filter.name); - } - - return { - propertyFilter: { - property: { - name: filter.name, - }, - op: OP_TO_OPERATOR[filter.op], - value, - }, - }; - }); - - queryProto.filter = { - compositeFilter: { - filters, - op: 'AND', - }, - }; + // Check to see if there is at least one type of legacy filter or new filter. + if (query.filters.length > 0 || query.entityFilters.length > 0) { + // Convert all legacy filters into new property filter objects + const filters = query.filters.map( + filter => new PropertyFilter(filter.name, filter.op, filter.val) + ); + const entityFilters = query.entityFilters; + const allFilters = entityFilters.concat(filters); + /* + To be consistent with prior implementation, apply an AND composite filter + to the collection of Filter objects. Then, set the filter property as before + to the output of the toProto method. + */ + queryProto.filter = and(allFilters).toProto(); } return queryProto; diff --git a/src/filter.ts b/src/filter.ts new file mode 100644 index 000000000..5897707e5 --- /dev/null +++ b/src/filter.ts @@ -0,0 +1,160 @@ +// Copyright 2023 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import {Operator, Filter as IFilter} from './query'; +import {entity} from './entity'; + +const OP_TO_OPERATOR = new Map([ + ['=', 'EQUAL'], + ['>', 'GREATER_THAN'], + ['>=', 'GREATER_THAN_OR_EQUAL'], + ['<', 'LESS_THAN'], + ['<=', 'LESS_THAN_OR_EQUAL'], + ['HAS_ANCESTOR', 'HAS_ANCESTOR'], + ['!=', 'NOT_EQUAL'], + ['IN', 'IN'], + ['NOT_IN', 'NOT_IN'], +]); + +enum CompositeOperator { + AND = 'AND', + OR = 'OR', +} + +export function and(filters: EntityFilter[]): CompositeFilter { + return new CompositeFilter(filters, CompositeOperator.AND); +} + +export function or(filters: EntityFilter[]): CompositeFilter { + return new CompositeFilter(filters, CompositeOperator.OR); +} + +/** + * A Filter is a class that contains data for a filter that can be translated + * into a proto when needed. + * + * @see {@link https://cloud.google.com/datastore/docs/concepts/queries#filters| Filters Reference} + * + */ +export abstract class EntityFilter { + /** + * Gets the proto for the filter. + * + */ + // eslint-disable-next-line + abstract toProto(): any; +} + +/** + * A PropertyFilter is a filter that gets applied to a query directly. + * + * @see {@link https://cloud.google.com/datastore/docs/concepts/queries#property_filters| Property filters Reference} + * + * @class + */ +export class PropertyFilter extends EntityFilter implements IFilter { + name: string; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + val: any; + op: Operator; + + /** + * Build a Property Filter object. + * + * @param {string} Property + * @param {Operator} operator + * @param {any} val + */ + constructor(property: string, operator: Operator, val: any) { + super(); + this.name = property; + this.op = operator; + this.val = val; + } + + private encodedValue(): any { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + let value: any = {}; + if (this.name === '__key__') { + value.keyValue = entity.keyToKeyProto(this.val); + } else { + value = entity.encodeValue(this.val, this.name); + } + return value; + } + + /** + * Gets the proto for the filter. + * + */ + // eslint-disable-next-line + toProto(): any { + const value = new PropertyFilter( + this.name, + this.op, + this.val + ).encodedValue(); + return { + propertyFilter: { + property: { + name: this.name, + }, + op: OP_TO_OPERATOR.get(this.op), + value, + }, + }; + } +} + +/** + * A CompositeFilter is a filter that combines other filters and applies that + * combination to a query. + * + * @see {@link https://cloud.google.com/datastore/docs/concepts/queries#composite_filters| Composite filters Reference} + * + * @class + */ +class CompositeFilter extends EntityFilter { + filters: EntityFilter[]; + op: string; + + /** + * Build a Composite Filter object. + * + * @param {EntityFilter[]} filters + */ + constructor(filters: EntityFilter[], op: CompositeOperator) { + super(); + this.filters = filters; + this.op = op; + } + + /** + * Gets the proto for the filter. + * + */ + // eslint-disable-next-line + toProto(): any { + return { + compositeFilter: { + filters: this.filters.map(filter => filter.toProto()), + op: this.op, + }, + }; + } +} + +export function isFilter(filter: any): filter is EntityFilter { + return (filter as EntityFilter).toProto !== undefined; +} diff --git a/src/query.ts b/src/query.ts index e8f333ca9..153a39414 100644 --- a/src/query.ts +++ b/src/query.ts @@ -18,10 +18,10 @@ import arrify = require('arrify'); import {Key} from 'readline'; import {Datastore} from '.'; import {Entity} from './entity'; +import {EntityFilter, isFilter} from './filter'; import {Transaction} from './transaction'; import {CallOptions} from 'google-gax'; import {RunQueryStreamOptions} from '../src/request'; -import {AggregateField, AggregateQuery} from './aggregate'; export type Operator = | '=' @@ -76,6 +76,7 @@ class Query { namespace?: string | null; kinds: string[]; filters: Filter[]; + entityFilters: EntityFilter[]; orders: Order[]; groupByVal: Array<{}>; selectVal: Array<{}>; @@ -123,6 +124,11 @@ class Query { * @type {array} */ this.filters = []; + /** + * @name Query#entityFilters + * @type {array} + */ + this.entityFilters = []; /** * @name Query#orders * @type {array} @@ -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 | EntityFilter} propertyOrFilter The field name. * @param {string} [operator="="] Operator (=, <, >, <=, >=). * @param {*} value Value to compare property to. * @returns {Query} @@ -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 | EntityFilter, value?: {} | null): Query; + filter(propertyOrFilter: string, operator: Operator, value: {} | null): Query; filter( - property: string, - operatorOrValue: Operator, + propertyOrFilter: string | EntityFilter, + operatorOrValue?: Operator, value?: {} | null ): Query { - let operator = operatorOrValue as Operator; - if (arguments.length === 2) { - value = operatorOrValue as {}; - operator = '='; - } + if (isFilter(propertyOrFilter)) { + this.entityFilters.push(propertyOrFilter); + return this; + } else { + let operator = operatorOrValue as Operator; + if (arguments.length === 2) { + value = operatorOrValue as {}; + operator = '='; + } - this.filters.push({ - name: property.trim(), - op: operator.trim() as Operator, - val: value, - }); + this.filters.push({ + name: (propertyOrFilter as String).trim(), + op: operator.trim() as Operator, + val: value, + }); + } return this; } diff --git a/system-test/data/index.yaml b/system-test/data/index.yaml index 5a2d2b1a8..0900b970f 100644 --- a/system-test/data/index.yaml +++ b/system-test/data/index.yaml @@ -16,6 +16,12 @@ indexes: - name: family - name: appearances +- kind: Character + ancestor: no + properties: + - name: family + - name: appearances + - kind: Character ancestor: yes properties: diff --git a/system-test/datastore.ts b/system-test/datastore.ts index 6af021a52..bf663fc56 100644 --- a/system-test/datastore.ts +++ b/system-test/datastore.ts @@ -21,6 +21,7 @@ import {Datastore, Index} from '../src'; import {google} from '../protos/protos'; import {Storage} from '@google-cloud/storage'; import {AggregateField} from '../src/aggregate'; +import {PropertyFilter, EntityFilter, and, or} from '../src/filter'; describe('Datastore', () => { const testKinds: string[] = []; @@ -811,6 +812,110 @@ describe('Datastore', () => { const [entities] = await datastore.runQuery(q); assert.strictEqual(entities!.length, 6); }); + describe('with the filter function using the Filter class', () => { + it('should run a query with one property filter', async () => { + const filter = new PropertyFilter('family', '=', 'Stark'); + const q = datastore + .createQuery('Character') + .filter(filter) + .hasAncestor(ancestor); + const [entities] = await datastore.runQuery(q); + assert.strictEqual(entities!.length, 8); + }); + it('should run a query with two property filters', async () => { + const q = datastore + .createQuery('Character') + .filter(new PropertyFilter('family', '=', 'Stark')) + .filter(new PropertyFilter('appearances', '>=', 20)); + const [entities] = await datastore.runQuery(q); + assert.strictEqual(entities!.length, 6); + }); + it('should run a query using new Filter class with filter', async () => { + const q = datastore + .createQuery('Character') + .filter('family', 'Stark') + .filter(new PropertyFilter('appearances', '>=', 20)); + const [entities] = await datastore.runQuery(q); + assert.strictEqual(entities!.length, 6); + for (const entity of entities) { + if (Array.isArray(entity.family)) { + assert.strictEqual(entity.family[0], 'Stark'); + } else { + assert.strictEqual(entity.family, 'Stark'); + } + assert(entity.appearances >= 20); + } + }); + it('should run a query using an AND composite filter', async () => { + const q = datastore + .createQuery('Character') + .filter( + and([ + new PropertyFilter('family', '=', 'Stark'), + new PropertyFilter('appearances', '>=', 20), + ]) + ); + const [entities] = await datastore.runQuery(q); + assert.strictEqual(entities!.length, 6); + for (const entity of entities) { + if (Array.isArray(entity.family)) { + assert.strictEqual(entity.family[0], 'Stark'); + } else { + assert.strictEqual(entity.family, 'Stark'); + } + assert(entity.appearances >= 20); + } + }); + it('should run a query using an OR composite filter', async () => { + const q = datastore + .createQuery('Character') + .filter( + or([ + new PropertyFilter('family', '=', 'Stark'), + new PropertyFilter('appearances', '>=', 20), + ]) + ); + const [entities] = await datastore.runQuery(q); + assert.strictEqual(entities!.length, 8); + let atLeastOne = false; + for (const entity of entities) { + const familyHasStark = Array.isArray(entity.family) + ? entity.family[0] === 'Stark' + : entity.family === 'Stark'; + const hasEnoughAppearances = entity.appearances >= 20; + if (familyHasStark && !hasEnoughAppearances) { + atLeastOne = true; + } + } + assert(atLeastOne); + }); + describe('using hasAncestor and Filter class', () => { + const secondAncestor = datastore.key([ + 'Book', + 'GoT', + 'Character', + 'Rickard', + 'Character', + 'Eddard', + ]); + it('should run a query using hasAncestor last', async () => { + const q = datastore + .createQuery('Character') + .filter(new PropertyFilter('appearances', '<', 30)) + .hasAncestor(secondAncestor); + const [entities] = await datastore.runQuery(q); + assert.strictEqual(entities!.length, 3); + }); + it('should run a query using hasAncestor first', async () => { + const q = datastore + .createQuery('Character') + .hasAncestor(secondAncestor) + .filter(new PropertyFilter('appearances', '<', 30)); + const [entities] = await datastore.runQuery(q); + assert.strictEqual(entities!.length, 3); + }); + }); + }); describe('with a count filter', () => { it('should run a count aggregation', async () => { const q = datastore.createQuery('Character'); diff --git a/test/entity.ts b/test/entity.ts index 53d9cd364..0557884b6 100644 --- a/test/entity.ts +++ b/test/entity.ts @@ -19,6 +19,7 @@ import * as sinon from 'sinon'; import {Datastore} from '../src'; import {Entity} from '../src/entity'; import {IntegerTypeCastOptions} from '../src/query'; +import {PropertyFilter, EntityFilter, and} from '../src/filter'; export function outOfBoundsError(opts: { propertyName?: string; @@ -1891,6 +1892,55 @@ describe('entity', () => { assert.deepStrictEqual(entity.queryToQueryProto(query), queryProto); }); + it('should support the filter method with Filter objects', () => { + const ancestorKey = new entity.Key({ + path: ['Kind2', 'somename'], + }); + + const ds = new Datastore({projectId: 'project-id'}); + + const query = ds + .createQuery('Kind1') + .filter(new PropertyFilter('name', '=', 'John')) + .start('start') + .end('end') + .groupBy(['name']) + .order('name') + .select('name') + .limit(1) + .offset(1) + .hasAncestor(ancestorKey); + assert.deepStrictEqual(entity.queryToQueryProto(query), queryProto); + }); + + it('should support the filter method with AND', () => { + const ancestorKey = new entity.Key({ + path: ['Kind2', 'somename'], + }); + + const ds = new Datastore({projectId: 'project-id'}); + + const query = ds + .createQuery('Kind1') + .filter( + and([ + new PropertyFilter('name', '=', 'John'), + new PropertyFilter('__key__', 'HAS_ANCESTOR', ancestorKey), + ]) + ) + .start('start') + .end('end') + .groupBy(['name']) + .order('name') + .select('name') + .limit(1) + .offset(1); + const testFilters = queryProto.filter; + const computedFilters = + entity.queryToQueryProto(query).filter.compositeFilter.filters[0]; + assert.deepStrictEqual(computedFilters, testFilters); + }); + it('should handle buffer start and end values', () => { const ds = new Datastore({projectId: 'project-id'}); const startVal = Buffer.from('start'); diff --git a/test/query.ts b/test/query.ts index 1f539b243..2836e596a 100644 --- a/test/query.ts +++ b/test/query.ts @@ -19,6 +19,7 @@ const {Query} = require('../src/query'); // eslint-disable-next-line @typescript-eslint/no-var-requires import {Datastore} from '../src'; import {AggregateField, AggregateQuery} from '../src/aggregate'; +import {PropertyFilter, EntityFilter, or} from '../src/filter'; describe('Query', () => { const SCOPE = {} as Datastore; @@ -163,6 +164,40 @@ describe('Query', () => { assert.strictEqual(filter.op, '='); assert.strictEqual(filter.val, 'Stephen'); }); + }); + + describe('filter with Filter class', () => { + it('should support filter with Filter', () => { + const now = new Date(); + const query = new Query(['kind1']).filter( + new PropertyFilter('date', '<=', now) + ); + const filter = query.entityFilters[0]; + + assert.strictEqual(filter.name, 'date'); + assert.strictEqual(filter.op, '<='); + assert.strictEqual(filter.val, now); + }); + it('should support filter with OR', () => { + const now = new Date(); + const query = new Query(['kind1']).filter( + or([ + new PropertyFilter('date', '<=', now), + new PropertyFilter('name', '=', 'Stephen'), + ]) + ); + const filter = query.entityFilters[0]; + assert.strictEqual(filter.op, 'OR'); + // Check filters + const filters = filter.filters; + assert.strictEqual(filters.length, 2); + assert.strictEqual(filters[0].name, 'date'); + assert.strictEqual(filters[0].op, '<='); + assert.strictEqual(filters[0].val, now); + assert.strictEqual(filters[1].name, 'name'); + assert.strictEqual(filters[1].op, '='); + assert.strictEqual(filters[1].val, 'Stephen'); + }); it('should accept null as value', () => { assert.strictEqual( new Query(['kind1']).filter('status', null).filters.pop()?.val,