Add optional info for google.proto types #6153

khj68 · 2025-03-11T05:35:54Z

Motivation:

In the HTTP documentation generated by Armeria, all google.protobuf.* types (such as google.protobuf.StringValue) are incorrectly marked as required: true. This caused confusion when collaborating with frontend teams, as optional fields were not clearly distinguished.

Modifications:

Added explicit handling for google.protobuf.* wrapper types to correctly mark them as optional.
Updated the logic to ensure that optional parameters are properly reflected in the documentation.

Result:

This change ensures that google.protobuf.* wrapper types are correctly marked as optional in the generated HTTP documentation.
Improves clarity for frontend and backend collaboration by accurately distinguishing required and optional fields.
Closes Clarifying Required and Optional Fields in Armeria Docs (Proto) #6152

CLAassistant · 2025-03-11T05:35:59Z

All committers have signed the CLA.

github-actions · 2025-03-11T06:13:51Z

🔍 Build Scan® (commit: `a024ee4`)

Job name	Status	Build Scan®
build-ubicloud-standard-16-jdk-8	✅	https://ge.armeria.dev/s/tmkpilsrw4i4o
build-ubicloud-standard-16-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/ppbqd4zt32hkw
build-ubicloud-standard-16-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/6wge47g4erxjg
build-ubicloud-standard-16-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/uqz73xi4zkpl2
build-ubicloud-standard-16-jdk-17-leak	✅	https://ge.armeria.dev/s/qkv2omy5gf3yk
build-ubicloud-standard-16-jdk-11	✅	https://ge.armeria.dev/s/sid64y4clatig
build-macos-latest-jdk-21	✅	https://ge.armeria.dev/s/ytlagjpc2mr4u

minwoox

Basically looks good. 👍

minwoox · 2025-03-11T11:48:00Z

grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java

@@ -418,6 +419,22 @@ private static Map<String, Field> buildFields(Descriptor desc,
        return builder.buildKeepingLast();
    }

