Bug fixes for gapic config v2 parsing (#2717)

* Fix null pointer exception when Api model is null
* Correctly handle annotations plus v2 config case for interfaces

Author: michaelbausor

src/main/java/com/google/api/codegen/config/GapicProductConfig.java

@@ -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;
@@ -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;
@@ -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;
@@ -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,
@@ -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);
 
@@ -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) {

src/main/java/com/google/api/codegen/config/ProtoInterfaceModel.java

@@ -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;
 
@@ -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();

src/test/java/com/google/api/codegen/config/GapicConfigProducerTest.java

@@ -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"});
 
@@ -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);
+  }
 }

src/test/java/com/google/api/codegen/config/testdata/missing_interface_v1.yaml

@@ -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