-
Notifications
You must be signed in to change notification settings - Fork 47
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.
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
feat: add RPCs returning unknown enum values over REST #856
base: main
Are you sure you want to change the base?
Conversation
NEXT: - clean up init function for go view creator - add custom operation for unknown enum
NEXT: need tests, clean up code
Asa result of the test, also fixed NPE by ensuring submessages are present before referencing them.
Need to test this next.
This allows isolating unknown enum values in specific fields.
rpc RepeatWithUnknownEnum(RepeatRequest) returns (RepeatResponse) { | ||
option (google.api.http) = { | ||
post: "/v1beta1/repeat:invalidenum" | ||
body: "*" | ||
}; | ||
} | ||
|
||
// This method returns an unknown value for an optional enum field. Client libraries should either | ||
// error gracefully (not crash), ignore the value, or, depending on the API, set it to the zero | ||
// default. | ||
// | ||
// This only works over REST currently. Requests over gRPC will simply echo the request. | ||
rpc RepeatWithUnknownOptionalEnum(RepeatRequest) returns (RepeatResponse) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could these be one request with two fields and maybe a field to toggle behavior? For example, we have the Echo
RPC which could return either an EchoResponse or an error status depending on what was sent in the request oneof. Might simplify the implementation a bit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been thinking about this (and sidetracked by other issues, sorry). We could keep just the previously existing RPCs and add a top-level field to RepeatRequest
to generate the unknown enum. In fact, I would change RepeatRequest.server_verify
to be an enum like server_behavior
that could be verify
, or generate_unknown_enum
, or something else in the future.
Not too concerned about that being a breaking change because (a) most generators are not yet using the compliance suite, and (b) the compliance suite is meant to be parsed form this repo and then used to issue requests, without the client code modifying the request.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that sounds great to me. Thanks for considering this.
This adds two new RPCs,
Compliance.RepeatWithUnknown{,Optional}Enum()
which, when called over REST, populate enum fields in the JSON responses with string values that do not correspond to enum values in the protobuf definition. This should be used by generators to ensure that when the server returns an unknown enum value as part of a REST response, the GAPICs handle it gracefully.Below are command-line examples of calling these RPCs. Note that
LIFE_KINGDOM_NEW
is the unknown enum value not present in the proto.Non-optional enum fields:
Optional enum fields: