feat: add global filtering that applies aross all queries #546
Conversation
Codecov Report
@@ Coverage Diff @@
## main #546 +/- ##
==========================================
+ Coverage 85.57% 85.59% +0.01%
==========================================
Files 759 760 +1
Lines 15560 15581 +21
Branches 1987 1988 +1
==========================================
+ Hits 13316 13337 +21
Misses 2211 2211
Partials 33 33
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
.../distributed-tracing/src/shared/graphql/model/schema/filter/global-graphql-filter.service.ts
Show resolved
Hide resolved
| ...this.argBuilder.forOrderBy(request.sort), | ||
| ...this.argBuilder.forFilters(request.filters) | ||
| ...this.argBuilder.forFilters( | ||
| this.globalGraphQlFilterService.mergeGlobalFilters(resolveTraceType(request.traceType), request.filters) |
There was a problem hiding this comment.
Putting this everywhere doesn't seem too scalable and may be error prone. Can we add it in idk GraphQlQueryEventService may be so that the inherited filters already have the desired filter?
There was a problem hiding this comment.
I don't love it, but it seems like the best place we have right now. There's no place with central knowledge of all queries, by design - and filters aren't a concept that applies to every request in the same way. QueryEventService for example pushes filters down to the data sources to apply, which is even more decentralized (and only works in the dashboard code) - the service itself doesn't know the request shapes to provide them.
Open to other ideas, but I think the handlers are the narrowest (and most stable point) that still has the context on how to apply the filters without a larger redesign.
There was a problem hiding this comment.
Then can we do it using an abstract method which the concrete handler has to provide? I could see people easily missing to apply the right global filters in new handlers.
There was a problem hiding this comment.
it wouldn't work very well as an abstract method. The transformation depends on the shape of the request, the logic of the handler and the shape of the constructed query. We could make a util, but at that point we're basically wrapping this.globalGraphQlFilterService.mergeGlobalFilters with another function, so no real purpose. Also, there's no base class for the handlers (and if there were, we wouldn't want to add an injection dependency to it for something that's only applicable to certain handlers).
There was a problem hiding this comment.
okay. I was thinking we could just define a function below. While it doesn't do much, it would just force the dev to consider the case of global filter application. Otherwise it is easy to miss. It is still a hack.
export interface GraphQlHandler<TRequest, TResponse> {
readonly type: GraphQlHandlerType;
matchesRequest(request: unknown): request is TRequest;
/**
* Converts the provided request into a single selection, or a map of selections with arbitrary keys.
* If a map is provided, the response provided to the converter will use the same keys to disambiguate
* the response for each selection.
*/
convertRequest(request: TRequest): GraphQlSelection | Map<unknown, GraphQlSelection>;
/**
* Converts the provided response, or map of responses (if a map of selections were provided in the request
* conversion), to a response object.
*/
convertResponse(response: unknown | Map<unknown, unknown>, request: TRequest): TResponse | Observable<TResponse>;
getRequestOptions?(request: TRequest): GraphQlRequestOptions;
}
We do need a better solution for this.
This comment has been minimized.
This comment has been minimized.
Description
This change adds support for registering global filter rules which will be applied to all graphql handlers. This allows rules to be managed independently of the various handlers, a more extensible design.
Testing
Added and updated unit tests
Checklist: