Add filter on array and jsonb field types #7839

gitstart-app · 2024-10-19T00:16:21Z

This PR was created by GitStart to address the requirements from this ticket: TWNTY-6784.
This ticket was imported from: TWNTY-6784

Description

Add filter on array and jsonb field types
We did not implement the contains any filter for arrays on the frontend because we would need to change the UI design since this should be an array of values, and now we have only one input

Demo

https://www.loom.com/share/0facf752b63f4120b5d4ea4ee9772d35?sid=d7bde469-e6a9-4298-a637-d81d40695a86

Fixes #6784

greptile-apps

PR Summary

This pull request adds filtering capabilities for array and jsonb field types, addressing the requirements outlined in issue #6784. Key changes include:

Added support for FieldMetadataType.Array and FieldMetadataType.RawJson in filter definitions
Introduced new ArrayFilter and RawJsonFilter types in GraphQL operations
Implemented isMatchingArrayFilter and isMatchingRawJsonFilter utility functions
Updated GraphqlQueryFilterFieldParser and computeWhereConditionParts to handle new array and jsonb operators
Modified UI components to support filtering on these new field types

The implementation appears consistent with existing patterns and integrates well with the current filtering system. However, there are a few points to consider:

The frontend implementation for "contains any" filter for arrays is not included due to UI design constraints
Some error handling and edge cases may need further review
Performance implications of new filtering operations, especially on large datasets, should be evaluated
Additional testing may be required to ensure robustness of the new filtering capabilities

_{13 file(s) reviewed, 26 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2024-10-19T00:17:06Z

packages/twenty-front/src/modules/object-record/graphql/types/RecordGqlOperationFilter.ts

+export type ArrayFilter = {
+  contains?: string[];
+  contains_any?: string[];
+  not_contains?: string[];
+};


style: Consider adding a type for the array elements instead of using string[] for all cases. This would provide better type safety for different array types.

greptile-apps · 2024-10-19T00:17:07Z

packages/twenty-front/src/modules/object-record/graphql/types/RecordGqlOperationFilter.ts

+};
+
+export type RawJsonFilter = {
+  contains_filter?: string;


style: The contains_filter property uses a string type. Consider using a more specific type or adding a comment explaining the expected format of this filter.

greptile-apps · 2024-10-19T00:18:01Z

...nty-front/src/modules/object-record/object-filter-dropdown/utils/getOperandsForFilterType.ts

+    case 'ARRAY':
+      return [ViewFilterOperand.Contains, ViewFilterOperand.DoesNotContain];


style: ARRAY type doesn't include empty operands. Consider adding them for consistency with other types

Yes let's add isEmpty here as greptile suggested 👍

greptile-apps · 2024-10-19T00:18:02Z

...nty-front/src/modules/object-record/object-filter-dropdown/utils/getOperandsForFilterType.ts

    default:
      return [];
  }


style: Default case returns an empty array. Consider throwing an error or logging a warning for unexpected filter types

greptile-apps · 2024-10-19T00:18:23Z

packages/twenty-front/src/modules/object-record/record-filter/utils/applyEmptyFilters.ts

+    case 'ARRAY':
+      emptyRecordFilter = {
+        [correspondingField.name]: { not_contains: undefined } as ArrayFilter,
+      };
+      break;


logic: The 'not_contains: undefined' filter for ARRAY might not work as expected. Consider using an empty array instead

greptile-apps · 2024-10-19T00:20:39Z

...ty-server/src/engine/api/graphql/graphql-query-runner/utils/compute-where-condition-parts.ts

+    case 'contains':
+      return {
+        sql: `"${objectNameSingular}"."${key}" @> ARRAY[:...${key}${uuid}]`,
+        params: { [`${key}${uuid}`]: value },
+      };


style: The 'contains' operator for arrays uses the @> operator, which checks if the array column contains all elements of the input array. This is correct, but consider adding a comment explaining the behavior for clarity.

greptile-apps · 2024-10-19T00:20:40Z

...ty-server/src/engine/api/graphql/graphql-query-runner/utils/compute-where-condition-parts.ts

