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 4 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
91 changes: 85 additions & 6 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 @@ -30,16 +30,23 @@
package com.google.api.gax.httpjson;

import com.google.api.core.BetaApi;
import com.google.common.collect.ImmutableList;
import com.google.protobuf.ByteString;
import com.google.protobuf.Descriptors.FieldDescriptor;
import com.google.protobuf.GeneratedMessageV3;
import com.google.protobuf.InvalidProtocolBufferException;
import com.google.protobuf.Message;
import com.google.protobuf.MessageOrBuilder;
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.Arrays;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;

/**
* This class serializes/deserializes protobuf {@link Message} for REST interactions. It serializes
Expand All @@ -51,6 +58,29 @@
public class ProtoRestSerializer<RequestT extends Message> {
private final TypeRegistry registry;

// well-known types obtained from
// https://github.com/googleapis/gapic-showcase/blob/fe414784c18878d704b884348d84c68fd6b87466/util/genrest/resttools/populatefield.go#L27
Copy link
Contributor

Choose a reason for hiding this comment

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

I proposed the list from gapic-showcase but I don't think we should just use it, showcase is only used for testing so the list may get out of date. We should probably go through the protobuf doc and add any type that have special handling to the list. For now, since Go already implemented it, we can probably follow what they did here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure, I can update the list of well-known types following the docs list, but I'm curious if there is a better way to programatically obtain a list of well-known types so we can update it as a dependency or so.

We have this documentation entry as a source of well-known types but we may have to manually check from time to time to keep our hardcoded list up to date.
However, I noted that the official definitions such as duration.proto would specify a special dotnet package Google.Protobuf.WellKnownTypes, whereas in java it falls into the common com.google.protobuf package, leaving us with no reliable programatic way to obtain a list of well-known types that would get updated as a dependency. As a last (and bad) source, there is a unit test in the same folder of the duration definition that contains a list of well-known types, but it is a unit test, so I don't think we have too many options

private static final Set<Class<GeneratedMessageV3>> jsonSerializableMessages =
new HashSet(
Arrays.asList(
Copy link
Member

Choose a reason for hiding this comment

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

Effective Java 26: Don't use raw types. To do that and make this immutable at the same time, consider:

  private static final Set<Class<? extends GeneratedMessageV3>> JSON_SERIALIZABLE_MESSAGES =
      ImmutableSet.of(
          BoolValue.class,

Note that the collection could be considered a constant since it is now immutable, thus now worthy of upper snake case.

com.google.protobuf.BoolValue.class,
com.google.protobuf.BytesValue.class,
com.google.protobuf.DoubleValue.class,
com.google.protobuf.Duration.class,
com.google.protobuf.FieldMask.class,
com.google.protobuf.FloatValue.class,
com.google.protobuf.Int32Value.class,
com.google.protobuf.Int64Value.class,
com.google.protobuf.StringValue.class,
com.google.protobuf.Timestamp.class,
com.google.protobuf.UInt32Value.class,
com.google.protobuf.UInt64Value.class));

private boolean isNonSerializableMessageValue(Object value) {
return value instanceof GeneratedMessageV3
&& !jsonSerializableMessages.contains(value.getClass());
}

private ProtoRestSerializer(TypeRegistry registry) {
this.registry = registry;
}
Expand All @@ -75,7 +105,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(MessageOrBuilder message, boolean numericEnum) {
try {
Printer printer = JsonFormat.printer().usingTypeRegistry(registry);
if (numericEnum) {
Expand Down Expand Up @@ -118,6 +148,18 @@ 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, Object fieldValue) {
for (Map.Entry<FieldDescriptor, Object> fieldEntry :
((GeneratedMessageV3) fieldValue).getAllFields().entrySet()) {
Copy link
Member

Choose a reason for hiding this comment

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

This cast is only safe due to an instanceof check that occurs elsewhere in the logic. Will the next maintainer know they must call isNonSerializableMessageValue() as a check before this method?

I would highly suggest either performing the check explicitly in this method, or changing the parameter from Object to GeneratedMessageV3, thus forcing the caller of this method to perform the cast.

In general, the type-check (instanceof) and type-assertion (cast) should always be colocated - as two actions are tightly coupled and will need to change together if one is modified.

Object value = fieldEntry.getValue();
putQueryParam(
Copy link
Contributor

Choose a reason for hiding this comment

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

This recursive logic seems complicated and could run into StackOverFlow issue if not careful, also doing all this during runtime could affect the performance for client libraries as well. So now I really think that we should do this traversing all leaf level fields part in the generator instead of gax.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good

fields,
String.format("%s.%s", fieldName, fieldEntry.getKey().toProto().getName()),
fieldEntry.getValue());
}
}

/**
* Puts a message field in {@code fields} map which will be used to populate query parameters of a
* request.
Expand All @@ -127,16 +169,34 @@ 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();
ArrayList<String> paramValueList = new ArrayList();
Copy link
Member

Choose a reason for hiding this comment

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

This ArrayList is a raw-type (so, please at minimum change to use the diamond operator new ArrayList<>()) and is changing the contract of this public function to modify the fields output collection to contain a mutable data structure rather than an immutable one. Is this intended, or do we want to keep this immutable?

if (fieldValue instanceof List<?>) {
boolean hasProcessedMessage = false;
for (Object fieldValueItem : (List<?>) fieldValue) {
paramValueList.add(String.valueOf(fieldValueItem));
if (isNonSerializableMessageValue(fieldValueItem)) {
putDecomposedMessageQueryParam(fields, fieldName, fieldValueItem);
hasProcessedMessage = true;
} else {
paramValueList.add(toQueryParamValue(fieldValueItem));
}
}
if (hasProcessedMessage) {
Copy link
Member

Choose a reason for hiding this comment

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

Rather than introducing this variable and a secondary exit point for the function, consider instead just performing a !paramValueList.isEmpty() check at the end to decide whether to add to the final fields map or not.

return;
}
} else {
paramValueList.add(String.valueOf(fieldValue));
if (isNonSerializableMessageValue(fieldValue)) {
putDecomposedMessageQueryParam(fields, fieldName, fieldValue);
return;
} 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.

paramValueList.add(toQueryParamValue(fieldValue));
}
}

fields.put(fieldName, paramValueList.build());
if (fields.containsKey(fieldName)) {
fields.get(fieldName).addAll(paramValueList);
} else {
fields.put(fieldName, paramValueList);
}
}

/**
Expand All @@ -159,4 +219,23 @@ public String toBody(String fieldName, RequestT fieldValue) {
public String toBody(String fieldName, RequestT fieldValue, boolean numericEnum) {
return toJson(fieldValue, numericEnum);
}

/**
* Serializes an object to a query parameter Handles the case of a message such as Duration,
* FieldMask or Int32Value to prevent wrong formatting that String.valueOf() would make
*
* @param fieldValue a field value to serialize
*/
public String toQueryParamValue(Object fieldValue) {
// This will match with message types that are serializable (e.g. FieldMask)
if (fieldValue instanceof GeneratedMessageV3 && !isNonSerializableMessageValue(fieldValue)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just use Message instead of GeneratedMessagev3 in all places since it's more generic. I also found that we could pass an Enum to query params, where Enum is not of Message type, so we may need to handle enum specifically as well.

return toJson(((GeneratedMessageV3) fieldValue).toBuilder(), false)
.replaceAll("^\"", "")
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks suspicious to me, any documentation mentioned that we need to do this manually? What would it looks like if we don't replace them?

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 did this to get the actual contents of a string value. For example, a FieldMask would serialize into "a.b.c,a.b", but this replace() will get a.b.c,a.b without quotes. I will see if there is a better way to ensure sanitization.

.replaceAll("\"$", "");
}
if (fieldValue instanceof ByteString) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need to handle ByteString separately here?

return ((ByteString) fieldValue).toStringUtf8();
}
return String.valueOf(fieldValue);
}
}
42 changes: 42 additions & 0 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,11 +31,21 @@
package com.google.api.gax.httpjson;

