chore | adding the support of edge filters in edges #167
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
|
|
||
| import graphql.schema.SelectedField; | ||
| import io.reactivex.rxjava3.core.Single; | ||
| import java.util.ArrayList; | ||
| import java.util.Collection; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
@@ -16,11 +17,14 @@ | |
| import javax.inject.Provider; | ||
| import lombok.Value; | ||
| import lombok.experimental.Accessors; | ||
| import org.hypertrace.core.graphql.common.request.AttributeAssociation; | ||
| import org.hypertrace.core.graphql.common.request.AttributeRequest; | ||
| import org.hypertrace.core.graphql.common.request.AttributeRequestBuilder; | ||
| import org.hypertrace.core.graphql.common.request.FilterRequestBuilder; | ||
| import org.hypertrace.core.graphql.common.schema.arguments.TimeRangeArgument; | ||
| import org.hypertrace.core.graphql.common.schema.attributes.arguments.AttributeExpression; | ||
| import org.hypertrace.core.graphql.common.schema.results.ResultSet; | ||
| import org.hypertrace.core.graphql.common.schema.results.arguments.filter.FilterArgument; | ||
| import org.hypertrace.core.graphql.context.GraphQlRequestContext; | ||
| import org.hypertrace.core.graphql.deserialization.ArgumentDeserializer; | ||
| import org.hypertrace.core.graphql.utils.schema.GraphQlSelectionFinder; | ||
|
|
@@ -40,6 +44,7 @@ class EdgeRequestBuilder { | |
| private final ArgumentDeserializer argumentDeserializer; | ||
| private final GraphQlSelectionFinder selectionFinder; | ||
| private final MetricAggregationRequestBuilder metricAggregationRequestBuilder; | ||
| private final FilterRequestBuilder filterRequestBuilder; | ||
| private final AttributeRequestBuilder attributeRequestBuilder; | ||
| // Use provider to avoid cycle | ||
| private final Provider<NeighborEntitiesRequestBuilder> neighborEntitiesRequestBuilderProvider; | ||
|
|
@@ -49,40 +54,44 @@ class EdgeRequestBuilder { | |
| ArgumentDeserializer argumentDeserializer, | ||
| GraphQlSelectionFinder selectionFinder, | ||
| MetricAggregationRequestBuilder metricAggregationRequestBuilder, | ||
| FilterRequestBuilder filterRequestBuilder, | ||
| AttributeRequestBuilder attributeRequestBuilder, | ||
| Provider<NeighborEntitiesRequestBuilder> neighborEntitiesRequestBuilderProvider) { | ||
| this.argumentDeserializer = argumentDeserializer; | ||
| this.selectionFinder = selectionFinder; | ||
| this.metricAggregationRequestBuilder = metricAggregationRequestBuilder; | ||
| this.attributeRequestBuilder = attributeRequestBuilder; | ||
| this.neighborEntitiesRequestBuilderProvider = neighborEntitiesRequestBuilderProvider; | ||
| this.filterRequestBuilder = filterRequestBuilder; | ||
| } | ||
|
|
||
| Single<EdgeSetGroupRequest> buildIncomingEdgeRequest( | ||
| GraphQlRequestContext context, | ||
| TimeRangeArgument timeRange, | ||
| Optional<String> space, | ||
| Stream<SelectedField> edgeSetFields) { | ||
| return this.buildEdgeRequest(context, timeRange, space, edgeSetFields, EdgeType.INCOMING); | ||
| } | ||
|
|
||
| Single<EdgeSetGroupRequest> buildOutgoingEdgeRequest( | ||
| GraphQlRequestContext context, | ||
| TimeRangeArgument timeRange, | ||
| Optional<String> space, | ||
| Stream<SelectedField> edgeSetFields) { | ||
| return this.buildEdgeRequest(context, timeRange, space, edgeSetFields, EdgeType.OUTGOING); | ||
| } | ||
|
|
||
| private Single<EdgeSetGroupRequest> buildEdgeRequest( | ||
| GraphQlRequestContext context, | ||
| TimeRangeArgument timeRange, | ||
| Optional<String> space, | ||
| Stream<SelectedField> edgeSetFields, | ||
| EdgeType edgeType) { | ||
|
|
||
| Set<SelectedField> edgeFields = edgeSetFields.collect(Collectors.toSet()); | ||
| List<FilterArgument> filterArguments = this.getFilters(edgeFields); | ||
|
|
||
| Map<String, Set<SelectedField>> edgesByType = this.getEdgesByType(edgeFields.stream()); | ||
| Set<SelectedField> allEdges = | ||
| edgesByType.values().stream() | ||
| .flatMap(Collection::stream) | ||
|
|
@@ -94,7 +103,9 @@ private Single<EdgeSetGroupRequest> buildEdgeRequest( | |
| this.getNeighborTypeAttribute(context, edgeType), | ||
| this.metricAggregationRequestBuilder.build( | ||
| context, HypertraceAttributeScopeString.INTERACTION, allEdges.stream()), | ||
| this.filterRequestBuilder.build( | ||
| context, HypertraceAttributeScopeString.INTERACTION, filterArguments), | ||
| (attributeRequests, neighborIdRequest, neighborTypeRequest, metricRequests, filters) -> | ||
| new DefaultEdgeSetGroupRequest( | ||
| edgesByType.keySet(), | ||
| attributeRequests, | ||
|
|
@@ -110,7 +121,8 @@ private Single<EdgeSetGroupRequest> buildEdgeRequest( | |
| timeRange, | ||
| space, | ||
| neighborIds, | ||
| edgesByType.get(entityType)), | ||
| filters)); | ||
| } | ||
|
|
||
| private Map<String, Set<SelectedField>> getEdgesByType(Stream<SelectedField> edgeSetStream) { | ||
|
|
@@ -165,6 +177,17 @@ private Single<AttributeRequest> getNeighborIdAttribute( | |
| } | ||
| } | ||
|
|
||
| private List<FilterArgument> getFilters(Set<SelectedField> selectedFields) { | ||
| return selectedFields.stream() | ||
| .map( | ||
|
Contributor
There was a problem hiding this comment. referred to in other comment, but this is combining the filter arguments from each independent edge request (each field here is one edge request like
Member
Author
There was a problem hiding this comment. This I have already discussed with @skjindal93 . With this PR we are adding two limitations in the case of using edge filters,
we are introducing this to solve the deliverable first. Immediately after this we will make the changes to remove above two limitations. Currently, no one is using multiple entity types interactions, so it won't break anything. also old behaviour is still intact.
Member
Author
There was a problem hiding this comment. The reason we didnt solve this is because it will need big change in gateway service where we will have to make interactions request as an array of request rather than one single filter which it is right now. https://github.com/hypertrace/gateway-service/blob/main/gateway-service-api/src/main/proto/org/hypertrace/gateway/service/v1/entities.proto#L41 this is the filter i am referring to
Contributor
There was a problem hiding this comment. I'm not sure I agree here, but we can discuss further.
Confused here - the main application flow uses multiple entity types? It shows services with downstream services and downstream backends.
We already handle multiple entity types through a filter, is this different? You'd need to just change how you build the filter to OR across selection types and then AND for each selection type.
Member
Author
There was a problem hiding this comment.
It won't break the current workflows. Those api will work. These limitations only come into the picture when filters are being used.
Contributor
There was a problem hiding this comment. We don't want to leave an api in this broken state though - so if this is going to impact how we implement things, we should be considering that now.
Contributor
There was a problem hiding this comment. We were just planning to do it in iterations. Have a basic application flow working with limitations (not breaking the existing application flow), and then refactor it next to make it perfect. (because, refactoring would take considerable amount of time otherwise, causing delays for Third Party APIs) We are definitely planning to pick up the refactor as the next task
Contributor
There was a problem hiding this comment. It shouldn't require a big refactor though - that's the part that's confusing me. The refactor would just be in the new code that's being written in these 2 PRs, right? I can definitely be mistaken, but aren't we talking a handful of lines (since we already have the infrastructure to separate out the requests then merge them for the existing code)? If we choose not to however, can we at least have this error out if we receive an unsupported request? My bigger concern is that certain calls will just result in success but have inaccurate results (because filters get merged that shouldn't be).
Member
Author
There was a problem hiding this comment.
This won't work actually if I am understanding it correctly. Consider this for an example, let's say we query for two edges like this Now the interaction request to the gateway service will be like this Now this will return wrong results for both service and api. Since each filter corresponding to the edge is specific to that interaction we cannot do it in a single query. We will need to change the API contract and make gateway service to run each edge query separately. For now, we have made the code error out in case of above-mentioned limitations
Contributor
There was a problem hiding this comment.
I was actually suggesting: Would that be problematic? |
||
| selectedField -> | ||
| this.argumentDeserializer.deserializeObjectList( | ||
| selectedField.getArguments(), FilterArgument.class)) | ||
| .flatMap(arguments -> Stream.of(arguments.orElse(new ArrayList<>()))) | ||
aman-bansal marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| .flatMap(Collection::stream) | ||
| .collect(Collectors.toList()); | ||
aman-bansal marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| private Single<AttributeRequest> getNeighborTypeAttribute( | ||
| GraphQlRequestContext context, EdgeType edgeType) { | ||
| switch (edgeType) { | ||
|
|
@@ -191,12 +214,14 @@ private enum EdgeType { | |
| @Value | ||
| @Accessors(fluent = true) | ||
| private static class DefaultEdgeSetGroupRequest implements EdgeSetGroupRequest { | ||
|
|
||
| Set<String> entityTypes; | ||
| Collection<AttributeRequest> attributeRequests; | ||
| Collection<MetricAggregationRequest> metricAggregationRequests; | ||
| AttributeRequest neighborIdAttribute; | ||
| AttributeRequest neighborTypeAttribute; | ||
| BiFunction<String, Collection<String>, Single<EntityRequest>> neighborRequestBuilder; | ||
| Collection<AttributeAssociation<FilterArgument>> filterArguments; | ||
|
|
||
| @Override | ||
| public Single<EntityRequest> buildNeighborRequest( | ||
|
|
||
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 looks to be off. There are potentially multiple edge requests that each can have different fields and filters - hence the previous map of type to fields. We're splitting them up later here, but we're not doing the same thing for filters.