Fix TypeAwareBuyForOneEmeraldFactory trade offer crash (#4457)

* Fix TypeAwareBuyForOneEmeraldFactory trade offer crash

Fixes #4456

* Fix extra separation style issue

* Fix style issues in EmptyTypeAwareBuyForOneEmeraldTradeOfferGameTest

* Rename for clarity in TypeAwareBuyForOneEmeraldFactory mixin

* Further clarify TypeAwareBuyForOneEmeraldFactory mixin docs

fabric-object-builder-api-v1/src/main/java/net/fabricmc/fabric/mixin/object/builder/TradeOffersTypeAwareBuyForOneEmeraldFactoryMixin.java

@@ -18,16 +18,14 @@ package net.fabricmc.fabric.mixin.object.builder;
 
 import java.util.stream.Stream;
 
-import com.llamalad7.mixinextras.sugar.Local;
+import com.llamalad7.mixinextras.injector.ModifyExpressionValue;
+import com.llamalad7.mixinextras.sugar.Cancellable;
 import org.spongepowered.asm.mixin.Mixin;
 import org.spongepowered.asm.mixin.injection.At;
-import org.spongepowered.asm.mixin.injection.Inject;
 import org.spongepowered.asm.mixin.injection.Redirect;
 import org.spongepowered.asm.mixin.injection.callback.CallbackInfoReturnable;
 
-import net.minecraft.entity.Entity;
 import net.minecraft.registry.DefaultedRegistry;
-import net.minecraft.util.math.random.Random;
 import net.minecraft.village.TradeOffer;
 import net.minecraft.village.TradeOffers;
 import net.minecraft.village.TradedItem;
@@ -47,12 +45,18 @@ public abstract class TradeOffersTypeAwareBuyForOneEmeraldFactoryMixin {
 	}
 
 	/**
-	 * To prevent "item" -> "air" trades, if the result of a type aware trade is air, make sure no offer is created.
+	 * To prevent crashes due to passing a {@code null} item to a {@link TradedItem}, return a {@code null} trade offer
+	 * early before {@code null} is passed to the constructor.
 	 */
-	@Inject(method = "create", at = @At(value = "NEW", target = "net/minecraft/village/TradeOffer"), cancellable = true)
-	private void failOnNullItem(Entity entity, Random random, CallbackInfoReturnable<TradeOffer> cir, @Local() TradedItem tradedItem) {
-		if (tradedItem.itemStack().isEmpty()) { // Will return true for an "empty" item stack that had null passed in the ctor
-			cir.setReturnValue(null); // Return null to prevent creation of empty trades
+	@ModifyExpressionValue(
+			method = "create",
+			at = @At(value = "INVOKE", target = "Ljava/util/Map;get(Ljava/lang/Object;)Ljava/lang/Object;")
+	)
+	private Object failOnNullItem(Object item, @Cancellable CallbackInfoReturnable<TradeOffer> cir) {
+		if (item == null) {
+			cir.setReturnValue(null);
 		}
+
+		return item;
 	}
 }

fabric-object-builder-api-v1/src/testmod/java/net/fabricmc/fabric/test/object/builder/EmptyTypeAwareBuyForOneEmeraldTradeOfferGameTest.java

@@ -0,0 +1,44 @@
+/*
+ * Copyright (c) 2016, 2017, 2018, 2019 FabricMC
+ *
+ * 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 net.fabricmc.fabric.test.object.builder;
+
+import com.google.common.collect.ImmutableMap;
+import org.jetbrains.annotations.NotNull;
+
+import net.minecraft.entity.EntityType;
+import net.minecraft.entity.passive.VillagerEntity;
+import net.minecraft.test.GameTest;
+import net.minecraft.test.TestContext;
+import net.minecraft.util.math.random.Random;
+import net.minecraft.village.TradeOffers;
+import net.minecraft.village.VillagerType;
+
+import net.fabricmc.fabric.api.gametest.v1.FabricGameTest;
+
+public class EmptyTypeAwareBuyForOneEmeraldTradeOfferGameTest implements FabricGameTest {
+	@GameTest(templateName = FabricGameTest.EMPTY_STRUCTURE)
+	public void testEmptyTypeAwareTradeOffer(@NotNull TestContext context) {
+		VillagerEntity villager = new VillagerEntity(EntityType.VILLAGER, context.getWorld(), VillagerType.PLAINS);
+
+		// Create a type-aware trade offer with no villager types specified
+		TradeOffers.Factory typeAwareFactory = new TradeOffers.TypeAwareBuyForOneEmeraldFactory(1, 12, 5, ImmutableMap.of());
+		// Create an offer with that factory to ensure it doesn't crash when a villager type is missing from the map
+		typeAwareFactory.create(villager, Random.create());
+
+		context.complete();
+	}
+}

fabric-object-builder-api-v1/src/testmod/resources/fabric.mod.json

@@ -29,7 +29,8 @@
       "net.fabricmc.fabric.test.object.builder.PersistentStateManagerTest"
     ],
     "fabric-gametest": [
-      "net.fabricmc.fabric.test.object.builder.ObjectBuilderGameTest"
+      "net.fabricmc.fabric.test.object.builder.ObjectBuilderGameTest",
+      "net.fabricmc.fabric.test.object.builder.EmptyTypeAwareBuyForOneEmeraldTradeOfferGameTest"
     ]
   }
 }