diff --git a/paper-server/src/main/java/org/bukkit/craftbukkit/CraftRegistry.java b/paper-server/src/main/java/org/bukkit/craftbukkit/CraftRegistry.java index 2ae0b01892..5900c5b34e 100644 --- a/paper-server/src/main/java/org/bukkit/craftbukkit/CraftRegistry.java +++ b/paper-server/src/main/java/org/bukkit/craftbukkit/CraftRegistry.java @@ -279,6 +279,16 @@ public class CraftRegistry implements Registry { return bukkit; } + @NotNull + @Override + public B getOrThrow(@NotNull NamespacedKey namespacedKey) { + B object = get(namespacedKey); + + Preconditions.checkArgument(object != null, "No registry entry found for key " + namespacedKey); + + return object; + } + @NotNull @Override public Stream stream() { diff --git a/paper-server/src/main/java/org/bukkit/craftbukkit/generator/structure/CraftStructure.java b/paper-server/src/main/java/org/bukkit/craftbukkit/generator/structure/CraftStructure.java index 2352b69b33..342ba3cbb4 100644 --- a/paper-server/src/main/java/org/bukkit/craftbukkit/generator/structure/CraftStructure.java +++ b/paper-server/src/main/java/org/bukkit/craftbukkit/generator/structure/CraftStructure.java @@ -1,5 +1,7 @@ package org.bukkit.craftbukkit.generator.structure; +import com.google.common.base.Suppliers; +import java.util.function.Supplier; import net.minecraft.core.registries.Registries; import org.bukkit.NamespacedKey; import org.bukkit.Registry; @@ -20,12 +22,12 @@ public class CraftStructure extends Structure implements Handleable structureType; public CraftStructure(NamespacedKey key, net.minecraft.world.level.levelgen.structure.Structure structure) { this.key = key; this.structure = structure; - this.structureType = CraftStructureType.minecraftToBukkit(structure.type()); + this.structureType = Suppliers.memoize(() -> CraftStructureType.minecraftToBukkit(structure.type())); } @Override @@ -35,7 +37,7 @@ public class CraftStructure extends Structure implements Handleable, Data> INIT_DATA = new HashMap<>(); + private static final List FIELD_DATA_CACHE = new ArrayList<>(); + private static IRegistryCustom.Dimension vanilla_registry; + + public static Stream fieldData() { + return FIELD_DATA_CACHE.stream(); + } + + @BeforeAll + public static void init() { + initValueClasses(); + initFieldDataCache(); + } + + @AfterAll + public static void cleanUp() { + INIT_DATA.clear(); + FIELD_DATA_CACHE.clear(); + } + + private static void initValueClasses() { + vanilla_registry = RegistryHelper.createRegistry(FeatureFlags.VANILLA_SET); + + Map, List> outsideRequest = new LinkedHashMap<>(); + + // First init listening for outside requests + RegistriesArgumentProvider.getData() + .map(Arguments::get).map(args -> args[0]) + .map(type -> (Class) type) + .forEach(type -> { + Registry spyRegistry = Bukkit.getRegistry(type); + spyOutsideRequests(outsideRequest, type, spyRegistry); + }); + + // Init all registries and recorde the outcome + RegistriesArgumentProvider.getData() + .map(Arguments::get) + .map(args -> args[0]) + .map(type -> (Class) type) + .forEachOrdered(type -> { + try { + Registry spyRegistry = Bukkit.getRegistry(type); + Registry realRegistry = AllFeaturesExtension.getRealRegistry(type); + + Set nullAble = new HashSet<>(); + Set notNullAble = new HashSet<>(); + Set nullValue = new HashSet<>(); + + outsideRequest.clear(); + MockUtil.resetMock(spyRegistry); + doAnswer(invocation -> { + Keyed item = realRegistry.get(invocation.getArgument(0)); + + if (item == null) { + nullValue.add(invocation.getArgument(0)); + } + + nullAble.add(invocation.getArgument(0)); + + return item; + }).when(spyRegistry).get(any()); + + doAnswer(invocation -> { + Keyed item = realRegistry.get(invocation.getArgument(0)); + + if (item == null) { + nullValue.add(invocation.getArgument(0)); + + // We return a mock here so that it (hopefully) does not error out immediately. + // It may cause errors in other tests, but this test class should always pass before other tests can run safely. + // This is not necessary in the normal get method since the API contract states that the method can return null, + // whereas in this method, it is guaranteed to be non-null. + item = mock(type); + } + + notNullAble.add(invocation.getArgument(0)); + + return item; + }).when(spyRegistry).getOrThrow(any()); + + // Load class + try { + Class.forName(type.getName()); + } catch (Throwable e) { + e.printStackTrace(); // Print stacktrace, since JUnit eats the error in BeforeAll + fail(e); + } finally { + MockUtil.resetMock(spyRegistry); + spyOutsideRequests(outsideRequest, type, spyRegistry); + } + + INIT_DATA.put(type, new Data(nullAble, notNullAble, nullValue, new LinkedHashMap<>(outsideRequest))); + } catch (Throwable e) { + e.printStackTrace(); // Print stacktrace, since JUnit eats the error in BeforeAll + fail(e); + } + }); + + // Cleanup + RegistriesArgumentProvider.getData() + .map(Arguments::get) + .map(args -> args[0]) + .map(type -> (Class) type) + .forEach(type -> MockUtil.resetMock(Bukkit.getRegistry(type))); + } + + private static void spyOutsideRequests(Map, List> outsideRequest, Class type, Registry spyRegistry) { + doAnswer(invocation -> { + outsideRequest + .computeIfAbsent(type, ty -> new ArrayList<>()).add(invocation.getArgument(0)); + return mock(type); + }).when(spyRegistry).get(any()); + + doAnswer(invocation -> { + outsideRequest + .computeIfAbsent(type, ty -> new ArrayList<>()).add(invocation.getArgument(0)); + return mock(type); + }).when(spyRegistry).getOrThrow(any()); + } + + private static void initFieldDataCache() { + RegistriesArgumentProvider.getData().map(arguments -> { + Class type = (Class) arguments.get()[0]; + Map>> annotations = getFieldAnnotations(type); + + List fields = new ArrayList<>(); + for (Field field : type.getFields()) { + // We ignore each field that does not have the class itself as its type, + // is not static, public, or is deprecated. + if (!isValidField(type, field)) { + continue; + } + + Keyed keyed; + try { + keyed = (Keyed) field.get(null); + } catch (IllegalAccessException e) { + Logger.getGlobal().warning(String.format("Something went wrong while trying to read the field %s from class %s.\n" + + "Please see the stack trace below for more information.", field, type)); + e.printStackTrace(); + throw new RuntimeException(e); + } + + if (keyed == null) { + // We have a test case for those scenarios, + // so we ignore them here to ensure that at least the other fields are being tested. + continue; + } + + if (MockUtil.isMock(keyed)) { + // If it is a mock, it means that there was no Minecraft registry item for that key. + // In this case, ignore it, as we already test and inform about this in another test. + continue; + } + + fields.add(new FieldData(field, annotations.computeIfAbsent(field.getName(), k -> new ArrayList<>()))); + } + + return Arguments.arguments(arguments.get()[0], arguments.get()[1], fields); + }).forEach(FIELD_DATA_CACHE::add); + } + + private static Map>> getFieldAnnotations(Class aClass) { + Map>> annotation = new HashMap<>(); + + try (JarFile jarFile = new JarFile(new File(aClass.getProtectionDomain().getCodeSource().getLocation().toURI()))) { + JarEntry jarEntry = jarFile.getJarEntry(aClass.getName().replace('.', '/') + ".class"); + + try (InputStream inputStream = jarFile.getInputStream(jarEntry)) { + ClassReader classReader = new ClassReader(inputStream); + classReader.accept(new ClassVisitor(Opcodes.ASM9) { + @Override + public FieldVisitor visitField(int access, String name, String descriptor, String signature, Object value) { + return new FieldVisitor(Opcodes.ASM9) { + @Override + public AnnotationVisitor visitAnnotation(String descriptor, boolean visible) { + try { + annotation.computeIfAbsent(name, k -> new ArrayList<>()).add((Class) Class.forName(Type.getType(descriptor).getClassName())); + } catch (ClassNotFoundException e) { + e.printStackTrace(); + throw new RuntimeException(e); + } + return null; + } + }; + } + }, Opcodes.ASM9); + } + } catch (URISyntaxException | IOException e) { + e.printStackTrace(); + throw new RuntimeException(e); + } + + return annotation; + } + + @RegistriesTest + public void testOutsideRequests(Class type) { + Data data = INIT_DATA.get(type); + + assertNotNull(data, String.format("No data present for %s. This should never happen since the same data source is used.\n" + + "Something has gone horribly wrong.", type)); + + assertTrue(data.outsideRequests.isEmpty(), String.format(""" + There were outside registry accesses while loading registry items for class %s. + This can happen if you try to read any registry item in the constructor. + For ease of testing, please remove this from the constructor. + Subsequent tests may fail because of this. + + You can use a Supplier instead. For easy caching, you can use com.google.common.base.Suppliers#memoize(Supplier) + Example: + + private final Supplier otherRegisterItem; + + public CraftRegisterItem([...]) { + [...] + this.otherRegisterItem = Suppliers.memoize(() -> CraftRegisterItem.getSome("other_key")); + } + + public RegisterItem getOtherRegisterItem() { + return otherRegisterItem.get(); + } + The following outside requests were registered: + %s""", type.getName(), Joiner.on('\n').withKeyValueSeparator(" <-> ").join(data.outsideRequests))); + } + + @RegistriesTest + public void testNoNullValuePresent(Class type) { + Data data = INIT_DATA.get(type); + + assertNotNull(data, String.format("No data present for %s. This should never happen since the same data source is used.\n" + + "Something has gone horribly wrong.", type)); + + assertTrue(data.nullValue.isEmpty(), String.format(""" + %s tried to get registry items that are not present. + This can be caused if the affected registry item was removed, renamed, or if you mistyped the name. + Alternatively, there maybe was an attempt to read another registry item in the constructor of a registry item. + Subsequent tests may fail because of this. + The following registry items were requested, but there is no registry item for them: + %s""", type, Joiner.on('\n').join(data.nullValue))); + } + + @RegistriesTest + public void testForNullValues(Class type) throws IllegalAccessException { + List nullFields = new ArrayList<>(); + + for (Field field : type.getFields()) { + // We ignore each field that does not have the class itself as its type, is not static, public, or is deprecated. + if (!isValidField(type, field)) { + continue; + } + + Keyed keyed = (Keyed) field.get(null); + + if (keyed == null) { + // This should not happen since, even if the item is not present in the Minecraft registry, we would use a mock. + nullFields.add(field); + } + } + + assertTrue(nullFields.isEmpty(), String.format(""" + The class %s has fields with no value assigned to them. + This should not normally happen, since all feature flags are set, + and a dummy is returned in cases where the registry item does not exist. + Please make sure the following fields are not null: + %s""", type, Joiner.on('\n').join(nullFields))); + } + + @ParameterizedTest + @MethodSource("fieldData") + public void testExcessExperimentalAnnotation(Class type, ResourceKey> registryKey, List fieldDataList) throws IllegalAccessException { + List excess = new ArrayList<>(); + + for (FieldData fieldData : fieldDataList) { + if (!fieldData.annotations().contains(MinecraftExperimental.class) && !fieldData.annotations().contains(ApiStatus.Experimental.class)) { + // No annotation -> no problem + continue; + } + + IRegistry registry = vanilla_registry.registryOrThrow(registryKey); + + Optional optionalValue = registry.getOptional(CraftNamespacedKey.toMinecraft(((Keyed) fieldData.field().get(null)).getKey())); + + if (optionalValue.isEmpty()) { + // The value is not present, which means it comes from a feature flag. + continue; + } + + Object value = optionalValue.get(); + + if (value instanceof FeatureElement featureElement && !featureElement.isEnabled(FeatureFlags.VANILLA_SET)) { + // It is a FeatureElement that is not enabled for vanilla FeatureSet so ignore it. + continue; + } + + excess.add(fieldData.field()); + } + + assertTrue(excess.isEmpty(), String.format(""" + The class %s has fields with the MinecraftExperimental and/or ApiStatus.Experimental annotation. + However, the value is present and active with the vanilla feature flags. + The annotation should be removed from the following fields: + %s""", type, Joiner.on('\n').join(excess))); + } + + @ParameterizedTest + @MethodSource("fieldData") + public void testMissingExperimentalAnnotation(Class type, ResourceKey> registryKey, List fieldDataList) throws IllegalAccessException { + IRegistry registry = vanilla_registry.registryOrThrow(registryKey); + List missing = new ArrayList<>(); + + for (FieldData fieldData : fieldDataList) { + if (fieldData.annotations().contains(MinecraftExperimental.class) && fieldData.annotations().contains(ApiStatus.Experimental.class)) { + // Annotation present -> no problem + continue; + } + + Optional optionalValue = registry.getOptional(CraftNamespacedKey.toMinecraft(((Keyed) fieldData.field().get(null)).getKey())); + + if (optionalValue.isEmpty()) { + // The value is not present, which means it comes from a feature flag. + missing.add(fieldData.field()); + continue; + } + + Object value = optionalValue.get(); + + if (value instanceof FeatureElement featureElement && !featureElement.isEnabled(FeatureFlags.VANILLA_SET)) { + // It is a FeatureElement that is not enabled for vanilla FeatureSet. + missing.add(fieldData.field()); + } + } + + assertTrue(missing.isEmpty(), String.format(""" + The class %s has fields that don't have the MinecraftExperimental and/or ApiStatus.Experimental annotation. + However, the value is not present or active with the vanilla feature flags. + The annotation should be added to the following fields: %s + """, type, Joiner.on('\n').join(missing))); + } + + @ParameterizedTest + @MethodSource("fieldData") + public void testExcessNullCheck(Class type, ResourceKey> registryKey, List fieldDataList) throws IllegalAccessException { + IRegistry registry = vanilla_registry.registryOrThrow(registryKey); + List excess = new ArrayList<>(); + Data data = INIT_DATA.get(type); + + for (FieldData fieldData : fieldDataList) { + NamespacedKey key = ((Keyed) fieldData.field().get(null)).getKey(); + + Optional optionalValue = registry.getOptional(CraftNamespacedKey.toMinecraft(key)); + + if (optionalValue.isPresent()) { + // The value is present, which means it cannot have an unnecessary null check, so ignore it. + continue; + } + + if (data.notNullAble.contains(key)) { + // The value is null, but there was a null check, which it should not have. + excess.add(fieldData.field()); + } + } + + assertTrue(excess.isEmpty(), String.format(""" + The class %s has fields that do have a null check. + However, the value can be null, and there should not be a null check, so that it won't error out, and an IDE can handle it accordingly. + If there is no null check, make sure that org.bukkit.Registry#get(NamespaceKey) is used. + The following fields do have a null check: + %s""", type, Joiner.on('\n').join(excess))); + } + + @ParameterizedTest + @MethodSource("fieldData") + public void testMissingNullCheck(Class type, ResourceKey> registryKey, List fieldDataList) throws IllegalAccessException { + IRegistry registry = vanilla_registry.registryOrThrow(registryKey); + List missing = new ArrayList<>(); + Data data = INIT_DATA.get(type); + + for (FieldData fieldData : fieldDataList) { + NamespacedKey key = ((Keyed) fieldData.field().get(null)).getKey(); + + Optional optionalValue = registry.getOptional(CraftNamespacedKey.toMinecraft(key)); + + if (optionalValue.isEmpty()) { + // The value is not present, which means there is no need for a null check, so ignore it. + continue; + } + + if (data.nullAble.contains(key)) { + // The value is not null, and there was no null check, which means it is missing. + missing.add(fieldData.field()); + } + } + + assertTrue(missing.isEmpty(), String.format(""" + The class %s has fields that don't have a null check. + However, the value cannot be null, and there should be a null check so that an IDE can handle it accordingly. + If there is a null check, make sure that org.bukkit.Registry#getOrThrow(NamespaceKey) is used. + The following fields don't have a null check: + %s""", type, Joiner.on('\n').join(missing))); + } + + @ParameterizedTest + @MethodSource("fieldData") + public void testMatchingFieldNames(Class type, ResourceKey> registryKey, List fieldDataList) throws IllegalAccessException { + Map notMatching = new LinkedHashMap<>(); + + for (FieldData fieldData : fieldDataList) { + NamespacedKey key = ((Keyed) fieldData.field().get(null)).getKey(); + + if (fieldData.field().getName().equals(convertToFieldName(key.getKey()))) { + continue; + } + + // Field names to not match. + notMatching.put(fieldData.field(), key); + } + + assertTrue(notMatching.isEmpty(), String.format(""" + The class %s has fields whose keys don't match up. + The names of each field should match up with their key. + If there is a '.' in the key, it should be replaced with '_' in the field name. + If the registry item no longer exists and is replaced by a new one, + add the @Deprecated annotation to the field and create a new field for the new registry item. + The following fields have mismatching keys: + %s""", type, Joiner.on('\n').withKeyValueSeparator(" <-> ").join(notMatching))); + } + + @ParameterizedTest + @MethodSource("fieldData") + public void testMissingConstants(Class type, ResourceKey> registryKey) throws IllegalAccessException { + IRegistry registry = RegistryHelper.getRegistry().registryOrThrow(registryKey); + List missingKeys = new ArrayList<>(); + + for (Object nmsObject : registry) { + MinecraftKey minecraftKey = registry.getKey(nmsObject); + + try { + Field field = type.getField(convertToFieldName(minecraftKey.getPath())); + + // Only fields which are not Deprecated + // and have the right registry item associated with the field count. + if (!isValidField(type, field)) { + missingKeys.add(minecraftKey); + continue; + } + + T keyed = (T) field.get(null); + + if (keyed == null) { + missingKeys.add(minecraftKey); + continue; + } + + if (!keyed.getKey().equals(CraftNamespacedKey.fromMinecraft(minecraftKey))) { + missingKeys.add(minecraftKey); + } + } catch (NoSuchFieldException e) { + missingKeys.add(minecraftKey); + } + } + + assertTrue(missingKeys.isEmpty(), String.format(""" + The class %s has missing fields for some registry items. + There should be a field for each registry item. + In case there is a field with the correct name (not misspelled), make sure that: + The field is not null, the field is not annotated with @Deprecated, the field has the right registry item key. + The following registry items don't have a field: + %s""", type, Joiner.on('\n').join(missingKeys))); + } + + private String convertToFieldName(String value) { + return switch (value) { + // JukeboxSong does have keys which start with a number, which is not a valid field name + case "5" -> "FIVE"; + case "11" -> "ELEVEN"; + case "13" -> "THIRTEEN"; + default -> value.toUpperCase(Locale.ROOT).replace('.', '_'); + }; + } + + private static boolean isValidField(Class type, Field field) { + return type.isAssignableFrom(field.getType()) && Modifier.isStatic(field.getModifiers()) + && Modifier.isPublic(field.getModifiers()) && !field.isAnnotationPresent(Deprecated.class); + } + + private record Data(Set nullAble, Set notNullAble, + Set nullValue, + Map, List> outsideRequests) { + } + + private record FieldData(Field field, List> annotations) { + } +} diff --git a/paper-server/src/test/java/org/bukkit/support/suite/AllFeaturesTestSuite.java b/paper-server/src/test/java/org/bukkit/support/suite/AllFeaturesTestSuite.java index e02ee7ebce..d786611988 100644 --- a/paper-server/src/test/java/org/bukkit/support/suite/AllFeaturesTestSuite.java +++ b/paper-server/src/test/java/org/bukkit/support/suite/AllFeaturesTestSuite.java @@ -1,8 +1,12 @@ package org.bukkit.support.suite; +import org.bukkit.registry.PerRegistryTest; +import org.bukkit.registry.RegistryClassTest; +import org.bukkit.registry.RegistryConversionTest; import org.junit.platform.suite.api.ConfigurationParameter; import org.junit.platform.suite.api.ExcludeClassNamePatterns; import org.junit.platform.suite.api.IncludeTags; +import org.junit.platform.suite.api.SelectClasses; import org.junit.platform.suite.api.SelectPackages; import org.junit.platform.suite.api.Suite; import org.junit.platform.suite.api.SuiteDisplayName; @@ -11,6 +15,7 @@ import org.junit.platform.suite.api.SuiteDisplayName; @SuiteDisplayName("Test suite for test which need registry values present, with all feature flags set") @IncludeTags("AllFeatures") @SelectPackages("org.bukkit") +@SelectClasses({RegistryClassTest.class, PerRegistryTest.class, RegistryConversionTest.class}) // Make sure general registry tests are run first @ExcludeClassNamePatterns("org.bukkit.craftbukkit.inventory.ItemStack.*Test") @ConfigurationParameter(key = "TestSuite", value = "AllFeatures") public class AllFeaturesTestSuite {