Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

fix(httpjson): handle message derived query params #1784

Merged
merged 9 commits into from
Sep 13, 2022
Merged

fix(httpjson): handle message derived query params #1784

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e7a9caa
fix(httpjson): handle message derived query params
diegomarquezp Aug 31, 2022
a61065b
fix(protoparser): decompose messages in query prms
diegomarquezp Sep 2, 2022
63e0118
test(serializer): add test for complex msg obj
diegomarquezp Sep 6, 2022
16689a9
fix(format): format ProtoRestSerializer files
diegomarquezp Sep 6, 2022
2edd120
fix(queryparam): use json approach to process msgs
diegomarquezp Sep 9, 2022
08b655b
fix(queryparam): use numeric value for root enums
diegomarquezp Sep 9, 2022
46b8bd7
chore: Refactoring the fix.
blakeli0 Sep 9, 2022
471a134
test(queryparam): atomized tests
diegomarquezp Sep 9, 2022
f7443b9
test(queryparam): test objects w/ well-known types
diegomarquezp Sep 12, 2022
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
48 changes: 39 additions & 9 deletions gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoRestSerializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,16 @@

import com.google.api.core.BetaApi;
import com.google.common.collect.ImmutableList;
import com.google.gson.JsonElement;
import com.google.gson.JsonParser;
import com.google.protobuf.InvalidProtocolBufferException;
import com.google.protobuf.Message;
import com.google.protobuf.TypeRegistry;
import com.google.protobuf.util.JsonFormat;
import com.google.protobuf.util.JsonFormat.Printer;
import java.io.IOException;
import java.io.Reader;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;

