Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: bring back unused resnames for Ads [ggj] #821

Merged
merged 2 commits into from
Aug 6, 2021
Merged

fix: bring back unused resnames for Ads [ggj] #821

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4179dd7
fix: bring back unused resnames for Ads
miraleung Aug 6, 2021
3bfb240
Merge branch 'master' into dev/unused_resnames
miraleung Aug 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 36 additions & 2 deletions src/main/java/com/google/api/generator/gapic/protoparser/Parser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,11 @@ public static GapicContext parse(CodeGeneratorRequest request) {
Map<String, ResourceName> resourceNames = parseResourceNames(request);
messages = updateResourceNamesInMessages(messages, resourceNames.values());

// Contains only resource names that are actually used. That is, resource name definitions
// or references that are simply defined, but not used, will not have corresponding Java helper
// Contains only resource names that are actually used. Usage refers to the presence of a
// request message's field in an RPC's method_signature annotation. That is, resource name
// definitions
// or references that are simply defined, but not used in such a manner, will not have
// corresponding Java helper
// classes generated.
Set<ResourceName> outputArgResourceNames = new HashSet<>();
List<Service> mixinServices = new ArrayList<>();
Expand All @@ -160,6 +163,37 @@ public static GapicContext parse(CodeGeneratorRequest request) {
transport);

Preconditions.checkState(!services.isEmpty(), "No services found to generate");

// Temporary workaround for Ads, who still need these resource names.
if (services.get(0).protoPakkage().startsWith("google.ads.googleads.v")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please put some TODO here (I hope we do plan to get rid of google.ads.googleads.v specific hack, don't we)?

Copy link
Contributor Author

@miraleung miraleung Aug 6, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do in #822 . Up to you whether we should bring back all these "unused" resource name helper classes altogether, or just for Ads.

Function<ResourceName, String> typeNameFn =
r -> r.resourceTypeString().substring(r.resourceTypeString().indexOf("/") + 1);
Function<Set<ResourceName>, Set<String>> typeStringSetFn =
sr -> sr.stream().map(r -> typeNameFn.apply(r)).collect(Collectors.toSet());

// Include all resource names present in message types for backwards-compatibility with the
// monolith. In the future, this should be removed on a client library major semver update.
// Resolve type name collisions with the ones present in the method arguments.
final Set<String> typeStringSet = typeStringSetFn.apply(outputArgResourceNames);
outputArgResourceNames.addAll(
resourceNames.values().stream()
.filter(r -> r.hasParentMessageName() && !typeStringSet.contains(typeNameFn.apply(r)))
.collect(Collectors.toSet()));

String servicePackage = services.get(0).pakkage();
Map<String, ResourceName> patternsToResourceNames =
ResourceParserHelpers.createPatternResourceNameMap(resourceNames);
for (ResourceReference resourceReference : outputResourceReferencesSeen) {
final Set<String> interimTypeStringSet = typeStringSetFn.apply(outputArgResourceNames);
outputArgResourceNames.addAll(
ResourceReferenceParser.parseResourceNames(
resourceReference, servicePackage, null, resourceNames, patternsToResourceNames)
.stream()
.filter(r -> !interimTypeStringSet.contains(typeNameFn.apply(r)))
.collect(Collectors.toSet()));
}
}

return GapicContext.builder()
.setServices(services)
.setMixinServices(mixinServices)
Expand Down