-
Notifications
You must be signed in to change notification settings - Fork 52
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
fix: Use parent type instead of child_type in method doc sample #862
Changes from 3 commits
49f0b39
8c59769
7578d45
c9f0b07
9350266
a4543a7
bde778e
8c8ab88
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,6 +48,7 @@ | |
import java.util.HashSet; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.Optional; | ||
import java.util.stream.Collectors; | ||
|
||
public class DefaultValueComposer { | ||
|
@@ -76,6 +77,7 @@ public static Expr createDefaultValue( | |
Expr defValue = | ||
createDefaultValue( | ||
resourceName, | ||
methodArg.field().resourceReference().isChildType(), | ||
resourceNames.values().stream().collect(Collectors.toList()), | ||
methodArg.field().name()); | ||
|
||
|
@@ -175,17 +177,45 @@ static Expr createDefaultValue(Field f, boolean useExplicitInitTypeInGenerics) { | |
} | ||
|
||
public static Expr createDefaultValue( | ||
ResourceName resourceName, List<ResourceName> resnames, String fieldOrMessageName) { | ||
return createDefaultValueResourceHelper(resourceName, resnames, fieldOrMessageName, true); | ||
ResourceName resourceName, | ||
boolean isChildType, | ||
List<ResourceName> resnames, | ||
String fieldOrMessageName) { | ||
return createDefaultValueResourceHelper( | ||
resourceName, isChildType, resnames, fieldOrMessageName, true); | ||
} | ||
|
||
private static Optional<ResourceName> findParentResource( | ||
ResourceName childResource, List<ResourceName> resourceNames) { | ||
for (ResourceName parent : resourceNames) { | ||
for (String parentPattern : parent.patterns()) { | ||
String[] parentPatternParts = parentPattern.split("/"); | ||
for (String childPattern : childResource.patterns()) { | ||
String[] childPatternParts = childPattern.split("/"); | ||
if (parentPattern.length() < childPattern.length() | ||
&& childPattern.startsWith(parentPattern) | ||
&& childPatternParts.length - parentPatternParts.length == 2) { | ||
return Optional.of(parent); | ||
} | ||
} | ||
} | ||
} | ||
|
||
return Optional.empty(); | ||
} | ||
|
||
@VisibleForTesting | ||
static Expr createDefaultValueResourceHelper( | ||
ResourceName resourceName, | ||
boolean isChildType, | ||
List<ResourceName> resnames, | ||
String fieldOrMessageName, | ||
boolean allowAnonResourceNameClass) { | ||
|
||
if (isChildType) { | ||
resourceName = findParentResource(resourceName, resnames).orElse(resourceName); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep. Do you think we should fail instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, just wanted to make sure I understood it right. |
||
} | ||
|
||
boolean hasOnePattern = resourceName.patterns().size() == 1; | ||
if (resourceName.isOnlyWildcard()) { | ||
List<ResourceName> unexaminedResnames = new ArrayList<>(resnames); | ||
|
@@ -195,7 +225,7 @@ static Expr createDefaultValueResourceHelper( | |
continue; | ||
} | ||
unexaminedResnames.remove(resname); | ||
return createDefaultValue(resname, unexaminedResnames, fieldOrMessageName); | ||
return createDefaultValue(resname, false, unexaminedResnames, fieldOrMessageName); | ||
} | ||
|
||
if (unexaminedResnames.isEmpty()) { | ||
|
@@ -283,6 +313,7 @@ public static Expr createSimpleMessageBuilderExpr( | |
defaultExpr = | ||
createDefaultValueResourceHelper( | ||
resourceNames.get(field.resourceReference().resourceTypeString()), | ||
field.resourceReference().isChildType(), | ||
resourceNames.values().stream().collect(Collectors.toList()), | ||
message.name(), | ||
/* allowAnonResourceNameClass = */ false); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -881,7 +881,11 @@ static String parsePageSizeFieldName( | |
// with monolith-gnerated libraries. | ||
String pagedFieldName = null; | ||
|
||
if (inputMessage.fieldMap().containsKey("page_token") | ||
if (inputMessage != null | ||
&& inputMessage.fieldMap() != null | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did this if statement get so much bigger? Specifically, looks like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point on |
||
&& inputMessage.fieldMap().containsKey("page_token") | ||
&& outputMessage != null | ||
&& outputMessage.fieldMap() != null | ||
&& outputMessage.fieldMap().containsKey("next_page_token")) { | ||
// List of potential field names representing page size. | ||
// page_size gets priority over max_results if both are present | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,7 @@ | |
import com.google.pubsub.v1.PubsubProto; | ||
import com.google.showcase.v1beta1.EchoOuterClass; | ||
import com.google.showcase.v1beta1.IdentityOuterClass; | ||
import com.google.showcase.v1beta1.MessagingOuterClass; | ||
import com.google.showcase.v1beta1.TestingOuterClass; | ||
import com.google.testdata.v1.DeprecatedServiceOuterClass; | ||
import google.cloud.CommonResources; | ||
|
@@ -51,6 +52,7 @@ | |
import java.util.Set; | ||
|
||
public class TestProtoLoader { | ||
|
||
private static final TestProtoLoader INSTANCE = | ||
new TestProtoLoader(Transport.GRPC, "src/test/java/com/google/api/generator/gapic/testdata/"); | ||
private final String testFilesDirectory; | ||
|
@@ -170,6 +172,35 @@ public GapicContext parseShowcaseIdentity() { | |
.build(); | ||
} | ||
|
||
public GapicContext parseShowcaseMessaging() { | ||
FileDescriptor fileDescriptor = MessagingOuterClass.getDescriptor(); | ||
ServiceDescriptor messagingService = fileDescriptor.getServices().get(0); | ||
assertEquals(messagingService.getName(), "Messaging"); | ||
|
||
Map<String, Message> messageTypes = Parser.parseMessages(fileDescriptor); | ||
Map<String, ResourceName> resourceNames = Parser.parseResourceNames(fileDescriptor); | ||
|
||
FileDescriptor identityFileDescriptor = IdentityOuterClass.getDescriptor(); | ||
Map<String, ResourceName> identityResourceNames = Parser | ||
.parseResourceNames(identityFileDescriptor); | ||
|
||
resourceNames.put("showcase.googleapis.com/User", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this needed? If there is stuff in the messaging.proto file, which just causes troubles but is not essential for the tests, please feel free to remove it. If there are missing imports please just add them on proot/bazel lavel. |
||
identityResourceNames.get("showcase.googleapis.com/User")); | ||
|
||
Set<ResourceName> outputResourceNames = new HashSet<>(); | ||
List<Service> services = | ||
Parser.parseService( | ||
fileDescriptor, messageTypes, resourceNames, Optional.empty(), outputResourceNames); | ||
|
||
return GapicContext.builder() | ||
.setMessages(messageTypes) | ||
.setResourceNames(resourceNames) | ||
.setServices(services) | ||
.setHelperResourceNames(outputResourceNames) | ||
.setTransport(transport) | ||
.build(); | ||
} | ||
|
||
public GapicContext parseShowcaseTesting() { | ||
FileDescriptor testingFileDescriptor = TestingOuterClass.getDescriptor(); | ||
ServiceDescriptor testingService = testingFileDescriptor.getServices().get(0); | ||
|
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.
Can this whole thing be included in ResourceReferenceParser class? This seems like a general resource names resolution logic, not necessarily specific to sample generation.