Skip to content

Commit

Permalink
feat: support Discovery "format" specifier for google.protobuf.{Value…
Browse files Browse the repository at this point in the history 
…,ListValue,Struct} (#131)

The converter will now recognize a `format` specifier in certain schemas. Specifically, the following combinations are now recognized and result in a protobuf field whose type is the specified `format`:

| Discovery `"type"` | Discovery `"format"`        |
|--------------------|-----------------------------|
| `object`           | `google.protobuf.Struct`    |
| `any`              | `google.protobuf.Value`     |
| `array`            | `google.protobuf.ListValue` |

Note that, as was the case previously, other schemas with `"type" = "any"` and any other `format`, including none, are not accepted unless they are in an `error.details` field. (Googlers,  see also cl/683528084.)
 
Incidentally, this PR also restores proto imports in the intended order, undoing the workaround in #108 due to protobufjs/protobuf.js#1954 now that the solution protobufjs/protobuf.js#1960 is implemented.
  • Loading branch information
@vchudnov-g
vchudnov-g authored Oct 10, 2024
1 parent 2091fbe commit 82aeac1
Show file tree
Hide file tree
Showing 9 changed files with 2,200 additions and 30 deletions.
7 changes: 6 additions & 1 deletion src/main/java/com/google/cloud/discotoproto3converter/disco/Schema.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,12 @@ public enum Format {
UINT32("uint32"),
UINT64("uint64"),
FIXED32("fixed32"),
FIXED64("fixed64");
FIXED64("fixed64"),
// standard protobuf types:
ANY("google.protobuf.Any"),
LISTVALUE("google.protobuf.ListValue"),
STRUCT("google.protobuf.Struct"),
VALUE("google.protobuf.Value");

private String text;

Expand Down
110 changes: 95 additions & 15 deletions src/main/java/com/google/cloud/discotoproto3converter/proto3/DocumentToProtoConverter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.google.cloud.discotoproto3converter.disco.Method;
import com.google.cloud.discotoproto3converter.disco.Name;
import com.google.cloud.discotoproto3converter.disco.Schema;
import com.google.cloud.discotoproto3converter.disco.Schema.Format;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
Expand All @@ -46,6 +47,7 @@ public class DocumentToProtoConverter {
private final String relativeLinkPrefix;
private final boolean enumsAsStrings;
private boolean schemaRead;
private boolean usesStructProto;

// Set this to "true" to get some tracing output on stderr during development. Leave this as
// "false" for production code.
Expand All @@ -66,11 +68,14 @@ public DocumentToProtoConverter(
this.relativeLinkPrefix = relativeLinkPrefix;
this.protoFile.setMetadata(readDocumentMetadata(document, documentFileName));
this.enumsAsStrings = enumsAsStrings;
this.usesStructProto = false;

readSchema(document);
readResources(document);
cleanupEnumNamingConflicts();
this.protoFile.setHasLroDefinitions(applyLroConfiguration());
this.protoFile.setHasAnyFields(checkAnyFields());
this.protoFile.setUsesStructProto(this.usesStructProto);
convertEnumFieldsToStrings();
}

Expand All @@ -91,7 +96,7 @@ private ProtoFileMetadata readDocumentMetadata(Document document, String documen

private void readSchema(Document document) {
for (Map.Entry<String, Schema> entry : document.schemas().entrySet()) {
schemaToField(entry.getValue(), true, "*** readSchema\n");
schemaToField(entry.getValue(), true, "readSchema()");
}
for (Message message : protoFile.getMessages().values()) {
resolveReferences(message);
Expand Down Expand Up @@ -527,27 +532,55 @@ private Field schemaToField(Schema sch, boolean optional, String debugPreviousPa
Message valueType = null;
boolean repeated = false;
Message keyType = null;
String debugCurrentPath =
debugPreviousPath + String.format("SCHEMA: %s\n%s\n----\n", name, description);
String debugCurrentPath = String.format("%s.%s", debugPreviousPath, name);

if (trace) {
System.err.printf("*** schemaToField: \n%s", debugCurrentPath);
System.err.printf("*** schemaToField: %s\n", debugCurrentPath);
}

switch (sch.type()) {
case ANY:
valueType = Message.PRIMITIVES.get("google.protobuf.Any");
switch (sch.format()) {
case VALUE:
valueType = Message.PRIMITIVES.get("google.protobuf.Value");
this.usesStructProto = true;
break;
case EMPTY:
// intentional fall-through
case ANY:
valueType = Message.PRIMITIVES.get("google.protobuf.Any");
break;
default:
throw new IllegalStateException(
String.format(
"unexpected 'format' value ('%s') when processing ANY type in schema %s",
sch.format().toString(), debugCurrentPath));
}
break;
case ARRAY:
repeated = true;
if (sch.format() == Format.LISTVALUE) {
valueType = Message.PRIMITIVES.get("google.protobuf.ListValue");
this.usesStructProto = true;
// Since the `google.prootbuf.ListValue` whence this schema was generated is JSON-encoded
// as an array (see https://protobuf.dev/programming-guides/proto3/#json), the Discovery
// file describes the JSON array items. However, since we want to encode this schema back
// as an opaque `google.protobuf.ListValue` (which has the`repeated` semantics embedded
// internally), we should not make this field `repeated`.
} else {
repeated = true;
}
break;
case BOOLEAN:
valueType = Message.PRIMITIVES.get("bool");
break;
case EMPTY:
// This handles schemas with an "$ref" field
valueType = new Message(sch.reference(), true, false, null);
break;
case INTEGER:
switch (sch.format()) {
case EMPTY:
// intentional fall-through: if there's no format, we default to `int32`.
case INT32:
valueType = Message.PRIMITIVES.get("int32");
break;
Expand All @@ -566,24 +599,52 @@ private Field schemaToField(Schema sch, boolean optional, String debugPreviousPa
case FIXED64:
valueType = Message.PRIMITIVES.get("fixed64");
break;
default:
throw new IllegalStateException(
String.format(
"unexpected 'format' value ('%s') when processing INTEGER type in schema %s",
sch.format().toString(), debugCurrentPath));
}
break;
case NUMBER:
switch (sch.format()) {
case EMPTY:
// intentional fall-through: if there's no format, we default to `float`.
case FLOAT:
valueType = Message.PRIMITIVES.get("float");
break;
case DOUBLE:
valueType = Message.PRIMITIVES.get("double");
break;
default:
throw new IllegalStateException(
String.format(
"unexpected 'format' value ('%s') when processing NUMBER type in schema %s",
sch.format().toString(), debugCurrentPath));
}
break;
case OBJECT:
if (sch.additionalProperties() != null) {
repeated = true;
keyType = Message.PRIMITIVES.get("string");
} else {
valueType = new Message(getMessageName(sch), false, false, sanitizeDescr(description));
switch (sch.format()) {
case STRUCT:
valueType = Message.PRIMITIVES.get("google.protobuf.Struct");
this.usesStructProto = true;
// `additionalProperties' in the schema further specifies the JSON format, but
// "google.protobuf.Struct" is enough for specifying the proto message field type.
break;
case EMPTY:
if (sch.additionalProperties() != null) {
repeated = true;
keyType = Message.PRIMITIVES.get("string");
} else {
valueType =
new Message(getMessageName(sch), false, false, sanitizeDescr(description));
}
break;
default:
throw new IllegalStateException(
String.format(
"unexpected 'format' value ('%s') when processing OBJECT type in schema %s",
sch.format().toString(), debugCurrentPath));
}
break;
case STRING:
Expand All @@ -602,9 +663,26 @@ private Field schemaToField(Schema sch, boolean optional, String debugPreviousPa
case FIXED64:
valueType = Message.PRIMITIVES.get("fixed64");
break;
default:
case FLOAT:
valueType = Message.PRIMITIVES.get("float");
break;
case DOUBLE:
valueType = Message.PRIMITIVES.get("double");
break;
case BYTE:
// intentional fall-through for backwards compatibility. Ideally, we'd make this refer
// to the protobuf primitive type `byte`.
//
// TODO: use `byte` for new messages.
case EMPTY:
// If there's no format, we default to `string`.
valueType = Message.PRIMITIVES.get("string");
break;
default:
throw new IllegalStateException(
String.format(
"unexpected 'format' value ('%s') when processing STRING type in schema %s",
sch.format().toString(), debugCurrentPath));
}
}
break;
Expand All @@ -613,7 +691,9 @@ private Field schemaToField(Schema sch, boolean optional, String debugPreviousPa
if (repeated) {
Field subField =
schemaToField(
keyType == null ? sch.items() : sch.additionalProperties(), true, debugCurrentPath);
keyType == null ? sch.items() /* array */ : sch.additionalProperties() /* object */,
true,
debugCurrentPath);
valueType = subField.getValueType();
}

Expand Down Expand Up @@ -771,7 +851,7 @@ private void readResources(Document document) {

for (Schema pathParam : method.pathParams().values()) {
boolean required = methodSignatureParamNames.containsKey(pathParam.getIdentifier());
Field pathField = schemaToField(pathParam, !required, "readResources(A):) ");
Field pathField = schemaToField(pathParam, !required, "readResources(A)");
if (required) {
Option opt = createOption("google.api.field_behavior", ProtoOptionValues.REQUIRED);
pathField.getOptions().add(opt);
Expand All @@ -785,7 +865,7 @@ private void readResources(Document document) {

for (Schema queryParam : method.queryParams().values()) {
boolean required = methodSignatureParamNames.containsKey(queryParam.getIdentifier());
Field queryField = schemaToField(queryParam, !required, "readResources(B): ");
Field queryField = schemaToField(queryParam, !required, "readResources(B)");
if (required) {
Option opt = createOption("google.api.field_behavior", ProtoOptionValues.REQUIRED);
queryField.getOptions().add(opt);
Expand Down
13 changes: 9 additions & 4 deletions src/main/java/com/google/cloud/discotoproto3converter/proto3/Message.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,17 @@ public class Message extends ProtoElement<Message> {
PRIMITIVES.put("double", new Message("double", false, false, null));
PRIMITIVES.put("", new Message("", false, true, null));

// This isn't technically a primitive, but it is a fundamental well-known-type with no a priori
// structure.
// These aren't technically primitives, but they are opaque types we treat as such, essentially.
//
// TODO: If we start accepting additional well-known types, create a specific data structure for
// those rather than overloading "PRIMITIVES".
// TODO: Consider renaming this field more accurately, or creating a parallel field for these
// types.
PRIMITIVES.put("google.protobuf.Any", new Message("google.protobuf.Any", false, false, null));
PRIMITIVES.put(
"google.protobuf.Value", new Message("google.protobuf.Value", false, false, null));
PRIMITIVES.put(
"google.protobuf.ListValue", new Message("google.protobuf.ListValue", false, false, null));
PRIMITIVES.put(
"google.protobuf.Struct", new Message("google.protobuf.Struct", false, false, null));
}

private final SortedSet<Field> fields = new TreeSet<>();
Expand Down
15 changes: 8 additions & 7 deletions src/main/java/com/google/cloud/discotoproto3converter/proto3/Proto3Writer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,6 @@ public void writeToFile(PrintWriter writer, ProtoFile protoFile, boolean outputC

writer.println("package " + metadata.getProtoPkg() + ";\n");

// TODO: Place this import in the right alphabetical order. We are placing it here for now to
// work around an apparent bug in protobuf.js, where having this particular import be the last
// one makes the file not actually be imported.
if (protoFile.HasAnyFields()) {
writer.println("import \"google/protobuf/any.proto\";");
}

writer.println("import \"google/api/annotations.proto\";");
writer.println("import \"google/api/client.proto\";");
writer.println("import \"google/api/field_behavior.proto\";");
Expand All @@ -60,6 +53,14 @@ public void writeToFile(PrintWriter writer, ProtoFile protoFile, boolean outputC
if (protoFile.isHasLroDefinitions()) {
writer.println("import \"google/cloud/extended_operations.proto\";");
}

if (protoFile.HasAnyFields()) {
writer.println("import \"google/protobuf/any.proto\";");
}
if (protoFile.UsesStructProto()) {
writer.println("import \"google/protobuf/struct.proto\";");
}

writer.println();

// File Options
Expand Down
9 changes: 9 additions & 0 deletions src/main/java/com/google/cloud/discotoproto3converter/proto3/ProtoFile.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ public class ProtoFile {
private final Map<String, GrpcService> services = new TreeMap<>();
private boolean hasLroDefinitions;
private boolean hasAnyFields;
private boolean usesStructProto;

public ProtoFileMetadata getMetadata() {
return metadata;
Expand Down Expand Up @@ -56,4 +57,12 @@ public boolean HasAnyFields() {
public void setHasAnyFields(boolean hasAnyFields) {
this.hasAnyFields = hasAnyFields;
}

public boolean UsesStructProto() {
return usesStructProto;
}

public void setUsesStructProto(boolean usesStructProto) {
this.usesStructProto = usesStructProto;
}
}
45 changes: 43 additions & 2 deletions src/test/java/com/google/cloud/discotoproto3converter/DiscoToProto3ConverterAppTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ public void convertVersioned() throws IOException {
"src", "test", "resources", prefix.toString(), "compute-versioned.proto.baseline");
String baselineBody = readFile(baselineFilePath);
System.out.printf(
"*** @Test:convertVersioned():\n*** Discovery path: %s\n*** Generated file: %s\n*** Baseline file: %s\n",
"*** @Test:convertVersioned():\n Discovery path: %s\n Generated file: %s\n Baseline file: %s\n",
discoveryDocPath.toAbsolutePath(),
generatedFilePath.toAbsolutePath(),
baselineFilePath.toAbsolutePath());
Expand Down Expand Up @@ -203,7 +203,7 @@ public void convertVersionedTwoServices() throws IOException {
"compute-versioned-two-services.proto.baseline");
String baselineBody = readFile(baselineFilePath);
System.out.printf(
"*** @Test:convertVersionedTwoServices():\n*** Discovery path: %s\n*** Generated file: %s\n*** Baseline file: %s\n",
"*** @Test:convertVersionedTwoServices():\n Discovery path: %s\n Generated file: %s\n Baseline file: %s\n",
discoveryDocPath.toAbsolutePath(),
generatedFilePath.toAbsolutePath(),
baselineFilePath.toAbsolutePath());
Expand Down Expand Up @@ -392,6 +392,13 @@ public void convertAnyFieldInError() throws IOException {
Paths.get(
"src", "test", "resources", prefix.toString(), "compute.error-any.proto.baseline");
String baselineBody = readFile(baselineFilePath);

System.out.printf(
"*** @Test:convertAnyFieldInError():\n Discovery path: %s\n Generated file: %s\n Baseline file: %s\n",
discoveryDocPath.toAbsolutePath(),
generatedFilePath.toAbsolutePath(),
baselineFilePath.toAbsolutePath());

assertEquals(baselineBody, actualBody);
}

Expand Down Expand Up @@ -523,6 +530,40 @@ public void convertWithMerge() throws IOException {
assertEquals(baselineBody, actualBody);
}

@Test
public void convertAnyFieldWithFormat() throws IOException {
DiscoToProto3ConverterApp app = new DiscoToProto3ConverterApp();
Path prefix = Paths.get("google", "cloud", "compute", "v1small");
Path discoveryDocPath =
Paths.get("src", "test", "resources", prefix.toString(), "compute.v1small.any-format.json");
Path generatedFilePath =
Paths.get(outputDir.toString(), prefix.toString(), "compute.any-format.proto");

app.convert(
discoveryDocPath.toString(),
null,
generatedFilePath.toString(),
"",
"",
"https://cloud.google.com",
"true",
"true");

String actualBody = readFile(generatedFilePath);
Path baselineFilePath =
Paths.get(
"src", "test", "resources", prefix.toString(), "compute.any-format.proto.baseline");
String baselineBody = readFile(baselineFilePath);

System.out.printf(
"*** @Test:convertAnyFieldWithFormat():\n Discovery path: %s\n Generated file: %s\n Baseline file: %s\n",
discoveryDocPath.toAbsolutePath(),
generatedFilePath.toAbsolutePath(),
baselineFilePath.toAbsolutePath());

assertEquals(baselineBody, actualBody);
}

private static String readFile(Path path) throws IOException {
return new String(Files.readAllBytes(path), StandardCharsets.UTF_8);
}
Expand Down
Loading

0 comments on commit 82aeac1

Please sign in to comment.