Skip to content

chore(entity-query-service): better integration tests for update API #128

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

chore(entity-query-service): better integration tests for update API #128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
118 changes: 89 additions & 29 deletions ...rc/integrationTest/java/org/hypertrace/entity/service/service/EntityQueryServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,21 @@
import static org.junit.jupiter.api.Assertions.assertTrue;

import com.google.common.collect.Lists;
import com.google.protobuf.LazyStringList;
import com.typesafe.config.Config;
import com.typesafe.config.ConfigFactory;
import io.grpc.Channel;
import io.grpc.ManagedChannel;
import io.grpc.ManagedChannelBuilder;
import java.time.Instant;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.UUID;
import java.util.stream.Collectors;
import org.hypertrace.core.documentstore.Datastore;
import org.hypertrace.core.documentstore.DatastoreProvider;
import org.hypertrace.core.grpcutils.client.GrpcClientRequestContextUtil;
Expand All @@ -31,6 +34,7 @@
import org.hypertrace.entity.constants.v1.ServiceAttribute;
import org.hypertrace.entity.data.service.client.EntityDataServiceClient;
import org.hypertrace.entity.data.service.v1.AttributeValue;
import org.hypertrace.entity.data.service.v1.AttributeValueList;
import org.hypertrace.entity.data.service.v1.Entity;
import org.hypertrace.entity.data.service.v1.Value;
import org.hypertrace.entity.query.service.client.EntityQueryServiceClient;
Expand Down Expand Up @@ -94,6 +98,7 @@ public class EntityQueryServiceTest {
// attributes defined in application.conf in attribute map
private static final String API_DISCOVERY_STATE_ATTR = "API.apiDiscoveryState";
private static final String API_HTTP_METHOD_ATTR = "API.httpMethod";
private static final String API_LABELS_ATTR = "API.labels";
private static final int CONTAINER_STARTUP_ATTEMPTS = 5;
private static GenericContainer<?> mongo;

Expand Down Expand Up @@ -484,6 +489,11 @@ public void testBulkUpdate() {
.setTenantId(TENANT_ID)
.setEntityType(EntityType.API.name())
.setEntityName("api1")
.putAttributes(
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add an integer to the integration test, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't support updating INT yet via EQS update APIs. Planning to add support for that in a separate PR

Copy link
Contributor

Choose a reason for hiding this comment

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

we don't? maybe I'm misunderstanding or looking in the wrong spot, but it sure looks like we do - unless it's in the API but not the implementation?

message UpdateOperation {
  oneof operation {
    SetAttribute setAttribute = 1;
    // more update operations in the future
  }
}

->

message SetAttribute {
  ColumnIdentifier attribute = 1;
  LiteralConstant value = 2;
}

->

message LiteralConstant {
  Value value = 1;
}

->

message Value {
  ValueType valueType = 1;

  string string = 3;
  int64 long = 4;
  int32 int = 5;
  float float = 6;
  double double = 7;
  bytes bytes = 8;
  bool boolean = 9;
  sfixed64 timestamp = 15;
  repeated string string_array = 16;
  repeated int64 long_array = 17;
  repeated int32 int_array = 18;
  repeated float float_array = 19;
  repeated double double_array = 20;
  repeated bytes bytes_array = 21;
  repeated bool boolean_array = 22;
  map<string, string> string_map = 23;
}

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Sep 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

oh, the implementation only has long in its switch, not int. That's really confusing, but is actually right (other than the fact that it should yell loudly and not have it in its interface)- entities only have long fields, not int.

...and just to clarify further, because this is a complete mess - Entity the object returned by EDS can actually hold both int and long, and Row the EQS response can hold both (and those two don't match WRT certain other data types 😢 ), but our attribute schema, which represents the entity field data types does not have an int type - and our attribute schema should be the source of truth here.

So tl;dr - we shouldn't add support for int, but could we add long to the test?

apiAttributesMap.get(API_DISCOVERY_STATE_ATTR), createAttribute("DISCOVERED"))
.putAttributes(apiAttributesMap.get(API_HTTP_METHOD_ATTR), createAttribute("GET"))
.putAttributes(
apiAttributesMap.get(API_LABELS_ATTR), createArrayAttribute(List.of("Label1")))
.putIdentifyingAttributes(
EntityConstants.getValue(ServiceAttribute.SERVICE_ATTRIBUTE_ID),
createAttribute(SERVICE_ID))
Expand All @@ -492,17 +502,17 @@ public void testBulkUpdate() {
.putIdentifyingAttributes(
EntityConstants.getValue(ApiAttribute.API_ATTRIBUTE_API_TYPE),
createAttribute(API_TYPE));
apiEntityBuilder1
.putAttributes(
apiAttributesMap.get(API_DISCOVERY_STATE_ATTR), createAttribute("DISCOVERED"))
.putAttributes(apiAttributesMap.get(API_HTTP_METHOD_ATTR), createAttribute("GET"));

Entity entity1 = entityDataServiceClient.upsert(apiEntityBuilder1.build());

Entity.Builder apiEntityBuilder2 =
Entity.newBuilder()
.setTenantId(TENANT_ID)
.setEntityType(EntityType.API.name())
.setEntityName("api2")
.putAttributes(
apiAttributesMap.get(API_DISCOVERY_STATE_ATTR), createAttribute("UNDER_DISCOVERY"))
.putAttributes(apiAttributesMap.get(API_HTTP_METHOD_ATTR), createAttribute("GET"))
.putIdentifyingAttributes(
EntityConstants.getValue(ServiceAttribute.SERVICE_ATTRIBUTE_ID),
createAttribute(SERVICE_ID))
Expand All @@ -511,43 +521,44 @@ public void testBulkUpdate() {
.putIdentifyingAttributes(
EntityConstants.getValue(ApiAttribute.API_ATTRIBUTE_API_TYPE),
createAttribute(API_TYPE));
apiEntityBuilder2
.putAttributes(
apiAttributesMap.get(API_DISCOVERY_STATE_ATTR), createAttribute("UNDER_DISCOVERY"))
.putAttributes(apiAttributesMap.get(API_HTTP_METHOD_ATTR), createAttribute("GET"));
Entity entity2 = entityDataServiceClient.upsert(apiEntityBuilder2.build());

// create BulkUpdate request
UpdateOperation update1 =
UpdateOperation apiDiscoveryStateUpdateOperation =
UpdateOperation.newBuilder()
.setSetAttribute(
SetAttribute.newBuilder()
.setAttribute(
ColumnIdentifier.newBuilder().setColumnName(API_DISCOVERY_STATE_ATTR))
.setValue(
LiteralConstant.newBuilder()
.setValue(
org.hypertrace.entity.query.service.v1.Value.newBuilder()
.setString("DISCOVERED"))))
.setValue(createLiteral("DISCOVERED")))
.build();
UpdateOperation update2 =
UpdateOperation httpMethodUpdateOperation =
UpdateOperation.newBuilder()
.setSetAttribute(
SetAttribute.newBuilder()
.setAttribute(ColumnIdentifier.newBuilder().setColumnName(API_HTTP_METHOD_ATTR))
.setValue(
LiteralConstant.newBuilder()
.setValue(
org.hypertrace.entity.query.service.v1.Value.newBuilder()
.setString("POST"))))
.setValue(createLiteral("POST")))
.build();
UpdateOperation apiLabelsUpdateOperation =
UpdateOperation.newBuilder()
.setSetAttribute(
SetAttribute.newBuilder()
.setAttribute(ColumnIdentifier.newBuilder().setColumnName(API_LABELS_ATTR))
.setValue(createLiteral(List.of("Label1", "Label2"))))
.build();

EntityUpdateInfo updateInfo1 =
EntityUpdateInfo.newBuilder().addUpdateOperation(update2).build();
EntityUpdateInfo.newBuilder()
.addUpdateOperation(httpMethodUpdateOperation)
.addUpdateOperation(apiLabelsUpdateOperation)
.build();
EntityUpdateInfo updateInfo2 =
EntityUpdateInfo.newBuilder()
.addUpdateOperation(update1)
.addUpdateOperation(update2)
.addUpdateOperation(apiDiscoveryStateUpdateOperation)
.addUpdateOperation(httpMethodUpdateOperation)
.addUpdateOperation(apiLabelsUpdateOperation)
.build();

BulkEntityUpdateRequest bulkUpdateRequest =
BulkEntityUpdateRequest.newBuilder()
.setEntityType(EntityType.API.name())
Expand All @@ -563,30 +574,45 @@ public void testBulkUpdate() {
.setEntityType(EntityType.API.name())
.addSelection(createExpression(API_DISCOVERY_STATE_ATTR))
.addSelection(createExpression(API_HTTP_METHOD_ATTR))
.addSelection(createExpression(API_LABELS_ATTR))
.build();

Iterator<ResultSetChunk> resultSetChunkIterator =
GrpcClientRequestContextUtil.executeWithHeadersContext(
HEADERS, () -> entityQueryServiceClient.execute(entityQueryRequest));

List<String> values = new ArrayList<>();
List<Object> values = new ArrayList<>();

while (resultSetChunkIterator.hasNext()) {
ResultSetChunk chunk = resultSetChunkIterator.next();

for (Row row : chunk.getRowList()) {
for (int i = 0; i < row.getColumnCount(); i++) {
String value = row.getColumnList().get(i).getString();
values.add(value);
org.hypertrace.entity.query.service.v1.Value value = row.getColumnList().get(i);
switch (value.getValueType()) {
case STRING:
values.add(value.getString());
continue;
case STRING_ARRAY:
values.add(value.getStringArrayList());
continue;
default:
}
}
}
}

assertEquals(4, values.size());
assertEquals(6, values.size());
assertEquals("DISCOVERED", values.get(0));
assertEquals("POST", values.get(1));
assertEquals("DISCOVERED", values.get(2));
assertEquals("POST", values.get(3));
assertListEquals(
List.of("Label1", "Label2"),
((LazyStringList) values.get(2)).stream().collect(Collectors.toUnmodifiableList()));
assertEquals("DISCOVERED", values.get(3));
assertEquals("POST", values.get(4));
assertListEquals(
List.of("Label1", "Label2"),
((LazyStringList) values.get(5)).stream().collect(Collectors.toUnmodifiableList()));
}

@Nested
Expand Down Expand Up @@ -677,6 +703,34 @@ private AttributeValue createAttribute(String name) {
return AttributeValue.newBuilder().setValue(Value.newBuilder().setString(name).build()).build();
}

private AttributeValue createArrayAttribute(Collection<String> values) {
return AttributeValue.newBuilder()
.setValueList(
AttributeValueList.newBuilder()
.addAllValues(
values.stream().map(this::createAttribute).collect(Collectors.toList()))
.build())
.build();
}

private LiteralConstant createLiteral(String value) {
return LiteralConstant.newBuilder()
.setValue(
org.hypertrace.entity.query.service.v1.Value.newBuilder()
.setString(value)
.setValueType(ValueType.STRING))
.build();
}

private LiteralConstant createLiteral(Collection<String> values) {
return LiteralConstant.newBuilder()
.setValue(
org.hypertrace.entity.query.service.v1.Value.newBuilder()
.addAllStringArray(values)
.setValueType(ValueType.STRING_ARRAY))
.build();
}

private Entity.Builder createApiEntity(String serviceId, String apiName, String apiType) {
return Entity.newBuilder()
.setTenantId(TENANT_ID)
Expand Down Expand Up @@ -734,4 +788,10 @@ private static Map<String, Map<String, String>> getAttributesMap() {

return attributesMap;
}

private <T> void assertListEquals(List<T> expected, List<T> actual) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why can't this just use assertEquals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that. It wasn't working with the basic assertEquals, though it should have

Didn't spend much time on why it wasn't working

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, worth figuring out - something's off here. This check for example doesn't assert that their order matches, which is part of list equality.

assertEquals(expected.size(), actual.size());
assertTrue(actual.containsAll(expected));
assertTrue(expected.containsAll(actual));
}
}
5 changes: 5 additions & 0 deletions entity-service/src/integrationTest/resources/configs/entity-service/application.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ entity.service.attributeMap = [
"name": "API.apiDiscoveryState",
"subDocPath": "attributes.api_discovery_state"
},
{
"scope": "API",
"name": "API.labels",
"subDocPath": "attributes.labels"
},
{
"scope": "SERVICE",
"name": "SERVICE.id",
Expand Down