Don't retry permanent failures when uploading ICANN monthly reports (#328 )

* Don't retry permanent failures when uploading ICANN monthly reports There are two kinds of permanent failures that this checks for that we know will never succeed, so it makes no sense to continue retrying 11 more times before moving onto the next file to upload. These errors are: 1. com.google.api.client.http.HttpResponseException: 403 Your IP address xx.xx.xx.xx is not allowed to connect 2. com.google.api.client.http.HttpResponseException: 400 <?xml version="1.0" encoding="UTF-8" standalone="yes"?><response xmlns="urn:ietf:params:xml:ns:iirdea-1.0"><result code="2002"><msg>A report for that month already exists, the cut-off date already passed.</msg><description>Date: 2019-09</description></result></response> In order to implement this new functionality, this commit also adds a new way to call Retriable that allows specifying the isRetryable Predicate (which is quite useful).
Add a test task for all schema-dependent tests (#325 )
2026-07-26 10:03:23 +00:00 · 2019-10-25 13:54:47 -04:00 · 2019-10-25 13:25:01 -04:00 · 2019-10-25 12:40:43 -04:00 · 2019-10-25 10:24:59 -04:00
35 changed files with 375 additions and 83 deletions
@@ -16,7 +16,7 @@ com.jcraft:jsch:0.1.54
 com.jcraft:jzlib:1.1.1
 com.netflix.nebula:gradle-lint-plugin:10.4.2
 com.netflix.nebula:nebula-gradle-interop:1.0.11
-com.netflix.nebula:nebula-test:7.3.0
+com.netflix.nebula:nebula-test:7.4.0
 commons-codec:commons-codec:1.9
 commons-io:commons-io:2.5
 commons-lang:commons-lang:2.6
@@ -41,7 +41,7 @@ org.apache.maven:maven-builder-support:3.6.2
 org.apache.maven:maven-model-builder:3.6.2
 org.apache.maven:maven-model:3.6.2
 org.codehaus.gpars:gpars:1.2.1
-org.codehaus.groovy:groovy-all:2.4.9
+org.codehaus.groovy:groovy-all:2.4.15
 org.codehaus.groovy:groovy-ant:2.1.8
 org.codehaus.groovy:groovy-groovydoc:2.1.8
 org.codehaus.groovy:groovy-templates:2.1.8
@@ -65,4 +65,4 @@ org.multiverse:multiverse-core:0.7.0
 org.objenesis:objenesis:2.4
 org.ow2.asm:asm:7.0
 org.slf4j:slf4j-api:1.7.2
-org.spockframework:spock-core:1.1-groovy-2.4-rc-4
+org.spockframework:spock-core:1.3-groovy-2.4
@@ -90,6 +90,9 @@
    {
      "moduleLicense": "The BSD License"
    },
+    {
+      "moduleLicense": "The New BSD License"
+    },
    {
      "moduleLicense": "The PostgreSQL License"
    },
@@ -198,6 +201,9 @@
    {
      "moduleLicense": "The MIT license"
    },
+    {
+      "moduleLicense": "The MIT License (MIT)"
+    },
    {
      "moduleLicense": "The PostgreSQL License"
    },
@@ -192,7 +192,7 @@ dependencies {
  compile deps['com.jcraft:jsch']
  testCompile deps['com.thoughtworks.qdox:qdox']
  compile deps['dnsjava:dnsjava']
-  runtime deps['org.glassfish.jaxb:jaxb-runtime']
+  testCompile deps['io.github.classgraph:classgraph']
  testCompile deps['javax.annotation:jsr250-api']
  compile deps['javax.inject:javax.inject']
  compile deps['javax.mail:mail']
@@ -218,6 +218,7 @@ dependencies {
  compile deps['org.bouncycastle:bcpg-jdk15on']
  testCompile deps['org.bouncycastle:bcpkix-jdk15on']
  compile deps['org.bouncycastle:bcprov-jdk15on']
+  runtime deps['org.glassfish.jaxb:jaxb-runtime']
  compile deps['org.joda:joda-money']
  compile deps['org.json:json']
  testCompile deps['org.mortbay.jetty:jetty']
@@ -606,6 +607,15 @@ task outcastTest(type: FilteringTest) {
  maxParallelForks 5
 }

+// Dedicated test suite for schema-dependent tests.
+task sqlIntegrationTest(type: Test) {
+  include 'google/registry/schema/integration/SqlIntegrationTestSuite.*'
+  // Copied from FilteringTest. Not inheriting b/c it excludes TestSuites.
+  if (project.testFilter) {
+    testNameIncludePatterns = project.testFilter.split(',')
+  }
+}
+
 task findGoldenImages(type: JavaExec) {
  classpath = sourceSets.test.runtimeClasspath
  main = 'google.registry.webdriver.GoldenImageFinder'
@@ -130,6 +130,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -129,6 +129,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -141,6 +141,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -141,6 +141,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -14,8 +14,15 @@

 package google.registry.model.transaction;

+import static google.registry.config.RegistryEnvironment.ALPHA;
+import static google.registry.config.RegistryEnvironment.CRASH;
+
+import com.google.appengine.api.utils.SystemProperty;
+import com.google.appengine.api.utils.SystemProperty.Environment.Value;
 import com.google.common.annotations.VisibleForTesting;
+import google.registry.config.RegistryEnvironment;
 import google.registry.model.ofy.DatastoreTransactionManager;
+import google.registry.persistence.DaggerPersistenceComponent;

 /** Factory class to create {@link TransactionManager} instance. */
 // TODO: Rename this to PersistenceFactory and move to persistence package.
@@ -27,19 +34,11 @@ public class TransactionManagerFactory {
  private TransactionManagerFactory() {}

  private static JpaTransactionManager createJpaTransactionManager() {
-    // TODO(shicong): There is currently no environment where we want to create a real JPA
-    // transaction manager here.  The unit tests that require one are all set up using
-    // JpaTransactionManagerRule which launches its own PostgreSQL instance.  When we actually have
-    // PostgreSQL tables in production, ensure that all of the test environments are set up
-    // correctly and restore the code that creates a JpaTransactionManager when
-    // RegistryEnvironment.get() != UNITTEST.
-    //
-    // We removed the original code because it didn't work in sandbox (due to the absence of the
-    // persistence.xml file, which has since been added), and then (after adding this) didn't work
-    // in crash because the postgresql password hadn't been set up.  Prior to restoring, we'll need
-    // to do setup in all environments, and we probably only want to do this once we're actually
-    // using Cloud SQL for one of the new tables.
-    return DummyJpaTransactionManager.create();
+    if (shouldEnableJpaTm() && isInAppEngine()) {
+      return DaggerPersistenceComponent.create().appEngineJpaTransactionManager();
+    } else {
+      return DummyJpaTransactionManager.create();
+    }
  }

  private static TransactionManager createTransactionManager() {
@@ -54,8 +53,27 @@ public class TransactionManagerFactory {
   * {@link google.registry.tools.RegistryCli} to initialize jpaTm.
   */
  public static void initForTool() {
-    // TODO(shicong): Uncomment the line below when we set up Cloud SQL instance in all environments
-    // jpaTm = DaggerPersistenceComponent.create().nomulusToolJpaTransactionManager();
+    if (shouldEnableJpaTm()) {
+      jpaTm = DaggerPersistenceComponent.create().nomulusToolJpaTransactionManager();
+    }
+  }
+
+  // TODO(shicong): Enable JpaTm for all environments and remove this function
+  private static boolean shouldEnableJpaTm() {
+    return RegistryEnvironment.get() == ALPHA || RegistryEnvironment.get() == CRASH;
+  }
+
+  /**
+   * This function uses App Engine API to determine if the current runtime environment is App
+   * Engine.
+   *
+   * @see <a
+   *     href="https://cloud.google.com/appengine/docs/standard/java/javadoc/com/google/appengine/api/utils/SystemProperty">App
+   *     Engine API public doc</a>
+   */
+  private static boolean isInAppEngine() {
+    // SystemProperty.environment.value() returns null if the current runtime is local JVM
+    return SystemProperty.environment.value() == Value.Production;
  }

  /** Returns {@link TransactionManager} instance. */
@@ -51,6 +51,10 @@ public class PersistenceModule {
  public static final String HIKARI_MAXIMUM_POOL_SIZE = "hibernate.hikari.maximumPoolSize";
  public static final String HIKARI_IDLE_TIMEOUT = "hibernate.hikari.idleTimeout";

+  public static final String HIKARI_DS_SOCKET_FACTORY = "hibernate.hikari.dataSource.socketFactory";
+  public static final String HIKARI_DS_CLOUD_SQL_INSTANCE =
+      "hibernate.hikari.dataSource.cloudSqlInstance";
+
  @Provides
  @DefaultHibernateConfigs
  public static ImmutableMap<String, String> providesDefaultDatabaseConfigs() {
@@ -79,27 +83,31 @@ public class PersistenceModule {
    return properties.build();
  }

+  @Provides
+  @Singleton
+  @PartialCloudSqlConfigs
+  public static ImmutableMap<String, String> providesPartialCloudSqlConfigs(
+      @Config("cloudSqlJdbcUrl") String jdbcUrl,
+      @Config("cloudSqlInstanceConnectionName") String instanceConnectionName,
+      @DefaultHibernateConfigs ImmutableMap<String, String> defaultConfigs) {
+    HashMap<String, String> overrides = Maps.newHashMap(defaultConfigs);
+    overrides.put(Environment.URL, jdbcUrl);
+    overrides.put(HIKARI_DS_SOCKET_FACTORY, "com.google.cloud.sql.postgres.SocketFactory");
+    overrides.put(HIKARI_DS_CLOUD_SQL_INSTANCE, instanceConnectionName);
+    return ImmutableMap.copyOf(overrides);
+  }
+
  @Provides
  @Singleton
  @AppEngineJpaTm
  public static JpaTransactionManager providesAppEngineJpaTm(
-      @Config("cloudSqlJdbcUrl") String jdbcUrl,
      @Config("cloudSqlUsername") String username,
-      @Config("cloudSqlInstanceConnectionName") String instanceConnectionName,
      KmsKeyring kmsKeyring,
-      @DefaultHibernateConfigs ImmutableMap<String, String> defaultConfigs,
+      @PartialCloudSqlConfigs ImmutableMap<String, String> cloudSqlConfigs,
      Clock clock) {
-    HashMap<String, String> overrides = Maps.newHashMap(defaultConfigs);
-
-    // For Java users, the Cloud SQL JDBC Socket Factory can provide authenticated connections.
-    // See https://github.com/GoogleCloudPlatform/cloud-sql-jdbc-socket-factory for details.
-    overrides.put("socketFactory", "com.google.cloud.sql.postgres.SocketFactory");
-    overrides.put("cloudSqlInstance", instanceConnectionName);
-
-    overrides.put(Environment.URL, jdbcUrl);
+    HashMap<String, String> overrides = Maps.newHashMap(cloudSqlConfigs);
    overrides.put(Environment.USER, username);
    overrides.put(Environment.PASS, kmsKeyring.getCloudSqlPassword());
-
    return new JpaTransactionManagerImpl(create(overrides), clock);
  }

@@ -107,27 +115,13 @@ public class PersistenceModule {
  @Singleton
  @NomulusToolJpaTm
  public static JpaTransactionManager providesNomulusToolJpaTm(
-      @Config("toolsCloudSqlJdbcUrl") String jdbcUrl,
      @Config("toolsCloudSqlUsername") String username,
-      @Config("cloudSqlInstanceConnectionName") String instanceConnectionName,
      KmsKeyring kmsKeyring,
-      @DefaultHibernateConfigs ImmutableMap<String, String> defaultConfigs,
+      @PartialCloudSqlConfigs ImmutableMap<String, String> cloudSqlConfigs,
      Clock clock) {
-
-    // Cloud SQL JDBC Socket Factory library requires the jdbc url to include all connection
-    // information, otherwise the connection initialization will fail. See here for more details:
-    // https://github.com/GoogleCloudPlatform/cloud-sql-jdbc-socket-factory
-    String fullJdbcUrl =
-        new StringBuilder(jdbcUrl)
-            .append("?cloudSqlInstance=" + instanceConnectionName)
-            .append("&socketFactory=com.google.cloud.sql.postgres.SocketFactory")
-            .append("&user=" + username)
-            .append("&password=" + kmsKeyring.getCloudSqlPassword())
-            .toString();
-
-    HashMap<String, String> overrides = Maps.newHashMap(defaultConfigs);
-    overrides.put(Environment.URL, fullJdbcUrl);
-
+    HashMap<String, String> overrides = Maps.newHashMap(cloudSqlConfigs);
+    overrides.put(Environment.USER, username);
+    overrides.put(Environment.PASS, kmsKeyring.getToolsCloudSqlPassword());
    return new JpaTransactionManagerImpl(create(overrides), clock);
  }

@@ -166,6 +160,11 @@ public class PersistenceModule {
  @Documented
  @interface NomulusToolJpaTm {}

+  /** Dagger qualifier for the partial Cloud SQL configs. */
+  @Qualifier
+  @Documented
+  @interface PartialCloudSqlConfigs {}
+
  /** Dagger qualifier for the default Hibernate configurations. */
  // TODO(shicong): Change annotations in this class to none public or put them in a top level
  //  package
@@ -39,6 +39,7 @@ import google.registry.util.Retrier;
 import google.registry.util.SendEmailService;
 import java.io.IOException;
 import java.io.InputStream;
+import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 import javax.inject.Inject;
 import javax.mail.internet.InternetAddress;
@@ -102,7 +103,7 @@ public final class IcannReportingUploadAction implements Runnable {
                  final byte[] payload = readBytesFromGcs(gcsFilename);
                  return icannReporter.send(payload, reportFilename);
                },
-                IOException.class);
+                IcannReportingUploadAction::isUploadFailureRetryable);
      } catch (RuntimeException e) {
        logger.atWarning().withCause(e).log("Upload to %s failed.", gcsFilename);
      }
@@ -115,6 +116,21 @@ public final class IcannReportingUploadAction implements Runnable {
        String.format("OK, attempted uploading %d reports", manifestedFiles.size()));
  }

+  /** Don't retry when reports are already uploaded or can't be uploaded. */
+  private static final String ICANN_UPLOAD_PERMANENT_ERROR_MESSAGE =
+      "A report for that month already exists, the cut-off date already passed.";
+
+  /** Don't retry when the IP address isn't whitelisted, as retries go through the same IP. */
+  private static final Pattern ICANN_UPLOAD_WHITELIST_ERROR =
+      Pattern.compile("Your IP address .+ is not allowed to connect");
+
+  /** Predicate to retry uploads on IOException, so long as they aren't non-retryable errors. */
+  private static boolean isUploadFailureRetryable(Throwable e) {
+    return (e instanceof IOException)
+        && !e.getMessage().contains(ICANN_UPLOAD_PERMANENT_ERROR_MESSAGE)
+        && !ICANN_UPLOAD_WHITELIST_ERROR.matcher(e.getMessage()).matches();
+  }
+
  private void emailUploadResults(ImmutableMap<String, Boolean> reportSummary) {
    String subject = String.format(
        "ICANN Monthly report upload summary: %d/%d succeeded",
@@ -32,7 +32,7 @@ import java.util.List;

 /** A command to upload a {@link ClaimsListShard}. */
@Parameters(separators = " =", commandDescription = "Manually upload a new claims list file")
-final class UploadClaimsListCommand extends ConfirmingCommand implements CommandWithRemoteApi {
+final class UploadClaimsListCommand extends ConfirmingCommand implements CommandWithCloudSql {

  @Parameter(description = "Claims list filename")
  private List<String> mainParameters = new ArrayList<>();
@@ -154,6 +154,45 @@ public class IcannReportingUploadActionTest {
                new InternetAddress("recipient@example.com"),
                new InternetAddress("sender@example.com")));
  }
+
+  @Test
+  public void testFailure_quicklySkipsOverNonRetryableUploadException() throws Exception {
+    runTest_nonRetryableException(
+        new IOException(
+            "<msg>A report for that month already exists, the cut-off date already"
+                + " passed.</msg>"));
+  }
+
+  @Test
+  public void testFailure_quicklySkipsOverIpWhitelistException() throws Exception {
+    runTest_nonRetryableException(
+        new IOException("Your IP address 25.147.130.158 is not allowed to connect"));
+  }
+
+  private void runTest_nonRetryableException(Exception nonRetryableException) throws Exception {
+    IcannReportingUploadAction action = createAction();
+    when(mockReporter.send(PAYLOAD_FAIL, "a-activity-201706.csv"))
+        .thenThrow(nonRetryableException)
+        .thenThrow(
+            new AssertionError(
+                "This should never be thrown because the previous exception isn't retryable"));
+    action.run();
+    verify(mockReporter, times(1)).send(PAYLOAD_FAIL, "a-activity-201706.csv");
+    verify(mockReporter).send(PAYLOAD_SUCCESS, "test-transactions-201706.csv");
+    verifyNoMoreInteractions(mockReporter);
+    assertThat(((FakeResponse) action.response).getPayload())
+        .isEqualTo("OK, attempted uploading 2 reports");
+    verify(emailService)
+        .sendEmail(
+            EmailMessage.create(
+                "ICANN Monthly report upload summary: 1/2 succeeded",
+                "Report Filename - Upload status:\n"
+                    + "test-transactions-201706.csv - SUCCESS\n"
+                    + "a-activity-201706.csv - FAILURE",
+                new InternetAddress("recipient@example.com"),
+                new InternetAddress("sender@example.com")));
+  }
+
  @Test
  public void testFail_FileNotFound() throws Exception {
    IcannReportingUploadAction action = createAction();
@@ -0,0 +1,81 @@
+// Copyright 2018 The Nomulus Authors. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package google.registry.schema.integration;
+
+import static com.google.common.truth.Truth.assertWithMessage;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import com.google.common.collect.Sets.SetView;
+import google.registry.model.transaction.JpaTransactionManagerRule;
+import io.github.classgraph.ClassGraph;
+import io.github.classgraph.ScanResult;
+import java.lang.reflect.Field;
+import java.util.stream.Stream;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+import org.junit.runners.Suite.SuiteClasses;
+
+/**
+ * Verifies that all tests that depends on the Cloud SQL schema are included in the project's
+ * sqlIntegrationTest suite. Names of the test classes is set to the 'test.sqlIntergrationTests'
+ * system property as a comma-separated string.
+ *
+ * <p>A test is deemed dependent on the SQL schema iff it has a field with type {@link
+ * JpaTransactionManagerRule}.
+ */
+// TODO(weiminyu): consider generating a TestSuite class instead.
+@RunWith(JUnit4.class)
+public class SqlIntegrationMembershipTest {
+
+  @Test
+  public void sqlIntegrationMembershipComplete() {
+    ImmutableSet<String> sqlDependentTests;
+    try (ScanResult scanResult =
+        new ClassGraph()
+            .enableAnnotationInfo()
+            .whitelistPackages("google.registry")
+            .scan()) {
+      sqlDependentTests =
+          scanResult.getClassesWithAnnotation(RunWith.class.getName()).stream()
+              .filter(clazz -> clazz.getSimpleName().endsWith("Test"))
+              .map(clazz -> clazz.loadClass())
+              .filter(SqlIntegrationMembershipTest::isSqlDependent)
+              .map(Class::getName)
+              .collect(ImmutableSet.toImmutableSet());
+    }
+    ImmutableSet<String> declaredTests =
+        Stream.of(SqlIntegrationTestSuite.class.getAnnotation(SuiteClasses.class).value())
+            .map(Class::getName)
+            .collect(ImmutableSet.toImmutableSet());
+    SetView<String> undeclaredTests = Sets.difference(sqlDependentTests, declaredTests);
+    assertWithMessage(
+            "Undeclared sql-dependent tests found. "
+                + "Please add them to SqlIntegrationTestSuite.java.")
+        .that(undeclaredTests)
+        .isEmpty();
+    SetView<String> unnecessaryDeclarations = Sets.difference(declaredTests, sqlDependentTests);
+    assertWithMessage("Found tests that should not be included in SqlIntegrationTestSuite.java.")
+        .that(unnecessaryDeclarations)
+        .isEmpty();
+  }
+
+  private static boolean isSqlDependent(Class<?> testClass) {
+    return Stream.of(testClass.getDeclaredFields())
+        .map(Field::getType)
+        .anyMatch(JpaTransactionManagerRule.class::equals);
+  }
+}
@@ -0,0 +1,52 @@
+// Copyright 2018 The Nomulus Authors. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package google.registry.schema.integration;
+
+import google.registry.model.registry.RegistryLockDaoTest;
+import google.registry.model.tmch.ClaimsListDaoTest;
+import google.registry.model.transaction.JpaTransactionManagerImplTest;
+import google.registry.model.transaction.JpaTransactionManagerRuleTest;
+import google.registry.persistence.BloomFilterConverterTest;
+import google.registry.persistence.CreateAutoTimestampConverterTest;
+import google.registry.persistence.UpdateAutoTimestampConverterTest;
+import google.registry.persistence.ZonedDateTimeConverterTest;
+import google.registry.schema.tld.PremiumListDaoTest;
+import org.junit.runner.RunWith;
+import org.junit.runners.Suite;
+import org.junit.runners.Suite.SuiteClasses;
+
+/**
+ * Groups all tests that may depends on Cloud SQL schema. They will be run for server-schema
+ * compatibility check.
+ *
+ * <p>Schema dependency is approximated by the use of {@link
+ * google.registry.model.transaction.JpaTransactionManagerRule}.
+ *
+ * @see SqlIntegrationMembershipTest
+ */
+// TODO(weiminyu): refactor JpaTransactionManagerRule to eliminate false positives.
+@RunWith(Suite.class)
+@SuiteClasses({
+  BloomFilterConverterTest.class,
+  ClaimsListDaoTest.class,
+  CreateAutoTimestampConverterTest.class,
+  JpaTransactionManagerImplTest.class,
+  JpaTransactionManagerRuleTest.class,
+  PremiumListDaoTest.class,
+  RegistryLockDaoTest.class,
+  UpdateAutoTimestampConverterTest.class,
+  ZonedDateTimeConverterTest.class
+})
+public class SqlIntegrationTestSuite {}
@@ -193,6 +193,7 @@ public final class AppEngineRule extends ExternalResource {
        .setPassword("foo-BAR2")
        .setPhoneNumber("+1.3334445555")
        .setPhonePasscode("12345")
+        .setRegistryLockAllowed(false)
        .build();
  }

@@ -206,6 +207,7 @@ public final class AppEngineRule extends ExternalResource {
        .setPassword("password2")
        .setPhoneNumber("+1.2223334444")
        .setPhonePasscode("22222")
+        .setRegistryLockAllowed(true)
        .build();
  }

@@ -241,6 +243,19 @@ public final class AppEngineRule extends ExternalResource {
        .build();
  }

+  public static RegistrarContact makeRegistrarContact3() {
+    return new RegistrarContact.Builder()
+        .setParent(makeRegistrar2())
+        .setName("Marla Singer")
+        .setEmailAddress("Marla.Singer@crr.com")
+        .setPhoneNumber("+1.2128675309")
+        .setTypes(ImmutableSet.of(RegistrarContact.Type.TECH))
+        .setGaeUserId(THE_REGISTRAR_GAE_USER_ID)
+        .setAllowedToSetRegistryLockPassword(true)
+        .setRegistryLockPassword("hi")
+        .build();
+  }
+
  /** Hack to make sure AppEngineRule is always wrapped in a TemporaryFolder rule. */
  @Override
  public Statement apply(Statement base, Description description) {
@@ -412,6 +427,10 @@ public final class AppEngineRule extends ExternalResource {
  public static void loadInitialData() {
    persistSimpleResources(
        ImmutableList.of(
-            makeRegistrar1(), makeRegistrarContact1(), makeRegistrar2(), makeRegistrarContact2()));
+            makeRegistrar1(),
+            makeRegistrarContact1(),
+            makeRegistrar2(),
+            makeRegistrarContact2(),
+            makeRegistrarContact3()));
  }
 }
@@ -21,6 +21,7 @@ import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.testing.DatastoreHelper.createTld;
 import static google.registry.testing.DatastoreHelper.loadRegistrar;

+import com.google.common.collect.ImmutableSortedSet;
 import google.registry.model.ImmutableObject;
 import google.registry.model.ofy.CommitLogManifest;
 import google.registry.model.ofy.CommitLogMutation;
@@ -62,13 +63,16 @@ public class ResaveEnvironmentEntitiesCommandTest
        transform(
            ofy().load().type(CommitLogMutation.class).list(),
            mutation -> ofy().load().fromEntity(mutation.getEntity()));
+    ImmutableSortedSet<RegistrarContact> theRegistrarContacts =
+        loadRegistrar("TheRegistrar").getContacts();
    assertThat(savedEntities)
        .containsExactly(
            // The Registrars and RegistrarContacts are created by AppEngineRule.
            loadRegistrar("TheRegistrar"),
            loadRegistrar("NewRegistrar"),
            Registry.get("tld"),
-            getOnlyElement(loadRegistrar("TheRegistrar").getContacts()),
+            theRegistrarContacts.first(),
+            theRegistrarContacts.last(),
            getOnlyElement(loadRegistrar("NewRegistrar").getContacts()));
  }

@@ -35,6 +35,7 @@ import org.junit.Test;
 import org.junit.rules.Timeout;
 import org.junit.runner.RunWith;
 import org.openqa.selenium.By;
+import org.openqa.selenium.WebElement;

 /** WebDriver tests for Registrar Console UI. */
@RunWith(RepeatableRunner.class)
@@ -78,6 +79,12 @@ public class RegistrarConsoleWebTest extends WebDriverTestCase {
    assertThat(driver.waitForElement(By.id(eltId)).isDisplayed()).isFalse();
  }

+  /** Checks that searching with the given By produces at least one element with the given text. */
+  void assertEltTextPresent(By by, String toFind) {
+    assertThat(driver.findElements(by).stream().map(WebElement::getText).anyMatch(toFind::equals))
+        .isTrue();
+  }
+
  @Test
  public void testEditButtonsVisibility_owner() throws Throwable {
    driver.get(server.getUrl("/registrar#whois-settings"));
@@ -190,9 +197,11 @@ public class RegistrarConsoleWebTest extends WebDriverTestCase {
    ImmutableList<RegistrarContact> contacts =
        server.runInAppEngineEnvironment(
            () -> loadRegistrar("TheRegistrar").getContacts().asList());
-    assertEltText("contacts[0].name", contacts.get(0).getName());
-    assertEltText("contacts[0].emailAddress", contacts.get(0).getEmailAddress());
-    assertEltText("contacts[0].phoneNumber", contacts.get(0).getPhoneNumber());
+    for (RegistrarContact contact : contacts) {
+      assertEltTextPresent(By.id("contacts[0].name"), contact.getName());
+      assertEltTextPresent(By.id("contacts[0].emailAddress"), contact.getEmailAddress());
+      assertEltTextPresent(By.id("contacts[0].phoneNumber"), contact.getPhoneNumber());
+    }
  }

  @Test
@@ -13,6 +13,7 @@ contacts:
    ADDED:
        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Extra Terrestrial, emailAddress=etphonehome@example.com, phoneNumber=+1.2345678901, faxNumber=null, types=[ADMIN, BILLING, TECH, WHOIS], gaeUserId=null, visibleInWhoisAsAdmin=true, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false}
    REMOVED:
+        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Marla Singer, emailAddress=Marla.Singer@crr.com, phoneNumber=+1.2128675309, faxNumber=null, types=[TECH], gaeUserId=31337, visibleInWhoisAsAdmin=false, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false},
        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=John Doe, emailAddress=johndoe@theregistrar.com, phoneNumber=+1.1234567890, faxNumber=null, types=[ADMIN], gaeUserId=31337, visibleInWhoisAsAdmin=false, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false},
        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Jian-Yang, emailAddress=jyang@bachman.accelerator, phoneNumber=+1.1234567890, faxNumber=null, types=[TECH], gaeUserId=null, visibleInWhoisAsAdmin=false, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false}
    FINAL CONTENTS:
@@ -79,6 +79,7 @@ ext {
      'com.sun.xml.bind:jaxb-xjc:2.2.11',
      'com.thoughtworks.qdox:qdox:1.12.1',
      'dnsjava:dnsjava:2.1.7',
+      'io.github.classgraph:classgraph:4.8.52',
      'io.github.java-diff-utils:java-diff-utils:4.0',
      'io.netty:netty-buffer:4.1.31.Final',
      'io.netty:netty-codec:4.1.31.Final',
@@ -141,6 +141,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -130,6 +130,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -141,6 +141,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -141,6 +141,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -141,6 +141,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -130,6 +130,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -141,6 +141,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -141,6 +141,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -141,6 +141,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -130,6 +130,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -141,6 +141,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -141,6 +141,7 @@ commons-codec:commons-codec:1.11
 commons-logging:commons-logging:1.2
 dnsjava:dnsjava:2.1.7
 io.dropwizard.metrics:metrics-core:3.1.2
+io.github.classgraph:classgraph:4.8.52
 io.grpc:grpc-all:1.17.1
 io.grpc:grpc-alts:1.17.1
 io.grpc:grpc-auth:1.17.1
@@ -70,32 +70,8 @@ public class Retrier implements Serializable {
   *
   * @return <V> the value returned by the {@link Callable}.
   */
-  private <V> V callWithRetry(
-      Callable<V> callable,
-      FailureReporter failureReporter,
-      Predicate<Throwable> isRetryable) {
-    int failures = 0;
-    while (true) {
-      try {
-        return callable.call();
-      } catch (Throwable e) {
-        if (++failures == attempts || !isRetryable.test(e)) {
-          throwIfUnchecked(e);
-          throw new RuntimeException(e);
-        }
-        failureReporter.beforeRetry(e, failures, attempts);
-        try {
-          // Wait 100ms on the first attempt, doubling on each subsequent attempt.
-          sleeper.sleep(Duration.millis(pow(2, failures) * 100));
-        } catch (InterruptedException e2) {
-          // Since we're not rethrowing InterruptedException, set the interrupt state on the thread
-          // so the next blocking operation will know to abort the thread.
-          Thread.currentThread().interrupt();
-          throwIfUnchecked(e);
-          throw new RuntimeException(e);
-        }
-      }
-    }
+  public <V> V callWithRetry(Callable<V> callable, Predicate<Throwable> isRetryable) {
+    return callWithRetry(callable, LOGGING_FAILURE_REPORTER, isRetryable);
  }

  /**
@@ -169,6 +145,34 @@ public class Retrier implements Serializable {
    callWithRetry(callable.asCallable(), failureReporter, retryableError, moreRetryableErrors);
  }

+  private <V> V callWithRetry(
+      Callable<V> callable,
+      FailureReporter failureReporter,
+      Predicate<Throwable> isRetryable) {
+    int failures = 0;
+    while (true) {
+      try {
+        return callable.call();
+      } catch (Throwable e) {
+        if (++failures == attempts || !isRetryable.test(e)) {
+          throwIfUnchecked(e);
+          throw new RuntimeException(e);
+        }
+        failureReporter.beforeRetry(e, failures, attempts);
+        try {
+          // Wait 100ms on the first attempt, doubling on each subsequent attempt.
+          sleeper.sleep(Duration.millis(pow(2, failures) * 100));
+        } catch (InterruptedException e2) {
+          // Since we're not rethrowing InterruptedException, set the interrupt state on the thread
+          // so the next blocking operation will know to abort the thread.
+          Thread.currentThread().interrupt();
+          throwIfUnchecked(e);
+          throw new RuntimeException(e);
+        }
+      }
+    }
+  }
+
  private static final FailureReporter LOGGING_FAILURE_REPORTER =
      (thrown, failures, maxAttempts) ->
          logger.atInfo().withCause(thrown).log(
@@ -38,7 +38,7 @@ public class RetrierTest {
    }
  }

-  /** Test object that always throws an exception with the current count. */
+  /** Test object that throws CountingExceptions up to a given limit, then succeeds. */
  static class CountingThrower implements Callable<Integer> {

    int count = 0;
@@ -125,4 +125,21 @@ public class RetrierTest {
        .isEqualTo(0);
    assertThat(reporter.numBeforeRetry).isEqualTo(0);
  }
+
+  @Test
+  public void testRetryPredicate_succeedsWhenRetries() {
+    // Throws a retryable "1" exception is retryable, and then it succeeds on "1".
+    assertThat(retrier.callWithRetry(new CountingThrower(1), e -> e.getMessage().equals("1")))
+        .isEqualTo(1);
+  }
+
+  @Test
+  public void testRetryPredicate_failsWhenDoesntRetry() {
+    // Throws a retryable "1" exception, then a non-retryable "2" exception, resulting in failure.
+    CountingException ex =
+        assertThrows(
+            CountingException.class,
+            () -> retrier.callWithRetry(new CountingThrower(2), e -> e.getMessage().equals("1")));
+    assertThat(ex).hasMessageThat().isEqualTo("2");
+  }
 }
Author	SHA1	Message	Date
Ben McIlwainandGitHub	dbb4092680	Don't retry permanent failures when uploading ICANN monthly reports (#328 ) * Don't retry permanent failures when uploading ICANN monthly reports There are two kinds of permanent failures that this checks for that we know will never succeed, so it makes no sense to continue retrying 11 more times before moving onto the next file to upload. These errors are: 1. com.google.api.client.http.HttpResponseException: 403 Your IP address xx.xx.xx.xx is not allowed to connect 2. com.google.api.client.http.HttpResponseException: 400 <?xml version="1.0" encoding="UTF-8" standalone="yes"?><response xmlns="urn:ietf:params:xml:ns:iirdea-1.0"><result code="2002"><msg>A report for that month already exists, the cut-off date already passed.</msg><description>Date: 2019-09</description></result></response> In order to implement this new functionality, this commit also adds a new way to call Retriable that allows specifying the isRetryable Predicate (which is quite useful).	2019-10-25 13:54:47 -04:00
Weimin YuandGitHub	af3223d086	Add a test task for all schema-dependent tests (#325 ) Such tests are used in server-schema compatibility tests. Also added a test that verifies that all eligible tests are included.	2019-10-25 13:25:01 -04:00
Shicong HuangandGitHub	2d7f80baaf	Fix issues related to Cloud SQL connection (#321 ) * Reenable JpaTransactionManager for Alpha and Crash * Make UploadClaimsListCommand implement CommandWithCloudSql * Fix wrong call to get password * Use Cloud SQL Socket library to provision TransactionManager * Change to use dataSource configs	2019-10-25 12:40:43 -04:00
gbrodmanandGitHub	d4d5d6a570	Add another test contact for Registry Lock testing (#324 ) * Add another test contact for Registry Lock testing Previously, we only had two contacts -- one per registrar. This PR adds a second, registry-lock-enabled, contact to one registrar for two reasons: 1. For registry-lock-related testing, we'd like to be able to test both positively and negatively, making sure that the permissions work the way they should 2. In general, the UI tests should include the case where we have multiple contacts in the same registrar. Previously, this was never the case in tests. * Merge remote-tracking branch 'origin/master' into addTestContact	2019-10-25 10:24:59 -04:00