-
Notifications
You must be signed in to change notification settings - Fork 142
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
Custom filter support for typeorm and graphql #1397
base: master
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 1326807712
💛 - Coveralls |
@luca-nardelli thank you for looking into this! When I have investigated this before my hangup (I may be complicating this more than it needs to be) is approaching it generically. More specifically allowing custom operators across types. Putting myself in an end-users shoe's, I would want to specify a generic operation (e.g. I'd be interested in your thoughts/approach in making this more generic to allow specifying an operation instead of a (field, operation) tuple. Great job on the PR! |
I agree that defining common custom filters per (type, operation) rather than (field, operation) makes much more sense for standard scenarios (e.g. the distanceFrom case or getting multiples of a number). Simple example: A user has N projects, and each project has N tasks that can either be Harder example: A user belongs to N groups, projects belong to 1 group, groups can be nested inside of each other (parent-child), where users at the parent level can see the projects of all child groups, and where the task is to filter all projects based on the requesting user. if you store the parent-child relationship with a materialized path approach using e.g. JSON arrays, given a parent group ID you can get all its child groups, and then you can use that to filter projects. In order to implement this at the project level, I would probably define a custom filter Technically though, you could argue that these filtering operations could be defined on the relation type itself 😆 All this to say that I'd still like the flexibility of manually defining filters with the (Entity, field, operation) approach (also, maybe I'd want to expose a distanceFrom filter only on one field of one entity, for example), but having to do that manually for all fields can become quite a bother for situations where you have many entities. I'll try tinkering with the code to see what's possible! |
Idea: What do you say about explicitly defining filter classes instead of generating them? It's definitely a bit more boilerplate, but it could allow the right grade of flexibility, and potentially it could also give a bit more control to end users (maybe, if you are fetching books, you only want to filter on autor names and not author birth dates, whereas if you fetch authors directly you want to filter on both) I made a POC some time ago (as a learning project) that was something like this:
I would still have a filter registry, but registration only linked the filter class with the filter handler.
Processing of the filter object then was similar to what I did now, with the difference that I was using the filter type to invoke the right handlers. I think this could allow both generic type filters, that are associated to the type of the field, but also highly specific filters, provided that you registered them only on the right class. The drawback, however, is that the filter class needs to be manually defined, but since it's basically just fields and annotations maybe it's acceptable? |
cf18c16
to
c20dfbb
Compare
Hey @doug-martin , sorry for the radio silence but I've been swamped by things and while I was able to think about this in the past weeks, I just found some time to go over the code. Let's ignore for now my previous comment (about defining the filter classes) as it is mostly unrelated. I gave some more thoughts on the Nothing changed regarding how I apply filters in the where builder, but now filters can be registered in 2 ways: global (i.e. Since we could potentially have a conflict between an Example from the updated tests: // Test for (operation) filter registration (this is valid for all fields of all entities)
customFilterRegistry.setFilter('isMultipleOf', {
apply(field, cmp, val: number, alias): CustomFilterResult {
alias = alias ? alias : '';
const pname = `param${randomString()}`;
return {
sql: `("${alias}"."${field}" % :${pname}) == 0`,
params: { [pname]: val },
};
},
});
// Test for (class, field, operation) filter overriding the previous operation filter on a specific field
customFilterRegistry.setFilter<TestEntity>(
'isMultipleOf',
{
apply(field, cmp, val: number, alias): CustomFilterResult {
alias = alias ? alias : '';
const pname = `param${randomString()}`;
return {
sql: `(EXTRACT(EPOCH FROM "${alias}"."${field}") / 3600 / 24) % :${pname}) == 0`,
params: { [pname]: val },
};
},
},
{ klass: TestEntity, field: 'dateType' },
);
// Test for (class, field, operation) filter on a virtual property 'fakePointType' that does not really exist on the entity
customFilterRegistry.setFilter<TestEntity>(
'distanceFrom',
{
apply(field, cmp, val: { point: { lat: number; lng: number }; radius: number }, alias): CustomFilterResult {
alias = alias ? alias : '';
const plat = `param${randomString()}`;
const plng = `param${randomString()}`;
const prad = `param${randomString()}`;
return {
sql: `ST_Distance("${alias}"."${field}", ST_MakePoint(:${plat},:${plng})) <= :${prad}`,
params: { [plat]: val.point.lat, [plng]: val.point.lng, [prad]: val.radius },
};
},
},
{ klass: TestEntity, field: 'fakePointType' },
); it('should accept custom filters alongside regular filters', (): void => {
expectSQLSnapshot({
// This has the global isMultipleOf filter
numberType: { gte: 1, lte: 10, isMultipleOf: 5 },
// Here, the isMultipleOf filter was overridden for dateType only
dateType: { isMultipleOf: 3 },
// This is a more complex filter involving geospatial queries
fakePointType: { distanceFrom: { point: { lat: 45.3, lng: 9.5 }, radius: 50000 } },
} as any);
}); This setup could potentially allow us to define new filter types with intermediate specificity (e.g. a filter that works on Let me know your thoughts! |
c20dfbb
to
b1f7566
Compare
Hey, I was able to put some further work on this and I started getting close to an external API that other devs could use in their application code. Example: @Entity()
// This binds the filter to ALL fields of the entity
@TypeormQueryFilter<TestEntity>({
filter: IsMultipleOfCustomFilter,
})
// This binds the filter to only the specific fields mentioned in the fields array
@TypeormQueryFilter<TestEntity>({
filter: IsMultipleOfDateCustomFilter,
fields: ['dateType'],
})
export class TestEntity {
@PrimaryColumn({ name: 'test_entity_pk' })
testEntityPk!: string;
} These decorators only set metadata. During NesjJsQueryTypeormModule This is how a custom filter looks like: @Injectable()
export class IsMultipleOfCustomFilter implements CustomFilter<IsMultipleOfOpType> {
readonly operations: string[] = ['isMultipleOf'];
apply(field: string, cmp: string, val: unknown, alias?: string): CustomFilterResult {
alias = alias ? alias : '';
const pname = `param${randomString()}`;
return {
sql: `(${alias}.${field} % :${pname}) == 0`,
params: { [pname]: val },
};
}
} I added further tests at the filter-query-builder level and at the typeorm-query-service level (had to force the timezone of the tests to UTC otherwise some tests would fail) and everything seems to check out so far. Note that the decorators are applied at the entity level, and not at the DTO level. This is because we are still not in GraphQL-land here, but we are in Typeorm-land, that's why I wend this route. I think I'm going in the right direction and I like this decorator based API, but I'd love to hear your thoughts on this! Next steps:
|
4e046ce
to
45aff05
Compare
45aff05
to
5929f56
Compare
- Force UTC timezone to make some tests (e.g. typeorm-query-service.spec) deterministic - Allow to define and register custom filters on types and entities (virtual fields as well) at the Typeorm (persistence) layer - Allow extending filters on the built-in graphql types - Implement custom filters on custom graphql scalars - Implement allowedComparisons for extended filters and for custom scalar defined graphql filters - Implement custom graphql filters on virtual properties - Documentation - Tests
5929f56
to
8733a1d
Compare
Hey @doug-martin , I got sidetracked and didn't have time to look into this until recently. Basically the process happens in 2 steps:
This is similar to how things like enums are registered. As for how the external API looks like, you can have a quick look at the 2 documentation pages I wrote:
I believe we are getting close to your initial idea of "allowing custom operators across types.", but definitely let me know your thoughts on this! I could probably also use some help in naming the public API interfaces/entities, as right now I didn't spend too much time thinking about naming. You can see them in use in the doc pages or in the https://github.com/luca-nardelli/nestjs-query/tree/8733a1d8b046e61d2912cff515dfe66a9dc62c81/examples/typeorm example |
Just wanted to mention that there is an actively developed fork: https://github.com/tripss/nestjs-query Maybe you can create this pull request there? @luca-nardelli Because @doug-martin probably abandoned this project (see #1538) |
Hey! In the past few days I spent some time thinking about a general solution for having custom user-defined filters ( related issues: #702 and #720 ), for now only at the typeorm level.
I'm opening this draft PR to ask for feedback on my solution. If this can work for you, I can probably keep working on this and transform it in a real PR.
Problem: It is currently not possible to define user-defined filters on entities. This means that, for example, if we're using postgis and our entity has a Point field (e.g. GPS coordinates) it's impossible to create a filter that returns all entities that are closer than X to a given point (radius search), since the only allowed operations are the ones defined by the sqlComparisonBuilder.
Assuming our entity has a
location
field which represents its position, I'd like to be able to filter on it with something like{location: {distanceFrom: {point: {lat: ..., lng: ...}, radius: ....}}}
.Proposed solution:
Extend the filtering mechanism to allow custom user defined filters that can be applied during the transformation of the
Query
object into the actual typeorm query.A custom filter is basically the following interface:
(note that CustomFilterResult is the same as CmpSQLType https://github.com/luca-nardelli/nestjs-query/blob/custom-filters/packages/query-typeorm/src/query/sql-comparison.builder.ts#L9)
Each custom filter can be registered for a specific entity class, field and operation name, and is then invoked by the where builder when it's processing the filter object. Here, "operation name" basically means the individual operation of the filter (same as
cmpType
in the where builder)In the above example,
numberType
is the field, andgt
is the operation name. Filter registration happens through a newCustomFilterRegistry
class that basically holds all custom filters for all entities in a Map. Storing them flat allows to easily fetch all custom filters when traversing relations in the where builder.I started writing some code with the above idea in mind and I've managed to cobble together something that allowed me to write and pass some tests ( added here https://github.com/luca-nardelli/nestjs-query/blob/custom-filters/packages/query-typeorm/__tests__/query/where.builder.spec.ts).
More specifically, this is how it looks like at the code level for 2 different custom filters: an extension for numeric types where I want to get values that are multiples of a user provided value, and an example with a PostGIS query.
The custom filter registry here is a standard class and I am registering filters manually, but at a higher level we could register custom filters using decorators + DI. Apart from this, the most notable changes I had to make so far are the following:
At the wherebuilder level, the main change happened in the
withFilterComparison
, where now I try to call a custom filter and I use the sqlcomparisonbuilder only as a fallback.There are obviously a lot of things to do here (as an example, there's some type fixing that needs to happen because right now we are assuming that the only possible values in the
Filter*
types are the common ones), but for now could you @doug-martin (or someone else) have a look and tell me if you think this is an acceptable approach?Thanks!