ExternalData: Use ResourceCache to get data (osmlab#398)

* Gradle: Add JUnit5 JUnit5 has better support for exceptions (assertThrows, assertDoesNotThrow, etc.). Signed-off-by: Taylor Smock <[email protected]> * ExternalData: Allow checks to obtain and use arbitrary external data WaterWayChecks: Add note in documentation about resolution and NASA SRTM ElevationUtilities: External data: Add tests to ensure that data is fetched. ExternalData: * Use ResourceCache to get data * Use check constructors to fetch necessary data or to store the file fetcher to be used later. Each check can decide how it fetches data. * See docs/externalData.md for implementation notes. CheckResourceLoader: Fix all code smells Signed-off-by: Taylor Smock <[email protected]>
sayas01 · Dec 30, 2020 · 56533dc · 56533dc
1 parent 6435658
commit 56533dc
Show file tree

Hide file tree

Showing 15 changed files with 509 additions and 95 deletions.
diff --git a/build.gradle b/build.gradle
@@ -97,6 +97,13 @@ dependencies
     checkstyle packages.atlas_checkstyle
 
     shaded project.configurations.getByName('compile')
+
+    // Support Junit 5 tests
+    testImplementation packages.junit.api
+    testRuntimeOnly packages.junit.engine
+    // Support JUnit 3/4 tests
+    testCompileOnly packages.junit.junit4
+    testRuntimeOnly packages.junit.vintage
 }
 
 /**

diff --git a/dependencies.gradle b/dependencies.gradle
@@ -9,6 +9,8 @@ project.ext.versions = [
     postgres: '42.2.6',
     spring: '4.2.2.RELEASE',
     mockito: '2.23.0',
+    junit4: '4.13.1',
+    junit: '5.7.0',
     log4j: '1.2.17'
 ]
 
@@ -22,5 +24,11 @@ project.ext.packages = [
     postgres: "org.postgresql:postgresql:${versions.postgres}",
     spring: "org.springframework:spring-jdbc:${versions.spring}",
     mockito: "org.mockito:mockito-core:${versions.mockito}",
+    junit: [
+        junit4: "junit:junit:${versions.junit4}",
+        vintage: "org.junit.vintage:junit-vintage-engine:${versions.junit}",
+        api: "org.junit.jupiter:junit-jupiter-api:${versions.junit}",
+        engine: "org.junit.jupiter:junit-jupiter-engine:${versions.junit}",
+    ],
     log4j: "log4j:log4j:${versions.log4j}"
 ]
diff --git a/docs/checks/waterWayChecks.md b/docs/checks/waterWayChecks.md
@@ -21,6 +21,8 @@ This check identifies waterways that are closed (i.e., would have a circular wat
     }
 }
 ```
+If you are using the NASA SRTM elevation files, their resolution is either 30m or 90m. _This means that the elevation checks will ignore uphill waterways by default when using the NASA SRTM files_.
+
 You may also want to look at [ElevationUtilities](../utilities/elevationUtilities.md).
 
 #### Code Review

diff --git a/docs/externalData.md b/docs/externalData.md
@@ -0,0 +1,42 @@
+# External Data
+## Common information
+External data is defined as any data that is not an atlas file. External data
+_MUST_ be placed in the same directory as the atlas files, with the name
+`extra`.
+
+In some cases, if the external data file is _very large_, the data transfer
+may be interrupted, and the process will fail, absent error handling.
+
+## Using external data in a check
+There must be a constructor for the check that follows this definition:
+`public Check(Configuration configuration, ExternalDataFetcher fetcher)`
+
+At this point, the `ExternalDataFetcher` can be used to prefetch data, or
+stored later to be used to dynamically get data as the check progresses.
+The latter is generally recommended, since the data is cached as it is needed.
+This means that the data will not be transferred if the check is cancelled or
+has an error prior to needing the data.
+
+### Example code
+```java
+public Check(Configuration configuration, ExternalDataFetcher fetcher) {
+    super(configuration);
+    Optional<Resource> optionalResource = fetcher.apply("filename");
+    if (optionalResource.isPresent()) {
+        // Congratulations, you have the data
+        Resource resource = optionalResource.get();
+        // At this point, you can get an InputStream.
+        InputStream inputStream = resource.read();
+        /* Currently, there is no good way to get the actual file on the local
+         * filesystem. This should be implemented as soon as that functionality
+         * is required.
+         *
+         * Implementation note: The returned resource will most likely be a
+         * InputStreamResource. This, however, can change.
+         */
+    } else {
+        // You don't have the data. Either error out or log, depending upon
+        // how critical the data is.
+    }
+}
+```
diff --git a/gradle/quality.gradle b/gradle/quality.gradle
@@ -24,6 +24,7 @@ sourceSets
 
 test
 {
+    useJUnitPlatform()
     testLogging
     {
         events "failed"

diff --git a/src/main/java/org/openstreetmap/atlas/checks/base/CheckResourceLoader.java b/src/main/java/org/openstreetmap/atlas/checks/base/CheckResourceLoader.java
@@ -61,6 +61,7 @@ public class CheckResourceLoader
             + BaseCheck.PARAMETER_DENYLIST_COUNTRIES;
     private final Class<?> checkType;
     private final Configuration configuration;
+    private final ExternalDataFetcher fileFetcher;
     private final MultiMap<String, String> countryGroups = new MultiMap<>();
     private final Boolean enabledByDefault;
     private final String enabledKeyTemplate;
@@ -74,18 +75,30 @@ public class CheckResourceLoader
      * @param configuration
      *            the {@link Configuration} for loaded checks
      */
-    @SuppressWarnings("unchecked")
     public CheckResourceLoader(final Configuration configuration)
+    {
+        this(configuration, null);
+    }
+
+    /**
+     * Default constructor
+     *
+     * @param configuration
+     *            the {@link Configuration} for loaded checks
+     * @param fileFetcher
+     *            the {@link ExternalDataFetcher} to load additional data with
+     */
+    @SuppressWarnings("unchecked")
+    public CheckResourceLoader(final Configuration configuration,
+            final ExternalDataFetcher fileFetcher)
     {
         this.packages = Collections.unmodifiableSet(Iterables.asSet((Iterable<String>) configuration
                 .get("CheckResourceLoader.scanUrls", Collections.singletonList(DEFAULT_PACKAGE))
                 .value()));
         final Map<String, List<String>> groups = configuration.get("groups", Collections.emptyMap())
                 .value();
-        groups.keySet().forEach(group ->
-        {
-            groups.get(group).forEach(country -> this.countryGroups.add(country, group));
-        });
+        groups.keySet().forEach(group -> groups.get(group)
+                .forEach(country -> this.countryGroups.add(country, group)));
 
         final ClassLoader loader = Thread.currentThread().getContextClassLoader();
         try
@@ -106,6 +119,7 @@ public CheckResourceLoader(final Configuration configuration)
         this.checkPermitList = configuration.get("CheckResourceLoader.checks.permitlist")
                 .valueOption();
         this.checkDenyList = configuration.get("CheckResourceLoader.checks.denylist").valueOption();
+        this.fileFetcher = fileFetcher;
     }
 
     public Configuration getConfiguration()
@@ -137,14 +151,14 @@ public Configuration getConfigurationForCountry(final String country)
         return specializedConfiguration;
     }
 
-    public <T extends Check> Set<T> loadChecks(final Predicate<Class> isEnabled)
+    public <T extends Check> Set<T> loadChecks(final Predicate<Class<?>> isEnabled)
     {
-        return this.loadChecks(isEnabled, this.configuration);
+        return this.loadChecks(isEnabled, this.configuration, this.fileFetcher);
     }
 
     public <T extends Check> Set<T> loadChecks(final Configuration configuration)
     {
-        return this.loadChecks(this::isEnabledByConfiguration, configuration);
+        return this.loadChecks(this::isEnabledByConfiguration, configuration, this.fileFetcher);
     }
 
     /**
@@ -154,21 +168,40 @@ public <T extends Check> Set<T> loadChecks(final Configuration configuration)
      *            {@link Predicate} used to determine if a check is enabled
      * @param configuration
      *            {@link Configuration} used to loadChecks {@link CheckResourceLoader}
+     * @param fileFetcher
+     *            {@link ExternalDataFetcher} used to load additional data
      * @param <T>
      *            check type
      * @return a {@link Set} of checks
      */
-    @SuppressWarnings("unchecked")
-    public <T extends Check> Set<T> loadChecks(final Predicate<Class> isEnabled,
-            final Configuration configuration)
+    public <T extends Check> Set<T> loadChecks(final Predicate<Class<?>> isEnabled,
+            final Configuration configuration, final ExternalDataFetcher fileFetcher)
     {
-        final Class<?>[][] constructorArgumentTypes = new Class<?>[][] { { Configuration.class },
-                {} };
-        final Object[][] constructorArguments = new Object[][] { { configuration }, {} };
+        final Class<?>[][] constructorArgumentTypes = new Class<?>[][] {
+                { Configuration.class, ExternalDataFetcher.class }, { Configuration.class }, {} };
+        final Object[][] constructorArguments = new Object[][] { { configuration, fileFetcher },
+                { configuration }, {} };
         return this.loadChecksUsingConstructors(isEnabled, constructorArgumentTypes,
                 constructorArguments);
     }
 
+    /**
+     * Loads checks that are enabled by some other means, defined by {@code isEnabled}
+     *
+     * @param isEnabled
+     *            {@link Predicate} used to determine if a check is enabled
+     * @param configuration
+     *            {@link Configuration} used to loadChecks {@link CheckResourceLoader}
+     * @param <T>
+     *            check type
+     * @return a {@link Set} of checks
+     */
+    public <T extends Check> Set<T> loadChecks(final Predicate<Class<?>> isEnabled,
+            final Configuration configuration)
+    {
+        return this.loadChecks(isEnabled, configuration, this.fileFetcher);
+    }
+
     /**
      * Loads checks that are enabled by configuration
      *
@@ -178,14 +211,15 @@ public <T extends Check> Set<T> loadChecks(final Predicate<Class> isEnabled,
      */
     public <T extends Check> Set<T> loadChecks()
     {
-        return this.loadChecks(this::isEnabledByConfiguration, this.configuration);
+        return this.loadChecks(this::isEnabledByConfiguration, this.configuration,
+                this.fileFetcher);
     }
 
     public <T extends Check> Set<T> loadChecksForCountry(final String country)
     {
         final Configuration countryConfiguration = this.getConfigurationForCountry(country);
         return this.loadChecks(checkClass -> this.isEnabledByConfiguration(countryConfiguration,
-                checkClass, country), countryConfiguration);
+                checkClass, country), countryConfiguration, this.fileFetcher);
     }
 
     public <T extends Check> Set<T> loadChecksUsingConstructors(
@@ -217,7 +251,8 @@ public <T extends Check> Set<T> loadChecksUsingConstructors(
      *            Any class that extends Check.
      * @return A set of enabled, initialized checks.
      */
-    public <T extends Check> Set<T> loadChecksUsingConstructors(final Predicate<Class> isEnabled,
+    @SuppressWarnings("unchecked")
+    public <T extends Check> Set<T> loadChecksUsingConstructors(final Predicate<Class<?>> isEnabled,
             final Class<?>[][] constructorArgumentTypes, final Object[][] constructorArguments)
     {
         final Set<T> checks = new HashSet<>();
@@ -301,20 +336,20 @@ private <T extends Check> Optional<T> initializeCheckWithArguments(final Class<T
         return Optional.empty();
     }
 
-    private boolean isEnabledByConfiguration(final Class checkClass)
+    private boolean isEnabledByConfiguration(final Class<?> checkClass)
     {
         return this.isEnabledByConfiguration(this.configuration, checkClass);
     }
 
     private boolean isEnabledByConfiguration(final Configuration configuration,
-            final Class checkClass)
+            final Class<?> checkClass)
     {
         final String key = String.format(this.enabledKeyTemplate, checkClass.getSimpleName());
         return configuration.get(key, this.enabledByDefault).value();
     }
 
     private boolean isEnabledByConfiguration(final Configuration configuration,
-            final Class checkClass, final String country)
+            final Class<?> checkClass, final String country)
     {
         final List<String> countryPermitlist = configuration
                 .get(String.format(COUNTRY_PERMITLIST_TEMPLATE, checkClass.getSimpleName()),
-Original file line number
+Diff line change
@@ Expand Up @@
         }
     }
     ```
+    If you are using the NASA SRTM elevation files, their resolution is either 30m or 90m. _This means that the elevation checks will ignore uphill waterways by default when using the NASA SRTM files_.
     You may also want to look at [ElevationUtilities](../utilities/elevationUtilities.md).
     #### Code Review
@@ Expand Down @@