-
Notifications
You must be signed in to change notification settings - Fork 102
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: introduce EntityFilter class with support for and/or filters #1061
Changes from 34 commits
d370b54
3291df8
69390d4
c90e932
c2b29fc
69f1cbf
3f720c0
20f616b
5dea24a
bb931e7
44be8a9
0055a09
b6bb3b3
83668a1
c4fe304
c397608
37400a6
8ab6b80
db02a50
bc15f32
e84582f
86841d6
fe50d56
1159b37
49ffcf9
3aa7a35
a7a8d27
1c77b6c
5f38179
059d4dc
ec752e8
a5c41ab
1ddeea6
8e96bf5
5883096
0a68ed1
35bae06
262894a
30c3832
fa2235d
4e7f9d5
c72fcc9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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,12 @@ 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__') { | ||
kolea2 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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', | ||
}, | ||
}; | ||
if (query.filters.length > 0 || query.newFilters.length > 0) { | ||
const filters = query.filters.map( | ||
filter => new PropertyFilter(filter.name, filter.op, filter.val) | ||
); | ||
const newFilters = query.newFilters; | ||
queryProto.filter = AND(newFilters.concat(filters)).toProto(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a little confused by this, what is this logic doing? Can you add some comments? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added comments to the code here. This is mainly just a refactor. Old code is being moved into the |
||
} | ||
|
||
return queryProto; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,159 @@ | ||
// 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'; | ||
|
||
enum CompositeOperator { | ||
AND = 'AND', | ||
OR = 'OR', | ||
} | ||
|
||
export function AND(filters: Filter[]): CompositeFilter { | ||
return new CompositeFilter(filters, CompositeOperator.AND); | ||
} | ||
|
||
export function OR(filters: Filter[]): 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 Filter { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe this should be an interface. |
||
/** | ||
* 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 Filter 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 OP_TO_OPERATOR = new Map([ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I would probably hoist this to the top of the file, rather than declaring in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good plan. |
||
['=', '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 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 Filter { | ||
filters: Filter[]; | ||
op: string; | ||
|
||
/** | ||
* Build a Composite Filter object. | ||
* | ||
* @param {Filter[]} filters | ||
*/ | ||
constructor(filters: Filter[], 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 Filter { | ||
return (filter as Filter).toProto !== undefined; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,10 +18,10 @@ import arrify = require('arrify'); | |
import {Key} from 'readline'; | ||
import {Datastore} from '.'; | ||
import {Entity} from './entity'; | ||
import {Filter as NewFilter, 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[]; | ||
newFilters: NewFilter[]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need another variable for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At first I put all the filters into the Currently, users may be popping off the |
||
orders: Order[]; | ||
groupByVal: Array<{}>; | ||
selectVal: Array<{}>; | ||
|
@@ -123,6 +124,11 @@ class Query { | |
* @type {array} | ||
*/ | ||
this.filters = []; | ||
/** | ||
* @name Query#newFilters | ||
* @type {array} | ||
*/ | ||
this.newFilters = []; | ||
/** | ||
* @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 | NewFilter} propertyOrFilter The field name. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't this be EntityFilter now? (side note - how is this compiling?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right. This needs to change. This is compiling because this is actually just a documentation comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
* @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 | NewFilter, value?: {} | null): Query; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why can't this just be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this module, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Filter has been renamed |
||
filter(propertyOrFilter: string, operator: Operator, value: {} | null): Query; | ||
filter( | ||
property: string, | ||
operatorOrValue: Operator, | ||
propertyOrFilter: string | NewFilter, | ||
operatorOrValue?: Operator, | ||
value?: {} | null | ||
): Query { | ||
let operator = operatorOrValue as Operator; | ||
if (arguments.length === 2) { | ||
value = operatorOrValue as {}; | ||
operator = '='; | ||
} | ||
if (isFilter(propertyOrFilter)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is good use of TypeScript's There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks |
||
this.newFilters.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; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
, orapplyAnd
,applyOr
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choosing the former