From 0a3779f41dc6eae20cc27fbcec3f7cddee6cb0a5 Mon Sep 17 00:00:00 2001 From: modmuss Date: Sat, 9 Sep 2023 22:37:02 +0100 Subject: [PATCH] Fix and test FabricApiExtension not supporting deprecated modules. (#950) --- .../net/fabricmc/loom/LoomGradlePlugin.java | 2 +- .../configuration/FabricApiExtension.java | 74 +++++++++++++------ .../fabricmc/loom/util/download/Download.java | 8 +- .../loom/util/download/DownloadBuilder.java | 5 ++ .../loom/util/download/DownloadException.java | 17 +++++ .../test/unit/FabricApiExtensionTest.groovy | 69 +++++++++++++++++ .../unit/download/DownloadFileTest.groovy | 23 +++++- .../unit/download/DownloadStringTest.groovy | 40 +++++++++- .../loom/test/util/GradleTestUtil.groovy | 25 ++++++- 9 files changed, 231 insertions(+), 32 deletions(-) create mode 100644 src/test/groovy/net/fabricmc/loom/test/unit/FabricApiExtensionTest.groovy diff --git a/src/main/java/net/fabricmc/loom/LoomGradlePlugin.java b/src/main/java/net/fabricmc/loom/LoomGradlePlugin.java index 9c9c938e..8231476d 100644 --- a/src/main/java/net/fabricmc/loom/LoomGradlePlugin.java +++ b/src/main/java/net/fabricmc/loom/LoomGradlePlugin.java @@ -89,7 +89,7 @@ public class LoomGradlePlugin implements BootstrappedPlugin { // Setup extensions project.getExtensions().create(LoomGradleExtensionAPI.class, "loom", LoomGradleExtensionImpl.class, project, LoomFiles.create(project)); - project.getExtensions().create("fabricApi", FabricApiExtension.class, project); + project.getExtensions().create("fabricApi", FabricApiExtension.class); for (Class jobClass : SETUP_JOBS) { project.getObjects().newInstance(jobClass).run(); diff --git a/src/main/java/net/fabricmc/loom/configuration/FabricApiExtension.java b/src/main/java/net/fabricmc/loom/configuration/FabricApiExtension.java index e043fef9..732bfa0f 100644 --- a/src/main/java/net/fabricmc/loom/configuration/FabricApiExtension.java +++ b/src/main/java/net/fabricmc/loom/configuration/FabricApiExtension.java @@ -26,9 +26,11 @@ package net.fabricmc.loom.configuration; import java.io.File; import java.io.UncheckedIOException; +import java.util.Collections; import java.util.HashMap; import java.util.Map; +import javax.inject.Inject; import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; @@ -41,25 +43,29 @@ import org.w3c.dom.NodeList; import net.fabricmc.loom.LoomGradleExtension; import net.fabricmc.loom.util.download.DownloadException; -public class FabricApiExtension { - private final Project project; - - public FabricApiExtension(Project project) { - this.project = project; - } +public abstract class FabricApiExtension { + @Inject + public abstract Project getProject(); private static final HashMap> moduleVersionCache = new HashMap<>(); + private static final HashMap> deprecatedModuleVersionCache = new HashMap<>(); public Dependency module(String moduleName, String fabricApiVersion) { - return project.getDependencies() + return getProject().getDependencies() .create(getDependencyNotation(moduleName, fabricApiVersion)); } public String moduleVersion(String moduleName, String fabricApiVersion) { String moduleVersion = moduleVersionCache - .computeIfAbsent(fabricApiVersion, this::populateModuleVersionMap) + .computeIfAbsent(fabricApiVersion, this::getApiModuleVersions) .get(moduleName); + if (moduleVersion == null) { + moduleVersion = deprecatedModuleVersionCache + .computeIfAbsent(fabricApiVersion, this::getDeprecatedApiModuleVersions) + .get(moduleName); + } + if (moduleVersion == null) { throw new RuntimeException("Failed to find module version for module: " + moduleName); } @@ -71,9 +77,24 @@ public class FabricApiExtension { return String.format("net.fabricmc.fabric-api:%s:%s", moduleName, moduleVersion(moduleName, fabricApiVersion)); } - private Map populateModuleVersionMap(String fabricApiVersion) { - File pomFile = getApiMavenPom(fabricApiVersion); + private Map getApiModuleVersions(String fabricApiVersion) { + try { + return populateModuleVersionMap(getApiMavenPom(fabricApiVersion)); + } catch (PomNotFoundException e) { + throw new RuntimeException("Could not find fabric-api version: " + fabricApiVersion); + } + } + private Map getDeprecatedApiModuleVersions(String fabricApiVersion) { + try { + return populateModuleVersionMap(getDeprecatedApiMavenPom(fabricApiVersion)); + } catch (PomNotFoundException e) { + // Not all fabric-api versions have deprecated modules, return an empty map to cache this fact. + return Collections.emptyMap(); + } + } + + private Map populateModuleVersionMap(File pomFile) { try { DocumentBuilderFactory docFactory = DocumentBuilderFactory.newInstance(); DocumentBuilder docBuilder = docFactory.newDocumentBuilder(); @@ -101,27 +122,36 @@ public class FabricApiExtension { } } - private File getApiMavenPom(String fabricApiVersion) { - LoomGradleExtension extension = LoomGradleExtension.get(project); + private File getApiMavenPom(String fabricApiVersion) throws PomNotFoundException { + return getPom("fabric-api", fabricApiVersion); + } - File mavenPom = new File(extension.getFiles().getUserCache(), "fabric-api/" + fabricApiVersion + ".pom"); + private File getDeprecatedApiMavenPom(String fabricApiVersion) throws PomNotFoundException { + return getPom("fabric-api-deprecated", fabricApiVersion); + } - if (project.getGradle().getStartParameter().isOffline()) { - if (!mavenPom.exists()) { - throw new RuntimeException("Cannot retrieve fabric-api pom due to being offline"); - } - - return mavenPom; - } + private File getPom(String name, String version) throws PomNotFoundException { + final LoomGradleExtension extension = LoomGradleExtension.get(getProject()); + final var mavenPom = new File(extension.getFiles().getUserCache(), "fabric-api/%s-%s.pom".formatted(name, version)); try { - extension.download(String.format("https://maven.fabricmc.net/net/fabricmc/fabric-api/fabric-api/%1$s/fabric-api-%1$s.pom", fabricApiVersion)) + extension.download(String.format("https://maven.fabricmc.net/net/fabricmc/fabric-api/%2$s/%1$s/%2$s-%1$s.pom", version, name)) .defaultCache() .downloadPath(mavenPom.toPath()); } catch (DownloadException e) { - throw new UncheckedIOException("Failed to download maven info for " + fabricApiVersion, e); + if (e.getStatusCode() == 404) { + throw new PomNotFoundException(e); + } + + throw new UncheckedIOException("Failed to download maven info to " + mavenPom.getName(), e); } return mavenPom; } + + private static class PomNotFoundException extends Exception { + PomNotFoundException(Throwable cause) { + super(cause); + } + } } diff --git a/src/main/java/net/fabricmc/loom/util/download/Download.java b/src/main/java/net/fabricmc/loom/util/download/Download.java index a21d4ce4..2ba074bf 100644 --- a/src/main/java/net/fabricmc/loom/util/download/Download.java +++ b/src/main/java/net/fabricmc/loom/util/download/Download.java @@ -129,7 +129,7 @@ public final class Download { if (!successful) { progressListener.onEnd(); - throw error("HTTP request to (%s) returned unsuccessful status (%d)", url, statusCode); + throw statusError("HTTP request to (%s) returned unsuccessful status".formatted(url) + "(%d)", statusCode); } try (InputStream inputStream = decodeOutput(response)) { @@ -228,7 +228,7 @@ public final class Download { } } } else { - throw error("HTTP request returned unsuccessful status (%d)", statusCode); + throw statusError("HTTP request returned unsuccessful status (%d)", statusCode); } if (useEtag) { @@ -430,6 +430,10 @@ public final class Download { } } + private DownloadException statusError(String message, int statusCode) { + return new DownloadException(String.format(Locale.ENGLISH, message, statusCode), statusCode); + } + private DownloadException error(String message, Object... args) { return new DownloadException(String.format(Locale.ENGLISH, message, args)); } diff --git a/src/main/java/net/fabricmc/loom/util/download/DownloadBuilder.java b/src/main/java/net/fabricmc/loom/util/download/DownloadBuilder.java index c2fd6679..ab787bfb 100644 --- a/src/main/java/net/fabricmc/loom/util/download/DownloadBuilder.java +++ b/src/main/java/net/fabricmc/loom/util/download/DownloadBuilder.java @@ -158,6 +158,11 @@ public class DownloadBuilder { return supplier.get(build(i)); } catch (DownloadException e) { + if (e.getStatusCode() == 404) { + // Don't retry on 404's + throw e; + } + if (i == maxRetries) { throw new DownloadException(String.format(Locale.ENGLISH, "Failed download after %d attempts", maxRetries), e); } diff --git a/src/main/java/net/fabricmc/loom/util/download/DownloadException.java b/src/main/java/net/fabricmc/loom/util/download/DownloadException.java index 993fa257..36db54cc 100644 --- a/src/main/java/net/fabricmc/loom/util/download/DownloadException.java +++ b/src/main/java/net/fabricmc/loom/util/download/DownloadException.java @@ -27,15 +27,32 @@ package net.fabricmc.loom.util.download; import java.io.IOException; public class DownloadException extends IOException { + private final int statusCode; + public DownloadException(String message) { super(message); + statusCode = -1; + } + + public DownloadException(String message, int statusCode) { + super(message); + this.statusCode = statusCode; } public DownloadException(String message, Throwable cause) { super(message, cause); + statusCode = cause instanceof DownloadException downloadException ? downloadException.getStatusCode() : -1; } public DownloadException(Throwable cause) { super(cause); + statusCode = cause instanceof DownloadException downloadException ? downloadException.getStatusCode() : -1; + } + + /** + * @return -1 when the status code is unknown. + */ + public int getStatusCode() { + return statusCode; } } diff --git a/src/test/groovy/net/fabricmc/loom/test/unit/FabricApiExtensionTest.groovy b/src/test/groovy/net/fabricmc/loom/test/unit/FabricApiExtensionTest.groovy new file mode 100644 index 00000000..860e65eb --- /dev/null +++ b/src/test/groovy/net/fabricmc/loom/test/unit/FabricApiExtensionTest.groovy @@ -0,0 +1,69 @@ +/* + * This file is part of fabric-loom, licensed under the MIT License (MIT). + * + * Copyright (c) 2023 FabricMC + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +package net.fabricmc.loom.test.unit + +import org.gradle.api.Project +import spock.lang.Specification + +import net.fabricmc.loom.configuration.FabricApiExtension +import net.fabricmc.loom.test.util.GradleTestUtil + +class FabricApiExtensionTest extends Specification { + def "get module version"() { + when: + def fabricApi = new FabricApiExtension() { + Project project = GradleTestUtil.mockProject() + } + def version = fabricApi.moduleVersion(moduleName, apiVersion) + + then: + version == expectedVersion + + where: + moduleName | apiVersion | expectedVersion + "fabric-api-base" | "0.88.3+1.20.2" | "0.4.32+fce67b3299" // Normal module, new version + "fabric-api-base" | "0.13.1+build.257-1.14" | "0.1.2+28f8190f42" // Normal module, old version before deprecated modules. + "fabric-networking-v0" | "0.88.0+1.20.1" | "0.3.50+df3654b377" // Deprecated module, opt-out version + "fabric-networking-v0" | "0.85.0+1.20.1" | "0.3.48+df3654b377" // Deprecated module, opt-in version + } + + def "unknown module"() { + when: + def fabricApi = new FabricApiExtension() { + Project project = GradleTestUtil.mockProject() + } + fabricApi.moduleVersion("fabric-api-unknown", apiVersion) + + then: + def e = thrown RuntimeException + e.getMessage() == "Failed to find module version for module: fabric-api-unknown" + + where: + apiVersion | _ + "0.88.0+1.20.1" | _ // Deprecated opt-out + "0.85.0+1.20.1" | _ // Deprecated opt-int + "0.13.1+build.257-1.14" | _ // No deprecated modules + } +} \ No newline at end of file diff --git a/src/test/groovy/net/fabricmc/loom/test/unit/download/DownloadFileTest.groovy b/src/test/groovy/net/fabricmc/loom/test/unit/download/DownloadFileTest.groovy index 820647f9..1d977203 100644 --- a/src/test/groovy/net/fabricmc/loom/test/unit/download/DownloadFileTest.groovy +++ b/src/test/groovy/net/fabricmc/loom/test/unit/download/DownloadFileTest.groovy @@ -76,16 +76,33 @@ class DownloadFileTest extends DownloadTest { def "File: Not found"() { setup: server.get("/fileNotfound") { - it.status(404) + it.status(HttpStatus.NOT_FOUND) } def output = new File(File.createTempDir(), "file.txt").toPath() when: - def result = Download.create("$PATH/stringNotFound").downloadPath(output) + def result = Download.create("$PATH/fileNotfound").downloadPath(output) then: - thrown DownloadException + def e = thrown DownloadException + e.statusCode == 404 + } + + def "File: Server error"() { + setup: + server.get("/fileServerError") { + it.status(HttpStatus.INTERNAL_SERVER_ERROR) + } + + def output = new File(File.createTempDir(), "file.txt").toPath() + + when: + def result = Download.create("$PATH/fileServerError").downloadPath(output) + + then: + def e = thrown DownloadException + e.statusCode == 500 } def "Cache: Sha1"() { diff --git a/src/test/groovy/net/fabricmc/loom/test/unit/download/DownloadStringTest.groovy b/src/test/groovy/net/fabricmc/loom/test/unit/download/DownloadStringTest.groovy index f6397a63..48ed5fce 100644 --- a/src/test/groovy/net/fabricmc/loom/test/unit/download/DownloadStringTest.groovy +++ b/src/test/groovy/net/fabricmc/loom/test/unit/download/DownloadStringTest.groovy @@ -46,7 +46,7 @@ class DownloadStringTest extends DownloadTest { def "String: Not found"() { setup: server.get("/stringNotFound") { - it.status(404) + it.status(HttpStatus.NOT_FOUND) } when: @@ -55,7 +55,24 @@ class DownloadStringTest extends DownloadTest { .downloadString() then: - thrown DownloadException + def e = thrown DownloadException + e.statusCode == 404 + } + + def "String: Server error"() { + setup: + server.get("/stringNotFound") { + it.status(HttpStatus.INTERNAL_SERVER_ERROR) + } + + when: + def result = Download.create("$PATH/stringNotFound") + .maxRetries(3) // Ensure we still error as expected when retrying + .downloadString() + + then: + def e = thrown DownloadException + e.statusCode == 500 } def "String: Redirect"() { @@ -97,6 +114,25 @@ class DownloadStringTest extends DownloadTest { result == "Hello World 3" } + def "String: Retries 404"() { + setup: + int requests = 0 + server.get("/retryString") { + requests ++ + it.status(HttpStatus.NOT_FOUND) + } + + when: + def result = Download.create("$PATH/retryString") + .maxRetries(3) + .downloadString() + + then: + def e = thrown DownloadException + e.statusCode == 404 + requests == 1 + } + def "String: File cache"() { setup: server.get("/downloadString2") { diff --git a/src/test/groovy/net/fabricmc/loom/test/util/GradleTestUtil.groovy b/src/test/groovy/net/fabricmc/loom/test/util/GradleTestUtil.groovy index 423201cf..49448cf0 100644 --- a/src/test/groovy/net/fabricmc/loom/test/util/GradleTestUtil.groovy +++ b/src/test/groovy/net/fabricmc/loom/test/util/GradleTestUtil.groovy @@ -36,12 +36,16 @@ import org.gradle.api.provider.Property import org.gradle.api.tasks.SourceSet import org.gradle.api.tasks.util.PatternFilterable import org.jetbrains.annotations.Nullable +import org.mockito.invocation.InvocationOnMock +import org.mockito.stubbing.Answer import net.fabricmc.loom.LoomGradleExtension +import net.fabricmc.loom.extension.LoomFiles +import net.fabricmc.loom.test.LoomTestConstants +import net.fabricmc.loom.util.download.Download import static org.mockito.ArgumentMatchers.any -import static org.mockito.Mockito.mock -import static org.mockito.Mockito.when +import static org.mockito.Mockito.* class GradleTestUtil { static Property mockProperty(T value) { @@ -73,7 +77,18 @@ class GradleTestUtil { static LoomGradleExtension mockLoomGradleExtension() { def mock = mock(LoomGradleExtension.class) + def loomFiles = mockLoomFiles() when(mock.refreshDeps()).thenReturn(false) + when(mock.getFiles()).thenReturn(loomFiles) + when(mock.download(any())).thenAnswer { + Download.create(it.getArgument(0)) + } + return mock + } + + static LoomFiles mockLoomFiles() { + def mock = mock(LoomFiles.class, new RequiresStubAnswer()) + doReturn(LoomTestConstants.TEST_DIR).when(mock).getUserCache() return mock } @@ -121,4 +136,10 @@ class GradleTestUtil { def mock = mock(RepositoryHandler.class) return mock } + + static class RequiresStubAnswer implements Answer { + Object answer(InvocationOnMock invocation) throws Throwable { + throw new RuntimeException("${invocation.getMethod().getName()} is not stubbed") + } + } }