import com.google.common.truth.Truth;
import com.google.longrunning.Operation;
import com.google.protobuf.Any;
import com.google.protobuf.ByteString;
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.rpc.Status;
import java.io.IOException;
import java.io.StringReader;
import java.nio.charset.Charset;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
Expand Down Expand Up @@ -170,13 +180,45 @@ public void putQueryParam() {
requestSerializer.putQueryParam(fields, "optName3", "three");
requestSerializer.putQueryParam(fields, "optName4", "");
requestSerializer.putQueryParam(fields, "optName5", Arrays.asList("four", "five"));
requestSerializer.putQueryParam(
fields, "optName6", Duration.newBuilder().setSeconds(1).setNanos(1).build());
requestSerializer.putQueryParam(
fields, "optName7", Timestamp.newBuilder().setSeconds(1).setNanos(1).build());
requestSerializer.putQueryParam(
fields, "optName8", FieldMask.newBuilder().addPaths("a.b").addPaths("c.d").build());
requestSerializer.putQueryParam(fields, "optName9", Int32Value.of(1));
requestSerializer.putQueryParam(fields, "optName10", FloatValue.of(1.1f));
com.google.longrunning.Operation operation =
Operation.newBuilder()
.setDone(true)
.setError(
Status.newBuilder()
.addDetails(
Any.newBuilder()
.setValue(ByteString.copyFrom("error-1", Charset.defaultCharset()))
.build())
.addDetails(
Any.newBuilder()
.setValue(ByteString.copyFrom("error-2", Charset.defaultCharset()))
.build()))
.setName("test")
.build();
requestSerializer.putQueryParam(fields, "optName11", operation);

Map<String, List<String>> expectedFields = new HashMap<>();
expectedFields.put("optName1", Arrays.asList("1"));
expectedFields.put("optName2", Arrays.asList("0"));
expectedFields.put("optName3", Arrays.asList("three"));
expectedFields.put("optName4", Arrays.asList(""));
expectedFields.put("optName5", Arrays.asList("four", "five"));
expectedFields.put("optName6", Arrays.asList("1.000000001s"));
expectedFields.put("optName7", Arrays.asList("1970-01-01T00:00:01.000000001Z"));
expectedFields.put("optName8", Arrays.asList("a.b,c.d"));
expectedFields.put("optName9", Arrays.asList("1"));
expectedFields.put("optName10", Arrays.asList("1.1"));
expectedFields.put("optName11.name", Arrays.asList("test"));
expectedFields.put("optName11.done", Arrays.asList("true"));
expectedFields.put("optName11.error.details.value", Arrays.asList("error-1", "error-2"));

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