From 90c4f38df675056042cd91ea0f410c4b6d77e4c8 Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Tue, 27 Oct 2020 10:39:18 -0700 Subject: [PATCH] fix: support non-name fields with res-refs in resname def parsing --- .../gapic/protoparser/ResourceNameParser.java | 23 ++++++-- .../protoparser/ResourceNameParserTest.java | 53 ++++++++++++++++--- .../testdata/bad_message_resname_def.proto | 11 ++++ .../api/generator/gapic/testdata/locker.proto | 13 ++++- 4 files changed, 87 insertions(+), 13 deletions(-) diff --git a/src/main/java/com/google/api/generator/gapic/protoparser/ResourceNameParser.java b/src/main/java/com/google/api/generator/gapic/protoparser/ResourceNameParser.java index 4a66becd4e..16e8f96beb 100644 --- a/src/main/java/com/google/api/generator/gapic/protoparser/ResourceNameParser.java +++ b/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 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())); } diff --git a/src/test/java/com/google/api/generator/gapic/protoparser/ResourceNameParserTest.java b/src/test/java/com/google/api/generator/gapic/protoparser/ResourceNameParserTest.java index c02a8f4427..9ecd696e3a 100644 --- a/src/test/java/com/google/api/generator/gapic/protoparser/ResourceNameParserTest.java +++ b/src/test/java/com/google/api/generator/gapic/protoparser/ResourceNameParserTest.java @@ -125,7 +125,7 @@ public void parseResourceNames_messageResourceDefinition() { List messageDescriptors = lockerServiceFileDescriptor.getMessageTypes(); Map 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 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 messageDescriptors = lockerServiceFileDescriptor.getMessageTypes(); + Descriptor documentMessageDescriptor = messageDescriptors.get(0); + assertEquals("Document", documentMessageDescriptor.getName()); + Optional 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 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 messageDescriptors = lockerServiceFileDescriptor.getMessageTypes(); + Descriptor binderMessageDescriptor = messageDescriptors.get(2); + assertEquals("Binder", binderMessageDescriptor.getName()); + Optional 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 messageDescriptors = protoFileDescriptor.getMessageTypes(); - Descriptor messageDescriptor = messageDescriptors.get(0); String pakkage = TypeParser.getPackage(protoFileDescriptor); - + List 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 diff --git a/src/test/java/com/google/api/generator/gapic/testdata/bad_message_resname_def.proto b/src/test/java/com/google/api/generator/gapic/testdata/bad_message_resname_def.proto index 705082e96a..c79e10b9b8 100644 --- a/src/test/java/com/google/api/generator/gapic/testdata/bad_message_resname_def.proto +++ b/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; +} diff --git a/src/test/java/com/google/api/generator/gapic/testdata/locker.proto b/src/test/java/com/google/api/generator/gapic/testdata/locker.proto index a6744c7bbe..9be138d33a 100644 --- a/src/test/java/com/google/api/generator/gapic/testdata/locker.proto +++ b/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 =