Fix and test FabricApiExtension not supporting deprecated modules. (#950

)

src/main/java/net/fabricmc/loom/LoomGradlePlugin.java

@@ -89,7 +89,7 @@ public void apply(Project project) {
 
 		// 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<? extends Runnable> jobClass : SETUP_JOBS) {
 			project.getObjects().newInstance(jobClass).run();

src/main/java/net/fabricmc/loom/configuration/FabricApiExtension.java

@@ -26,9 +26,11 @@
 
 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 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<String, Map<String, String>> moduleVersionCache = new HashMap<>();
+	private static final HashMap<String, Map<String, String>> 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 @@ private String getDependencyNotation(String moduleName, String fabricApiVersion)
 		return String.format("net.fabricmc.fabric-api:%s:%s", moduleName, moduleVersion(moduleName, fabricApiVersion));
 	}
 
-	private Map<String, String> populateModuleVersionMap(String fabricApiVersion) {
-		File pomFile = getApiMavenPom(fabricApiVersion);
+	private Map<String, String> getApiModuleVersions(String fabricApiVersion) {
+		try {
+			return populateModuleVersionMap(getApiMavenPom(fabricApiVersion));
+		} catch (PomNotFoundException e) {
+			throw new RuntimeException("Could not find fabric-api version: " + fabricApiVersion);
+		}
+	}
+
+	private Map<String, String> 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<String, String> populateModuleVersionMap(File pomFile) {
 		try {
 			DocumentBuilderFactory docFactory = DocumentBuilderFactory.newInstance();
 			DocumentBuilder docBuilder = docFactory.newDocumentBuilder();
@@ -101,27 +122,36 @@ private Map<String, String> populateModuleVersionMap(String fabricApiVersion) {
 		}
 	}
 
-	private File getApiMavenPom(String fabricApiVersion) {
-		LoomGradleExtension extension = LoomGradleExtension.get(project);
-
-		File mavenPom = new File(extension.getFiles().getUserCache(), "fabric-api/" + fabricApiVersion + ".pom");
+	private File getApiMavenPom(String fabricApiVersion) throws PomNotFoundException {
+		return getPom("fabric-api", fabricApiVersion);
+	}
 
-		if (project.getGradle().getStartParameter().isOffline()) {
-			if (!mavenPom.exists()) {
-				throw new RuntimeException("Cannot retrieve fabric-api pom due to being offline");
-			}
+	private File getDeprecatedApiMavenPom(String fabricApiVersion) throws PomNotFoundException {
+		return getPom("fabric-api-deprecated", fabricApiVersion);
+	}
 
-			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);
+		}
+	}
 }

src/main/java/net/fabricmc/loom/util/download/Download.java

@@ -129,7 +129,7 @@ String downloadString() throws DownloadException {
 
 		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 @@ private void doDownload(Path output) throws DownloadException {
 				}
 			}
 		} 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 @@ private void createLock(Path output) throws DownloadException {
 		}
 	}
 
+	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));
 	}

src/main/java/net/fabricmc/loom/util/download/DownloadBuilder.java

@@ -158,6 +158,11 @@ private <T> T withRetries(DownloadFunction<T> supplier) throws DownloadException
 
 				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);
 				}

src/main/java/net/fabricmc/loom/util/download/DownloadException.java

@@ -27,15 +27,32 @@
 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;
 	}
 }

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
+	}
+}

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"() {

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") {