feat: filter changes #1422
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,23 +1,26 @@ | ||
| import { Dictionary } from '@hypertrace/common'; | ||
| import { FilterAttribute } from './filter-attribute'; | ||
| import { FilterOperator, incompatibleOperators } from './filter-operators'; | ||
|
|
||
| export interface Filter<TValue extends FilterValue = FilterValue> extends IncompleteFilter { | ||
| operator: FilterOperator; | ||
| value: TValue; | ||
| urlString: string; | ||
| } | ||
|
|
||
| export interface IncompleteFilter<TValue extends FilterValue = FilterValue> extends FieldFilter<TValue> { | ||
| metadata: FilterAttribute; | ||
| userString: string; | ||
| } | ||
|
|
||
| export interface FieldFilter<TValue extends FilterValue = FilterValue> { | ||
| field: string; | ||
| subpath?: string; | ||
| operator?: FilterOperator; | ||
| value?: TValue; | ||
| } | ||
|
|
||
| export type FilterValue = string | number | boolean | Date | Dictionary<FilterValue> | FilterValue[]; | ||
|
|
||
| export const areCompatibleFilters = (f1: Filter, f2: Filter) => | ||
| f1.field !== f2.field || f1.subpath !== f2.subpath || !incompatibleOperators(f1.operator).includes(f2.operator); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,36 @@ | ||
| import { FieldFilter, FilterValue } from './../../../../../components/src/filtering/filter/filter'; | ||
| import { TableFilter } from './../../../../../components/src/table/table-api'; | ||
| import { Injectable } from '@angular/core'; | ||
| import { assertUnreachable } from '@hypertrace/common'; | ||
| import { FilterOperator } from '@hypertrace/components'; | ||
| import { GraphQlArgumentValue } from '@hypertrace/graphql-client'; | ||
| import { GraphQlFieldFilter } from '../../graphql/model/schema/filter/field/graphql-field-filter'; | ||
| import { GraphQlFilter, GraphQlOperatorType } from '../../graphql/model/schema/filter/graphql-filter'; | ||
|
|
||
| @Injectable({ providedIn: 'root' }) | ||
| export class GraphQlFilterBuilderService { | ||
| public buildFiltersFromGraphQlFieldFilters(filters: GraphQlFieldFilter[]): FieldFilter[] { | ||
|
Contributor
There was a problem hiding this comment. Why do we want to convert back from GQL filters to field filters? If we're doing refactoring, we should change any components using GQL filters to use the UI construct instead of supporting going bidrectionally.
Contributor
Author
There was a problem hiding this comment. Yeah, so that would be a big change. If we want to do it in iterations then temporarily this method will be used in the basic filter bar for now. Slowly, we will migrate the filters to the UI construct. We would also need to revisit the relationship between
Contributor
There was a problem hiding this comment. So I admittedly have limited context here, but rather than updating the existing basic filter bar component in place, it seems like creating a new one and deprecating the old might be cleaner for a number of reasons - this included. We would save ourselves from introducing new tech debt, as well as bending over backwards to support compatibility with existing usages like the preferences stuff you brought up the other day.
Oh, please do! The concept of IncompleteFilter should be internal to any free-text filter input. I'm not sure how I'd design the other interfaces, I think I would probably start the exercise with just It all depends on how deep you want to go with this, but if you're going to work with the generic stuff, we should try to get it right rather than keep digging this hole (and if doing this, I would strongly suggest doing it parallel to the existing filters, as mentioned before, rather than in place). If we don't want to take that up now, should create use-case specific components.
Contributor
There was a problem hiding this comment. Disclaimer: I haven't gone through the code yet and have only read the comments. But what I'm gathering from the comments is that we are trying to decide on meshing the new filtering system with the old one vs just making a clean break and building a new one. I think we all agree deprecating the old one is the right thing to do long term. Lots of history of patching up the old one for new use cases. We know a lot more about all of our use cases and we can code this much better to accommodate the full picture now. That said, it's always a balance of priorities and timelines. @anandtiwary do you have a sense of how long @aaron-steinfeld's suggestions might take? Are there some things we can do now and phase in the rest of the improvements over time?
Contributor
Author
There was a problem hiding this comment. Things I am re-using:
Things I am changing:
@jake-bassett Migrating to UI construct (point 1) has a much bigger blast radius. We should do it in stages. Let's wait for other higher-priority tasks to get completed first.
Contributor
There was a problem hiding this comment. Okay, got it. Yeah, the items you are re-using are in desperate need of rework but I know that's not at all trivial. I'm good with doing this in phases. As much as I know that code is hard to work with, and making things a bit of a challenge, I don't think we can wait on these filter changes much longer. I vote for not making this PR any larger than what it already is and revisiting replacing the rest of the filtering at a later date. Would appreciate if @aaron-steinfeld can chime in on any more quick wins we can do here in the meantime though.
Contributor
Author
There was a problem hiding this comment. okay. approve this PR :) |
||
| return filters.map(filter => ({ | ||
| field: typeof filter.keyOrExpression === 'string' ? filter.keyOrExpression : filter.keyOrExpression.key, | ||
| subpath: typeof filter.keyOrExpression === 'string' ? undefined : filter.keyOrExpression.subpath, | ||
| operator: toFilterOperator(filter.operator), | ||
| value: filter.value as FilterValue, | ||
| urlString: '' | ||
| })); | ||
| } | ||
|
|
||
| public buildGraphQlFilters(filters: FieldFilter[]): GraphQlFilter[] { | ||
| return filters.map( | ||
| filter => | ||
| new GraphQlFieldFilter( | ||
| { key: filter.field, subpath: filter.subpath }, | ||
| toGraphQlOperator(filter.operator!), // Todo : Very weird | ||
| filter.value as GraphQlArgumentValue | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| public buildGraphQlFiltersFromTableFilters(filters: TableFilter[]): GraphQlFilter[] { | ||
| return filters.map( | ||
| filter => | ||
| new GraphQlFieldFilter( | ||
|
|
@@ -37,9 +60,47 @@ export const toGraphQlOperator = (operator: FilterOperator): GraphQlOperatorType | |
| return GraphQlOperatorType.Like; | ||
| case FilterOperator.In: | ||
| return GraphQlOperatorType.In; | ||
| case FilterOperator.NotIn: | ||
| return GraphQlOperatorType.NotIn; | ||
| case FilterOperator.ContainsKey: | ||
| return GraphQlOperatorType.ContainsKey; | ||
| default: | ||
| return assertUnreachable(operator); | ||
| } | ||
| }; | ||
|
|
||
| export const toFilterOperator = (operator: GraphQlOperatorType): FilterOperator => { | ||
| switch (operator) { | ||
| case GraphQlOperatorType.Equals: | ||
| return FilterOperator.Equals; | ||
|
|
||
| case GraphQlOperatorType.NotEquals: | ||
| return FilterOperator.NotEquals; | ||
|
|
||
| case GraphQlOperatorType.LessThan: | ||
| return FilterOperator.LessThan; | ||
|
|
||
| case GraphQlOperatorType.LessThanOrEqualTo: | ||
| return FilterOperator.LessThanOrEqualTo; | ||
|
|
||
| case GraphQlOperatorType.GreaterThan: | ||
| return FilterOperator.GreaterThan; | ||
|
|
||
| case GraphQlOperatorType.GreaterThanOrEqualTo: | ||
| return FilterOperator.GreaterThanOrEqualTo; | ||
|
|
||
| case GraphQlOperatorType.Like: | ||
| return FilterOperator.Like; | ||
|
|
||
| case GraphQlOperatorType.In: | ||
| return FilterOperator.In; | ||
|
|
||
| case GraphQlOperatorType.NotIn: | ||
| return FilterOperator.NotIn; | ||
|
|
||
| case GraphQlOperatorType.ContainsKey: | ||
| return FilterOperator.ContainsKey; | ||
| default: | ||
| return assertUnreachable(operator); | ||
| } | ||
| }; | ||
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.
Seems completely dependent on the filter implementation - what's the point of typing it?
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.
This is required for Filter to GqlFilter transformation
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.
This is the kind of thing I was saying the other day we shouldn't try to make generic, because there is no one GQL filter representation.