add: nested filter input-types #143

hedwiggggg · 2023-05-25T09:58:41Z

Hello, in this pull request I added the possibility to create filter types at any depth. So far I have only run the unit-tests.

So still missing:

run e2e-tests
add unit-tests
add e2e-tests
add documentation

Internal tests already ran successfully, but I noticed a bug with the query builder. In the generated joins the alias is set to the filter-property. So a hypothetical filter like this:

const articlesFilter: ArticleFilter = {
  group: {
    articles: {
      group: {
        name: {
          eq: 'tools',
        },
      },
    },
  },
};

Ends with the error: table name "group" specified more than once. Here is the log:

query failed: SELECT "Article"."id" AS "Article_id", "Article"."created_at" AS "Article_created_at", "Article"."updated_at" AS "Article_updated_at", "Article"."deleted_at" AS "Article_deleted_at", "Article"."status" AS "Article_status", "Article"."order_no" AS "Article_order_no", "Article"."name" AS "Article_name", "Article"."description" AS "Article_description", "Article"."condition" AS "Article_condition", "Article"."weight" AS "Article_weight", "Article"."unit" AS "Article_unit", "Article"."is_sellable" AS "Article_is_sellable", "Article"."is_storable" AS "Article_is_storable", "Article"."is_discountable" AS "Article_is_discountable", "Article"."group_id" AS "Article_group_id", "Article"."country_of_origin" AS "Article_country_of_origin", "Article"."customs_tariff_number" AS "Article_customs_tariff_number", "Article"."supplier_id" AS "Article_supplier_id", "Article"."supplier_order_number" AS "Article_supplier_order_number", "Article"."designation" AS "Article_designation" FROM "article" "Article" LEFT JOIN "article_group" "group" ON "group"."id"="Article"."group_id" AND ("group"."deleted_at" IS NULL)  LEFT JOIN "article" "articles" ON "articles"."group_id"="group"."id" AND ("articles"."deleted_at" IS NULL)  LEFT JOIN "article_group" "group" ON "group"."id"="articles"."group_id" AND ("group"."deleted_at" IS NULL) WHERE ( (((((("group"."name" = $1))))) AND 1=1 AND 1=1) ) AND ( "Article"."deleted_at" IS NULL ) ORDER BY "Article"."id" ASC LIMIT 26 -- PARAMETERS: ["Tools"]

But this is a problem for an own issue / pull request.

hedwiggggg · 2023-05-25T10:09:30Z

To configure the filter-depth for an entity, you pass the filterDepth property to the QueryOptions-decorator.

@ObjectType('TodoItem')
// `filterDepth` is a number, 1 is the default
@QueryOptions({ filterDepth: 3 })
// `filterDepth: Number.POSITIVE_INFINITY` will result in a infinitely deep filter
@QueryOptions({ filterDepth: Number.POSITIVE_INFINITY })
export class TodoItemDTO {
  @IDField(() => ID)
  id!: number;

  @FilterableField()
  title!: string;

  @FilterableField({ nullable: true })
  description?: string;

  @FilterableField()
  completed!: boolean;

  @FilterableField(() => GraphQLISODateTime)
  created!: Date;

  @FilterableField(() => GraphQLISODateTime)
  updated!: Date;
}

codecov-commenter · 2023-05-25T10:43:59Z

Codecov Report

Merging #143 (2444712) into master (11a647d) will increase coverage by 0.15%.
The diff coverage is 90.00%.

@@            Coverage Diff             @@
##           master     #143      +/-   ##
==========================================
+ Coverage   78.16%   78.31%   +0.15%     
==========================================
  Files         619      626       +7     
  Lines        8682     8771      +89     
  Branches      831      839       +8     
==========================================
+ Hits         6786     6869      +83     
+ Misses       1169     1167       -2     
- Partials      727      735       +8

Impacted Files	Coverage Δ
...query-graphql/src/decorators/relation.decorator.ts	`91.42% <ø> (ø)`
...kages/query-graphql/src/types/query/filter.type.ts	`77.92% <69.44%> (-7.57%)`	⬇️
examples/filters-deep/e2e/fixtures.ts	`100.00% <100.00%> (ø)`
examples/filters-deep/src/app.module.ts	`100.00% <100.00%> (ø)`
...mples/filters-deep/src/category/category.entity.ts	`100.00% <100.00%> (ø)`
examples/filters-deep/src/post/post.entity.ts	`100.00% <100.00%> (ø)`
examples/filters-deep/src/post/post.module.ts	`100.00% <100.00%> (ø)`
examples/filters-deep/src/user/user.entity.ts	`100.00% <100.00%> (ø)`
examples/filters-deep/src/user/user.module.ts	`100.00% <100.00%> (ø)`

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

TriPSs · 2023-05-25T10:55:54Z

Thanks for the PR! Could you show me a query filter example that before did work and does not now? I'm not quite sure I completely understand what this does.