Expand All @@ -49,6 +52,7 @@
*/
@BetaApi
public class ProtoRestSerializer<RequestT extends Message> {

private final TypeRegistry registry;

private ProtoRestSerializer(TypeRegistry registry) {
Expand All @@ -75,7 +79,7 @@ static <RequestT extends Message> ProtoRestSerializer<RequestT> create(TypeRegis
* @throws InvalidProtocolBufferException if failed to serialize the protobuf message to JSON
* format
*/
String toJson(RequestT message, boolean numericEnum) {
String toJson(Message message, boolean numericEnum) {
try {
Printer printer = JsonFormat.printer().usingTypeRegistry(registry);
if (numericEnum) {
Expand Down Expand Up @@ -118,6 +122,23 @@ public void putPathParam(Map<String, String> fields, String fieldName, Object fi
fields.put(fieldName, String.valueOf(fieldValue));
}

private void putDecomposedMessageQueryParam(
Map<String, List<String>> fields, String fieldName, JsonElement parsed) {
if (parsed.isJsonPrimitive() || parsed.isJsonNull()) {
putQueryParam(fields, fieldName, parsed.getAsString());
} else if (parsed.isJsonArray()) {
for (JsonElement element : parsed.getAsJsonArray()) {
putDecomposedMessageQueryParam(fields, fieldName, element);
}
} else {
// it is a json object
for (String key : parsed.getAsJsonObject().keySet()) {
putDecomposedMessageQueryParam(
fields, String.format("%s.%s", fieldName, key), parsed.getAsJsonObject().get(key));
}
}
}

/**
* Puts a message field in {@code fields} map which will be used to populate query parameters of a
* request.
Expand All @@ -127,16 +148,25 @@ public void putPathParam(Map<String, String> fields, String fieldName, Object fi
* @param fieldValue a field value
*/
public void putQueryParam(Map<String, List<String>> fields, String fieldName, Object fieldValue) {
ImmutableList.Builder<String> paramValueList = ImmutableList.builder();
if (fieldValue instanceof List<?>) {
for (Object fieldValueItem : (List<?>) fieldValue) {
paramValueList.add(String.valueOf(fieldValueItem));
List<String> currentParamValueList = new ArrayList<>();
List<Object> toProcess =
fieldValue instanceof List<?> ? (List<Object>) fieldValue : ImmutableList.of(fieldValue);
for (Object fieldValueItem : toProcess) {
if (fieldValueItem instanceof Message) {
String json = toJson(((Message) fieldValueItem), true);
JsonElement parsed = JsonParser.parseString(json);
putDecomposedMessageQueryParam(fields, fieldName, parsed);
} else {
Copy link
Member

Choose a reason for hiding this comment

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

When working with the pattern:

if (x is Collection)
  foreach i in x
    // do A(i)
else
  // do A(x)

We can often refactor to:

Iterable<?> fieldValues =
    fieldValue instanceof Iterable
        ? (Iterable<?>) fieldValue
        : Collections.singleton(fieldValue);

foreach i in x
  // do A(i)

This eliminates the need to study both implementations of A to determine if they're the same or not, with the added benefit of it being easier to test the single code path rather than both. As the complexity of A increases, this simplification becomes more and more beneficial.

currentParamValueList.add(String.valueOf(fieldValueItem));
}
} else {
paramValueList.add(String.valueOf(fieldValue));
}

fields.put(fieldName, paramValueList.build());
if (currentParamValueList.isEmpty()) {
// We try to avoid putting non-leaf level fields to the query params
return;
}
List<String> accumulativeParamValueList = fields.getOrDefault(fieldName, new ArrayList<>());
accumulativeParamValueList.addAll(currentParamValueList);
fields.put(fieldName, accumulativeParamValueList);
}

/**
Expand Down
96 changes: 94 additions & 2 deletions gax-httpjson/src/test/java/com/google/api/gax/httpjson/ProtoRestSerializerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,17 @@
package com.google.api.gax.httpjson;

import com.google.common.truth.Truth;
import com.google.protobuf.Duration;
import com.google.protobuf.Field;
import com.google.protobuf.Field.Cardinality;
import com.google.protobuf.FieldMask;
import com.google.protobuf.FloatValue;
import com.google.protobuf.Int32Value;
import com.google.protobuf.Option;
import com.google.protobuf.Timestamp;
import com.google.protobuf.TypeRegistry;
import com.google.rpc.RetryInfo;
import com.google.type.Interval;
import java.io.IOException;
import java.io.StringReader;
import java.util.Arrays;
Expand All @@ -53,7 +61,14 @@ public class ProtoRestSerializerTest {

@Before
public void setUp() {
requestSerializer = ProtoRestSerializer.create();
// tests with Any type messages require corresponding descriptors in the type registry
requestSerializer =
ProtoRestSerializer.create(
TypeRegistry.newBuilder()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you check if these descriptors are added to ProtoRestSerializer on generating client libraries? If not, we should added the well knowntypes to the registry as well.

.add(FieldMask.getDescriptor())
.add(Duration.getDescriptor())
.build());

field =
Field.newBuilder()
.setNumber(2)
Expand Down Expand Up @@ -163,7 +178,7 @@ public void putPathParam() {
}

@Test
public void putQueryParam() {
public void putQueryParamPrimitive() {
Map<String, List<String>> fields = new HashMap<>();
requestSerializer.putQueryParam(fields, "optName1", 1);
requestSerializer.putQueryParam(fields, "optName2", 0);
Expand All @@ -181,6 +196,83 @@ public void putQueryParam() {
Truth.assertThat(fields).isEqualTo(expectedFields);
}

@Test
public void putQueryParamComplexObject() {
Map<String, List<String>> fields = new HashMap<>();
requestSerializer.putQueryParam(fields, "object", field);

Map<String, List<String>> expectedFields = new HashMap<>();
expectedFields.put("object.cardinality", Arrays.asList("1"));
expectedFields.put("object.name", Arrays.asList("field_name1"));
expectedFields.put("object.number", Arrays.asList("2"));
expectedFields.put("object.options.name", Arrays.asList("opt_name1", "opt_name2"));

Truth.assertThat(fields).isEqualTo(expectedFields);
}

@Test
public void putQueryParamComplexObjectDuration() {
Map<String, List<String>> fields = new HashMap<>();
Duration duration = Duration.newBuilder().setSeconds(1).setNanos(1).build();
RetryInfo input = RetryInfo.newBuilder().setRetryDelay(duration).build();
requestSerializer.putQueryParam(fields, "retry_info", input);

Map<String, List<String>> expectedFields = new HashMap<>();
expectedFields.put("retry_info.retryDelay", Arrays.asList("1.000000001s"));

Truth.assertThat(fields).isEqualTo(expectedFields);
}

@Test
public void putQueryParamComplexObjectTimestamp() {
Map<String, List<String>> fields = new HashMap<>();
Timestamp start = Timestamp.newBuilder().setSeconds(1).setNanos(1).build();
Timestamp end = Timestamp.newBuilder().setSeconds(2).setNanos(2).build();
Interval input = Interval.newBuilder().setStartTime(start).setEndTime(end).build();

requestSerializer.putQueryParam(fields, "object", input);

Map<String, List<String>> expectedFields = new HashMap<>();
expectedFields.put("object.startTime", Arrays.asList("1970-01-01T00:00:01.000000001Z"));
expectedFields.put("object.endTime", Arrays.asList("1970-01-01T00:00:02.000000002Z"));

Truth.assertThat(fields).isEqualTo(expectedFields);
}

@Test
public void putQueryParamDuration() {
queryParamHelper(Duration.newBuilder().setSeconds(1).setNanos(1).build(), "1.000000001s");
}

@Test
public void putQueryParamTimestamp() {
queryParamHelper(
Timestamp.newBuilder().setSeconds(1).setNanos(1).build(), "1970-01-01T00:00:01.000000001Z");
}

@Test
public void putQueryParamFieldMask() {
queryParamHelper(FieldMask.newBuilder().addPaths("a.b").addPaths("c.d").build(), "a.b,c.d");
}

@Test
public void putQueryParamInt32Value() {
queryParamHelper(Int32Value.of(1), "1");
}

@Test
public void putQueryParamFloatValue() {
queryParamHelper(FloatValue.of(1.1f), "1.1");
}

private void queryParamHelper(Object value, String expected) {
Map<String, List<String>> fields = new HashMap<>();
requestSerializer.putQueryParam(fields, "value", value);
Map<String, List<String>> expectedFields = new HashMap<>();
expectedFields.put("value", Arrays.asList(expected));
Truth.assertThat(fields).isEqualTo(expectedFields);
}

@Test
public void toBody() {
String body = requestSerializer.toBody("bodyField1", field, false);
Expand Down