Repository: smithy-lang/smithy (java)

Issue:
Snapshot dependencies
Just updated to 1.34.0, and I bumped into this:

https://github.com/awslabs/smithy/blob/1ce05b2640eb006c737d8cb278a33ca2c31ad821/smithy-cli/src/main/java/software/amazon/smithy/cli/dependencies/MavenDependencyResolver.java#L155-L157

Snapshot dependencies not being supported seems like a very strange limitation - we used to rely on snapshots a lot for developing new features in a Smithy library, while being able to test out things like packaging, LSP support, etc. without committing to a stable release.

Why is this requirement in there? Could it be relaxed? After the recent change to the LSP, which now uses this custom resolver rather than Coursier (which allows snapshots), it's going to be a problem for orgs that share Smithy libraries (e.g. for custom protocols) between repositories.

Pull request:
Support SNAPSHOTs and fix dependency cache
*Issue #, if available:* #1850

*Description of changes:*
We will no longer fail when a SNAPSHOT dependency is used. In order to make sure that SNAPSHOTs are periodically re-resolved, a TTL of 1 day was added to the cache file so that the cache is invalidated every 24 hours. The cache can be manually invalidated using `smithy clean`.

When implementing a TTL, I noticed that caching wasn't actually working and was always invalidated. That's fixed now by using the correct timestamp comparisons, and several new test cases were added.

Closes #1850

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.


Patch:
diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/dependencies/FileCacheResolver.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/dependencies/FileCacheResolver.java
index 6d1fe3d4b..3c439f7e0 100644
--- a/smithy-cli/src/main/java/software/amazon/smithy/cli/dependencies/FileCacheResolver.java
+++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/dependencies/FileCacheResolver.java
@@ -22,8 +22,10 @@ import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
+import java.time.Duration;
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.Date;
 import java.util.List;
 import java.util.Map;
 import java.util.logging.Logger;