hedwiggggg · 2023-05-25T11:00:11Z

I'm not quite sure I completely understand what this does.

This PR implements the feature to generate infinitely deep input-filter-types (doug-martin/nestjs-query#805 and #121).

Could you show me a query filter example that before did work and does not now?

No because the problem with the query-builder only occurs if the nesting is deeper than one level, which is impossible until now. But this PR changes that, that's why I stumbled upon this bug. But this PR is only for the infinitely deep filters.

hedwiggggg · 2023-05-25T11:01:29Z

This is how the generated schema will look like if the filterDepth is Number.POSITIVE_INFINITY.

hedwiggggg · 2023-05-25T11:03:25Z

This is how the generated schema will look like if the filterDepth is 2.

TriPSs · 2023-05-25T11:04:32Z

packages/query-graphql/src/types/query/filter.type.ts

+  return filterableRelations
+}
+
+function getOrCreateFilterType<T>(TClass: Class<T>, name: string, depth = 0): FilterConstructor<T> {


Since we call getOrCreateFilterType already with dept = 1 do we need the dept = 0 here?

Yes, because we call it with depth = 1 only on the (Delete / Update / Subscription / Aggregate)FilterType. The base FilterType is called without an argument.

What we can do is to remove the default value and move the depth = 0 into the base FilterType.

Okay check, maybe indeed call it there with dept = 0 so we don't need the default.

TriPSs · 2023-05-25T11:08:01Z

So if I understand correctly, default it create a AddressFilterBankFilter so you can filter the address on the bank relation, if Number.POSITIVE_INFINITY is used it will use the "normal" BankFilter with all the filters that Bank has. And if you provide filterDepth = 2 it will create a other one so you can filter address -> bank(1) -> other bank relation(2) correct?

hedwiggggg · 2023-05-25T11:08:38Z

So if I understand correctly, default it create a AddressFilterBankFilter so you can filter the address on the bank relation, if Number.POSITIVE_INFINITY is used it will use the "normal" BankFilter with all the filters that Bank has. And if you provide filterDepth = 2 it will create a other one so you can filter address -> bank(1) -> other bank relation(2) correct?

Exactly.

TriPSs · 2023-05-25T11:09:32Z

And a question: If we where to release this like this it will not change any schema except if you provide the filterDepth?

hedwiggggg · 2023-05-25T11:12:32Z

And a question: If we where to release this like this it will not change any schema except if you provide the filterDepth?

Nope, otherwise the test wouldn't have passed.

TriPSs · 2023-05-25T11:13:24Z

And a question: If we where to release this like this it will not change any schema except if you provide the filterDepth?

Nope, otherwise the test wouldn't have passed.

True, thought of that after I send it 😅

Everything looks ok, it's a nice addition this so thanks for that! If the other tasks are also completed we can merge and release!

hedwiggggg · 2023-05-26T08:32:10Z

Should be ready now @TriPSs 🚀

TriPSs · 2023-05-26T13:32:45Z

Thanks for the PR! Changes look good, I do want to check this out locally before merging if you don't mind.

TriPSs · 2023-05-29T15:02:13Z

@hedwiggggg after checking this locally I did notice one thing:

@FilterableRelation('relation', () => Relation, {
  // This one does not work
  filterDepth: 2,
})

Can we either remove it or add support for it? (I do think it's nice we can specific it for a specific relation instead of all relations)

hedwiggggg · 2023-05-29T16:32:02Z

Unfortunately, this can only be realized with breaking changes, since the target depth would have to be embedded in the filter name. Otherwise, it cannot be guaranteed which filter depth will be generated because the root filter names are the same for different depths. And configuring each relation individually, would possibly lead to different depths for the same filter type.

I have specially designed this feature so that there are no breaking changes.

hedwiggggg · 2023-05-29T16:45:14Z

tldr; we would need to generate n filter-types for the same entity where n is equal to the count of the unique {Entity, filterDepth} occurrences.

In the following example we would need 3 different filter types, but in the current implementation all filter types would have the same name, which is invalid in GraphQL.

// Example-A.entity.ts
@FilterableRelation('relation', () => Relation, { filterDepth: 2 })

// Example-B.entity.ts
@FilterableRelation('relation', () => Relation)

// Example-C.entity.ts
@FilterableRelation('relation', () => Relation, { filterDepth: 3 })

TriPSs · 2023-05-29T17:42:58Z

Understandable, could we then remove it from there so the option is not shown?

hedwiggggg · 2023-05-30T05:41:57Z

I thought about it again last night. There is a way to configure this per relation, without breaking-changes. By default the filter depth is 1. With this filter depth the generated name can also be ${ObjectTypeName}Filter by default. If the depth differs from the default filter depth, then we embed the target depth in the name.

// Example-A.entity.ts
@ObjectType('EntityA')
@FilterableRelation('relation', () => relation)
class EntityA { /* ... */ }

// leads to
// input EntityAFilter {
// ...
// }

// Example-A.entity.ts
@ObjectType('EntityA')
@FilterableRelation('relation', () => relation, { filterDepth: 2 )
class EntityA { /* ... */ }

// leads to
// input EntityADepth2Filter {
// ...
// }

But nevertheless I would take it out for now and put the feature-request into a separate issue, since I don't have the resources for it right now.

Edit: above assumption is complete bs, because the parentEntity-name is prefixed for n-depth filters. A better example would be:

// Example-A.entity.ts
@ObjectType('EntityA')
@FilterableRelation('relation', () => relation)
class EntityA { /* ... */ }

// leads to
// input EntityAFilter {
//    relation: EntityAFilterRelationFilter
// }

// Example-B.entity.ts
@ObjectType('EntityB')
@FilterableRelation('relation', () => relation, { filterDepth: 2 )
class EntityB { /* ... */ }

// leads to
// input EntityBFilter {
//   relation: EntityBFilterRelationFilter
// }

This means that we don't get name conflicts when we specify a fliterDepth per relationship. I will have a look at this in the future.

TriPSs · 2023-05-30T08:03:31Z

@hedwiggggg massive thanks for this addition!

Silas Rosenkranz added 2 commits May 25, 2023 09:34

add: nested filter input-types

211b50d

refactor: use number-type only

956c46d

fix: increment depth

bfc9d49

TriPSs reviewed May 25, 2023

View reviewed changes

Silas Rosenkranz added 9 commits May 25, 2023 11:14

refactor: make depth a required argument

5e0336c

fix: name clashing and filter depth overriding

827fe91

add unit-tests

1445a43

refactor: prefix instead suffix -> better semantic

edd9987

refactor: compromise between semantics and harmony

c613bd3

add: unit tests for suffix / cached types

8d7d52c

add: documentation

dfec958

fix broken links

d21d005

add: e2e-tests

d596a8d

hedwiggggg changed the title ~~add: nested filter input-types (work in progress)~~ add: nested filter input-types May 26, 2023

hedwiggggg mentioned this pull request May 26, 2023

fix(query-typeorm): name collision on deeply nested filters #144

Merged

fix: omit filterDepth from relation-decorators

2444712

TriPSs merged commit b6ff9eb into TriPSs:master May 30, 2023
10 checks passed

hedwiggggg mentioned this pull request May 30, 2023

Extend nested filter input-types to allow filter-depth configuration per relation. #145

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add: nested filter input-types #143

add: nested filter input-types #143

hedwiggggg commented May 25, 2023 •

edited

hedwiggggg commented May 25, 2023

codecov-commenter commented May 25, 2023 •

edited

TriPSs commented May 25, 2023

hedwiggggg commented May 25, 2023 •

edited

hedwiggggg commented May 25, 2023

hedwiggggg commented May 25, 2023

TriPSs May 25, 2023

hedwiggggg May 25, 2023

TriPSs May 25, 2023

TriPSs commented May 25, 2023

hedwiggggg commented May 25, 2023

TriPSs commented May 25, 2023

hedwiggggg commented May 25, 2023

TriPSs commented May 25, 2023

hedwiggggg commented May 26, 2023

TriPSs commented May 26, 2023

TriPSs commented May 29, 2023

hedwiggggg commented May 29, 2023

hedwiggggg commented May 29, 2023

TriPSs commented May 29, 2023

hedwiggggg commented May 30, 2023 •

edited

TriPSs commented May 30, 2023

add: nested filter input-types #143

add: nested filter input-types #143

Conversation

hedwiggggg commented May 25, 2023 • edited

hedwiggggg commented May 25, 2023

codecov-commenter commented May 25, 2023 • edited

Codecov Report

TriPSs commented May 25, 2023

hedwiggggg commented May 25, 2023 • edited

hedwiggggg commented May 25, 2023

hedwiggggg commented May 25, 2023

TriPSs May 25, 2023

Choose a reason for hiding this comment

hedwiggggg May 25, 2023

Choose a reason for hiding this comment

TriPSs May 25, 2023

Choose a reason for hiding this comment

TriPSs commented May 25, 2023

hedwiggggg commented May 25, 2023

TriPSs commented May 25, 2023

hedwiggggg commented May 25, 2023

TriPSs commented May 25, 2023

hedwiggggg commented May 26, 2023

TriPSs commented May 26, 2023

TriPSs commented May 29, 2023

hedwiggggg commented May 29, 2023

hedwiggggg commented May 29, 2023

TriPSs commented May 29, 2023

hedwiggggg commented May 30, 2023 • edited

TriPSs commented May 30, 2023

hedwiggggg commented May 25, 2023 •

edited

codecov-commenter commented May 25, 2023 •

edited

hedwiggggg commented May 25, 2023 •

edited

hedwiggggg commented May 30, 2023 •

edited