Skip to content
This repository was archived by the owner on Jun 28, 2022. It is now read-only.

Bug fixes for gapic config v2 parsing #2717

Merged
merged 6 commits into from
Apr 19, 2019
Merged

Bug fixes for gapic config v2 parsing #2717

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9134f50
Fix null pointer exception when Api model is null
michaelbausor Apr 16, 2019
173d08a
Correctly handle annotations plus v2 config case for interfaces
michaelbausor Apr 16, 2019
45ccb36
Fix imports
michaelbausor Apr 17, 2019
3446a1e
Trigger CI
michaelbausor Apr 17, 2019
1a22df4
Add test for missing interface
michaelbausor Apr 17, 2019
756c58c
remove unnecessary changes
michaelbausor Apr 17, 2019
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
82 changes: 54 additions & 28 deletions src/main/java/com/google/api/codegen/config/GapicProductConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import com.google.api.tools.framework.model.Interface;
import com.google.api.tools.framework.model.Method;
import com.google.api.tools.framework.model.Model;
import com.google.api.tools.framework.model.ProtoElement;
import com.google.api.tools.framework.model.ProtoFile;
import com.google.api.tools.framework.model.SimpleLocation;
import com.google.api.tools.framework.model.SymbolTable;
Expand All @@ -47,17 +48,16 @@
import com.google.common.collect.Iterables;
import com.google.protobuf.Api;
import com.google.protobuf.DescriptorProtos;
import java.util.Collection;
import java.util.Comparator;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.function.Function;
import java.util.stream.Collectors;
import javax.annotation.Nullable;
import org.apache.commons.lang3.StringUtils;
Expand Down Expand Up @@ -279,16 +279,18 @@ public static GapicProductConfig create(
getInterfacesFromProtoFile(diagCollector, sourceProtos, symbolTable);

ImmutableList<GapicInterfaceInput> interfaceInputs;
if (!configProto.equals(ConfigProto.getDefaultInstance())) {
if (protoParser.isProtoAnnotationsEnabled()) {
interfaceInputs =
createInterfaceInputsWithAnnotationsAndGapicConfig(
diagCollector, configProto.getInterfacesList(), protoInterfaces, language);
} else {
interfaceInputs =
createInterfaceInputsWithGapicConfig(
createInterfaceInputsWithGapicConfigOnly(
diagCollector,
configProto.getInterfacesList(),
protoInterfaces,
symbolTable,
language);
} else {
interfaceInputs = createInterfaceInputsWithoutGapicConfig(protoInterfaces.values());
}
if (interfaceInputs == null) {
return null;
Expand Down Expand Up @@ -475,7 +477,22 @@ private static GapicProductConfig createDummyInstance(
}

/** Return the list of information about clients to be generated. */
private static ImmutableList<GapicInterfaceInput> createInterfaceInputsWithGapicConfig(
private static ImmutableList<GapicInterfaceInput>
createInterfaceInputsWithAnnotationsAndGapicConfig(
DiagCollector diagCollector,
List<InterfaceConfigProto> interfaceConfigProtosList,
ImmutableMap<String, Interface> protoInterfaces,
TargetLanguage language) {
return createGapicInterfaceInputList(
diagCollector,
language,
protoInterfaces.values(),
interfaceConfigProtosList
.stream()
.collect(Collectors.toMap(InterfaceConfigProto::getName, Function.identity())));
}

private static ImmutableList<GapicInterfaceInput> createInterfaceInputsWithGapicConfigOnly(
DiagCollector diagCollector,
List<InterfaceConfigProto> interfaceConfigProtosList,
ImmutableMap<String, Interface> protoInterfaces,
Expand All @@ -491,23 +508,48 @@ private static ImmutableList<GapicInterfaceInput> createInterfaceInputsWithGapic
// Parse GAPIC config for interfaceConfigProtos.
for (InterfaceConfigProto interfaceConfigProto : interfaceConfigProtosList) {
Interface apiInterface = symbolTable.lookupInterface(interfaceConfigProto.getName());
if (apiInterface == null || !apiInterface.isReachable()) {
if (apiInterface == null) {
List<String> interfaces =
symbolTable
.getInterfaces()
.stream()
.map(ProtoElement::getFullName)
.collect(Collectors.toList());
String interfacesString = String.join(",", interfaces);
diagCollector.addDiag(
Diag.error(
SimpleLocation.TOPLEVEL,
"interface not found: %s. Interfaces: [%s]",
interfaceConfigProto.getName(),
interfacesString));
continue;
}
if (!apiInterface.isReachable()) {
diagCollector.addDiag(
Diag.error(
SimpleLocation.TOPLEVEL,
"interface not found: %s",
"interface not reachable: %s.",
interfaceConfigProto.getName()));
continue;
}
interfaceConfigProtos.put(interfaceConfigProto.getName(), interfaceConfigProto);
interfaceMap.put(interfaceConfigProto.getName(), apiInterface);
}

return createGapicInterfaceInputList(
diagCollector, language, interfaceMap.values(), interfaceConfigProtos);
}

private static ImmutableList<GapicInterfaceInput> createGapicInterfaceInputList(
DiagCollector diagCollector,
TargetLanguage language,
Iterable<Interface> interfaceList,
Map<String, InterfaceConfigProto> interfaceConfigProtos) {

// Store info about each Interface in a GapicInterfaceInput object.
ImmutableList.Builder<GapicInterfaceInput> interfaceInputs = ImmutableList.builder();
for (Entry<String, Interface> interfaceEntry : interfaceMap.entrySet()) {
String serviceFullName = interfaceEntry.getKey();
Interface apiInterface = interfaceEntry.getValue();
for (Interface apiInterface : interfaceList) {
String serviceFullName = apiInterface.getFullName();
GapicInterfaceInput.Builder interfaceInput =
GapicInterfaceInput.newBuilder().setInterface(apiInterface);

Expand Down Expand Up @@ -552,22 +594,6 @@ private static ImmutableMap<String, Interface> getInterfacesFromProtoFile(
return protoInterfaces.build();
}

/** Return the list of information about clients to be generated. */
private static ImmutableList<GapicInterfaceInput> createInterfaceInputsWithoutGapicConfig(
Collection<Interface> protoInterfaces) {
// Store info about each Interface in a GapicInterfaceInput object.
ImmutableList.Builder<GapicInterfaceInput> interfaceInputs = ImmutableList.builder();
for (Interface apiInterface : protoInterfaces) {
GapicInterfaceInput.Builder interfaceInput =
GapicInterfaceInput.newBuilder()
.setInterface(apiInterface)
.setInterfaceConfigProto(InterfaceConfigProto.getDefaultInstance())
.setMethodsToGenerate(findMethodsToGenerateWithoutConfigYaml(apiInterface));
interfaceInputs.add(interfaceInput.build());
}
return interfaceInputs.build();
}

/** Find the methods that should be generated on the surface when no GAPIC config was given. */
private static ImmutableMap<Method, MethodConfigProto> findMethodsToGenerateWithoutConfigYaml(
Interface apiInterface) {
Expand Down
12 changes: 8 additions & 4 deletions src/main/java/com/google/api/codegen/config/ProtoInterfaceModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.google.api.tools.framework.model.Method;
import com.google.api.tools.framework.model.SymbolTable;
import com.google.common.collect.ImmutableList;
import com.google.protobuf.Api;
import com.google.protobuf.Mixin;
import java.util.List;

Expand Down Expand Up @@ -73,10 +74,13 @@ public List<MethodModel> getMethods() {
methods.add(new ProtoMethodModel(method));
}
SymbolTable symbolTable = protoInterface.getModel().getSymbolTable();
for (Mixin mixin : protoInterface.getConfig().getMixinsList()) {
Interface mixinInterface = symbolTable.lookupInterface(mixin.getName());
for (Method method : mixinInterface.getMethods()) {
methods.add(new ProtoMethodModel(method));
Api protoInterfaceConfig = protoInterface.getConfig();
if (protoInterfaceConfig != null) {
for (Mixin mixin : protoInterface.getConfig().getMixinsList()) {
Interface mixinInterface = symbolTable.lookupInterface(mixin.getName());
for (Method method : mixinInterface.getMethods()) {
methods.add(new ProtoMethodModel(method));
}
}
}
return methods.build();
Expand Down
29 changes: 24 additions & 5 deletions src/test/java/com/google/api/codegen/config/GapicConfigProducerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,11 @@ public class GapicConfigProducerTest {

@ClassRule public static TemporaryFolder tempDir = new TemporaryFolder();

private static Model model;
private static GapicProductConfig productConfig;

@Test
public void missingConfigSchemaVersion() {
TestDataLocator locator = MixedPathTestDataLocator.create(this.getClass());
locator.addTestDataSource(CodegenTestUtil.class, "testsrc/common");
model =
Model model =
CodegenTestUtil.readModel(
locator, tempDir, new String[] {"myproto.proto"}, new String[] {"myproto.yaml"});

Expand All @@ -48,11 +45,33 @@ public void missingConfigSchemaVersion() {
model.getDiagReporter().getDiagCollector(),
locator,
new String[] {"missing_config_schema_version.yaml"});
productConfig = GapicProductConfig.create(model, configProto, null, null, TargetLanguage.JAVA);
GapicProductConfig.create(model, configProto, null, null, TargetLanguage.JAVA);
Diag expectedError =
Diag.error(
SimpleLocation.TOPLEVEL, "config_schema_version field is required in GAPIC yaml.");
assertThat(model.getDiagReporter().getDiagCollector().hasErrors()).isTrue();
assertThat(model.getDiagReporter().getDiagCollector().getDiags()).contains(expectedError);
}

@Test
public void missingInterface() {
TestDataLocator locator = MixedPathTestDataLocator.create(this.getClass());
locator.addTestDataSource(CodegenTestUtil.class, "testsrc/common");
Model model =
CodegenTestUtil.readModel(
locator, tempDir, new String[] {"myproto.proto"}, new String[] {"myproto.yaml"});

ConfigProto configProto =
CodegenTestUtil.readConfig(
model.getDiagReporter().getDiagCollector(),
locator,
new String[] {"missing_interface_v1.yaml"});
GapicProductConfig.create(model, configProto, null, null, TargetLanguage.JAVA);
Diag expectedError =
Diag.error(
SimpleLocation.TOPLEVEL,
"interface not found: google.example.myproto.v1.MyUnknownProto. Interfaces: [google.example.myproto.v1.MyProto]");
assertThat(model.getDiagReporter().getDiagCollector().hasErrors()).isTrue();
assertThat(model.getDiagReporter().getDiagCollector().getDiags()).contains(expectedError);
}
}
60 changes: 60 additions & 0 deletions src/test/java/com/google/api/codegen/config/testdata/missing_interface_v1.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
type: com.google.api.codegen.ConfigProto
config_schema_version: 1.0.0
language_settings:
go:
package_name: cloud.google.com/go/myapi/apiv1
interfaces:
- name: google.example.myproto.v1.MyProto
retry_codes_def:
- name: idempotent
retry_codes:
- DEADLINE_EXCEEDED
- name: non_idempotent
retry_codes:
retry_params_def:
- name: default
initial_retry_delay_millis: 100
retry_delay_multiplier: 1.2
max_retry_delay_millis: 1000
initial_rpc_timeout_millis: 300
rpc_timeout_multiplier: 1.3
max_rpc_timeout_millis: 3000
total_timeout_millis: 30000
methods:
- name: MyMethod
flattening:
groups:
- parameters:
- myfield
required_fields:
- myfield
retry_codes_name: non_idempotent
retry_params_name: default
timeout_millis: 1000
- name: google.example.myproto.v1.MyUnknownProto
retry_codes_def:
- name: idempotent
retry_codes:
- DEADLINE_EXCEEDED
- name: non_idempotent
retry_codes:
retry_params_def:
- name: default
initial_retry_delay_millis: 100
retry_delay_multiplier: 1.2
max_retry_delay_millis: 1000
initial_rpc_timeout_millis: 300
rpc_timeout_multiplier: 1.3
max_rpc_timeout_millis: 3000
total_timeout_millis: 30000
methods:
- name: MyMethod
flattening:
groups:
- parameters:
- myfield
required_fields:
- myfield
retry_codes_name: non_idempotent
retry_params_name: default
timeout_millis: 1000