Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add optional info for google.proto types #6153

Merged
merged 16 commits into from
Mar 13, 2025
+70 −24
Merged

Add optional info for google.proto types #6153

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
41dc06f
Add optional info for google.proto types
khj68 Mar 11, 2025
c863988
fix lint error
khj68 Mar 11, 2025
22bbc07
fix test
khj68 Mar 11, 2025
68fb08c
change name of function
khj68 Mar 11, 2025
32fa834
add optional condition
khj68 Mar 11, 2025
63924ef
feat: change to check required behavior
khj68 Mar 11, 2025
80f212b
fix: lint
khj68 Mar 11, 2025
da2d4f2
fix: test
khj68 Mar 11, 2025
dd4cc09
fix: noSuchMethod Error
khj68 Mar 11, 2025
4c997bb
fix: hasField error
khj68 Mar 12, 2025
7740c9e
fix: lint
khj68 Mar 12, 2025
daa55b8
fix: protobuf4 error
khj68 Mar 12, 2025
6334464
feat: add test case
khj68 Mar 12, 2025
ac1ec53
feat: adopt required check to other java types
khj68 Mar 12, 2025
1997946
fix: rollback space
khj68 Mar 12, 2025
a024ee4
Merge branch 'main' into fix_grpc_doc_field_error
khj68 Mar 12, 2025
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
12 changes: 8 additions & 4 deletions grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,10 +427,14 @@ private static boolean hasRequiredFieldBehavior(FieldDescriptor field) {
return false;
}

final List<FieldBehavior> fieldBehaviors = field
.getOptions()
.getExtension(FieldBehaviorProto.fieldBehavior);
return fieldBehaviors.contains(FieldBehavior.REQUIRED);
try {
final List<FieldBehavior> fieldBehaviors = field
.getOptions()
.getExtension(FieldBehaviorProto.fieldBehavior);
return fieldBehaviors.contains(FieldBehavior.REQUIRED);
} catch (NoSuchMethodError error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

When could NoSuchMethodError be raised?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

In this case, protobuf4 throw NoSuchMethodError.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@khj68 khj68 Mar 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

image

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

omg thanks..!
I'll try it.

return false;
}
}

@Nullable
Expand Down
6 changes: 5 additions & 1 deletion grpc/src/test/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePluginTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this optional?

Copy link
Contributor Author

@khj68 khj68 Mar 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Java String type should be Required without behavior I guess 🙇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@khj68 khj68 Mar 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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? 🙇

Copy link
Contributor

Choose a reason for hiding this comment

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

That's correct. 👍 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed this with @ikhoon and understood his concern about the principle.
He mentioned that we don't have to follow protobuf's nullability principle.
Kotlin, for example, treats all fields required unless explicitly marked as optional.
I agreed with him on this point, so we might introduce an option to choose the default FieldRequirement in the near future.

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();
}

Expand Down
3 changes: 3 additions & 0 deletions grpc/src/test/proto/testing/grpc/transcoding.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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 {
Expand Down
Loading