+    private static boolean isOptionalFieldType(FieldDescriptor field) {


probably isScalarValueWrapperMessage?
https://protobuf.dev/programming-guides/proto3/#scalar
Could you also call this method from the line 454?

That name is much better.
I’ve applied it to line 454 as well to remove duplicate code.
Thanks for the suggestion.

minwoox · 2025-03-11T11:51:48Z

grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/HttpEndpointSpecification.java

            this.type = requireNonNull(type, "type");
            this.isRepeated = isRepeated;
+            this.isOptional = isOptional;


Probably we can do this.isOptional = isOptional || isRepeated; here

Great. I've applied it 👍

minwoox · 2025-03-11T11:54:58Z

grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java

                    break;
                case MESSAGE:
                    @Nullable
                    final JavaType wellKnownFieldType = getJavaTypeForWellKnownTypes(field);
+                    final boolean isOptional = isOptionalFieldType(field);


Can't we use field.isOptional(); here?

field.isOptional() only checks if the field has LABEL_OPTIONAL set, which is not sufficient to determine whether it’s a google.protobuf.StringValue or similar wrapper type. Since wrapper types in Protobuf do not explicitly have LABEL_OPTIONAL, we need a separate function to correctly identify them.

Let me know if I might be misunderstanding something 🙇

I realized that all fields are optional in proto3:
https://protobuf.dev/programming-guides/proto3/#field-labels
What do you think of making the fields required only when [(google.api.field_behavior) = REQUIRED]; used instead?
https://google.aip.dev/203

Thanks for providing the documentation references.
Since maintaining backward compatibility is important, using [(google.api.field_behavior) = REQUIRED] as the criteria for required fields makes sense.
I’ve updated the implementation accordingly 👍

minwoox

This looks great, thanks!
One last request—could you add a test case to verify the change?

minwoox · 2025-03-12T05:15:54Z

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(),


Isn't this optional?

Java String type should be Required without behavior I guess 🙇

I thought we agreed that fields are optional by default if REQUIRED is not specified. 😆
https://protobuf.dev/programming-guides/proto3/#field-labels

I think this should be false
https://github.com/line/armeria/pull/6153/files#diff-a9d03eb106960c77af7b39790659dfa479f644ef0745c2e0075e56a5de8badacR373

What I meant is doing something like:
builder.put(key, new Field(field, parentNames, field.getJavaType(), hasRequiredFieldBehavior(field)));

I see.
Then, if in this case
string text = 1 [(google.api.field_behavior) = REQUIRED];

it should be Required, right?

so, it only depends on field_behavior, is it right? 🙇

That's correct. 👍 👍

I thought we agreed that fields are optional by default if REQUIRED is not specified.

Some people may want to make the fields required by default. Should we provide a configuration to override the default behavior?

Some people may want to make the fields required by default.

Can we do this when there's a clear demand for it? I really hope users follow the AIP if they want to make it required.
Of course, there could be issues if the users rely on third-party-provided protobufs, but even in that case,
I don't think we should make all the fields required.

ikhoon · 2025-03-12T05:21:38Z

grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java

+                    .getOptions()
+                    .getExtension(FieldBehaviorProto.fieldBehavior);
+            return fieldBehaviors.contains(FieldBehavior.REQUIRED);
+        } catch (NoSuchMethodError error) {


When could NoSuchMethodError be raised?

./gradlew :it:grpc:protobuf4:shadedTest --info

In this case, protobuf4 throw NoSuchMethodError.

It would be nice if we can support [(google.api.field_behavior) = REQUIRED] for protobuf 4 version.

okay, then I should find other logic. because in protobuf4, GeneratedMessageV3 is deprecated.
So I can't use getExtension function.
I'll try other approach 🙇
but it was really hard to find.. do you have another idea?

We can fix it:

private static boolean hasRequiredFieldBehavior(FieldDescriptor field) { if (field.isRepeated()) { return false; } final List<FieldBehavior> fieldBehaviors = field .getOptions() .getExtension((ExtensionLite<FieldOptions, List<FieldBehavior>>) FieldBehaviorProto.fieldBehavior); return fieldBehaviors.contains(FieldBehavior.REQUIRED); }

omg thanks..!
I'll try it.

minwoox

Nice work, @khj68! 👍

khj68 · 2025-03-12T07:32:02Z

@minwoox @ikhoon
Thank you for your detailed review. 👍

I appreciate your insights, and I hope to have another opportunity to contribute in the future.
Thanks again!

minwoox · 2025-03-12T07:54:07Z

I hope to have another opportunity to contribute in the future.

Hope to see you again. 😆

Add optional info for google.proto types

41dc06f

khj68 requested review from ikhoon, jrhee17, minwoox and trustin as code owners March 11, 2025 05:35

khj68 added 2 commits March 11, 2025 16:14

fix lint error

c863988

fix test

22bbc07

minwoox added the defect label Mar 11, 2025

minwoox added this to the 1.32.1 milestone Mar 11, 2025

minwoox reviewed Mar 11, 2025

View reviewed changes

khj68 added 6 commits March 11, 2025 21:21

change name of function

68fb08c

add optional condition

32fa834

feat: change to check required behavior

63924ef

fix: lint

80f212b

fix: test

da2d4f2

fix: noSuchMethod Error

dd4cc09

minwoox reviewed Mar 12, 2025

View reviewed changes

khj68 added 2 commits March 12, 2025 11:15

fix: hasField error

4c997bb

fix: lint

7740c9e

minwoox modified the milestones: 1.32.1, 1.33.0 Mar 12, 2025

khj68 added 2 commits March 12, 2025 13:58

fix: protobuf4 error

daa55b8

feat: add test case

6334464

minwoox reviewed Mar 12, 2025

View reviewed changes

ikhoon reviewed Mar 12, 2025

View reviewed changes

minwoox modified the milestones: 1.33.0, 1.32.1 Mar 12, 2025

feat: adopt required check to other java types

ac1ec53

fix: rollback space

1997946

This comment was marked as duplicate.

Sign in to view

minwoox approved these changes Mar 12, 2025

View reviewed changes

Merge branch 'main' into fix_grpc_doc_field_error

a024ee4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add optional info for google.proto types #6153

Add optional info for google.proto types #6153

khj68 commented Mar 11, 2025

CLAassistant commented Mar 11, 2025 •

edited

Loading

github-actions bot commented Mar 11, 2025 •

edited

Loading

minwoox left a comment

minwoox Mar 11, 2025

khj68 Mar 11, 2025

minwoox Mar 11, 2025

khj68 Mar 11, 2025

minwoox Mar 11, 2025

khj68 Mar 11, 2025

minwoox Mar 11, 2025 •

edited

Loading

khj68 Mar 11, 2025

minwoox left a comment

minwoox Mar 12, 2025

khj68 Mar 12, 2025 •

edited

Loading

khj68 Mar 12, 2025

minwoox Mar 12, 2025

minwoox Mar 12, 2025

khj68 Mar 12, 2025 •

edited

Loading

minwoox Mar 12, 2025

ikhoon Mar 12, 2025

minwoox Mar 12, 2025

ikhoon Mar 12, 2025

khj68 Mar 12, 2025

ikhoon Mar 12, 2025

khj68 Mar 12, 2025 •

edited

Loading

minwoox Mar 12, 2025

khj68 Mar 12, 2025

This comment was marked as duplicate.

minwoox left a comment

khj68 commented Mar 12, 2025

minwoox commented Mar 12, 2025

Add optional info for google.proto types #6153

Are you sure you want to change the base?

Add optional info for google.proto types #6153

Conversation

khj68 commented Mar 11, 2025

CLAassistant commented Mar 11, 2025 • edited Loading

github-actions bot commented Mar 11, 2025 • edited Loading

🔍 Build Scan® (commit: a024ee4)

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khj68 Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khj68 Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khj68 Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

minwoox left a comment

Choose a reason for hiding this comment

khj68 commented Mar 12, 2025

minwoox commented Mar 12, 2025

CLAassistant commented Mar 11, 2025 •

edited

Loading

github-actions bot commented Mar 11, 2025 •

edited

Loading

🔍 Build Scan® (commit: `a024ee4`)

minwoox Mar 11, 2025 •

edited

Loading

khj68 Mar 12, 2025 •

edited

Loading

khj68 Mar 12, 2025 •

edited

Loading

khj68 Mar 12, 2025 •

edited

Loading