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

fix: allow empty services and java keywords as a method names #985

Merged
merged 4 commits into from
Apr 29, 2022
Merged

fix: allow empty services and java keywords as a method names #985

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7c0e42f
fix: Fix empty service and keyword as a method name cases
vam-google Apr 22, 2022
9bffc5e
add empty service test case
vam-google Apr 25, 2022
f90d1ae
address PR feedback
vam-google Apr 26, 2022
7ca4aaa
address PR feedback
vam-google Apr 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions .../google/api/generator/gapic/composer/common/AbstractServiceStubSettingsClassComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,8 +392,7 @@ private static List<CommentStatement> createClassHeaderComments(
.filter(m -> m.stream() == Stream.NONE && !m.hasLro() && !m.isPaged())
.findFirst()
.orElse(service.methods().get(0)));
Optional<String> methodNameOpt =
methodOpt.isPresent() ? Optional.of(methodOpt.get().name()) : Optional.empty();
Optional<String> methodNameOpt = methodOpt.map(Method::name);
Optional<Sample> sampleCode =
SettingsSampleComposer.composeSettingsSample(
methodNameOpt, ClassNames.getServiceSettingsClassName(service), classType);
Expand Down Expand Up @@ -437,7 +436,7 @@ private static Map<String, VariableExpr> createMethodSettingsClassMemberVarExprs
!Objects.isNull(serviceConfig) && serviceConfig.hasBatchingSetting(service, method);
TypeNode settingsType =
getCallSettingsType(method, typeStore, hasBatchingSettings, isNestedClass);
String varName = JavaStyle.toLowerCamelCase(String.format("%sSettings", method.name()));
String varName = String.format("%sSettings", JavaStyle.toLowerCamelCase(method.name()));
if (method.isDeprecated()) {
deprecatedSettingVarNames.add(varName);
}
Expand Down
8 changes: 8 additions & 0 deletions ...com/google/api/generator/gapic/composer/samplecode/ServiceClientHeaderSampleComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,21 @@ public static Sample composeClassHeaderSample(
TypeNode clientType,
Map<String, ResourceName> resourceNames,
Map<String, Message> messageTypes) {
if (service.methods().isEmpty()) {
RegionTag regionTag =
RegionTag.builder().setServiceName(service.name()).setRpcName("emtpy").build();

return Sample.builder().setRegionTag(regionTag).build();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think each Sample will end up as a Java file, instead of creating an empty sample, we should probably either check it before entering the method, or return null and handle it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Compiling an empty file does not result in error in java, so I was under impression that empy sample is a valid case here (i.e. empty sample for empty service and no need to have special handling for nulls). On the other hand, empty service is a suspicious thing in API surface in a first place, so maybe the best way to fix it is for API team to stop publishing empty service definition.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I agree that empty service should probably be prohibited in the first place, but in case it happens, is an empty sample file more helpful to developers? or not generating samples at all more helpful? I think the former might cause more confusion unless we are required to generate samples for empty services. @eaball35 what's your take on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree this shouldn't even be a case in the first place, but here we are. I think it would be the preferred developer experience to not generate the empty sample. It's not that big of a deal to generate it, but it won't provide much value and will be confusing. Checking before entering or return null/optional and handling makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@blakeli0, @eaball35 Ok, I changed it to generate a legit sample to the extent possible - it shows how to create a client, but does not call any methods on the generated client, since there are none. I think it is a good balance between possible options. This will produce a valid compilable sample - a pretty useless (but valid) example for a pretty useless (empty) service. The rest is on the API side, I think - once they add methods we would not have this problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this better because it's clear this behavior is more intentional, but "empty" is kind of a confusing name.

The goal of generating these samples in files with region tags is so they can be more easily be pulled into documentation/sample browser as desired. I can't see why anyone would ever want these "empty" samples. It's not going to necessarily hurt to include them, but it doesn't provide any value. What is the pushback against just not generating them?

Copy link
Contributor Author

@vam-google vam-google Apr 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Because we still generate the client class. The class still has methods, create is one of them, so the sample generates as much as it can for a specific client class and it is still a valid example. Moving nulls around is an antipattern in java, plus, if we stop generating the sample we will have to change accordingly the whole header of the generated class documentation, which currently says:

This class provides the ability to make remote calls to the backing service through method calls that map to API methods. Sample code to get started:

I.e. not generating sample at all will have impliaitons on this comment (we can't say "Sample codeto get started" if there is no sample code). Also, assuming Emily's samples are there in their final form: still generating sample would let that code not to have to bother with special null handling, it can simply assume that sample is always generated. I.e. it literally feels to me like a better, simpler and more robust design. Otherwise all sampling logic would always have to be null-aware (not the case now), which unnecessarily complicates things. Yes, the emply sample is basically useless. But it is a useless (but formally valid) sample for a useless (but formally valid) client class, so they are in a perfect harmony here...

On the other hand, this is a very minor thing. Mainly, empty services should not even be a thing. Here I'm just trying to make presence of an empty service in API not fail the generation of the client for the whole API (which has many other non-empty services). Basically the whole goal of this is to make generator stop failing if there is a bogus service and ensure some more or less reasonable behavior of generator in case like this, without imposing a burden on the rest of the team on maintaining nulls.

Copy link
Contributor

@eaball35 eaball35 Apr 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for explaining. I was thinking maybe returning an Optional< Sample> for that method and handling but can see the annoyance still. I still think creating the empty sample is kind of weird, but I understand why you want to do this. Because this is such a minor case that will rarely appear I guess it's fine to do this way. I can always add logic to the Writer to ignore writing "empty" samples. Maybe add some additional comments on composeEmptySample to explain what it's for, otherwise lgtm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using Optional<Sample> would break the whole contract in samples generator and make it inconsistent with the rest of the methods (having one method in ServiceClientMethodSampleComposer return Optional<Sample> while the rest return Sample would be weird).

}

// Use the first pure unary RPC method's sample code as showcase, if no such method exists, use
// the first method in the service's methods list.
Method method =
service.methods().stream()
.filter(m -> m.stream() == Method.Stream.NONE && !m.hasLro() && !m.isPaged())
.findFirst()
.orElse(service.methods().get(0));

if (method.stream() == Method.Stream.NONE) {
if (method.methodSignatures().isEmpty()) {
return ServiceClientMethodSampleComposer.composeCanonicalSample(
Expand Down
10 changes: 8 additions & 2 deletions src/main/java/com/google/api/generator/gapic/utils/JavaStyle.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

package com.google.api.generator.gapic.utils;

import com.google.api.generator.engine.lexicon.Keyword;
import com.google.common.base.CaseFormat;
import com.google.common.base.Strings;
import java.util.stream.IntStream;
Expand All @@ -32,8 +33,13 @@ public static String toLowerCamelCase(String s) {
s = CaseFormat.LOWER_UNDERSCORE.to(CaseFormat.UPPER_CAMEL, s);
}

return capitalizeLettersAfterDigits(
String.format("%s%s", s.substring(0, 1).toLowerCase(), s.substring(1)));
String name =
capitalizeLettersAfterDigits(
String.format("%s%s", s.substring(0, 1).toLowerCase(), s.substring(1)));

// Some APIs use legit java keywords as method names. Both protobuf and gGRPC add an underscore
// in generated stubs to resolve name conflict, so we need to do the same.
return Keyword.isKeyword(name) ? name + '_' : name;
}

public static String toUpperCamelCase(String s) {
Expand Down
10 changes: 10 additions & 0 deletions src/test/java/com/google/api/generator/gapic/utils/JavaStyleTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,4 +111,14 @@ public void acronyms() {
assertEquals("iamHttpXmlDog", JavaStyle.toLowerCamelCase(value));
assertEquals("IamHttpXmlDog", JavaStyle.toUpperCamelCase(value));
}

@Test
public void keyword() {
String value = "import";
assertEquals("import_", JavaStyle.toLowerCamelCase(value));
assertEquals("Import", JavaStyle.toUpperCamelCase(value));
value = "IMPORT_";
assertEquals("import_", JavaStyle.toLowerCamelCase(value));
assertEquals("Import", JavaStyle.toUpperCamelCase(value));
}
}