fix: support non-name fields with res-refs in resname def parsing (#418)

Author: miraleung

src/main/java/com/google/api/generator/gapic/protoparser/ResourceNameParser.java

@@ -22,9 +22,11 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Strings;
+import com.google.protobuf.DescriptorProtos.FieldOptions;
 import com.google.protobuf.DescriptorProtos.FileOptions;
 import com.google.protobuf.DescriptorProtos.MessageOptions;
 import com.google.protobuf.Descriptors.Descriptor;
+import com.google.protobuf.Descriptors.FieldDescriptor;
 import com.google.protobuf.Descriptors.FileDescriptor;
 import java.util.ArrayList;
 import java.util.HashMap;
@@ -107,12 +109,25 @@ static Optional<ResourceName> parseResourceNameFromMessageType(
  }
 
  ResourceDescriptor protoResource = messageOptions.getExtension(ResourceProto.resource);
- // aip.dev/4231.
+ // Validation - check that a resource name field is present.
  if (Strings.isNullOrEmpty(protoResource.getNameField())) {
- Preconditions.checkNotNull(
- messageTypeDescriptor.findFieldByName(ResourceNameConstants.NAME_FIELD_NAME),
+ // aip.dev/4231
+ boolean resourceNameFieldFound =
+ messageTypeDescriptor.findFieldByName(ResourceNameConstants.NAME_FIELD_NAME) != null;
+ // If this is null, look for a field with a resource reference is found.
+ // Example: AccountBudgetProposal.
+ for (FieldDescriptor fieldDescriptor : messageTypeDescriptor.getFields()) {
+ FieldOptions fieldOptions = fieldDescriptor.getOptions();
+ if (fieldOptions.hasExtension(ResourceProto.resourceReference)) {
+ resourceNameFieldFound = true;
+ break;
+ }
+ }
+ Preconditions.checkState(
+ resourceNameFieldFound,
  String.format(
- "Message %s has a resource annotation but no \"name\" field",
+ "Message %s has a resource annotation but no field titled \"name\" or containing a"
+ + " resource reference",
  messageTypeDescriptor.getName()));
  }
 

src/test/java/com/google/api/generator/gapic/protoparser/ResourceNameParserTest.java

@@ -125,7 +125,7 @@ public void parseResourceNames_messageResourceDefinition() {
  List<Descriptor> messageDescriptors = lockerServiceFileDescriptor.getMessageTypes();
  Map<String, ResourceName> typeStringsToResourceNames =
  ResourceNameParser.parseResourceNamesFromMessages(messageDescriptors, pakkage);
- assertEquals(1, typeStringsToResourceNames.size());
+ assertEquals(2, typeStringsToResourceNames.size());
 
  ResourceName resourceName = typeStringsToResourceNames.get("testgapic.googleapis.com/Document");
  assertEquals(2, resourceName.patterns().size());
@@ -140,7 +140,31 @@ public void parseResourceNames_messageResourceDefinition() {
  }
 
  @Test
- public void parseResourceNames_messageWithoutResourceDefinition() {
+ public void parseResourceNames_badMessageResourceNameDefinitionMissingNameField() {
+ FileDescriptor protoFileDescriptor = BadMessageResnameDefProto.getDescriptor();
+ List<Descriptor> messageDescriptors = protoFileDescriptor.getMessageTypes();
+ Descriptor messageDescriptor = messageDescriptors.get(0);
+ String pakkage = TypeParser.getPackage(protoFileDescriptor);
+
+ assertThrows(
+ IllegalStateException.class,
+ () -> ResourceNameParser.parseResourceNameFromMessageType(messageDescriptor, pakkage));
+ }
+
+ @Test
+ public void parseResourceNameFromMessage_basicResourceDefinition() {
+ String pakkage = TypeParser.getPackage(lockerServiceFileDescriptor);
+ List<Descriptor> messageDescriptors = lockerServiceFileDescriptor.getMessageTypes();
+ Descriptor documentMessageDescriptor = messageDescriptors.get(0);
+ assertEquals("Document", documentMessageDescriptor.getName());
+ Optional<ResourceName> resourceNameOpt =
+ ResourceNameParser.parseResourceNameFromMessageType(documentMessageDescriptor, pakkage);
+ assertTrue(resourceNameOpt.isPresent());
+ assertEquals("testgapic.googleapis.com/Document", resourceNameOpt.get().resourceTypeString());
+ }
+
+ @Test
+ public void parseResourceNamesFromMessage_noResourceDefinition() {
  String pakkage = TypeParser.getPackage(lockerServiceFileDescriptor);
  List<Descriptor> messageDescriptors = lockerServiceFileDescriptor.getMessageTypes();
  Descriptor folderMessageDescriptor = messageDescriptors.get(1);
@@ -151,15 +175,28 @@ public void parseResourceNames_messageWithoutResourceDefinition() {
  }
 
  @Test
- public void parseResourceNames_badMessageResourceNameDefinitionMissingNameField() {
+ public void parseResourceNameFromMessage_nonNameResourceReferenceField() {
+ String pakkage = TypeParser.getPackage(lockerServiceFileDescriptor);
+ List<Descriptor> messageDescriptors = lockerServiceFileDescriptor.getMessageTypes();
+ Descriptor binderMessageDescriptor = messageDescriptors.get(2);
+ assertEquals("Binder", binderMessageDescriptor.getName());
+ Optional<ResourceName> resourceNameOpt =
+ ResourceNameParser.parseResourceNameFromMessageType(binderMessageDescriptor, pakkage);
+ assertTrue(resourceNameOpt.isPresent());
+ assertEquals("testgapic.googleapis.com/Binder", resourceNameOpt.get().resourceTypeString());
+ }
+
+ @Test
+ public void parseResourceNamesFromMessage_noNameOrResourceReferenceField() {
  FileDescriptor protoFileDescriptor = BadMessageResnameDefProto.getDescriptor();
- List<Descriptor> messageDescriptors = protoFileDescriptor.getMessageTypes();
- Descriptor messageDescriptor = messageDescriptors.get(0);
  String pakkage = TypeParser.getPackage(protoFileDescriptor);
-
+ List<Descriptor> messageDescriptors = protoFileDescriptor.getMessageTypes();
+ Descriptor pencilMessageDescriptor = messageDescriptors.get(1);
+ assertEquals("Pencil", pencilMessageDescriptor.getName());
  assertThrows(
- NullPointerException.class,
- () -> ResourceNameParser.parseResourceNameFromMessageType(messageDescriptor, pakkage));
+ IllegalStateException.class,
+ () ->
+ ResourceNameParser.parseResourceNameFromMessageType(pencilMessageDescriptor, pakkage));
  }
 
  @Test

src/test/java/com/google/api/generator/gapic/testdata/bad_message_resname_def.proto

@@ -32,3 +32,14 @@ message BarFoo {
 
  string display_name = 1;
 }
+
+// A proto with a resource definition, but missing a field titled "name" or
+// containing a resource reference.
+message Pencil {
+ option (google.api.resource) = {
+ type: "testgapic.googleapis.com/Pencil"
+ pattern: "pencils/{pencil}"
+ };
+
+ string owner = 1;
+}

src/test/java/com/google/api/generator/gapic/testdata/locker.proto

@@ -79,7 +79,7 @@ message Document {
  pattern: "*"
  };
 
- // The resource name of the user.
+ // The resource name of the document.
  string name = 1;
 }
 
@@ -88,6 +88,17 @@ message Folder {
  "cloudresourcemanager.googleapis.com/Folder"];
 }
 
+message Binder {
+ option (google.api.resource) = {
+ type: "testgapic.googleapis.com/Binder"
+ pattern: "binders/{binder}"
+ };
+
+ // The resource name of the binder.
+ string binder_name = 1 [(google.api.resource_reference).type =
+ "testgapic.googleapis.com/Binder"];
+}
+
 message GetFolderRequest {
  string name = 1 [
  (google.api.resource_reference).type =