diff --git a/cmd/gapic-showcase/compliance_suite_errors_test.go b/cmd/gapic-showcase/compliance_suite_errors_test.go index 593bd87e9..3b7c9b708 100644 --- a/cmd/gapic-showcase/compliance_suite_errors_test.go +++ b/cmd/gapic-showcase/compliance_suite_errors_test.go @@ -18,7 +18,6 @@ import ( "fmt" "io/ioutil" "net/http" - "net/url" "strings" "testing" @@ -47,11 +46,8 @@ func TestComplianceSuiteErrors(t *testing.T) { prepRepeatDataBodyInfoNegativeTestSnakeCasedFieldNames, }, "Compliance.RepeatDataQuery": { - prepRepeatDataQueryNegativeTestNumericEnums, - prepRepeatDataQueryNegativeTestNumericOptionalEnums, prepRepeatDataQueryNegativeTestSnakeCasedFieldNames, }, - "Compliance.RepeatDataSimplePath": {prepRepeatDataSimplePathNegativeTestEnum}, } for groupIdx, group := range masterSuite.GetGroup() { @@ -271,63 +267,6 @@ func prepRepeatDataQueryNegativeTestSnakeCasedFieldNames(request *genproto.Repea return name, "GET", "/v1beta1/repeat:query" + queryString, body, "(QueryParameterNameIncorrectlyCasedError)", err } -func prepRepeatDataQueryNegativeTestNumericEnums(request *genproto.RepeatRequest) (verb string, name string, path string, body string, expect string, err error) { - name = "Compliance.RepeatDataQuery#NegativeTestNumericEnums" - info := request.GetInfo() - badQueryParam := fmt.Sprintf("info.fKingdom=%d", info.GetFKingdom()) // purposefully use a number, which should cause an error - - // We clear the field so we don't set the same query param correctly below. This change - // modifies the request, but since these tests only check that calls fail, we never need to - // refer back to the request proto after constructing the REST query. - info.FKingdom = pb.ComplianceData_LIFE_KINGDOM_UNSPECIFIED - queryParams := append(prepRepeatDataTestsQueryParams(request, nil, queryStringLowerCamelCaser), badQueryParam) - - queryString := prepQueryString(queryParams) - return name, "GET", "/v1beta1/repeat:query" + queryString, body, "(EnumValueNotStringError)", err -} - -func prepRepeatDataQueryNegativeTestNumericOptionalEnums(request *genproto.RepeatRequest) (verb string, name string, path string, body string, expect string, err error) { - name = "Compliance.RepeatDataQuery#NegativeTestNumericOptionalEnums" - info := request.GetInfo() - badQueryParam := fmt.Sprintf("info.pKingdom=%d", info.GetPKingdom()) // purposefully use a number, which should cause an error - - // We clear the field so we don't set the same query param correctly below. This change - // modifies the request, but since these tests only check that calls fail, we never need to - // refer back to the request proto after constructing the REST query. - info.PKingdom = nil - queryParams := append(prepRepeatDataTestsQueryParams(request, nil, queryStringLowerCamelCaser), badQueryParam) - - queryString := prepQueryString(queryParams) - return name, "GET", "/v1beta1/repeat:query" + queryString, body, "(EnumValueNotStringError)", err -} - -func prepRepeatDataSimplePathNegativeTestEnum(request *genproto.RepeatRequest) (verb string, name string, path string, body string, expect string, err error) { - name = "Compliance.RepeatDataSimplePath#NegativeTestNumericEnum" - info := request.GetInfo() - - pathParts := []string{} - nonQueryParamNames := map[string]bool{} - - for _, part := range []struct { - name string - format string - value interface{} - }{ - {"f_string", "%s", info.GetFString()}, - {"f_int32", "%d", info.GetFInt32()}, - {"f_double", "%g", info.GetFDouble()}, - {"f_bool", "%t", info.GetFBool()}, - {"f_kingdom", "%d", info.GetFKingdom()}, // purposefully use a number, which should cause an error - } { - pathParts = append(pathParts, url.PathEscape(fmt.Sprintf(part.format, part.value))) - nonQueryParamNames["info."+part.name] = true - } - path = fmt.Sprintf("/v1beta1/repeat/%s:simplepath", strings.Join(pathParts, "/")) - - queryString := prepRepeatDataTestsQueryString(request, nonQueryParamNames) - return name, "GET", path + queryString, body, "(EnumValueNotStringError)", err -} - // checkExpectedFailure issues a request using the specified verb, URL, and request body. It expects // a failing HTTP code and a response message containing the substring in `failure`. Test errors are // reported using the given errorPrefix and the name prepName of the prepping function. @@ -364,5 +303,5 @@ func checkExpectedFailure(t *testing.T, verb, url, requestBody, failure, errorPr } -//requestModifer is a function that modifies a request in-place. +// requestModifer is a function that modifies a request in-place. type requestModifier func(*pb.RepeatRequest) diff --git a/cmd/gapic-showcase/endpoint_test.go b/cmd/gapic-showcase/endpoint_test.go index cbf01956d..04cad82d4 100644 --- a/cmd/gapic-showcase/endpoint_test.go +++ b/cmd/gapic-showcase/endpoint_test.go @@ -76,8 +76,8 @@ func TestRESTCalls(t *testing.T) { }, { verb: "GET", - path: "/v1beta1/repeat:query?info.pKingdom=1", - statusCode: 400, // numeric value for enum + path: "/v1beta1/repeat:query?info.p_kingdom=EXTRATERRESTRIAL", + statusCode: 400, // unknown enum value symbol }, { verb: "GET", @@ -90,6 +90,33 @@ func TestRESTCalls(t *testing.T) { statusCode: 400, // non-lower-camel-cased field name }, + { + // Test sending an enum as a number in the query parameter + verb: "GET", + path: "/v1beta1/repeat:query?info.pKingdom=1", + want: `{ + "request":{ + "info":{ + "pKingdom":"ARCHAEBACTERIA" + } + }, + "bindingUri":"/v1beta1/repeat:query" + }`, + }, + { + // Test sending an enum as a number in the body + verb: "POST", + path: "/v1beta1/repeat:body", + body: `{"info":{"pKingdom": 1}}`, + want: `{ + "request":{ + "info":{ + "pKingdom":"ARCHAEBACTERIA" + } + }, + "bindingUri":"/v1beta1/repeat:body" + }`, + }, { // Test responses: // 1. unset optional field absent diff --git a/schema/google/showcase/v1beta1/compliance.proto b/schema/google/showcase/v1beta1/compliance.proto index 7136c474e..9b6c0b7d7 100644 --- a/schema/google/showcase/v1beta1/compliance.proto +++ b/schema/google/showcase/v1beta1/compliance.proto @@ -24,8 +24,9 @@ option java_package = "com.google.showcase.v1beta1"; option java_multiple_files = true; option ruby_package = "Google::Showcase::V1beta1"; -// This service is used to test that GAPICs can transcode proto3 requests to -// REST format correctly for various types of HTTP annotations. +// This service is used to test that GAPICs implement various REST-related features correctly. This mostly means transcoding proto3 requests to REST format +// correctly for various types of HTTP annotations, but it also includes verifying that unknown (numeric) enums received by clients can be round-tripped +// correctly. service Compliance { // This service is meant to only run locally on the port 7469 (keypad digits // for "show"). @@ -100,6 +101,30 @@ service Compliance { }; } + // This method requests an enum value from the server. Depending on the contents of EnumRequest, the enum value returned will be a known enum declared in the + // .proto file, or a made-up enum value the is unknown to the client. To verify that clients can round-trip unknown enum vaues they receive, use the + // response from this RPC as the request to VerifyEnum() + // + // The values of enums sent by the server when a known or unknown value is requested will be the same within a single Showcase server run (this is needed for + // VerifyEnum() to work) but are not guaranteed to be the same across separate Showcase server runs. + rpc GetEnum(EnumRequest) returns (EnumResponse) { + option (google.api.http) = { + get: "/v1beta1/compliance/enum" + }; + } + + // This method is used to verify that clients can round-trip enum values, which is particularly important for unknown enum values over REST. VerifyEnum() + // verifies that its request, which is presumably the response that the client previously got to a GetEnum(), contains the correct data. If so, it responds + // with the same EnumResponse; otherwise, the RPC errors. + // + // This works because the values of enums sent by the server when a known or unknown value is requested will be the same within a single Showcase server run, + // although they are not guaranteed to be the same across separate Showcase server runs. + rpc VerifyEnum(EnumResponse) returns (EnumResponse) { + option (google.api.http) = { + post: "/v1beta1/compliance/enum" + }; + } + } message RepeatRequest { @@ -228,3 +253,17 @@ enum Continent { AUSTRALIA = 4; EUROPE = 5; } + + +message EnumRequest { + // Whether the client is requesting a new, unknown enum value or a known enum value already declard in this proto file. + bool unknown_enum = 1; +} + +message EnumResponse { + // The original request for a known or unknown enum from the server. + EnumRequest request = 1; + + // The actual enum the server provided. + Continent continent = 2; +} diff --git a/server/services/compliance_service.go b/server/services/compliance_service.go index 067560f34..873e67598 100644 --- a/server/services/compliance_service.go +++ b/server/services/compliance_service.go @@ -18,6 +18,7 @@ import ( "context" _ "embed" // for storing compliance suite data, used to verify incoming requests "fmt" + "os" "github.com/google/go-cmp/cmp" "github.com/googleapis/gapic-showcase/server" @@ -123,9 +124,56 @@ func (csi *complianceServerImpl) RepeatDataBodyPatch(ctx context.Context, in *pb // complianceSuiteBytes contains the contents of the compliance suite JSON file. This requires Go // 1.16. Note that embedding can only be applied to global variables at package scope. +// //go:embed compliance_suite.json var complianceSuiteBytes []byte +//// Enum support testing. + +// These enums are not part of the current compliance suite because they don't +// have the server echo the client's request. +var existingEnumValue, unknownEnumValue pb.Continent + +// storeEnumTestValues stores the values for known and unknown enums used in GetEnum() and +// VerifyEnum() in this session. This function should be run from init() +func storeEnumTestValues() { + deterministicInt := os.Getpid() + unknownEnumValue = pb.Continent(-deterministicInt) + existingEnumValue = pb.Continent(deterministicInt%(len(pb.Continent_name)-1) + 1) + +} + +func (csi *complianceServerImpl) GetEnum(ctx context.Context, in *pb.EnumRequest) (*pb.EnumResponse, error) { + response := &pb.EnumResponse{ + Request: in, + } + + if in.GetUnknownEnum() { + response.Continent = unknownEnumValue + } else { + response.Continent = existingEnumValue + } + + return response, nil +} + +func (csi *complianceServerImpl) VerifyEnum(ctx context.Context, in *pb.EnumResponse) (*pb.EnumResponse, error) { + usingUnknownEnum := in.Request.GetUnknownEnum() + + expectedEnum := existingEnumValue + if usingUnknownEnum { + expectedEnum = unknownEnumValue + } + + if actualEnum := in.GetContinent(); actualEnum != expectedEnum { + return nil, fmt.Errorf("(UnexpectedEnumError) enum received (%d) is not the value expected (%d) when unknown_enum = %t", actualEnum, expectedEnum, usingUnknownEnum) + } + + return in, nil +} + +//// Compliance suite support. + // ComplianceSuiteInitStatus contains the status result of loading the compliance test suite type ComplianceSuiteInitStatus int @@ -201,4 +249,5 @@ func indexTestingRequests(suiteBytes []byte) (err error) { func init() { indexTestingRequests(complianceSuiteBytes) + storeEnumTestValues() } diff --git a/util/genrest/resttools/populatefield.go b/util/genrest/resttools/populatefield.go index 94ece7df3..da5e66a06 100644 --- a/util/genrest/resttools/populatefield.go +++ b/util/genrest/resttools/populatefield.go @@ -152,10 +152,16 @@ func PopulateOneField(protoMessage proto.Message, fieldPath string, fieldValues parseError, protoValue = err, protoreflect.ValueOfBool(parsedValue) case protoreflect.EnumKind: - if _, parseError = strconv.ParseFloat(value, 32); parseError == nil { - return fmt.Errorf("(EnumValueNotStringError) enum value %q for field path %q appears to be a number rather than an identifier", fieldName, fieldPath) + var parsedValue protoreflect.EnumNumber + if numericValue, err := strconv.ParseFloat(value, 32); err == nil { + parsedValue = protoreflect.EnumNumber(numericValue) + } else { + enum := fieldDescriptor.Enum().Values().ByName(protoreflect.Name(value)) + if enum == nil { + return fmt.Errorf("(UnknownEnumStringError) unknown enum symbol %q for field path %q", value, fieldPath) + } + parsedValue = enum.Number() } - parsedValue := fieldDescriptor.Enum().Values().ByName(protoreflect.Name(value)).Number() parseError, protoValue = nil, protoreflect.ValueOfEnum(parsedValue) default: