-
Notifications
You must be signed in to change notification settings - Fork 7
chore | adding the support of edge filters in edges #167
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.
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
Changes from all commits
f370fb5
e99f6b6
75f6247
e0d1fe0
b59fe7a
1ccc1f0
db2ef52
97f4849
16511fd
e2dee52
32c4d8e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| package org.hypertrace.graphql.entity.dao; | ||
|
|
||
| import graphql.schema.DataFetcher; | ||
| import graphql.schema.DataFetchingEnvironment; | ||
| import java.util.Collections; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Optional; | ||
| import java.util.concurrent.CompletableFuture; | ||
| import org.hypertrace.core.graphql.common.schema.results.arguments.filter.FilterArgument; | ||
| import org.hypertrace.core.graphql.deserialization.ArgumentDeserializer; | ||
| import org.hypertrace.graphql.entity.schema.EdgeResultSet; | ||
| import org.hypertrace.graphql.entity.schema.Entity; | ||
| import org.hypertrace.graphql.entity.schema.EntityType; | ||
| import org.hypertrace.graphql.entity.schema.argument.NeighborEntityScopeArgument; | ||
| import org.hypertrace.graphql.entity.schema.argument.NeighborEntityTypeArgument; | ||
|
|
||
| abstract class EdgesDataFetcherImpl implements DataFetcher<CompletableFuture<EdgeResultSet>> { | ||
|
|
||
| private final ArgumentDeserializer argumentDeserializer; | ||
|
|
||
| protected abstract EdgeResultSet getEdges( | ||
| Entity entity, EntityType neighborType, String neighborScope, List<FilterArgument> filterBy); | ||
|
|
||
| EdgesDataFetcherImpl(ArgumentDeserializer argumentDeserializer) { | ||
| this.argumentDeserializer = argumentDeserializer; | ||
| } | ||
|
|
||
| @Override | ||
| public CompletableFuture<EdgeResultSet> get(DataFetchingEnvironment environment) { | ||
| // TODO: Edges when used with filters have two limitations for now | ||
| // 1. We can only have one entityType incoming/outgoing edge | ||
| // 2. If different filters for same entity type, filters will be merged into one AND filter. | ||
| // | ||
| // We are passing the empty filter as argument to query the edges. | ||
| // This will work for now because we have restricted multiple edges selection in a request | ||
| // Fix this when we will solve the above two limitations. | ||
| return CompletableFuture.completedFuture( | ||
| getEdges( | ||
| environment.getSource(), | ||
| getNeighborEntityType(environment.getArguments()).orElse(null), | ||
| getNeighborEntityScope(environment.getArguments()).orElse(null), | ||
| Collections.emptyList())); | ||
| } | ||
|
|
||
| private Optional<String> getNeighborEntityScope(Map<String, Object> arguments) { | ||
| return this.argumentDeserializer.deserializePrimitive( | ||
| arguments, NeighborEntityScopeArgument.class); | ||
| } | ||
|
|
||
| private Optional<EntityType> getNeighborEntityType(Map<String, Object> arguments) { | ||
| return this.argumentDeserializer.deserializePrimitive( | ||
| arguments, NeighborEntityTypeArgument.class); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| package org.hypertrace.graphql.entity.dao; | ||
|
|
||
| import java.util.List; | ||
| import javax.inject.Inject; | ||
| import org.hypertrace.core.graphql.common.fetcher.InjectableDataFetcher; | ||
| import org.hypertrace.core.graphql.common.schema.results.arguments.filter.FilterArgument; | ||
| import org.hypertrace.core.graphql.deserialization.ArgumentDeserializer; | ||
| import org.hypertrace.graphql.entity.schema.EdgeResultSet; | ||
| import org.hypertrace.graphql.entity.schema.Entity; | ||
| import org.hypertrace.graphql.entity.schema.EntityType; | ||
|
|
||
| public class IncomingEdgesDataFetcher extends InjectableDataFetcher<EdgeResultSet> { | ||
| public IncomingEdgesDataFetcher() { | ||
| super(IncomingEdgesDataFetcherImpl.class); | ||
| } | ||
|
|
||
| private static class IncomingEdgesDataFetcherImpl extends EdgesDataFetcherImpl { | ||
|
|
||
| @Inject | ||
| IncomingEdgesDataFetcherImpl(ArgumentDeserializer argumentDeserializer) { | ||
| super(argumentDeserializer); | ||
| } | ||
|
|
||
| @Override | ||
| protected EdgeResultSet getEdges( | ||
| Entity entity, | ||
| EntityType neighborType, | ||
| String neighborScope, | ||
| List<FilterArgument> filterBy) { | ||
| return entity.incomingEdges(neighborType, neighborScope, filterBy); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| package org.hypertrace.graphql.entity.dao; | ||
|
|
||
| import java.util.List; | ||
| import javax.inject.Inject; | ||
| import org.hypertrace.core.graphql.common.fetcher.InjectableDataFetcher; | ||
| import org.hypertrace.core.graphql.common.schema.results.arguments.filter.FilterArgument; | ||
| import org.hypertrace.core.graphql.deserialization.ArgumentDeserializer; | ||
| import org.hypertrace.graphql.entity.schema.EdgeResultSet; | ||
| import org.hypertrace.graphql.entity.schema.Entity; | ||
| import org.hypertrace.graphql.entity.schema.EntityType; | ||
|
|
||
| public class OutgoingEdgesDataFetcher extends InjectableDataFetcher<EdgeResultSet> { | ||
|
|
||
| public OutgoingEdgesDataFetcher() { | ||
| super(OutgoingEdgesDataFetcherImpl.class); | ||
| } | ||
|
|
||
| private static class OutgoingEdgesDataFetcherImpl extends EdgesDataFetcherImpl { | ||
|
|
||
| @Inject | ||
| OutgoingEdgesDataFetcherImpl(ArgumentDeserializer argumentDeserializer) { | ||
| super(argumentDeserializer); | ||
| } | ||
|
|
||
| @Override | ||
| protected EdgeResultSet getEdges( | ||
| Entity entity, | ||
| EntityType neighborType, | ||
| String neighborScope, | ||
| List<FilterArgument> filterBy) { | ||
| return entity.outgoingEdges(neighborType, neighborScope, filterBy); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
| import static io.reactivex.rxjava3.core.Single.zip; | ||
|
|
||
| import graphql.schema.SelectedField; | ||
| import io.grpc.Status; | ||
| import io.reactivex.rxjava3.core.Single; | ||
| import java.util.Collection; | ||
| import java.util.List; | ||
|
|
@@ -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; | ||
|
|
@@ -33,13 +37,15 @@ | |
| import org.hypertrace.graphql.metric.request.MetricAggregationRequestBuilder; | ||
|
|
||
| class EdgeRequestBuilder { | ||
|
|
||
| private final String INCOMING_ENTITY_ID_KEY = "fromEntityId"; | ||
| private final String INCOMING_ENTITY_TYPE_KEY = "fromEntityType"; | ||
| private final String OUTGOING_ENTITY_ID_KEY = "toEntityId"; | ||
| private final String OUTGOING_ENTITY_TYPE_KEY = "toEntityType"; | ||
| 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 +55,49 @@ 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, this.getEdgesByType(edgeSetFields), EdgeType.INCOMING); | ||
| 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, this.getEdgesByType(edgeSetFields), EdgeType.OUTGOING); | ||
| return this.buildEdgeRequest(context, timeRange, space, edgeSetFields, EdgeType.OUTGOING); | ||
| } | ||
|
|
||
| private Single<EdgeSetGroupRequest> buildEdgeRequest( | ||
| GraphQlRequestContext context, | ||
| TimeRangeArgument timeRange, | ||
| Optional<String> space, | ||
| Map<String, Set<SelectedField>> edgesByType, | ||
| Stream<SelectedField> edgeSetFields, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| EdgeType edgeType) { | ||
| Set<SelectedField> edgeFields = edgeSetFields.collect(Collectors.toUnmodifiableSet()); | ||
| List<FilterArgument> filterArguments = this.getFilters(edgeFields); | ||
|
|
||
| if (!filterArguments.isEmpty() && edgeFields.size() > 1) { | ||
| throw Status.UNIMPLEMENTED | ||
| .withDescription("Cannot specify more than one edge type with edge filters") | ||
| .asRuntimeException(); | ||
| } | ||
|
|
||
| Map<String, Set<SelectedField>> edgesByType = this.getEdgesByType(edgeFields.stream()); | ||
| Set<SelectedField> allEdges = | ||
| edgesByType.values().stream() | ||
| .flatMap(Collection::stream) | ||
|
|
@@ -94,7 +109,9 @@ private Single<EdgeSetGroupRequest> buildEdgeRequest( | |
| this.getNeighborTypeAttribute(context, edgeType), | ||
| this.metricAggregationRequestBuilder.build( | ||
| context, HypertraceAttributeScopeString.INTERACTION, allEdges.stream()), | ||
| (attributeRequests, neighborIdRequest, neighborTypeRequest, metricRequests) -> | ||
| this.filterRequestBuilder.build( | ||
| context, HypertraceAttributeScopeString.INTERACTION, filterArguments), | ||
| (attributeRequests, neighborIdRequest, neighborTypeRequest, metricRequests, filters) -> | ||
| new DefaultEdgeSetGroupRequest( | ||
| edgesByType.keySet(), | ||
| attributeRequests, | ||
|
|
@@ -110,7 +127,8 @@ private Single<EdgeSetGroupRequest> buildEdgeRequest( | |
| timeRange, | ||
| space, | ||
| neighborIds, | ||
| edgesByType.get(entityType)))); | ||
| edgesByType.get(entityType)), | ||
| filters)); | ||
| } | ||
|
|
||
| private Map<String, Set<SelectedField>> getEdgesByType(Stream<SelectedField> edgeSetStream) { | ||
|
|
@@ -165,6 +183,17 @@ private Single<AttributeRequest> getNeighborIdAttribute( | |
| } | ||
| } | ||
|
|
||
| private List<FilterArgument> getFilters(Set<SelectedField> selectedFields) { | ||
| return selectedFields.stream() | ||
| .map( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I was actually suggesting: Would that be problematic? |
||
| selectedField -> | ||
| this.argumentDeserializer.deserializeObjectList( | ||
| selectedField.getArguments(), FilterArgument.class)) | ||
| .flatMap(Optional::stream) | ||
| .flatMap(Collection::stream) | ||
| .collect(Collectors.toUnmodifiableList()); | ||
| } | ||
|
|
||
| private Single<AttributeRequest> getNeighborTypeAttribute( | ||
| GraphQlRequestContext context, EdgeType edgeType) { | ||
| switch (edgeType) { | ||
|
|
@@ -191,12 +220,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( | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.