Fix missing default retries (#2718)

* Fix missing default retries
* Revert python and ruby baselines
* Manual update of py and ruby baselines
* Remove unneeded diag tracking

Author: michaelbausor

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

@@ -71,8 +71,7 @@ static DiscoGapicInterfaceConfig createInterfaceConfig(
       ResourceNameMessageConfigs messageConfigs,
       ImmutableMap<String, ResourceNameConfig> resourceNameConfigs) {
 
-    RetryCodesConfig retryCodesConfig =
-        RetryCodesConfig.create(model.getDiagCollector(), interfaceConfigProto);
+    RetryCodesConfig retryCodesConfig = RetryCodesConfig.create(interfaceConfigProto);
     ImmutableMap<String, RetryParamsDefinitionProto> retrySettingsDefinition =
         RetryDefinitionsTransformer.createRetrySettingsDefinition(interfaceConfigProto);
 

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

@@ -119,7 +119,6 @@ static GapicInterfaceConfig createInterfaceConfig(
 
     RetryCodesConfig retryCodesConfig =
         RetryCodesConfig.create(
-            diagCollector,
             interfaceConfigProto,
             new ArrayList<>(interfaceInput.getMethodsToGenerate().keySet()),
             protoParser);

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

@@ -23,17 +23,14 @@
 import com.google.api.codegen.RetryCodesDefinitionProto;
 import com.google.api.codegen.util.ProtoParser;
 import com.google.api.codegen.util.SymbolTable;
-import com.google.api.tools.framework.model.Diag;
-import com.google.api.tools.framework.model.DiagCollector;
 import com.google.api.tools.framework.model.Method;
-import com.google.api.tools.framework.model.SimpleLocation;
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
 import java.util.Collection;
 import java.util.HashMap;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -44,14 +41,15 @@ public class RetryCodesConfig {
 
   public static ImmutableList<String> RETRY_CODES_FOR_HTTP_GET =
       DEFAULT_RETRY_CODES.get(RETRY_CODES_IDEMPOTENT_NAME);
+  public static ImmutableList<String> RETRY_CODES_FOR_HTTP_NON_GET =
+      DEFAULT_RETRY_CODES.get(RETRY_CODES_NON_IDEMPOTENT_NAME);
 
   private Map<String, ImmutableList<String>> retryCodesDefinition = new HashMap<>();
   private Map<String, String> methodRetryNames = new HashMap<>();
 
   private ImmutableSet<String> retryCodeDefsFromGapicConfig;
   private ImmutableMap<String, ImmutableList<String>> finalRetryCodesDefinition;
   private ImmutableMap<String, String> finalMethodRetryNames;
-  private boolean error = false;
 
   /**
    * A map of retry config names to the list of codes to retry on, e.g. { "idempotent" :
@@ -75,35 +73,22 @@ public Set<String> getRetryCodeDefsFromGapicConfig() {
 
   private RetryCodesConfig() {}
 
-  public static RetryCodesConfig create(
-      DiagCollector diagCollector, InterfaceConfigProto interfaceConfigProto) {
+  public static RetryCodesConfig create(InterfaceConfigProto interfaceConfigProto) {
     RetryCodesConfig retryCodesConfig = new RetryCodesConfig();
 
-    retryCodesConfig.populateRetryCodesDefinitionFromConfigProto(
-        diagCollector, interfaceConfigProto);
-    if (retryCodesConfig.error) {
-      return null;
-    }
-    retryCodesConfig.finalMethodRetryNames = ImmutableMap.copyOf(retryCodesConfig.methodRetryNames);
-    retryCodesConfig.finalRetryCodesDefinition =
-        ImmutableMap.copyOf(retryCodesConfig.retryCodesDefinition);
+    retryCodesConfig.populateRetryCodesDefinitionFromConfigProto(interfaceConfigProto);
+    retryCodesConfig.setFinalRetryProperties();
     return retryCodesConfig;
   }
 
   public static RetryCodesConfig create(
-      DiagCollector diagCollector,
       InterfaceConfigProto interfaceConfigProto,
       List<Method> methodsToGenerate,
       ProtoParser protoParser) {
     RetryCodesConfig retryCodesConfig = new RetryCodesConfig();
     retryCodesConfig.populateRetryCodesDefinition(
-        diagCollector, interfaceConfigProto, methodsToGenerate, protoParser);
-    if (retryCodesConfig.error) {
-      return null;
-    }
-    retryCodesConfig.finalMethodRetryNames = ImmutableMap.copyOf(retryCodesConfig.methodRetryNames);
-    retryCodesConfig.finalRetryCodesDefinition =
-        ImmutableMap.copyOf(retryCodesConfig.retryCodesDefinition);
+        interfaceConfigProto, methodsToGenerate, protoParser);
+    retryCodesConfig.setFinalRetryProperties();
     return retryCodesConfig;
   }
 
@@ -122,47 +107,32 @@ private static ImmutableMap<String, String> createMethodRetryNamesFromConfigProt
 
   @Nullable
   private static ImmutableMap<String, ImmutableList<String>>
-      createRetryCodesDefinitionFromConfigProto(
-          DiagCollector diagCollector, InterfaceConfigProto interfaceConfigProto) {
+      createRetryCodesDefinitionFromConfigProto(InterfaceConfigProto interfaceConfigProto) {
     ImmutableMap.Builder<String, ImmutableList<String>> builder = ImmutableMap.builder();
     for (RetryCodesDefinitionProto retryDef : interfaceConfigProto.getRetryCodesDefList()) {
       // Enforce ordering on set for baseline test consistency.
-      Set<String> codes = new TreeSet<>();
-      for (String codeText : retryDef.getRetryCodesList()) {
-        try {
-          codes.add(String.valueOf(codeText));
-        } catch (IllegalArgumentException e) {
-          diagCollector.addDiag(
-              Diag.error(
-                  SimpleLocation.TOPLEVEL,
-                  "status code not found: '%s' (in interface %s)",
-                  codeText,
-                  interfaceConfigProto.getName()));
-        }
-      }
+      Set<String> codes = new TreeSet<>(retryDef.getRetryCodesList());
       builder.put(retryDef.getName(), ImmutableList.copyOf(codes));
     }
-    if (diagCollector.getErrorCount() > 0) {
-      return null;
-    }
     return builder.build();
   }
 
+  private void setFinalRetryProperties() {
+    finalMethodRetryNames = ImmutableMap.copyOf(methodRetryNames);
+    finalRetryCodesDefinition = ImmutableMap.copyOf(retryCodesDefinition);
+  }
+
   /**
    * Returns a mapping of a retryCodeDef name to the list of retry codes it contains. Also populates
    * the @param methodNameToRetryCodeNames with a mapping of a Method name to its retry code
    * settings name.
    */
   private void populateRetryCodesDefinition(
-      DiagCollector diagCollector,
       InterfaceConfigProto interfaceConfigProto,
       Collection<Method> methodsToGenerate,
       ProtoParser protoParser) {
     // First create the retry codes definitions from the GAPIC config.
-    populateRetryCodesDefinitionFromConfigProto(diagCollector, interfaceConfigProto);
-    if (error) {
-      return;
-    }
+    populateRetryCodesDefinitionFromConfigProto(interfaceConfigProto);
 
     // Then create the retry codes defs from the proto annotations, but don't overwrite
     // existing retry codes defs from the GAPIC config.
@@ -175,10 +145,10 @@ private void populateRetryCodesDefinition(
    * settings name.
    */
   private void populateRetryCodesDefinitionFromConfigProto(
-      DiagCollector diagCollector, InterfaceConfigProto interfaceConfigProto) {
+      InterfaceConfigProto interfaceConfigProto) {
 
     ImmutableMap<String, ImmutableList<String>> retryCodesDefFromConfigProto =
-        createRetryCodesDefinitionFromConfigProto(diagCollector, interfaceConfigProto);
+        createRetryCodesDefinitionFromConfigProto(interfaceConfigProto);
     if (retryCodesDefFromConfigProto == null) {
       return;
     }
@@ -205,6 +175,11 @@ private void populateRetryCodesDefinitionWithProtoFile(
 
     SymbolTable symbolTable = new SymbolTable();
 
+    // Add retry codes for default cases (idempotent and non_idempotent) if they have not
+    // already be added previously (in the GAPIC config).
+    retryCodesDefinition.putIfAbsent(RETRY_CODES_IDEMPOTENT_NAME, RETRY_CODES_FOR_HTTP_GET);
+    retryCodesDefinition.putIfAbsent(RETRY_CODES_NON_IDEMPOTENT_NAME, RETRY_CODES_FOR_HTTP_NON_GET);
+
     for (String retryCodesName : retryCodesDefinition.keySet()) {
       // Record all the preexisting retryCodeNames from configProto.
       symbolTable.getNewSymbol(retryCodesName);
@@ -213,11 +188,8 @@ private void populateRetryCodesDefinitionWithProtoFile(
     // Unite all HTTP GET methods that have no additional retry codes under one retry code name to
     // reduce duplication.
     String httpGetRetryName;
-    Set<String> defaultRetryCodesForHttpGet = new HashSet<>(RETRY_CODES_FOR_HTTP_GET);
-    Set<String> existingIdempotentRetryCodes =
-        new HashSet<>(
-            retryCodesDefinition.getOrDefault(RETRY_CODES_IDEMPOTENT_NAME, ImmutableList.of()));
-    if (defaultRetryCodesForHttpGet.equals(existingIdempotentRetryCodes)) {
+    if (Sets.newHashSet(RETRY_CODES_FOR_HTTP_GET)
+        .containsAll(retryCodesDefinition.get(RETRY_CODES_IDEMPOTENT_NAME))) {
       // The GAPIC config defined RETRY_CODES_IDEMPOTENT_NAME to have the same retry codes as
       // this method would have,
       // so we can just reuse RETRY_CODES_IDEMPOTENT_NAME.
@@ -228,9 +200,8 @@ private void populateRetryCodesDefinitionWithProtoFile(
 
     // Unite all methods that have no retry codes under one retry code name to reduce duplication.
     String noRetryName;
-    if (retryCodesDefinition
-        .getOrDefault(RETRY_CODES_NON_IDEMPOTENT_NAME, ImmutableList.of())
-        .isEmpty()) {
+    if (Sets.newHashSet(RETRY_CODES_FOR_HTTP_NON_GET)
+        .containsAll(retryCodesDefinition.get(RETRY_CODES_NON_IDEMPOTENT_NAME))) {
       noRetryName = RETRY_CODES_NON_IDEMPOTENT_NAME;
     } else {
       noRetryName = symbolTable.getNewSymbol(RETRY_CODES_NON_IDEMPOTENT_NAME);

src/test/java/com/google/api/codegen/gapic/testdata/java/java_multiple_services.baseline

@@ -1086,6 +1086,9 @@ public class DecrementerServiceStubSettings extends StubSettings<DecrementerServ
 
     static {
       ImmutableMap.Builder<String, ImmutableSet<StatusCode.Code>> definitions = ImmutableMap.builder();
+      definitions.put(
+          "idempotent",
+          ImmutableSet.copyOf(Lists.<StatusCode.Code>newArrayList(StatusCode.Code.DEADLINE_EXCEEDED, StatusCode.Code.UNAVAILABLE)));
       definitions.put(
           "non_idempotent",
           ImmutableSet.copyOf(Lists.<StatusCode.Code>newArrayList()));
@@ -1877,6 +1880,9 @@ public class IncrementerServiceStubSettings extends StubSettings<IncrementerServ
 
     static {
       ImmutableMap.Builder<String, ImmutableSet<StatusCode.Code>> definitions = ImmutableMap.builder();
+      definitions.put(
+          "idempotent",
+          ImmutableSet.copyOf(Lists.<StatusCode.Code>newArrayList(StatusCode.Code.DEADLINE_EXCEEDED, StatusCode.Code.UNAVAILABLE)));
       definitions.put(
           "non_idempotent",
           ImmutableSet.copyOf(Lists.<StatusCode.Code>newArrayList()));

src/test/java/com/google/api/codegen/gapic/testdata/java/java_no_path_templates.baseline

@@ -959,6 +959,9 @@ public class NoTemplatesApiServiceStubSettings extends StubSettings<NoTemplatesA
 
     static {
       ImmutableMap.Builder<String, ImmutableSet<StatusCode.Code>> definitions = ImmutableMap.builder();
+      definitions.put(
+          "idempotent",
+          ImmutableSet.copyOf(Lists.<StatusCode.Code>newArrayList(StatusCode.Code.DEADLINE_EXCEEDED, StatusCode.Code.UNAVAILABLE)));
       definitions.put(
           "non_idempotent",
           ImmutableSet.copyOf(Lists.<StatusCode.Code>newArrayList()));

src/test/java/com/google/api/codegen/gapic/testdata/nodejs/nodejs_multiple_services.baseline

@@ -392,6 +392,10 @@ module.exports = DecrementerServiceClient;
   "interfaces": {
     "google.cloud.example.v1.foo.DecrementerService": {
       "retry_codes": {
+        "idempotent": [
+          "DEADLINE_EXCEEDED",
+          "UNAVAILABLE"
+        ],
         "non_idempotent": []
       },
       "retry_params": {
@@ -710,6 +714,10 @@ module.exports = IncrementerServiceClient;
   "interfaces": {
     "google.cloud.example.v1.foo.IncrementerService": {
       "retry_codes": {
+        "idempotent": [
+          "DEADLINE_EXCEEDED",
+          "UNAVAILABLE"
+        ],
         "non_idempotent": []
       },
       "retry_params": {

src/test/java/com/google/api/codegen/gapic/testdata/nodejs/nodejs_no_path_templates.baseline

@@ -387,6 +387,10 @@ module.exports = NoTemplatesApiServiceClient;
   "interfaces": {
     "google.cloud.example.v1.NoTemplatesAPIService": {
       "retry_codes": {
+        "idempotent": [
+          "DEADLINE_EXCEEDED",
+          "UNAVAILABLE"
+        ],
         "non_idempotent": []
       },
       "retry_params": {

src/test/java/com/google/api/codegen/gapic/testdata/php/php_no_path_templates.baseline

@@ -282,6 +282,10 @@ class NoTemplatesApiServiceClient extends NoTemplatesApiServiceGapicClient
   "interfaces": {
     "google.cloud.example.v1.NoTemplatesAPIService": {
       "retry_codes": {
+        "idempotent": [
+          "DEADLINE_EXCEEDED",
+          "UNAVAILABLE"
+        ],
         "non_idempotent": []
       },
       "retry_params": {

src/test/java/com/google/api/codegen/gapic/testdata/py/python_multiple_services.baseline

@@ -1073,6 +1073,10 @@ config = {
   "interfaces": {
     "google.cloud.example.v1.foo.DecrementerService": {
       "retry_codes": {
+        "idempotent": [
+          "DEADLINE_EXCEEDED",
+          "UNAVAILABLE"
+        ],
         "non_idempotent": []
       },
       "retry_params": {
@@ -1307,6 +1311,10 @@ config = {
   "interfaces": {
     "google.cloud.example.v1.foo.IncrementerService": {
       "retry_codes": {
+        "idempotent": [
+          "DEADLINE_EXCEEDED",
+          "UNAVAILABLE"
+        ],
         "non_idempotent": []
       },
       "retry_params": {

src/test/java/com/google/api/codegen/gapic/testdata/py/python_no_path_templates.baseline

@@ -1012,6 +1012,10 @@ config = {
   "interfaces": {
     "google.cloud.example.v1.NoTemplatesAPIService": {
       "retry_codes": {
+        "idempotent": [
+          "DEADLINE_EXCEEDED",
+          "UNAVAILABLE"
+        ],
         "non_idempotent": []
       },
       "retry_params": {

src/test/java/com/google/api/codegen/gapic/testdata/ruby/ruby_multiple_services.baseline

@@ -1146,6 +1146,10 @@ end
   "interfaces": {
     "google.cloud.example.v1.foo.DecrementerService": {
       "retry_codes": {
+        "idempotent": [
+          "DEADLINE_EXCEEDED",
+          "UNAVAILABLE"
+        ],
         "non_idempotent": []
       },
       "retry_params": {
@@ -1425,6 +1429,10 @@ end
   "interfaces": {
     "google.cloud.example.v1.foo.IncrementerService": {
       "retry_codes": {
+        "idempotent": [
+          "DEADLINE_EXCEEDED",
+          "UNAVAILABLE"
+        ],
         "non_idempotent": []
       },
       "retry_params": {

src/test/java/com/google/api/codegen/protoannotations/testdata/java_library_no_gapic_config.baseline

@@ -8278,6 +8278,9 @@ public class MyProtoStubSettings extends StubSettings<MyProtoStubSettings> {
 
     static {
       ImmutableMap.Builder<String, ImmutableSet<StatusCode.Code>> definitions = ImmutableMap.builder();
+      definitions.put(
+          "idempotent",
+          ImmutableSet.copyOf(Lists.<StatusCode.Code>newArrayList(StatusCode.Code.DEADLINE_EXCEEDED, StatusCode.Code.UNAVAILABLE)));
       definitions.put(
           "non_idempotent",
           ImmutableSet.copyOf(Lists.<StatusCode.Code>newArrayList()));

src/test/java/com/google/api/codegen/protoannotations/testdata/ruby_library_no_gapic_config.baseline

@@ -5143,6 +5143,10 @@ end
   "interfaces": {
     "google.example.library.v1.MyProto": {
       "retry_codes": {
+        "idempotent": [
+          "DEADLINE_EXCEEDED",
+          "UNAVAILABLE"
+        ],
         "non_idempotent": []
       },
       "retry_params": {

src/test/java/com/google/api/codegen/transformer/RetryDefinitionsTransformerTest.java

@@ -99,8 +99,7 @@ public void testWithConfigAndInterface() {
     DiagCollector diagCollector = new BoundedDiagCollector();
 
     RetryCodesConfig retryCodesConfig =
-        RetryCodesConfig.create(
-            diagCollector, interfaceConfigProto, apiInterface.getMethods(), protoParser);
+        RetryCodesConfig.create(interfaceConfigProto, apiInterface.getMethods(), protoParser);
 
     Map<String, ImmutableList<String>> retryCodesDef = retryCodesConfig.getRetryCodesDefinition();
     Map<String, String> retryCodesMap = retryCodesConfig.getMethodRetryNames();
@@ -154,11 +153,8 @@ public void testWithInterfaceOnly() {
             .addMethods(MethodConfigProto.newBuilder().setName(PERMISSION_DENIED_METHOD_NAME))
             .build();
 
-    DiagCollector diagCollector = new BoundedDiagCollector();
-
     RetryCodesConfig retryCodesConfig =
-        RetryCodesConfig.create(
-            diagCollector, bareBonesConfigProto, apiInterface.getMethods(), protoParser);
+        RetryCodesConfig.create(bareBonesConfigProto, apiInterface.getMethods(), protoParser);
 
     Map<String, ImmutableList<String>> retryCodesDef = retryCodesConfig.getRetryCodesDefinition();
     Map<String, String> retryCodesMap = retryCodesConfig.getMethodRetryNames();