@@ -39,6 +41,9 @@ import software.amazon.smithy.model.node.ObjectNode;
  */
 public final class FileCacheResolver implements DependencyResolver {
 
+    // This is hard-coded for now to 1 day, and it can become an environment variable in the future if needed.
+    private static final Duration SMITHY_MAVEN_TTL = Duration.parse("P1D");
+
     private static final Logger LOGGER = Logger.getLogger(FileCacheResolver.class.getName());
     private final DependencyResolver delegate;
     private final File location;
@@ -80,31 +85,45 @@ public final class FileCacheResolver implements DependencyResolver {
     }
 
     private List<ResolvedArtifact> load() {
-        // Invalidate the cache if smithy-build.json was updated after the cache was written.
-        Path filePath = location.toPath();
-        if (!Files.exists(filePath)) {
+        // Invalidate the cache if:
+        // 1. smithy-build.json was updated after the cache was written.
+        // 2. the cache file is older than the allowed TTL.
+        // 3. a cached artifact was deleted.
+        // 4. a cached artifact is newer than the cache file.
+        long cacheLastModifiedMillis = location.lastModified();
+        long currentTimeMillis = new Date().getTime();
+        long ttlMillis = SMITHY_MAVEN_TTL.toMillis();
+
+        if (location.length() == 0) {
             return Collections.emptyList();
-        } else if (!isCacheValid(location)) {
-            invalidate(filePath);
+        } else if (!isCacheValid(cacheLastModifiedMillis)) {
+            LOGGER.fine("Invalidating dependency cache: config is newer than the cache");
+            invalidate();
+            return Collections.emptyList();
+        } else if (currentTimeMillis - cacheLastModifiedMillis > ttlMillis) {
+            LOGGER.fine(() -> "Invalidating dependency cache: Cache exceeded TTL (TTL: " + ttlMillis + ")");
+            invalidate();
             return Collections.emptyList();
         }
 
         ObjectNode node;
-        try (InputStream stream = Files.newInputStream(filePath)) {
+        try (InputStream stream = Files.newInputStream(location.toPath())) {
             node = Node.parse(stream, location.toString()).expectObjectNode();
         } catch (ModelSyntaxException | IOException e) {
-            throw new DependencyResolverException("Error loading dependency cache file from " + filePath, e);
+            throw new DependencyResolverException("Error loading dependency cache file from " + location, e);
         }
 
         List<ResolvedArtifact> result = new ArrayList<>(node.getStringMap().size());
         for (Map.Entry<String, Node> entry : node.getStringMap().entrySet()) {
-            Path location = Paths.get(entry.getValue().expectStringNode().getValue());
-            // Invalidate the cache if the JAR file was updated after the cache was written.
-            if (isArtifactUpdatedSinceReferenceTime(location)) {
-                invalidate(filePath);
+            Path artifactLocation = Paths.get(entry.getValue().expectStringNode().getValue());
+            long lastModifiedOfArtifact = artifactLocation.toFile().lastModified();
+            // Invalidate the cache if the JAR file was updated since the cache was created.
+            if (lastModifiedOfArtifact == 0 || lastModifiedOfArtifact > cacheLastModifiedMillis) {
+                LOGGER.fine(() -> "Invalidating dependency cache: artifact is newer than cache: " + artifactLocation);
+                invalidate();
                 return Collections.emptyList();
             }
-            result.add(ResolvedArtifact.fromCoordinates(location, entry.getKey()));
+            result.add(ResolvedArtifact.fromCoordinates(artifactLocation, entry.getKey()));
         }
 
         return result;
@@ -130,23 +149,13 @@ public final class FileCacheResolver implements DependencyResolver {
         }
     }
 
-    private boolean isCacheValid(File file) {
-        return referenceTimeInMillis <= file.lastModified() && file.length() > 0;
-    }
-
-    private boolean isArtifactUpdatedSinceReferenceTime(Path path) {
-        File file = path.toFile();
-        return !file.exists() || (referenceTimeInMillis > 0 && file.lastModified() > referenceTimeInMillis);
+    private boolean isCacheValid(long cacheLastModifiedMillis) {
+        return referenceTimeInMillis <= cacheLastModifiedMillis;
     }
 
-    private void invalidate(Path filePath) {
-        try {
-            if (Files.exists(filePath)) {
-                LOGGER.fine("Invalidating dependency cache file: " + location);
-                Files.delete(filePath);
-            }
-        } catch (IOException e) {
-            throw new DependencyResolverException("Unable to delete cache file: " + e.getMessage(), e);
+    private void invalidate() {
+        if (location.exists() && !location.delete()) {
+            LOGGER.warning("Unable to invalidate dependency cache file: " + location);
         }
     }
 }
diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/dependencies/MavenDependencyResolver.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/dependencies/MavenDependencyResolver.java
index 08061dce9..b10d0da50 100644
--- a/smithy-cli/src/main/java/software/amazon/smithy/cli/dependencies/MavenDependencyResolver.java
+++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/dependencies/MavenDependencyResolver.java
@@ -152,9 +152,6 @@ public final class MavenDependencyResolver implements DependencyResolver {
         } catch (IllegalArgumentException e) {
             throw new DependencyResolverException("Invalid dependency: " + e.getMessage());
         }
-        if (artifact.isSnapshot()) {
-            throw new DependencyResolverException("Snapshot dependencies are not supported: " + artifact);
-        }
         validateDependencyVersion(artifact);
         return new Dependency(artifact, scope);
     }


Test patch:
diff --git a/smithy-cli/src/it/java/software/amazon/smithy/cli/MavenResolverTest.java b/smithy-cli/src/it/java/software/amazon/smithy/cli/MavenResolverTest.java
index 37bc7f8a7..40bb424a8 100644
--- a/smithy-cli/src/it/java/software/amazon/smithy/cli/MavenResolverTest.java
+++ b/smithy-cli/src/it/java/software/amazon/smithy/cli/MavenResolverTest.java
@@ -117,7 +117,7 @@ public class MavenResolverTest {
                 RunResult result = IntegUtils.run(path, ListUtils.of("validate", "--debug", "model"));
 
                 assertThat(result.getExitCode(), is(0));
-                assertThat(result.getOutput(), containsString("Invalidating dependency cache"));
+                assertThat(result.getOutput(), containsString("Resolving Maven dependencies for Smithy CLI"));
             } catch (IOException e) {
                 throw new RuntimeException(e);
             }
diff --git a/smithy-cli/src/test/java/software/amazon/smithy/cli/dependencies/FileCacheResolverTest.java b/smithy-cli/src/test/java/software/amazon/smithy/cli/dependencies/FileCacheResolverTest.java
index 12b95c5b5..629219eb4 100644
--- a/smithy-cli/src/test/java/software/amazon/smithy/cli/dependencies/FileCacheResolverTest.java
+++ b/smithy-cli/src/test/java/software/amazon/smithy/cli/dependencies/FileCacheResolverTest.java
@@ -11,8 +11,11 @@ import java.io.File;
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
+import java.time.Duration;
 import java.util.ArrayList;
+import java.util.Date;
 import java.util.List;
+import java.util.function.Consumer;
 import org.junit.jupiter.api.Test;
 import software.amazon.smithy.build.model.MavenRepository;
 import software.amazon.smithy.utils.IoUtils;
@@ -48,9 +51,20 @@ public class FileCacheResolverTest {
     }
 
     @Test
-    public void loadsCacheFromDelegateWhenCacheMissingAndSaves() throws IOException {
+    public void invalidatesCacheWhenArtifactDeleted() throws IOException {
+        // Delete the "JAR" to invalidate the cache.
+        validateCacheScenario(File::delete);
+    }
+
+    @Test
+    public void invalidatesCacheWhenArtifactIsNewerThanCache() throws IOException {
+        // Set the last modified time of the "JAR" to the future to ensure the cache is invalidated.
+        validateCacheScenario(jar -> jar.setLastModified(new Date().getTime() + Duration.parse("P1D").toMillis()));
+    }
+
+    private void validateCacheScenario(Consumer<File> jarFileMutation) throws IOException {
         File cache = File.createTempFile("classpath", ".json");
-        File jar = File.createTempFile("foo", ".json");
+        File jar = File.createTempFile("foo", ".jar");
         Files.write(jar.toPath(), "{}".getBytes(StandardCharsets.UTF_8));
 
         ResolvedArtifact artifact = ResolvedArtifact.fromCoordinates(jar.toPath(), "com.foo:bar:1.0.0");
@@ -58,28 +72,92 @@ public class FileCacheResolverTest {
         result.add(artifact);
 
         Mock mock = new Mock(result);
-        DependencyResolver resolver = new FileCacheResolver(cache, jar.lastModified(), mock);
-        List<ResolvedArtifact> resolved = resolver.resolve();
+        DependencyResolver cachingResolver = new FileCacheResolver(cache, jar.lastModified(), mock);
+        List<ResolvedArtifact> resolved = cachingResolver.resolve();
 
+        // Make sure artifacts were cached as expected.
         assertThat(resolved, contains(artifact));
         assertThat(IoUtils.readUtf8File(cache.toPath()), containsString("com.foo:bar:1.0.0"));
 
-        // Remove the canned entry from the mock to ensure the cache is working before delegating.
+        // Remove the canned entry from the mock so that when the cache is invalidated, we get a different result.
         result.clear();
 
-        // Calling it again will load from the cached file.
-        assertThat(resolver.resolve(), contains(artifact));
+        // Calling it again will load from the cached file and not from the delegate mock that's now empty.
+        assertThat(cachingResolver.resolve(), contains(artifact));
 
         // The cache should still be there.
         assertThat(IoUtils.readUtf8File(cache.toPath()), containsString("com.foo:bar:1.0.0"));
 
-        // Removing the cache artifact invalidates the cache.
-        assertThat(jar.delete(), is(true));
+        // Mutate the JAR using the provided method. This method should invalidate the cache.
+        jarFileMutation.accept(jar);
 
-        assertThat(resolver.resolve(), empty());
+        // Resolving here skips the cache (which contains artifacts) and calls the delegate (which is now empty).
+        assertThat(cachingResolver.resolve(), empty());
+
+        // The caching resolver should now write an empty cache file.
         assertThat(IoUtils.readUtf8File(cache.toPath()), containsString("{}"));
     }
 
+    @Test
+    public void invalidatesCacheWhenConfigIsNewerThanCache() throws IOException {
+        File cache = File.createTempFile("classpath", ".json");
+        File jar = File.createTempFile("foo", ".jar");
+        Files.write(jar.toPath(), "{}".getBytes(StandardCharsets.UTF_8));
+
+        ResolvedArtifact artifact = ResolvedArtifact.fromCoordinates(jar.toPath(), "com.foo:bar:1.0.0");
+        List<ResolvedArtifact> result = new ArrayList<>();
+        result.add(artifact);
+
+        Mock mock = new Mock(result);
+        // Set the "config" last modified to a future date to ensure it's newer than the "JAR" file.
+        DependencyResolver cachingResolver = new FileCacheResolver(
+            cache,
+            jar.lastModified() + Duration.parse("P1D").toMillis(),
+            mock
+        );
+        List<ResolvedArtifact> resolved = cachingResolver.resolve();
+
+        // Make sure artifacts were cached as expected.
+        assertThat(resolved, contains(artifact));
+        assertThat(IoUtils.readUtf8File(cache.toPath()), containsString("com.foo:bar:1.0.0"));
+
+        // Remove the canned entry from the mock so that when the cache is invalidated, we get a different result.
+        result.clear();
+
+        // The cache will be invalidated here and reloaded from source, resulting in an empty result.
+        assertThat(cachingResolver.resolve(), empty());
+    }
+
+    @Test
+    public void invalidatesCacheWhenCacheExceedsTTL() throws IOException {
+        long tenDaysAgo = new Date().getTime() - Duration.parse("P10D").toMillis();
+        File cache = File.createTempFile("classpath", ".json");
+        File jar = File.createTempFile("foo", ".jar");
+        Files.write(jar.toPath(), "{}".getBytes(StandardCharsets.UTF_8));
+
+        ResolvedArtifact artifact = ResolvedArtifact.fromCoordinates(jar.toPath(), "com.foo:bar:1.0.0");
+        List<ResolvedArtifact> result = new ArrayList<>();
+        result.add(artifact);
+
+        Mock mock = new Mock(result);
+        // Make sure the config is set to 10 days ago too, so that config date checking doesn't invalidate.
+        DependencyResolver cachingResolver = new FileCacheResolver(cache, tenDaysAgo, mock);
+        List<ResolvedArtifact> resolved = cachingResolver.resolve();
+
+        // Make sure artifacts were cached as expected.
+        assertThat(resolved, contains(artifact));
+        assertThat(IoUtils.readUtf8File(cache.toPath()), containsString("com.foo:bar:1.0.0"));
+
+        // Remove the canned entry from the mock so that when the cache is invalidated, we get a different result.
+        result.clear();
+
+        // Change the last modified of the cache to a date in the distant past to invalidate the cache.
+        assertThat(cache.setLastModified(tenDaysAgo), is(true));
+
+        // The cache will be invalidated here and reloaded from source, resulting in an empty result.
+        assertThat(cachingResolver.resolve(), empty());
+    }
+
     private static final class Mock implements DependencyResolver {
         final List<ResolvedArtifact> artifacts;
         final List<MavenRepository> repositories = new ArrayList<>();
diff --git a/smithy-cli/src/test/java/software/amazon/smithy/cli/dependencies/MavenDependencyResolverTest.java b/smithy-cli/src/test/java/software/amazon/smithy/cli/dependencies/MavenDependencyResolverTest.java
index c618d1af2..05d861d6c 100644
--- a/smithy-cli/src/test/java/software/amazon/smithy/cli/dependencies/MavenDependencyResolverTest.java
+++ b/smithy-cli/src/test/java/software/amazon/smithy/cli/dependencies/MavenDependencyResolverTest.java
@@ -21,6 +21,7 @@ public class MavenDependencyResolverTest {
         resolver.addDependency("com.foo:baz1:1.0.0");
         resolver.addDependency("com.foo:baz2:[1.0.0]");
         resolver.addDependency("com.foo:baz3:[1.0.0,]");
+        resolver.addDependency("smithy.foo:bar:1.25.0-SNAPSHOT");
     }
 
     @ParameterizedTest
@@ -36,7 +37,6 @@ public class MavenDependencyResolverTest {
     public static Stream<Arguments> invalidDependencies() {
         return Stream.of(
             Arguments.of("X"),
-            Arguments.of("smithy.foo:bar:1.25.0-SNAPSHOT"),
             Arguments.of("smithy.foo:bar:RELEASE"),
             Arguments.of("smithy.foo:bar:latest-status"),
             Arguments.of("smithy.foo:bar:LATEST"),