From 41dc06fd528e63e8cc17294e3e849b2f0f36e7d7 Mon Sep 17 00:00:00 2001 From: Jun Kim Date: Tue, 11 Mar 2025 14:21:17 +0900 Subject: [PATCH 01/15] Add optional info for google.proto types --- .../server/grpc/GrpcDocServicePlugin.java | 9 +++-- .../grpc/HttpEndpointSpecification.java | 8 ++++- .../grpc/HttpJsonTranscodingService.java | 34 ++++++++++++++++--- 3 files changed, 43 insertions(+), 8 deletions(-) diff --git a/grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePlugin.java b/grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePlugin.java index 88ee2331f33..8763257acec 100644 --- a/grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePlugin.java +++ b/grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePlugin.java @@ -295,8 +295,13 @@ private static ServiceInfo buildHttpServiceInfo(String serviceName, builder = FieldInfo.builder(paramName, toTypeSignature(parameter)); } - fieldInfosBuilder.add(builder.requirement(FieldRequirement.REQUIRED) - .location(fieldLocation) + if (parameter.isOptional() || parameter.isRepeated()) { + builder.requirement(FieldRequirement.OPTIONAL); + } else { + builder.requirement(FieldRequirement.REQUIRED); + } + + fieldInfosBuilder.add(builder.location(fieldLocation) .build()); } }); diff --git a/grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/HttpEndpointSpecification.java b/grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/HttpEndpointSpecification.java index 06ffb286f53..56737d03e44 100644 --- a/grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/HttpEndpointSpecification.java +++ b/grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/HttpEndpointSpecification.java @@ -180,10 +180,12 @@ public String toString() { public static class Parameter { private final JavaType type; private final boolean isRepeated; + private final boolean isOptional; - public Parameter(JavaType type, boolean isRepeated) { + public Parameter(JavaType type, boolean isRepeated, boolean isOptional) { this.type = requireNonNull(type, "type"); this.isRepeated = isRepeated; + this.isOptional = isOptional; } public JavaType type() { @@ -194,6 +196,10 @@ public boolean isRepeated() { return isRepeated; } + public boolean isOptional() { + return isOptional; + } + @Override public boolean equals(Object o) { if (this == o) { diff --git a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java index 107f5aa0b44..c1b9000433c 100644 --- a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java +++ b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java @@ -368,13 +368,14 @@ private static Map buildFields(Descriptor desc, case BYTE_STRING: case ENUM: // Use field name which is specified in proto file. - builder.put(key, new Field(field, parentNames, field.getJavaType())); + builder.put(key, new Field(field, parentNames, field.getJavaType(), false)); break; case MESSAGE: @Nullable final JavaType wellKnownFieldType = getJavaTypeForWellKnownTypes(field); + final boolean isOptional = isOptionalFieldType(field); if (wellKnownFieldType != null) { - builder.put(key, new Field(field, parentNames, wellKnownFieldType)); + builder.put(key, new Field(field, parentNames, wellKnownFieldType, isOptional)); break; } @@ -408,7 +409,7 @@ private static Map buildFields(Descriptor desc, throw e; } - builder.put(key, new Field(field, parentNames, JavaType.MESSAGE)); + builder.put(key, new Field(field, parentNames, JavaType.MESSAGE, isOptional)); } break; } @@ -418,6 +419,22 @@ private static Map buildFields(Descriptor desc, return builder.buildKeepingLast(); } + private static boolean isOptionalFieldType(FieldDescriptor field) { + final String fullName = field.getFullName(); + if (DoubleValue.getDescriptor().getFullName().equals(fullName) || + FloatValue.getDescriptor().getFullName().equals(fullName) || + Int64Value.getDescriptor().getFullName().equals(fullName) || + UInt64Value.getDescriptor().getFullName().equals(fullName) || + Int32Value.getDescriptor().getFullName().equals(fullName) || + UInt32Value.getDescriptor().getFullName().equals(fullName) || + BoolValue.getDescriptor().getFullName().equals(fullName) || + StringValue.getDescriptor().getFullName().equals(fullName) || + BytesValue.getDescriptor().getFullName().equals(fullName)) { + return true; + } + return false; + } + @Nullable private static JavaType getJavaTypeForWellKnownTypes(FieldDescriptor fd) { // MapField can be sent only via HTTP body. @@ -606,7 +623,8 @@ public HttpEndpointSpecification httpEndpointSpecification(Route route) { spec.originalFields.entrySet().stream().collect( toImmutableMap(Entry::getKey, fieldEntry -> new Parameter(fieldEntry.getValue().type(), - fieldEntry.getValue().isRepeated()))); + fieldEntry.getValue().isRepeated(), + fieldEntry.getValue().isOptional()))); return new HttpEndpointSpecification(spec.order, route, paramNames, @@ -981,11 +999,13 @@ static final class Field { private final FieldDescriptor descriptor; private final List parentNames; private final JavaType javaType; + private final boolean isOptional; - private Field(FieldDescriptor descriptor, List parentNames, JavaType javaType) { + private Field(FieldDescriptor descriptor, List parentNames, JavaType javaType, boolean isOptional) { this.descriptor = descriptor; this.parentNames = parentNames; this.javaType = javaType; + this.isOptional = isOptional; } JavaType type() { @@ -999,6 +1019,10 @@ String name() { boolean isRepeated() { return descriptor.isRepeated(); } + + boolean isOptional() { + return isOptional; + } } /** From c863988eba4238c96051a239de96fca7ae0e49f5 Mon Sep 17 00:00:00 2001 From: Jun Kim Date: Tue, 11 Mar 2025 16:14:44 +0900 Subject: [PATCH 02/15] fix lint error --- .../armeria/server/grpc/HttpJsonTranscodingService.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java index c1b9000433c..67d5286884c 100644 --- a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java +++ b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java @@ -1001,7 +1001,11 @@ static final class Field { private final JavaType javaType; private final boolean isOptional; - private Field(FieldDescriptor descriptor, List parentNames, JavaType javaType, boolean isOptional) { + private Field(FieldDescriptor descriptor, + List parentNames, + JavaType javaType, + boolean isOptional + ) { this.descriptor = descriptor; this.parentNames = parentNames; this.javaType = javaType; From 22bbc0746192a5d8bfdf73849dcd962cdd70ac0c Mon Sep 17 00:00:00 2001 From: Jun Kim Date: Tue, 11 Mar 2025 16:45:40 +0900 Subject: [PATCH 03/15] fix test --- .../armeria/internal/server/grpc/GrpcDocServicePluginTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/grpc/src/test/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePluginTest.java b/grpc/src/test/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePluginTest.java index 061c7b7e34a..2d64175a907 100644 --- a/grpc/src/test/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePluginTest.java +++ b/grpc/src/test/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePluginTest.java @@ -422,7 +422,7 @@ void httpEndpoint() { .location(FieldLocation.PATH).requirement(FieldRequirement.REQUIRED).build(), FieldInfo.builder("revision", TypeSignature.ofList(TypeSignature.ofBase(JavaType.LONG.name()))) - .location(FieldLocation.QUERY).requirement(FieldRequirement.REQUIRED).build())); + .location(FieldLocation.QUERY).requirement(FieldRequirement.OPTIONAL).build())); assertThat(getMessageV3.useParameterAsRoot()).isFalse(); // Check HTTP PATCH method. From 68fb08c0a102c47d3b4cc57fe4e055115ee6c755 Mon Sep 17 00:00:00 2001 From: Jun Kim Date: Tue, 11 Mar 2025 21:21:07 +0900 Subject: [PATCH 04/15] change name of function --- .../grpc/HttpEndpointSpecification.java | 2 +- .../grpc/HttpJsonTranscodingService.java | 41 +++++++------------ 2 files changed, 16 insertions(+), 27 deletions(-) diff --git a/grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/HttpEndpointSpecification.java b/grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/HttpEndpointSpecification.java index 56737d03e44..74bc63061dc 100644 --- a/grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/HttpEndpointSpecification.java +++ b/grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/HttpEndpointSpecification.java @@ -185,7 +185,7 @@ public static class Parameter { public Parameter(JavaType type, boolean isRepeated, boolean isOptional) { this.type = requireNonNull(type, "type"); this.isRepeated = isRepeated; - this.isOptional = isOptional; + this.isOptional = isOptional || isRepeated; } public JavaType type() { diff --git a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java index 67d5286884c..365f934e9bc 100644 --- a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java +++ b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java @@ -373,7 +373,7 @@ private static Map buildFields(Descriptor desc, case MESSAGE: @Nullable final JavaType wellKnownFieldType = getJavaTypeForWellKnownTypes(field); - final boolean isOptional = isOptionalFieldType(field); + final boolean isOptional = isScalarValueWrapperMessage(field); if (wellKnownFieldType != null) { builder.put(key, new Field(field, parentNames, wellKnownFieldType, isOptional)); break; @@ -419,22 +419,6 @@ private static Map buildFields(Descriptor desc, return builder.buildKeepingLast(); } - private static boolean isOptionalFieldType(FieldDescriptor field) { - final String fullName = field.getFullName(); - if (DoubleValue.getDescriptor().getFullName().equals(fullName) || - FloatValue.getDescriptor().getFullName().equals(fullName) || - Int64Value.getDescriptor().getFullName().equals(fullName) || - UInt64Value.getDescriptor().getFullName().equals(fullName) || - Int32Value.getDescriptor().getFullName().equals(fullName) || - UInt32Value.getDescriptor().getFullName().equals(fullName) || - BoolValue.getDescriptor().getFullName().equals(fullName) || - StringValue.getDescriptor().getFullName().equals(fullName) || - BytesValue.getDescriptor().getFullName().equals(fullName)) { - return true; - } - return false; - } - @Nullable private static JavaType getJavaTypeForWellKnownTypes(FieldDescriptor fd) { // MapField can be sent only via HTTP body. @@ -451,15 +435,7 @@ private static JavaType getJavaTypeForWellKnownTypes(FieldDescriptor fd) { return JavaType.STRING; } - if (DoubleValue.getDescriptor().getFullName().equals(fullName) || - FloatValue.getDescriptor().getFullName().equals(fullName) || - Int64Value.getDescriptor().getFullName().equals(fullName) || - UInt64Value.getDescriptor().getFullName().equals(fullName) || - Int32Value.getDescriptor().getFullName().equals(fullName) || - UInt32Value.getDescriptor().getFullName().equals(fullName) || - BoolValue.getDescriptor().getFullName().equals(fullName) || - StringValue.getDescriptor().getFullName().equals(fullName) || - BytesValue.getDescriptor().getFullName().equals(fullName)) { + if (isScalarValueWrapperMessage(fd)) { // "value" field. Wrappers must have one field. assert messageType.getFields().size() == 1 : "Wrappers must have one 'value' field."; return messageType.getFields().get(0).getJavaType(); @@ -486,6 +462,19 @@ private static JavaType getJavaTypeForWellKnownTypes(FieldDescriptor fd) { return null; } + private static boolean isScalarValueWrapperMessage(FieldDescriptor field) { + final String fullName = field.getFullName(); + return DoubleValue.getDescriptor().getFullName().equals(fullName) || + FloatValue.getDescriptor().getFullName().equals(fullName) || + Int64Value.getDescriptor().getFullName().equals(fullName) || + UInt64Value.getDescriptor().getFullName().equals(fullName) || + Int32Value.getDescriptor().getFullName().equals(fullName) || + UInt32Value.getDescriptor().getFullName().equals(fullName) || + BoolValue.getDescriptor().getFullName().equals(fullName) || + StringValue.getDescriptor().getFullName().equals(fullName) || + BytesValue.getDescriptor().getFullName().equals(fullName); + } + // to make it more efficient, we calculate whether extract response body one time // if there is no matching toplevel field, we set it to null @Nullable From 32fa8348ea899d195f7a98586015c92e3cc0e1ff Mon Sep 17 00:00:00 2001 From: Jun Kim Date: Tue, 11 Mar 2025 21:32:05 +0900 Subject: [PATCH 05/15] add optional condition --- .../armeria/server/grpc/HttpJsonTranscodingService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java index 365f934e9bc..a3bab7f5f00 100644 --- a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java +++ b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java @@ -373,7 +373,7 @@ private static Map buildFields(Descriptor desc, case MESSAGE: @Nullable final JavaType wellKnownFieldType = getJavaTypeForWellKnownTypes(field); - final boolean isOptional = isScalarValueWrapperMessage(field); + final boolean isOptional = isScalarValueWrapperMessage(field) || field.isOptional(); if (wellKnownFieldType != null) { builder.put(key, new Field(field, parentNames, wellKnownFieldType, isOptional)); break; From 63924efa56db67d9eabe85f703d2363186ad2b8e Mon Sep 17 00:00:00 2001 From: Jun Kim Date: Tue, 11 Mar 2025 22:56:11 +0900 Subject: [PATCH 06/15] feat: change to check required behavior --- .../server/grpc/GrpcDocServicePlugin.java | 7 ++-- .../grpc/HttpEndpointSpecification.java | 10 +++--- .../grpc/HttpJsonTranscodingService.java | 35 ++++++++++++------- .../server/grpc/GrpcDocServicePluginTest.java | 2 +- 4 files changed, 30 insertions(+), 24 deletions(-) diff --git a/grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePlugin.java b/grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePlugin.java index 8763257acec..2ed9a722516 100644 --- a/grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePlugin.java +++ b/grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePlugin.java @@ -295,11 +295,8 @@ private static ServiceInfo buildHttpServiceInfo(String serviceName, builder = FieldInfo.builder(paramName, toTypeSignature(parameter)); } - if (parameter.isOptional() || parameter.isRepeated()) { - builder.requirement(FieldRequirement.OPTIONAL); - } else { - builder.requirement(FieldRequirement.REQUIRED); - } + builder.requirement(parameter.isRequired() ? FieldRequirement.REQUIRED + : FieldRequirement.OPTIONAL); fieldInfosBuilder.add(builder.location(fieldLocation) .build()); diff --git a/grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/HttpEndpointSpecification.java b/grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/HttpEndpointSpecification.java index 74bc63061dc..5c2031f18e0 100644 --- a/grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/HttpEndpointSpecification.java +++ b/grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/HttpEndpointSpecification.java @@ -180,12 +180,12 @@ public String toString() { public static class Parameter { private final JavaType type; private final boolean isRepeated; - private final boolean isOptional; + private final boolean isRequired; - public Parameter(JavaType type, boolean isRepeated, boolean isOptional) { + public Parameter(JavaType type, boolean isRepeated, boolean isRequired) { this.type = requireNonNull(type, "type"); this.isRepeated = isRepeated; - this.isOptional = isOptional || isRepeated; + this.isRequired = isRequired; } public JavaType type() { @@ -196,8 +196,8 @@ public boolean isRepeated() { return isRepeated; } - public boolean isOptional() { - return isOptional; + public boolean isRequired() { + return isRequired; } @Override diff --git a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java index a3bab7f5f00..45cf01598b4 100644 --- a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java +++ b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java @@ -36,6 +36,7 @@ import java.util.function.Function; import java.util.stream.Collectors; +import com.google.api.*; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -44,9 +45,6 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.node.ArrayNode; import com.fasterxml.jackson.databind.node.ObjectNode; -import com.google.api.AnnotationsProto; -import com.google.api.HttpBody; -import com.google.api.HttpRule; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.CaseFormat; import com.google.common.base.MoreObjects; @@ -368,14 +366,15 @@ private static Map buildFields(Descriptor desc, case BYTE_STRING: case ENUM: // Use field name which is specified in proto file. - builder.put(key, new Field(field, parentNames, field.getJavaType(), false)); + builder.put(key, new Field(field, parentNames, field.getJavaType(), true)); break; case MESSAGE: @Nullable final JavaType wellKnownFieldType = getJavaTypeForWellKnownTypes(field); - final boolean isOptional = isScalarValueWrapperMessage(field) || field.isOptional(); + final boolean isRequired = hasRequiredFieldBehavior(field); + if (wellKnownFieldType != null) { - builder.put(key, new Field(field, parentNames, wellKnownFieldType, isOptional)); + builder.put(key, new Field(field, parentNames, wellKnownFieldType, isRequired)); break; } @@ -409,7 +408,7 @@ private static Map buildFields(Descriptor desc, throw e; } - builder.put(key, new Field(field, parentNames, JavaType.MESSAGE, isOptional)); + builder.put(key, new Field(field, parentNames, JavaType.MESSAGE, isRequired)); } break; } @@ -419,6 +418,16 @@ private static Map buildFields(Descriptor desc, return builder.buildKeepingLast(); } + private static boolean hasRequiredFieldBehavior(FieldDescriptor field) { + if (!field.isRepeated()) { + List fieldBehaviors = field + .getOptions() + .getExtension(FieldBehaviorProto.fieldBehavior); + return fieldBehaviors.contains(FieldBehavior.REQUIRED); + } + return false; + } + @Nullable private static JavaType getJavaTypeForWellKnownTypes(FieldDescriptor fd) { // MapField can be sent only via HTTP body. @@ -613,7 +622,7 @@ public HttpEndpointSpecification httpEndpointSpecification(Route route) { toImmutableMap(Entry::getKey, fieldEntry -> new Parameter(fieldEntry.getValue().type(), fieldEntry.getValue().isRepeated(), - fieldEntry.getValue().isOptional()))); + fieldEntry.getValue().isRequired()))); return new HttpEndpointSpecification(spec.order, route, paramNames, @@ -988,17 +997,17 @@ static final class Field { private final FieldDescriptor descriptor; private final List parentNames; private final JavaType javaType; - private final boolean isOptional; + private final boolean isRequired; private Field(FieldDescriptor descriptor, List parentNames, JavaType javaType, - boolean isOptional + boolean isRequired ) { this.descriptor = descriptor; this.parentNames = parentNames; this.javaType = javaType; - this.isOptional = isOptional; + this.isRequired = isRequired; } JavaType type() { @@ -1013,8 +1022,8 @@ boolean isRepeated() { return descriptor.isRepeated(); } - boolean isOptional() { - return isOptional; + boolean isRequired() { + return isRequired; } } diff --git a/grpc/src/test/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePluginTest.java b/grpc/src/test/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePluginTest.java index 2d64175a907..061c7b7e34a 100644 --- a/grpc/src/test/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePluginTest.java +++ b/grpc/src/test/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePluginTest.java @@ -422,7 +422,7 @@ void httpEndpoint() { .location(FieldLocation.PATH).requirement(FieldRequirement.REQUIRED).build(), FieldInfo.builder("revision", TypeSignature.ofList(TypeSignature.ofBase(JavaType.LONG.name()))) - .location(FieldLocation.QUERY).requirement(FieldRequirement.OPTIONAL).build())); + .location(FieldLocation.QUERY).requirement(FieldRequirement.REQUIRED).build())); assertThat(getMessageV3.useParameterAsRoot()).isFalse(); // Check HTTP PATCH method. From 80f212b164a694d3cbf9152313fe2a41da36d28a Mon Sep 17 00:00:00 2001 From: Jun Kim Date: Tue, 11 Mar 2025 23:07:44 +0900 Subject: [PATCH 07/15] fix: lint --- .../server/grpc/HttpJsonTranscodingService.java | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java index 45cf01598b4..e4953d8cd28 100644 --- a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java +++ b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java @@ -36,7 +36,6 @@ import java.util.function.Function; import java.util.stream.Collectors; -import com.google.api.*; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -45,6 +44,11 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.node.ArrayNode; import com.fasterxml.jackson.databind.node.ObjectNode; +import com.google.api.AnnotationsProto; +import com.google.api.FieldBehavior; +import com.google.api.FieldBehaviorProto; +import com.google.api.HttpBody; +import com.google.api.HttpRule; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.CaseFormat; import com.google.common.base.MoreObjects; @@ -156,7 +160,7 @@ static GrpcService of(GrpcService delegate, HttpJsonTranscodingOptions httpJsonT } final HttpRule httpRule = methodOptions.getExtension( - (ExtensionLite)AnnotationsProto.http + (ExtensionLite) AnnotationsProto.http ); if (methodDefinition.getMethodDescriptor().getType() != MethodType.UNARY) { @@ -420,7 +424,7 @@ private static Map buildFields(Descriptor desc, private static boolean hasRequiredFieldBehavior(FieldDescriptor field) { if (!field.isRepeated()) { - List fieldBehaviors = field + final List fieldBehaviors = field .getOptions() .getExtension(FieldBehaviorProto.fieldBehavior); return fieldBehaviors.contains(FieldBehavior.REQUIRED); From da2d4f2bdb4763b17a5c63e59c307714cd3953a5 Mon Sep 17 00:00:00 2001 From: Jun Kim Date: Wed, 12 Mar 2025 00:02:34 +0900 Subject: [PATCH 08/15] fix: test --- .../armeria/server/grpc/HttpJsonTranscodingService.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java index e4953d8cd28..83b5155c39d 100644 --- a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java +++ b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java @@ -448,7 +448,7 @@ private static JavaType getJavaTypeForWellKnownTypes(FieldDescriptor fd) { return JavaType.STRING; } - if (isScalarValueWrapperMessage(fd)) { + if (isScalarValueWrapperMessage(fullName)) { // "value" field. Wrappers must have one field. assert messageType.getFields().size() == 1 : "Wrappers must have one 'value' field."; return messageType.getFields().get(0).getJavaType(); @@ -475,8 +475,7 @@ private static JavaType getJavaTypeForWellKnownTypes(FieldDescriptor fd) { return null; } - private static boolean isScalarValueWrapperMessage(FieldDescriptor field) { - final String fullName = field.getFullName(); + private static boolean isScalarValueWrapperMessage(String fullName) { return DoubleValue.getDescriptor().getFullName().equals(fullName) || FloatValue.getDescriptor().getFullName().equals(fullName) || Int64Value.getDescriptor().getFullName().equals(fullName) || From dd4cc09d352ddab58aa378773ab91a8a6b185fe2 Mon Sep 17 00:00:00 2001 From: Jun Kim Date: Wed, 12 Mar 2025 07:11:27 +0900 Subject: [PATCH 09/15] fix: noSuchMethod Error --- .../armeria/server/grpc/HttpJsonTranscodingService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java index 83b5155c39d..e7e35f9ae0e 100644 --- a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java +++ b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java @@ -423,7 +423,7 @@ private static Map buildFields(Descriptor desc, } private static boolean hasRequiredFieldBehavior(FieldDescriptor field) { - if (!field.isRepeated()) { + if (!field.isRepeated() && field.getOptions().hasExtension(FieldBehaviorProto.fieldBehavior)) { final List fieldBehaviors = field .getOptions() .getExtension(FieldBehaviorProto.fieldBehavior); From 4c997bbbdb87e2d4b8e79deb81692b8904164ae7 Mon Sep 17 00:00:00 2001 From: Jun Kim Date: Wed, 12 Mar 2025 11:15:11 +0900 Subject: [PATCH 10/15] fix: hasField error --- .../server/grpc/HttpJsonTranscodingService.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java index e7e35f9ae0e..b54022eded0 100644 --- a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java +++ b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java @@ -423,15 +423,15 @@ private static Map buildFields(Descriptor desc, } private static boolean hasRequiredFieldBehavior(FieldDescriptor field) { - if (!field.isRepeated() && field.getOptions().hasExtension(FieldBehaviorProto.fieldBehavior)) { - final List fieldBehaviors = field - .getOptions() - .getExtension(FieldBehaviorProto.fieldBehavior); - return fieldBehaviors.contains(FieldBehavior.REQUIRED); + if (field.isRepeated()) { + return false; } - return false; + + final List fieldBehaviors = field.getOptions().getExtension(FieldBehaviorProto.fieldBehavior); + return fieldBehaviors.contains(FieldBehavior.REQUIRED); } + @Nullable private static JavaType getJavaTypeForWellKnownTypes(FieldDescriptor fd) { // MapField can be sent only via HTTP body. From 7740c9e9b17f4fdeb5f28e5a4cdbff2ceae7f7c9 Mon Sep 17 00:00:00 2001 From: Jun Kim Date: Wed, 12 Mar 2025 11:23:09 +0900 Subject: [PATCH 11/15] fix: lint --- .../armeria/server/grpc/HttpJsonTranscodingService.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java index b54022eded0..ef7fad69d47 100644 --- a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java +++ b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java @@ -427,11 +427,12 @@ private static boolean hasRequiredFieldBehavior(FieldDescriptor field) { return false; } - final List fieldBehaviors = field.getOptions().getExtension(FieldBehaviorProto.fieldBehavior); + final List fieldBehaviors = field + .getOptions() + .getExtension(FieldBehaviorProto.fieldBehavior); return fieldBehaviors.contains(FieldBehavior.REQUIRED); } - @Nullable private static JavaType getJavaTypeForWellKnownTypes(FieldDescriptor fd) { // MapField can be sent only via HTTP body. From daa55b831bc92123d7e40110d7ad9dd1eb388fbb Mon Sep 17 00:00:00 2001 From: Jun Kim Date: Wed, 12 Mar 2025 13:58:29 +0900 Subject: [PATCH 12/15] fix: protobuf4 error --- .../server/grpc/HttpJsonTranscodingService.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java index ef7fad69d47..9f34c6c199a 100644 --- a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java +++ b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java @@ -427,10 +427,14 @@ private static boolean hasRequiredFieldBehavior(FieldDescriptor field) { return false; } - final List fieldBehaviors = field - .getOptions() - .getExtension(FieldBehaviorProto.fieldBehavior); - return fieldBehaviors.contains(FieldBehavior.REQUIRED); + try { + final List fieldBehaviors = field + .getOptions() + .getExtension(FieldBehaviorProto.fieldBehavior); + return fieldBehaviors.contains(FieldBehavior.REQUIRED); + } catch (NoSuchMethodError error) { + return false; + } } @Nullable From 6334464f29ba034f70690a4744da0644bdf5c5b9 Mon Sep 17 00:00:00 2001 From: Jun Kim Date: Wed, 12 Mar 2025 14:07:46 +0900 Subject: [PATCH 13/15] feat: add test case --- .../internal/server/grpc/GrpcDocServicePluginTest.java | 6 +++++- grpc/src/test/proto/testing/grpc/transcoding.proto | 3 +++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/grpc/src/test/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePluginTest.java b/grpc/src/test/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePluginTest.java index 061c7b7e34a..0a59f7590d5 100644 --- a/grpc/src/test/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePluginTest.java +++ b/grpc/src/test/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePluginTest.java @@ -451,7 +451,11 @@ void httpEndpoint() { FieldInfo.builder("message_id", TypeSignature.ofBase(JavaType.STRING.name())) .location(FieldLocation.PATH).requirement(FieldRequirement.REQUIRED).build(), FieldInfo.builder("text", TypeSignature.ofBase(JavaType.STRING.name())) - .location(FieldLocation.BODY).requirement(FieldRequirement.REQUIRED).build())); + .location(FieldLocation.BODY).requirement(FieldRequirement.REQUIRED).build(), + FieldInfo.builder("required_text", TypeSignature.ofBase(JavaType.STRING.name())) + .location(FieldLocation.BODY).requirement(FieldRequirement.REQUIRED).build(), + FieldInfo.builder("optional_text", TypeSignature.ofBase(JavaType.STRING.name())) + .location(FieldLocation.BODY).requirement(FieldRequirement.OPTIONAL).build())); assertThat(updateMessageV2.useParameterAsRoot()).isFalse(); } diff --git a/grpc/src/test/proto/testing/grpc/transcoding.proto b/grpc/src/test/proto/testing/grpc/transcoding.proto index a86572dafed..554c3ca9898 100644 --- a/grpc/src/test/proto/testing/grpc/transcoding.proto +++ b/grpc/src/test/proto/testing/grpc/transcoding.proto @@ -21,6 +21,7 @@ package armeria.grpc.testing; option java_package = "testing.grpc"; import "google/api/annotations.proto"; +import "google/api/field_behavior.proto"; import "google/protobuf/timestamp.proto"; import "google/protobuf/duration.proto"; import "google/protobuf/wrappers.proto"; @@ -292,6 +293,8 @@ message UpdateMessageRequestV1 { message Message { string text = 1; // The resource content. + google.protobuf.StringValue required_text = 2 [(google.api.field_behavior) = REQUIRED]; + google.protobuf.StringValue optional_text = 3; } enum MessageType { From ac1ec537a3d88810697bec493d4b8ca1c318890c Mon Sep 17 00:00:00 2001 From: Jun Kim Date: Wed, 12 Mar 2025 15:05:56 +0900 Subject: [PATCH 14/15] feat: adopt required check to other java types --- .../grpc/HttpJsonTranscodingService.java | 19 ++++++++----------- .../server/grpc/GrpcDocServicePluginTest.java | 12 ++++++------ 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java index 9f34c6c199a..e079eafaa4a 100644 --- a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java +++ b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java @@ -59,6 +59,7 @@ import com.google.protobuf.Any; import com.google.protobuf.BoolValue; import com.google.protobuf.BytesValue; +import com.google.protobuf.DescriptorProtos; import com.google.protobuf.DescriptorProtos.MethodOptions; import com.google.protobuf.Descriptors; import com.google.protobuf.Descriptors.Descriptor; @@ -330,7 +331,7 @@ private static Map buildFields(Descriptor desc, final ImmutableMap.Builder builder = ImmutableMap.builder(); for (FieldDescriptor field : desc.getFields()) { final JavaType type = field.getJavaType(); - + final boolean isRequired = hasRequiredFieldBehavior(field); final String fieldName; switch (currentMatchRule) { case ORIGINAL_FIELD: @@ -370,12 +371,11 @@ private static Map buildFields(Descriptor desc, case BYTE_STRING: case ENUM: // Use field name which is specified in proto file. - builder.put(key, new Field(field, parentNames, field.getJavaType(), true)); + builder.put(key, new Field(field, parentNames, field.getJavaType(), isRequired)); break; case MESSAGE: @Nullable final JavaType wellKnownFieldType = getJavaTypeForWellKnownTypes(field); - final boolean isRequired = hasRequiredFieldBehavior(field); if (wellKnownFieldType != null) { builder.put(key, new Field(field, parentNames, wellKnownFieldType, isRequired)); @@ -427,14 +427,11 @@ private static boolean hasRequiredFieldBehavior(FieldDescriptor field) { return false; } - try { - final List fieldBehaviors = field - .getOptions() - .getExtension(FieldBehaviorProto.fieldBehavior); - return fieldBehaviors.contains(FieldBehavior.REQUIRED); - } catch (NoSuchMethodError error) { - return false; - } + final List fieldBehaviors = field + .getOptions() + .getExtension((ExtensionLite>) + FieldBehaviorProto.fieldBehavior); + return fieldBehaviors.contains(FieldBehavior.REQUIRED); } @Nullable diff --git a/grpc/src/test/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePluginTest.java b/grpc/src/test/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePluginTest.java index 0a59f7590d5..fd9408109b6 100644 --- a/grpc/src/test/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePluginTest.java +++ b/grpc/src/test/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePluginTest.java @@ -403,11 +403,11 @@ void httpEndpoint() { FieldInfo.builder("message_id", TypeSignature.ofBase(JavaType.STRING.name())) .location(FieldLocation.PATH).requirement(FieldRequirement.REQUIRED).build(), FieldInfo.builder("revision", TypeSignature.ofBase(JavaType.LONG.name())) - .location(FieldLocation.QUERY).requirement(FieldRequirement.REQUIRED).build(), + .location(FieldLocation.QUERY).requirement(FieldRequirement.OPTIONAL).build(), FieldInfo.builder("sub.subfield", TypeSignature.ofBase(JavaType.STRING.name())) - .location(FieldLocation.QUERY).requirement(FieldRequirement.REQUIRED).build(), + .location(FieldLocation.QUERY).requirement(FieldRequirement.OPTIONAL).build(), FieldInfo.builder("type", TypeSignature.ofBase(JavaType.ENUM.name())) - .location(FieldLocation.QUERY).requirement(FieldRequirement.REQUIRED).build())); + .location(FieldLocation.QUERY).requirement(FieldRequirement.OPTIONAL).build())); assertThat(getMessageV2.useParameterAsRoot()).isFalse(); final MethodInfo getMessageV3 = serviceInfo.methods().stream() @@ -422,7 +422,7 @@ void httpEndpoint() { .location(FieldLocation.PATH).requirement(FieldRequirement.REQUIRED).build(), FieldInfo.builder("revision", TypeSignature.ofList(TypeSignature.ofBase(JavaType.LONG.name()))) - .location(FieldLocation.QUERY).requirement(FieldRequirement.REQUIRED).build())); + .location(FieldLocation.QUERY).requirement(FieldRequirement.OPTIONAL).build())); assertThat(getMessageV3.useParameterAsRoot()).isFalse(); // Check HTTP PATCH method. @@ -437,7 +437,7 @@ void httpEndpoint() { FieldInfo.builder("message_id", TypeSignature.ofBase(JavaType.STRING.name())) .location(FieldLocation.PATH).requirement(FieldRequirement.REQUIRED).build(), FieldInfo.builder("text", TypeSignature.ofBase(JavaType.STRING.name())) - .location(FieldLocation.BODY).requirement(FieldRequirement.REQUIRED).build())); + .location(FieldLocation.BODY).requirement(FieldRequirement.OPTIONAL).build())); assertThat(updateMessageV1.useParameterAsRoot()).isFalse(); final MethodInfo updateMessageV2 = serviceInfo.methods().stream() @@ -451,7 +451,7 @@ void httpEndpoint() { FieldInfo.builder("message_id", TypeSignature.ofBase(JavaType.STRING.name())) .location(FieldLocation.PATH).requirement(FieldRequirement.REQUIRED).build(), FieldInfo.builder("text", TypeSignature.ofBase(JavaType.STRING.name())) - .location(FieldLocation.BODY).requirement(FieldRequirement.REQUIRED).build(), + .location(FieldLocation.BODY).requirement(FieldRequirement.OPTIONAL).build(), FieldInfo.builder("required_text", TypeSignature.ofBase(JavaType.STRING.name())) .location(FieldLocation.BODY).requirement(FieldRequirement.REQUIRED).build(), FieldInfo.builder("optional_text", TypeSignature.ofBase(JavaType.STRING.name())) From 19979461a56ba6b584f1ad9701d79d8fcbc96564 Mon Sep 17 00:00:00 2001 From: Jun Kim Date: Wed, 12 Mar 2025 15:10:18 +0900 Subject: [PATCH 15/15] fix: rollback space --- .../armeria/server/grpc/HttpJsonTranscodingService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java index e079eafaa4a..209b4a5cd19 100644 --- a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java +++ b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java @@ -161,7 +161,7 @@ static GrpcService of(GrpcService delegate, HttpJsonTranscodingOptions httpJsonT } final HttpRule httpRule = methodOptions.getExtension( - (ExtensionLite) AnnotationsProto.http + (ExtensionLite)AnnotationsProto.http ); if (methodDefinition.getMethodDescriptor().getType() != MethodType.UNARY) {