feat(regapic): accept numeric enums, allow testing enum round trips (#…

…1159)

This introduces the RPCs `Compliance.GetEnum()` and `Compliance.VerifyEnum()`. The first returns one enum value that is either known or unknown to the client. The client can then send that response as the request message to the second RPC, which will verify that the value received was the same as the one originally sent. This thus tests that enum fields that are populated by the server with an unknown-to-the-client value (maybe because the client hasn't been regenerated for the latest proto version) and which are untouched by client code can be sent back to the server while preserving their value. This is particularly important in the REST transport case, and is the reason why we are separately implementing a feature to request that the server, if using REST, send all enums as numbers rather than strings.                   
    
To test various types of responses from the command line, you can use the following commands:
    
```
curl -i  -H "Content-Type: application/json" -H "X-Goog-Api-Client: rest/0.0.0 gapic/0.0.0" http://localhost:7469/v1beta1/compliance/enum\?unknownEnum\=true
    
curl -i  -H "Content-Type: application/json" -H "X-Goog-Api-Client: rest/0.0.0 gapic/0.0.0" http://localhost:7469/v1beta1/compliance/enum\?unknownEnum\=false
    
curl -i -X POST  -H "Content-Type: application/json" -H "X-Goog-Api-Client: rest/0.0.0 gapic/0.0.0" http://localhost:7469/v1beta1/compliance/enum?request.unknownEnum=true&continent=57                                                                                                                                                  
```
    
To test requesting and verifying an enum from the command line:
    
```
echo -e "\n**** Request enum:  ****\n" && \
  REGAPIC_ENUM=$(curl -i  -H "Content-Type: application/json" -H "X-Goog-Api-Client: rest/0.0.0 gapic/0.0.0" http://localhost:7469/v1beta1/compliance/enum\?unknownEnum\=true | tee /dev/tty | grep continent | grep -o -E -- '-?[0-9]+')  && \                                                                                    
  echo -e "\n\n**** Verify enum:  ****\n" && \
  curl -i -X POST  -H "Content-Type: application/json" -H "X-Goog-Api-Client: rest/0.0.0 gapic/0.0.0" http://localhost:7469/v1beta1/compliance/enum?request.unknownEnum=true&continent=$REGAPIC_ENUM                                                                                                                               
```

(GAPICs should simply call the corresponding generated methods analogously.)

As part of enabling this functionality, Showcase now allows receiving REST enum values JSON-encoded as numbers rather than strings. Earlier, there was an explicit prohibition against this.

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)

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

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

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

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: