Skip to content
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.

Sign up for GitHub

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

Add filter on array and jsonb field types #7839

Merged
merged 9 commits into from
Oct 21, 2024
Merged

Add filter on array and jsonb field types #7839

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2879c0d
Add filter on array and jsonb field types
gitstart-twenty Oct 19, 2024
a7c8588
update value type on isMatchingArrayFilter
gitstart-twenty Oct 19, 2024
fe13470
address comments
Weiko Oct 21, 2024
0eb2fe1
Merge branch 'main' into TWNTY-6784
Weiko Oct 21, 2024
8c9b769
remove contains_any
Weiko Oct 21, 2024
da1d23f
add feature flag
Weiko Oct 21, 2024
6288888
move feature flag check
Weiko Oct 21, 2024
f901804
fix lint
Weiko Oct 21, 2024
585789e
fix tests
Weiko Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ...ty-front/src/modules/object-metadata/utils/formatFieldMetadataItemsAsFilterDefinitions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ export const formatFieldMetadataItemsAsFilterDefinitions = ({
FieldMetadataType.Rating,
FieldMetadataType.Actor,
FieldMetadataType.Phones,
FieldMetadataType.Array,
FieldMetadataType.RawJson,
].includes(field.type)
) {
return acc;
Expand Down Expand Up @@ -92,6 +94,8 @@ export const getFilterTypeFromFieldType = (fieldType: FieldMetadataType) => {
return 'ACTOR';
case FieldMetadataType.Array:
return 'ARRAY';
case FieldMetadataType.RawJson:
return 'RAW_JSON';
default:
return 'TEXT';
}
Expand Down
13 changes: 13 additions & 0 deletions packages/twenty-front/src/modules/object-record/graphql/types/RecordGqlOperationFilter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,17 @@ export type PhonesFilter = {
primaryPhoneCountryCode?: StringFilter;
};

export type ArrayFilter = {
contains?: string[];
contains_any?: string[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

not_contains?: string[];
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


export type RawJsonFilter = {
contains_filter?: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

is?: IsFilter;
};

export type LeafFilter =
| UUIDFilter
| StringFilter
Expand All @@ -117,6 +128,8 @@ export type LeafFilter =
| LinksFilter
| ActorFilter
| PhonesFilter
| ArrayFilter
| RawJsonFilter
| undefined;

export type AndObjectRecordFilter = {
Expand Down
1 change: 1 addition & 0 deletions ...dules/object-record/object-filter-dropdown/components/ObjectFilterDropdownFilterInput.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ export const ObjectFilterDropdownFilterInput = ({
'ADDRESS',
'ACTOR',
'ARRAY',
'RAW_JSON',
'PHONES',
].includes(filterDefinitionUsedInDropdown.type) &&
!isActorSourceCompositeFilter(filterDefinitionUsedInDropdown) && (
Expand Down
1 change: 1 addition & 0 deletions ...wenty-front/src/modules/object-record/object-filter-dropdown/types/FilterableFieldType.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,5 @@ export type FilterableFieldType = PickLiteral<
| 'MULTI_SELECT'
| 'ACTOR'
| 'ARRAY'
| 'RAW_JSON'
>;
9 changes: 8 additions & 1 deletion ...-front/src/modules/object-record/object-filter-dropdown/utils/getOperandsForFilterType.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ export const getOperandsForFilterDefinition = (
case 'FULL_NAME':
case 'ADDRESS':
case 'LINKS':
case 'ARRAY':
case 'PHONES':
return [
ViewFilterOperand.Contains,
Expand All @@ -32,6 +31,12 @@ export const getOperandsForFilterDefinition = (
ViewFilterOperand.LessThan,
...emptyOperands,
];
case 'RAW_JSON':
return [
ViewFilterOperand.Contains,
ViewFilterOperand.DoesNotContain,
...emptyOperands,
];
case 'DATE_TIME':
case 'DATE':
return [
Expand Down Expand Up @@ -70,6 +75,8 @@ export const getOperandsForFilterDefinition = (
...emptyOperands,
];
}
case 'ARRAY':
return [ViewFilterOperand.Contains, ViewFilterOperand.DoesNotContain];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@Weiko Weiko Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes let's add isEmpty here as greptile suggested 👍

default:
return [];
}
Comment on lines 84 to 86
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Expand Down
16 changes: 16 additions & 0 deletions packages/twenty-front/src/modules/object-record/record-filter/utils/applyEmptyFilters.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import {
ActorFilter,
AddressFilter,
ArrayFilter,
CurrencyFilter,
DateFilter,
EmailsFilter,
FloatFilter,
RawJsonFilter,
RecordGqlOperationFilter,
RelationFilter,
StringFilter,
Expand Down Expand Up @@ -290,6 +292,20 @@ export const applyEmptyFilters = (
],
};
break;
case 'ARRAY':
emptyRecordFilter = {
[correspondingField.name]: { not_contains: undefined } as ArrayFilter,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};
break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

case 'RAW_JSON':
emptyRecordFilter = {
or: [
{
[correspondingField.name]: { is: 'NULL' } as RawJsonFilter,
},
],
};
break;
Comment on lines +304 to +312
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: The RAW_JSON filter only checks for NULL. Consider adding a check for empty object {}

case 'EMAILS':
emptyRecordFilter = {
or: [
Expand Down
24 changes: 24 additions & 0 deletions packages/twenty-front/src/modules/object-record/record-filter/utils/isMatchingArrayFilter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { ArrayFilter } from '@/object-record/graphql/types/RecordGqlOperationFilter';

export const isMatchingArrayFilter = ({
arrayFilter,
value,
}: {
arrayFilter: ArrayFilter;
value: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: The value parameter is typed as string, but it should likely be an array for proper comparison with array fields.

}) => {
switch (true) {
case arrayFilter.contains !== undefined: {
return arrayFilter.contains.every((item) => value.includes(item));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: This implementation assumes value is a string and uses includes, which may not work correctly if value is actually an array. Consider using Array.isArray(value) && value.includes(item) for more robust checking.

}
case arrayFilter.not_contains !== undefined: {
return !arrayFilter.not_contains.every((item) => value.includes(item));
Weiko marked this conversation as resolved.
Show resolved Hide resolved
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

default: {
throw new Error(
`Unexpected value for array filter: ${JSON.stringify(arrayFilter)}`,
);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: The switch statement on true is unconventional and could be replaced with if-else statements for better readability.

};
32 changes: 32 additions & 0 deletions ...ges/twenty-front/src/modules/object-record/record-filter/utils/isMatchingRawJsonFilter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import { RawJsonFilter } from '../../graphql/types/RecordGqlOperationFilter';

export const isMatchingRawJsonFilter = ({
rawJsonFilter,
value,
}: {
rawJsonFilter: RawJsonFilter;
value: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: value is typed as string, but could be null

}) => {
switch (true) {
case rawJsonFilter.contains_filter !== undefined: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const regexPattern = rawJsonFilter.contains_filter.replace(/%/g, '.*');
const regexCaseInsensitive = new RegExp(`^${regexPattern}$`, 'i');

const stringValue = JSON.stringify(value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: JSON.stringify may fail if value is not JSON-serializable


return regexCaseInsensitive.test(stringValue);
}
case rawJsonFilter.is !== undefined: {
if (rawJsonFilter.is === 'NULL') {
return value === null;
} else {
return value !== null;
}
Comment on lines +20 to +24
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: consider using strict equality (===) for null checks

}
default: {
throw new Error(
`Unexpected value for string filter : ${JSON.stringify(rawJsonFilter)}`,
);
}
}
};
17 changes: 17 additions & 0 deletions ...ages/twenty-front/src/modules/object-record/record-filter/utils/isRecordMatchingFilter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
ActorFilter,
AddressFilter,
AndObjectRecordFilter,
ArrayFilter,
BooleanFilter,
CurrencyFilter,
DateFilter,
Expand All @@ -16,14 +17,17 @@ import {
NotObjectRecordFilter,
OrObjectRecordFilter,
PhonesFilter,
RawJsonFilter,
RecordGqlOperationFilter,
StringFilter,
UUIDFilter,
} from '@/object-record/graphql/types/RecordGqlOperationFilter';
import { isMatchingArrayFilter } from '@/object-record/record-filter/utils/isMatchingArrayFilter';
import { isMatchingBooleanFilter } from '@/object-record/record-filter/utils/isMatchingBooleanFilter';
import { isMatchingCurrencyFilter } from '@/object-record/record-filter/utils/isMatchingCurrencyFilter';
import { isMatchingDateFilter } from '@/object-record/record-filter/utils/isMatchingDateFilter';
import { isMatchingFloatFilter } from '@/object-record/record-filter/utils/isMatchingFloatFilter';
import { isMatchingRawJsonFilter } from '@/object-record/record-filter/utils/isMatchingRawJsonFilter';
import { isMatchingStringFilter } from '@/object-record/record-filter/utils/isMatchingStringFilter';
import { isMatchingUUIDFilter } from '@/object-record/record-filter/utils/isMatchingUUIDFilter';
import { FieldMetadataType } from '~/generated-metadata/graphql';
Expand Down Expand Up @@ -165,6 +169,18 @@ export const isRecordMatchingFilter = ({
value: record[filterKey],
});
}
case FieldMetadataType.Array: {
return isMatchingArrayFilter({
arrayFilter: filterValue as ArrayFilter,
value: record[filterKey],
});
}
case FieldMetadataType.RawJson: {
return isMatchingRawJsonFilter({
rawJsonFilter: filterValue as RawJsonFilter,
value: record[filterKey],
});
}
case FieldMetadataType.FullName: {
const fullNameFilter = filterValue as FullNameFilter;

Expand Down Expand Up @@ -302,6 +318,7 @@ export const isRecordMatchingFilter = ({
`Not implemented yet, use UUID filter instead on the corredponding "${filterKey}Id" field`,
);
}

default: {
throw new Error(
`Not implemented yet for field type "${objectMetadataField.type}"`,
Expand Down
60 changes: 60 additions & 0 deletions .../src/modules/object-record/record-filter/utils/turnObjectDropdownFilterIntoQueryFilter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@ import { isNonEmptyString } from '@sniptt/guards';
import {
ActorFilter,
AddressFilter,
ArrayFilter,
CurrencyFilter,
DateFilter,
EmailsFilter,
FloatFilter,
RawJsonFilter,
RecordGqlOperationFilter,
RelationFilter,
StringFilter,
Expand Down Expand Up @@ -98,6 +100,39 @@ export const turnObjectDropdownFilterIntoQueryFilter = (
);
}
break;
case 'RAW_JSON':
switch (rawUIFilter.operand) {
case ViewFilterOperand.Contains:
objectRecordFilters.push({
[correspondingField.name]: {
contains_filter: `%${rawUIFilter.value}%`,
} as RawJsonFilter,
});
break;
case ViewFilterOperand.DoesNotContain:
objectRecordFilters.push({
not: {
[correspondingField.name]: {
contains_filter: `%${rawUIFilter.value}%`,
} as RawJsonFilter,
},
});
break;
case ViewFilterOperand.IsEmpty:
case ViewFilterOperand.IsNotEmpty:
applyEmptyFilters(
rawUIFilter.operand,
correspondingField,
objectRecordFilters,
rawUIFilter.definition,
);
break;
default:
throw new Error(
`Unknown operand ${rawUIFilter.operand} for ${rawUIFilter.definition.type} filter`,
);
}
break;
case 'DATE':
case 'DATE_TIME': {
const resolvedFilterValue = resolveFilterValue(rawUIFilter);
Expand Down Expand Up @@ -835,6 +870,31 @@ export const turnObjectDropdownFilterIntoQueryFilter = (
}
break;
}
case 'ARRAY': {
switch (rawUIFilter.operand) {
case ViewFilterOperand.Contains: {
objectRecordFilters.push({
[correspondingField.name]: {
contains: [`${rawUIFilter.value}`],
} as ArrayFilter,
});
Weiko marked this conversation as resolved.
Show resolved Hide resolved
break;
}
case ViewFilterOperand.DoesNotContain: {
objectRecordFilters.push({
[correspondingField.name]: {
not_contains: [`${rawUIFilter.value}`],
} as ArrayFilter,
});
break;
}
Weiko marked this conversation as resolved.
Show resolved Hide resolved
default:
throw new Error(
`Unknown operand ${rawUIFilter.operand} for ${rawUIFilter.definition.label} filter`,
);
}
break;
}
default:
throw new Error('Unknown filter type');
}
Expand Down
7 changes: 6 additions & 1 deletion ...ry-runner/graphql-query-parsers/graphql-query-filter/graphql-query-filter-field.parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,12 @@ export class GraphqlQueryFilterFieldParser {
}
const [[operator, value]] = Object.entries(filterValue);

if (operator === 'in') {
if (
operator === 'in' ||
operator === 'contains' ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

operator === 'contains_any' ||
operator === 'not_contains'
) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Consider using an array or Set for better readability and maintainability

if (!Array.isArray(value) || value.length === 0) {
throw new GraphqlQueryRunnerException(
`Invalid filter value for field ${key}. Expected non-empty array`,
Expand Down
23 changes: 23 additions & 0 deletions ...server/src/engine/api/graphql/graphql-query-runner/utils/compute-where-condition-parts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,29 @@ export const computeWhereConditionParts = (
sql: `"${objectNameSingular}"."${key}" LIKE :${key}${uuid}`,
params: { [`${key}${uuid}`]: `${value}` },
};
case 'contains':
return {
sql: `"${objectNameSingular}"."${key}" @> ARRAY[:...${key}${uuid}]`,
params: { [`${key}${uuid}`]: value },
};
Comment on lines +82 to +86
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return {
sql: `"${objectNameSingular}"."${key}"::text LIKE :${key}${uuid}`,
params: { [`${key}${uuid}`]: `%${value}%` },
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


default:
throw new GraphqlQueryRunnerException(
`Operator "${operator}" is not supported`,
Expand Down
3 changes: 2 additions & 1 deletion ...ne/api/graphql/workspace-schema-builder/graphql-types/input/raw-json-filter.input-type.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import { GraphQLInputObjectType } from 'graphql';
import { GraphQLInputObjectType, GraphQLString } from 'graphql';

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 },
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});
Loading