+    case 'contains_any':
+      return {
+        sql: `"${objectNameSingular}"."${key}" && ARRAY[:...${key}${uuid}]`,
+        params: { [`${key}${uuid}`]: value },
+      };


style: The 'contains_any' operator uses the && operator, which checks for any overlap between arrays. This is correct, but a comment explaining the behavior would be helpful.

greptile-apps · 2024-10-19T00:20:40Z

...ty-server/src/engine/api/graphql/graphql-query-runner/utils/compute-where-condition-parts.ts

+    case 'contains_filter':
+      return {
+        sql: `"${objectNameSingular}"."${key}"::text LIKE :${key}${uuid}`,
+        params: { [`${key}${uuid}`]: `%${value}%` },
+      };


style: The 'contains_filter' operator casts the column to text and uses LIKE. This works for jsonb, but might not be efficient for large objects. Consider using jsonb-specific operators for better performance.

greptile-apps · 2024-10-19T00:20:57Z

...ngine/api/graphql/workspace-schema-builder/graphql-types/input/raw-json-filter.input-type.ts


 import { FilterIs } from 'src/engine/api/graphql/workspace-schema-builder/graphql-types/input/filter-is.input-type';

 export const RawJsonFilterType = new GraphQLInputObjectType({
  name: 'RawJsonFilter',
  fields: {
    is: { type: FilterIs },
+    contains_filter: { type: GraphQLString },


logic: Ensure that the 'contains_filter' implementation in the backend correctly handles JSON string matching.

greptile-apps · 2024-10-19T00:20:58Z

...ngine/api/graphql/workspace-schema-builder/graphql-types/input/raw-json-filter.input-type.ts

 export const RawJsonFilterType = new GraphQLInputObjectType({
  name: 'RawJsonFilter',
  fields: {
    is: { type: FilterIs },
+    contains_filter: { type: GraphQLString },
  },
 });


style: Consider adding a description field to the GraphQLInputObjectType for better documentation.

Weiko

This was not mentioned in the initial issue but I would have added a featureFlag on the frontend. I left a few comments otherwise the rest LGTM

Weiko · 2024-10-21T12:11:00Z

packages/twenty-front/src/modules/object-record/graphql/types/RecordGqlOperationFilter.ts

@@ -104,6 +104,17 @@ export type PhonesFilter = {
  primaryPhoneCountryCode?: StringFilter;
 };

+export type ArrayFilter = {
+  contains?: string[];
+  contains_any?: string[];


I wouldn't put contains_any in this PR if not fully implemented.

We will actually re-implement it later when we will have advanced filter (using OR)

Weiko · 2024-10-21T12:12:09Z

packages/twenty-front/src/modules/object-record/graphql/types/RecordGqlOperationFilter.ts

+};
+
+export type RawJsonFilter = {
+  contains_filter?: string;


filter is redundant here, let's use "contains" as it is the name for other filters

Weiko · 2024-10-21T12:21:20Z

...nty-front/src/modules/object-record/object-filter-dropdown/utils/getOperandsForFilterType.ts

+    case 'ARRAY':
+      return [ViewFilterOperand.Contains, ViewFilterOperand.DoesNotContain];


Yes let's add isEmpty here as greptile suggested 👍

Weiko · 2024-10-21T12:48:27Z

packages/twenty-front/src/modules/object-record/record-filter/utils/applyEmptyFilters.ts

@@ -290,6 +292,20 @@ export const applyEmptyFilters = (
        ],
      };
      break;
+    case 'ARRAY':
+      emptyRecordFilter = {
+        [correspondingField.name]: { not_contains: undefined } as ArrayFilter,


applyEmptyFilter won't be called for array types, missing in turnObjectDropdownFilterIntoQueryFilter

Weiko · 2024-10-21T12:50:52Z

packages/twenty-front/src/modules/object-record/record-filter/utils/applyEmptyFilters.ts

@@ -290,6 +292,20 @@ export const applyEmptyFilters = (
        ],
      };
      break;
+    case 'ARRAY':
+      emptyRecordFilter = {
+        [correspondingField.name]: { not_contains: undefined } as ArrayFilter,


I don't think 'not_contains: undefined' will work as intended 🤔 Have you tested this use case?

Weiko · 2024-10-21T13:04:24Z

...query-runner/graphql-query-parsers/graphql-query-filter/graphql-query-filter-field.parser.ts

-    if (operator === 'in') {
+    if (
+      operator === 'in' ||
+      operator === 'contains' ||


I think we will have some issues in the future with this name. Contains is actually a bit "vague" and could be mistaken for "like" in StringsFilter 🤔 (the FE actually displays "Contains" in both cases)

cc @charlesBochet operators are not mapped to particular types and can be mistaken. We could add "array" in the operator here for example, just so you know.

Weiko · 2024-10-21T13:13:53Z

packages/twenty-front/src/modules/object-record/record-filter/utils/isMatchingArrayFilter.ts

+    case arrayFilter.not_contains !== undefined: {
+      return !arrayFilter.not_contains.every((item) => value.includes(item));
+    }
+


Let's add "IS" filter for array type to be able to filter with NULL, NOT_NULL in those columns.

Weiko · 2024-10-21T13:31:18Z

packages/twenty-front/src/modules/object-record/record-filter/utils/isMatchingRawJsonFilter.ts

+  value: string;
+}) => {
+  switch (true) {
+    case rawJsonFilter.contains_filter !== undefined: {


To my point above, let's use the term "like" like the string type here since that's actually what happens behind the scene

Weiko · 2024-10-21T13:34:11Z

...ty-server/src/engine/api/graphql/graphql-query-runner/utils/compute-where-condition-parts.ts

+        sql: `NOT ("${objectNameSingular}"."${key}" && ARRAY[:...${key}${uuid}])`,
+        params: { [`${key}${uuid}`]: value },
+      };
+    case 'contains_filter':


As mentioned above, let's not introduce a new operator and reuse "like" for JSON. We can imagine better operators later when we will actually leverage jsonb operators but right now we are simply doing a LIKE operation.

Add filter on array and jsonb field types

2879c0d

greptile-apps bot reviewed Oct 19, 2024

View reviewed changes

update value type on isMatchingArrayFilter

a7c8588

gitstart-twenty added the -PR: awaiting review label Oct 19, 2024

gitstart-twenty requested a review from ijreilly October 19, 2024 00:44

ijreilly requested review from Weiko and removed request for ijreilly October 21, 2024 08:04

charlesBochet force-pushed the main branch from 1c484d5 to 2f72e22 Compare October 21, 2024 11:19

Weiko reviewed Oct 21, 2024

View reviewed changes

Weiko added 7 commits October 21, 2024 15:42

address comments

fe13470

Merge branch 'main' into TWNTY-6784

0eb2fe1

remove contains_any

8c9b769

add feature flag

da1d23f

move feature flag check

6288888

fix lint

f901804

fix tests

585789e

Weiko approved these changes Oct 21, 2024

View reviewed changes

Weiko merged commit 7b10bfa into main Oct 21, 2024
18 checks passed

Weiko deleted the TWNTY-6784 branch October 21, 2024 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add filter on array and jsonb field types #7839

Add filter on array and jsonb field types #7839

gitstart-app bot commented Oct 19, 2024

greptile-apps bot left a comment

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

Weiko Oct 21, 2024 •

edited

Loading

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

Weiko left a comment

Weiko Oct 21, 2024

Weiko Oct 21, 2024

Weiko Oct 21, 2024

Weiko Oct 21, 2024 •

edited

Loading

Weiko Oct 21, 2024

Weiko Oct 21, 2024

Weiko Oct 21, 2024

Weiko Oct 21, 2024

Weiko Oct 21, 2024

Weiko Oct 21, 2024

Weiko Oct 21, 2024

		case 'ARRAY':
		return [ViewFilterOperand.Contains, ViewFilterOperand.DoesNotContain];

Add filter on array and jsonb field types #7839

Add filter on array and jsonb field types #7839

Conversation

gitstart-app bot commented Oct 19, 2024

Description

Demo

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

Weiko Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

Weiko 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

Weiko Oct 21, 2024 • edited Loading

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

Choose a reason for hiding this comment

Weiko Oct 21, 2024 •

edited

Loading

Weiko Oct 21, 2024 •

edited

Loading