Merge upstream #2
Conversation
This prevent the creaking from being pushed with knockback enchant when it can't move
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (16)
patches/api/0008-Use-ASM-for-event-executors.patch (5)
64-72: Consider explicit exception handling instead of using SneakyThrow.The current implementation swallows exceptions using SneakyThrow, which might make debugging more difficult. Consider wrapping the original exception in an EventException for better error reporting.
@Override public void execute(final Listener listener, final Event event) throws EventException { if (!this.eventClass.isInstance(event)) return; try { this.handle.invoke(listener, event); } catch (final Throwable t) { - SneakyThrow.sneaky(t); + throw new EventException(t); } }
115-122: Consider explicit exception handling instead of using SneakyThrow.Similar to MethodHandleEventExecutor, consider wrapping exceptions in EventException for better error reporting.
@Override public void execute(final Listener listener, final Event event) throws EventException { if (!this.eventClass.isInstance(event)) return; try { this.handle.invoke(event); } catch (final Throwable throwable) { - SneakyThrow.sneaky(throwable); + throw new EventException(throwable); } }
182-182: Consider making NEXT_ID private.The NEXT_ID field should be private to prevent external modification of the counter.
- public static AtomicInteger NEXT_ID = new AtomicInteger(1); + private static final AtomicInteger NEXT_ID = new AtomicInteger(1);
170-175: Consider deprecating non-void event handlers.The comment indicates that non-void event handlers will be unsupported in the future. Consider adding
@Deprecatedto methods with non-void return types or throwing an exception during generation.
350-379: Consider adding more detailed Javadoc for the create method.The static factory method would benefit from more detailed documentation explaining:
- The selection criteria between different executor types
- The caching behavior
- The performance implications of each executor type
+ /** + * Creates an EventExecutor for the given method and event class. + * + * <p>The factory method selects the most efficient executor type based on the method's characteristics: + * - Static methods use StaticMethodHandleEventExecutor + * - Public methods use ASM-generated executors for better performance + * - Private/Protected methods use MethodHandleEventExecutor + * + * <p>Generated executors are cached to improve performance. + * + * @param m the method to create an executor for + * @param eventClass the event class to handle + * @return an appropriate EventExecutor implementation + * @throws IllegalArgumentException if the method parameters don't match the event class + */ @NotNull public static EventExecutor create(@NotNull Method m, @NotNull Class<? extends Event> eventClass) {patches/api/0012-Add-command-line-option-to-load-extra-plugin-jars-no.patch (3)
42-52: LGTM! Consider versioning implicationsThe interface addition is well-documented and maintains consistency with the Bukkit class. Since this is an API change, ensure it's properly versioned in the next release.
Consider documenting this API addition in the changelog and updating the API version if this follows semantic versioning.
74-80: Enhance error handling and loggingWhile the implementation is functional, the error handling could be more informative:
Consider this improvement:
for (File file : extraPluginJars) { try { pluginList.add(this.paperPluginManager.loadPlugin(file)); } catch (Exception e) { - this.server.getLogger().log(Level.SEVERE, "Plugin loading error!", e); + this.server.getLogger().log(Level.SEVERE, + String.format("Failed to load plugin from %s", file.getAbsolutePath()), e); } }
7-7: Enhance command-line documentationWhile the example is helpful, consider adding more comprehensive documentation:
- Document this feature in the server's startup guide
- Add examples of common use cases
- Include any limitations or security considerations of loading external plugins
patches/api/0021-Add-exception-reporting-event.patch (3)
33-35: Restrict Visibility of Internal ConstructorThe constructor annotated with
@ApiStatus.Internalis intended for internal use and should not be public. Consider making it package-private to prevent external usage.Apply this diff to change the visibility:
- public ServerExceptionEvent(final ServerException exception) { + ServerExceptionEvent(final ServerException exception) {
248-248: Remove Unnecessary Semicolon After Method CallThere's an extraneous semicolon after the
callEventmethod call, which is redundant and may cause confusion.Apply this diff to remove the unnecessary semicolon:
Bukkit.getPluginManager().callEvent(new ServerExceptionEvent(new ServerInternalException(cause))); - ;
67-67: UseObjects.requireNonNullInstead of Guava'sPreconditions.checkNotNullTo reduce external dependencies and improve code maintainability, consider using
java.util.Objects.requireNonNullinstead ofcom.google.common.base.Preconditions.checkNotNull.Apply this diff to replace the imports and method calls:
- import static com.google.common.base.Preconditions.checkNotNull; + import static java.util.Objects.requireNonNull;And update the method calls accordingly:
- this.command = checkNotNull(command, "command"); + this.command = requireNonNull(command, "command");Also applies to: 138-138, 291-291, 334-334, 403-403
patches/api/0003-Test-changes.patch (1)
323-326: Consider adding more comprehensive mocking for server methodsThe test server setup could benefit from additional mock configurations for commonly used methods.
Consider adding more mock configurations:
when(instance.getTag(anyString(), any(NamespacedKey.class), any())).thenAnswer(ignored -> new io.papermc.paper.testing.EmptyTag()); when(instance.getScoreboardCriteria(anyString())).thenReturn(null); +when(instance.getPluginManager()).thenReturn(mock(PluginManager.class)); +when(instance.getScheduler()).thenReturn(mock(BukkitScheduler.class)); +when(instance.getServicesManager()).thenReturn(mock(ServicesManager.class));paper-api-generator/src/main/java/io/papermc/generator/types/goal/MobGoalNames.java (1)
289-291: Avoid usingSystem.out.printlnfor loggingAt line 290~, using
System.out.printlnfor logging is not recommended. Consider using a logging framework like SLF4J or Log4j for better control over log output.Apply this diff to utilize a logger:
- System.out.println("need to map " + original + " (" + name.toLowerCase(Locale.ROOT) + ")"); + Logger logger = LoggerFactory.getLogger(MobGoalNames.class); + logger.warn("Need to map {} ({})", original, name.toLowerCase(Locale.ROOT));README.md (1)
66-66: Add missing article 'an' before 'internet connection'At line 66~, there's a minor grammatical error. Consider adding 'an' before 'internet connection' for clarity.
Apply this diff to correct the sentence:
- To compile Paper, you need JDK 21 and internet connection. + To compile Paper, you need JDK 21 and an internet connection.🧰 Tools
🪛 LanguageTool
[uncategorized] ~66-~66: Possible missing article found.
Context: ...ling Jar From Source) ------ To compile Paper, you need JDK 21 and an internet connec...(AI_HYDRA_LEO_MISSING_THE)
patches/api/0002-Build-system-changes.patch (2)
15-19: Consider using a more modern JSON libraryWhile disabling transitive dependencies is good, json-simple 1.1.1 is quite dated (last updated in 2012). Consider using more modern alternatives like Jackson or Gson that are actively maintained and provide better performance.
Line range hint
1-18: Consider using RetentionPolicy.CLASS for the annotationThe annotation is well-documented and serves a clear purpose. However, since this is a marker annotation used for compile-time checks (as evidenced by the scanJarForBadCalls task), RetentionPolicy.CLASS would be more appropriate than RUNTIME, as it would reduce the runtime overhead while still being visible to the bytecode scanner.
-@Retention(RetentionPolicy.RUNTIME) +@Retention(RetentionPolicy.CLASS) @Target({ElementType.METHOD, ElementType.CONSTRUCTOR}) public @interface DoNotUse { }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (38)
paper-api-generator/generated/com/destroystokyo/paper/entity/ai/VanillaGoal.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/AttributeKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/BannerPatternKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/BiomeKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/BlockTypeKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/CatVariantKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/DamageTypeKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/EnchantmentKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/FluidKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/FrogVariantKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/GameEventKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/InstrumentKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/ItemTypeKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/JukeboxSongKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/MapDecorationTypeKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/MenuTypeKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/MobEffectKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/PaintingVariantKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/SoundEventKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/StructureKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/StructureTypeKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/TrimMaterialKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/TrimPatternKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/VillagerProfessionKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/VillagerTypeKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/WolfVariantKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/tags/BannerPatternTagKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/tags/BiomeTagKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/tags/BlockTypeTagKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/tags/CatVariantTagKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/tags/DamageTypeTagKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/tags/EnchantmentTagKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/tags/FluidTagKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/tags/GameEventTagKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/tags/InstrumentTagKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/tags/ItemTypeTagKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/tags/PaintingVariantTagKeys.javais excluded by!**/generated/**paper-api-generator/generated/io/papermc/paper/registry/keys/tags/StructureTagKeys.javais excluded by!**/generated/**
📒 Files selected for processing (23)
.github/workflows/build.yml(2 hunks)README.md(3 hunks)build.gradle.kts(6 hunks)gradle.properties(1 hunks)gradle/wrapper/gradle-wrapper.properties(1 hunks)paper-api-generator/build.gradle.kts(1 hunks)paper-api-generator/src/main/java/io/papermc/generator/types/GeneratedKeyType.java(1 hunks)paper-api-generator/src/main/java/io/papermc/generator/types/goal/MobGoalNames.java(1 hunks)patches/api/0001-Convert-project-to-Gradle.patch(14 hunks)patches/api/0002-Build-system-changes.patch(1 hunks)patches/api/0003-Test-changes.patch(8 hunks)patches/api/0004-Code-Generation.patch(1 hunks)patches/api/0005-Add-FastUtil-to-Bukkit.patch(1 hunks)patches/api/0008-Use-ASM-for-event-executors.patch(1 hunks)patches/api/0012-Add-command-line-option-to-load-extra-plugin-jars-no.patch(1 hunks)patches/api/0013-Player-affects-spawning-API.patch(1 hunks)patches/api/0014-Add-getTPS-method.patch(1 hunks)patches/api/0015-Expose-server-build-information.patch(1 hunks)patches/api/0016-Entity-Origin-API.patch(1 hunks)patches/api/0017-Add-view-distance-API.patch(6 hunks)patches/api/0019-Expose-server-CommandMap.patch(1 hunks)patches/api/0020-Graduate-bungeecord-chat-API-from-spigot-subclasses.patch(1 hunks)patches/api/0021-Add-exception-reporting-event.patch(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- patches/api/0005-Add-FastUtil-to-Bukkit.patch
- patches/api/0019-Expose-server-CommandMap.patch
- patches/api/0013-Player-affects-spawning-API.patch
- gradle/wrapper/gradle-wrapper.properties
- gradle.properties
- patches/api/0016-Entity-Origin-API.patch
- patches/api/0014-Add-getTPS-method.patch
- paper-api-generator/build.gradle.kts
- patches/api/0020-Graduate-bungeecord-chat-API-from-spigot-subclasses.patch
- paper-api-generator/src/main/java/io/papermc/generator/types/GeneratedKeyType.java
- patches/api/0017-Add-view-distance-API.patch
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~66-~66: Possible missing article found.
Context: ...ling Jar From Source) ------ To compile Paper, you need JDK 21 and an internet connec...
(AI_HYDRA_LEO_MISSING_THE)
🔇 Additional comments (24)
patches/api/0015-Expose-server-build-information.patch (5)
17-63: LGTM! Well-structured interface with proper documentation.
The VersionFetcher interface is well-designed with:
- Clear documentation for cache time handling
- Thread-safety considerations in comments
- Helpful dummy implementation with good error feedback
96-103: Consider implementing lazy initialization pattern
The current implementation loads the service immediately. A lazy holder pattern would be more efficient.
- static ServerBuildInfo buildInfo() {
- //<editor-fold defaultstate="collapsed" desc="Holder">
- final class Holder {
- static final Optional<ServerBuildInfo> INSTANCE = Services.service(ServerBuildInfo.class);
- }
- //</editor-fold>
- return Holder.INSTANCE.orElseThrow();
+ static ServerBuildInfo buildInfo() {
+ return LazyHolder.INSTANCE;
+ }
+
+ final class LazyHolder {
+ static final ServerBuildInfo INSTANCE = Services.service(ServerBuildInfo.class).orElseThrow();
+ private LazyHolder() {}
}198-235: LGTM! Well-implemented utility class with proper resource management.
The implementation shows good practices:
- Thread-safe caching with synchronized WeakHashMap
- Proper resource handling with try-with-resources
- Null-safe implementation
354-361: LGTM! Well-implemented lazy loading pattern.
Good implementation of lazy loading for the version fetcher.
381-391: LGTM! Enhanced user experience with rich text.
Good use of Adventure API to provide:
- Clickable version information
- Hover effects
- Clipboard integration
patches/api/0008-Use-ASM-for-event-executors.patch (4)
17-18: LGTM: ASM dependencies added correctly.
The addition of ASM dependencies is appropriate for the bytecode manipulation requirements of this patch.
49-52: Add null parameter validation in constructor.
The constructor parameters should be validated to prevent NullPointerException.
public MethodHandleEventExecutor(final Class<? extends Event> eventClass, final MethodHandle handle) {
+ Preconditions.checkNotNull(eventClass, "eventClass cannot be null");
+ Preconditions.checkNotNull(handle, "handle cannot be null");
this.eventClass = eventClass;
this.handle = handle;
}202-229: LGTM: Well-designed interface with clear documentation.
The interface is well-structured with proper documentation and clear method contracts.
246-301: LGTM: Thread-safe implementation with proper memory management.
The implementation demonstrates good practices:
- Thread-safe class loading with proper synchronization
- Memory leak prevention using weak keys
- Proper error handling and state validation
patches/api/0012-Add-command-line-option-to-load-extra-plugin-jars-no.patch (2)
17-29: LGTM! Well-documented API addition
The new getPluginsFolder() method is properly documented, null-safe, and correctly implements the singleton pattern by delegating to the server instance.
92-94: Verify impact on existing plugin data folders
This change modifies how plugin data folders are located, which could affect existing plugins.
Let's check for potential issues:
Consider:
- Adding migration documentation for plugin developers
- Implementing a fallback mechanism for backward compatibility
- Adding this as a breaking change in the release notes
✅ Verification successful
Plugin data folder location change appears safe to implement
The change to use server.getPluginsFolder() instead of file.getParentFile() standardizes plugin data folder locations. After extensive searching through the codebase:
- No evidence was found of plugins directly depending on JAR-relative data folder paths
- No existing patterns of custom data folder calculations were detected
- No test cases or configurations that would be affected by this change were found
This architectural change appears to be an improvement as it:
- Centralizes plugin data storage location
- Makes the data folder path more predictable
- Aligns with plugin isolation best practices
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for plugins that might rely on relative paths to their JAR location
rg -l "getParentFile\(\).*getDataFolder\(\)" --type java
# Look for custom data folder path calculations
ast-grep --pattern 'new File($_, plugin.getDataFolder())'
Length of output: 118
Script:
#!/bin/bash
# Let's try a broader search for data folder usage patterns
rg -A 3 "getDataFolder" --type java
# Search for File constructor usage with plugins folder
rg -A 3 "new File.*plugins" --type java
# Look for any references to plugin data folders in documentation
fd README -e md -x cat {}
# Check for any existing migration notes or breaking changes
fd CHANGELOG -e md -x cat {}
fd BREAKING -e md -x cat {}
Length of output: 4572
Script:
#!/bin/bash
# Let's check the specific JavaPluginLoader implementation and its usage
rg -A 5 "class JavaPluginLoader" --type java
# Look for any existing plugin data folder initialization patterns
rg -A 5 "dataFolder.*=.*new File" --type java
# Check for any plugin data folder related tests
fd -e java -e test test | xargs rg -l "dataFolder"
# Search for any plugin data folder related configurations
rg -A 3 "plugin.*folder" --type java
Length of output: 184
patches/api/0001-Convert-project-to-Gradle.patch (2)
121-124: LGTM: Test configuration is properly set up
The test configuration correctly uses JUnit Jupiter platform.
49-52: Verify the compatibility of updated dependency versions
The dependencies have been updated to newer versions:
- Guava: 33.3.1-jre
- Gson: 2.11.0
- SnakeYAML: 2.2
These are significant version jumps that may introduce breaking changes.
✅ Verification successful
Based on the analysis of the codebase usage patterns, I can now provide a final response:
The dependency version updates appear safe to proceed with
The codebase primarily uses stable and well-established APIs from these libraries:
- Guava: Mostly uses collection utilities (ImmutableList/Map/Set), base utilities (Preconditions, Objects), and concurrency helpers (ThreadFactoryBuilder) - all stable APIs that maintain compatibility
- Gson: Limited usage focused on basic JSON serialization/deserialization
- SnakeYAML: Minimal usage in core configuration handling
The version jumps are reasonable:
- Guava 33.3.1-jre is a minor version update focused on improvements
- Gson 2.11.0 maintains backward compatibility
- SnakeYAML 2.2 includes security fixes while maintaining API stability
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for breaking changes in the updated dependencies
# Check Guava's changelog
curl -s https://raw.githubusercontent.com/google/guava/master/CHANGES | grep -A 10 "33.3.1"
# Check Gson's release notes
curl -s https://raw.githubusercontent.com/google/gson/master/CHANGELOG.md | grep -A 10 "2.11.0"
# Check SnakeYAML's changelog
curl -s https://raw.githubusercontent.com/snakeyaml/snakeyaml/master/CHANGES.md | grep -A 10 "2.2"
Length of output: 1210
Script:
#!/bin/bash
# Let's check the actual usage of these dependencies in the codebase to assess impact
# Check Guava usage
echo "=== Guava Usage ==="
rg "com\.google\.common" -l
# Check Gson usage
echo -e "\n=== Gson Usage ==="
rg "com\.google\.gson" -l
# Check SnakeYAML usage
echo -e "\n=== SnakeYAML Usage ==="
rg "org\.yaml\.snakeyaml" -l
# Check for any existing version references
echo -e "\n=== Previous Versions ==="
rg -i "guava|gson|snakeyaml" pom.xml || echo "No pom.xml found"
Length of output: 8174
Script:
#!/bin/bash
# Let's check the actual API usage patterns to assess compatibility risks
# Check Guava API usage patterns
echo "=== Guava API Usage Patterns ==="
rg "com\.google\.common\.[a-zA-Z.]+" -A 2 patches/server
# Check Gson API usage patterns
echo -e "\n=== Gson API Usage Patterns ==="
rg "new Gson\(\)|GsonBuilder|toJson|fromJson" -A 2 patches/server
# Check SnakeYAML API usage patterns
echo -e "\n=== SnakeYAML API Usage Patterns ==="
rg "new Yaml\(\)|Yaml\." -A 2 patches/server
Length of output: 67968
patches/api/0003-Test-changes.patch (1)
24-34: LGTM: Mockito agent configuration is properly implemented
The configuration correctly addresses the changes needed for newer Java versions (JEP 451).
patches/api/0004-Code-Generation.patch (3)
306-306:
Consider thread safety for the static collection REGISTRY_KEYS
The REGISTRY_KEYS collection is a static IdentityHashSet. If accessed from multiple threads, it may lead to concurrency issues.
Apply this diff to make REGISTRY_KEYS a thread-safe collection:
-static final Set<RegistryKey<?>> REGISTRY_KEYS = Sets.newIdentityHashSet();
+static final Set<RegistryKey<?>> REGISTRY_KEYS = Collections.newSetFromMap(new ConcurrentHashMap<>());Don't forget to import:
+import java.util.Collections;
+import java.util.concurrent.ConcurrentHashMap;367-369: 🛠️ Refactor suggestion
Add parameter validation in the factory method
The create method should validate its parameters to prevent null values.
Apply this diff:
static <T> TypedKey<T> create(final RegistryKey<T> registryKey, final Key key) {
+ Objects.requireNonNull(registryKey, "registryKey cannot be null");
+ Objects.requireNonNull(key, "key cannot be null");
return new TypedKeyImpl<>(key, registryKey);
}409-412:
Add a comma after the last enum constant
In MinecraftExperimental.java, the newly added enum constants should be followed by a comma.
Apply this diff:
// Paper start
TRADE_REBALANCE,
REDSTONE_EXPERIMENTS,
- MINECART_IMPROVEMENTS
+ MINECART_IMPROVEMENTS,
// Paper endbuild.gradle.kts (2)
190-191: Ensure handling of empty list when retrieving unapplied patches
The unapplied() function at lines 190~-191~ may return an empty list. Verify that subsequent code correctly handles this scenario to avoid unexpected errors.
23-23: Verify compatibility with Java 21
The languageVersion and options.release have been updated to Java 21 at lines 23~ and 33~. Confirm that all dependencies and plugins are compatible with Java 21 to prevent any build or runtime issues.
Also applies to: 33-33
.github/workflows/build.yml (2)
98-105: Ensure the createMojmapPaperclipJar task generates the expected artifact
The build command at line 98~ has been updated to ./gradlew createMojmapPaperclipJar --stacktrace, and the upload path is now build/libs/paper-paperclip-*-mojmap.jar at line 105~. Verify that this task produces the correct artifact and that the upload path matches the generated file.
34-37: Verify Java 21 support with 'zulu' distribution
The Java setup at lines 34~-37~ uses the 'zulu' distribution for Java 21. Ensure that the 'zulu' distribution supports all the required features and is compatible with the build process.
patches/api/0002-Build-system-changes.patch (3)
12-14: LGTM: Dependency version updates look good
The updates to snakeyaml and joml are appropriate, bringing in the latest stable versions with security fixes.
21-35: LGTM: Good improvements to type safety
The changes improve the project in several ways:
- Updated Maven components to latest versions
- Moved away from Java 5 specific annotations
- Added checker-qual and jspecify for better type safety
41-51: LGTM: Complete Javadoc link coverage
Good addition of missing Javadoc links for all dependencies, ensuring complete API documentation coverage.
| +package io.papermc.paper; | ||
| + | ||
| +import java.time.Instant; | ||
| +import java.util.Optional; | ||
| +import java.util.OptionalInt; | ||
| +import net.kyori.adventure.key.Key; | ||
| +import org.jetbrains.annotations.NotNull; | ||
| + | ||
| +public class TestServerBuildInfo implements ServerBuildInfo { | ||
| + @Override | ||
| + public @NotNull Key brandId() { | ||
| + throw new UnsupportedOperationException(); | ||
| + } | ||
| + | ||
| + @Override | ||
| + public boolean isBrandCompatible(final @NotNull Key brandId) { | ||
| + throw new UnsupportedOperationException(); | ||
| + } | ||
| + | ||
| + @Override | ||
| + public @NotNull String brandName() { | ||
| + throw new UnsupportedOperationException(); | ||
| + } | ||
| + | ||
| + @Override | ||
| + public @NotNull String minecraftVersionId() { | ||
| + throw new UnsupportedOperationException(); | ||
| + } | ||
| + | ||
| + @Override | ||
| + public @NotNull String minecraftVersionName() { | ||
| + throw new UnsupportedOperationException(); | ||
| + } | ||
| + | ||
| + @Override | ||
| + public @NotNull OptionalInt buildNumber() { | ||
| + throw new UnsupportedOperationException(); | ||
| + } | ||
| + | ||
| + @Override | ||
| + public @NotNull Instant buildTime() { | ||
| + throw new UnsupportedOperationException(); | ||
| + } | ||
| + | ||
| + @Override | ||
| + public @NotNull Optional<String> gitBranch() { | ||
| + throw new UnsupportedOperationException(); | ||
| + } | ||
| + | ||
| + @Override | ||
| + public @NotNull Optional<String> gitCommit() { | ||
| + throw new UnsupportedOperationException(); | ||
| + } | ||
| + | ||
| + @Override | ||
| + public @NotNull String asString(final @NotNull StringRepresentation representation) { | ||
| + return ""; | ||
| + } | ||
| +} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing proper test coverage.
The current test implementation only throws UnsupportedOperationException for all methods. Consider implementing proper test cases with meaningful assertions to validate the ServerBuildInfo interface behavior.
Would you like me to help generate proper test implementations with meaningful test cases?
| From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 | ||
| From: Zach Brown <[email protected]> | ||
| Date: Mon, 29 Feb 2016 20:24:35 -0600 | ||
| Subject: [PATCH] Add exception reporting event |
There was a problem hiding this comment.
Avoid Including Personal Email Addresses in the Code
Including personal email addresses in the code may expose sensitive information and is not a best practice. Consider removing or obfuscating the email address to protect personal data.
| val f = redirect(proc.inputStream, out) | ||
|
|
There was a problem hiding this comment.
Undefined redirect function may cause compilation errors
The redirect function used at lines 223~ and 281~ is not defined within the provided code. Ensure that this function is properly defined or imported to prevent compilation errors during the build process.
Also applies to: 281-284
| val patchedDirPath = projectDir.path.resolve(patchedDir.get()) | ||
| if (patchedDirPath.isDirectory()) { | ||
| val status = Git(patchedDirPath)("status").getText() | ||
| if (status.contains("You are in the middle of an am session.")) { |
There was a problem hiding this comment.
Add null check for status to prevent NullPointerException
At line 206~, ensure that status is not null before invoking contains. Add a null check or handle potential null values to prevent a NullPointerException.
| val failed = unapplied.single { p -> | ||
| p.useLines { lines -> | ||
| val collect = mutableListOf<String>() | ||
| for (line in lines) { | ||
| if (line.startsWith(subjectPrefix)) { | ||
| collect += line | ||
| } else if (collect.size == 1) { | ||
| if (continuationRegex.matcher(line).matches()) { | ||
| collect += line | ||
| } else { | ||
| break | ||
| } | ||
| } | ||
| } | ||
| val subjectLine = collect.joinToString("").substringAfter(subjectPrefix) | ||
| subjectLine.startsWith(failedSubjectFragment) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential exceptions when using single method
At line 233~, the use of unapplied.single { ... } could throw an exception if no elements or multiple elements satisfy the predicate. To enhance robustness, consider using find and handle the null case appropriately.
Apply this diff to improve exception handling:
-val failed = unapplied.single { p ->
+val failed = unapplied.find { p ->
// existing code
}
+if (failed == null) {
+ throw IllegalStateException("Failed to determine the failed patch.")
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val failed = unapplied.single { p -> | |
| p.useLines { lines -> | |
| val collect = mutableListOf<String>() | |
| for (line in lines) { | |
| if (line.startsWith(subjectPrefix)) { | |
| collect += line | |
| } else if (collect.size == 1) { | |
| if (continuationRegex.matcher(line).matches()) { | |
| collect += line | |
| } else { | |
| break | |
| } | |
| } | |
| } | |
| val subjectLine = collect.joinToString("").substringAfter(subjectPrefix) | |
| subjectLine.startsWith(failedSubjectFragment) | |
| } | |
| } | |
| val failed = unapplied.find { p -> | |
| p.useLines { lines -> | |
| val collect = mutableListOf<String>() | |
| for (line in lines) { | |
| if (line.startsWith(subjectPrefix)) { | |
| collect += line | |
| } else if (collect.size == 1) { | |
| if (continuationRegex.matcher(line).matches()) { | |
| collect += line | |
| } else { | |
| break | |
| } | |
| } | |
| } | |
| val subjectLine = collect.joinToString("").substringAfter(subjectPrefix) | |
| subjectLine.startsWith(failedSubjectFragment) | |
| } | |
| } | |
| if (failed == null) { | |
| throw IllegalStateException("Failed to determine the failed patch.") | |
| } |
| } | ||
| } | ||
| } | ||
| throw new RuntimeException("Can't figure out applicable entity for mob goal " + goalClass); // maybe just return EntityInsentient? |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Throw specific exceptions instead of RuntimeException
Throwing a generic RuntimeException at lines 325~ and 332~ can make error handling difficult. Use a more specific exception to provide clearer context.
Apply this diff to throw a specific exception:
At line 325~:
- throw new RuntimeException("Can't figure out applicable entity for mob goal " + goalClass);
+ throw new IllegalArgumentException("Cannot determine applicable entity for mob goal: " + goalClass);At lines 332~-333~:
- throw new RuntimeException("Can't figure out applicable bukkit entity for nms entity " + nmsClass);
+ throw new IllegalArgumentException("Cannot determine Bukkit entity for NMS entity: " + nmsClass);Also applies to: 332-333
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (9)
patches/api/0004-Code-Generation.patch (4)
26-27: Consider adding documentation for the generated API path.The
generatedApiPathvariable would benefit from a comment explaining the purpose and structure of the generated files that will be placed in this directory.
74-77: Enhance documentation with usage example.Consider adding a code example to demonstrate how this annotation should be used.
/** * Used to mark classes which are generated from * a specific version of minecraft. * * Example: * {@code * @GeneratedFrom("1.20.4") * public class GeneratedClass { * // ... * } * } */
164-171: Clarify the reason for "DO NOT USE" API notes.The
@apiNote DO NOT USEcomments forBLOCKandITEMregistry keys should explain why these shouldn't be used to prevent accidental usage.
383-383: Add class-level documentation.Consider adding a class-level Javadoc explaining the implementation details and any assumptions made.
patches/api/0008-Use-ASM-for-event-executors.patch (5)
103-104: Enhance error messages for better debugging.Consider including more context in the error messages:
- Preconditions.checkArgument(Modifier.isStatic(m.getModifiers()), "Not a static method: %s", m); - Preconditions.checkArgument(eventClass != null, "eventClass is null"); + Preconditions.checkArgument(Modifier.isStatic(m.getModifiers()), "Method %s in %s must be static", m.getName(), m.getDeclaringClass().getName()); + Preconditions.checkArgument(eventClass != null, "Event class cannot be null for method %s", m.getName());
202-223: Add thread-safety documentation.Consider documenting the thread-safety guarantees of this interface.
/** + * Interface for defining classes at runtime. + * <p> + * Implementations of this interface must be thread-safe. + * * @throws ClassFormatError if the class data is invalid * @throws NullPointerException if any of the arguments are null */
253-253: Consider adding initial capacity hint for better performance.The loaders map could benefit from an initial capacity hint to reduce resizing operations.
- private final ConcurrentMap<ClassLoader, GeneratedClassLoader> loaders = new MapMaker().weakKeys().makeMap(); + private final ConcurrentMap<ClassLoader, GeneratedClassLoader> loaders = new MapMaker().weakKeys().initialCapacity(32).makeMap();
368-372: Improve code comment professionalism.The comment about "bukkit stupidity" should be rephrased more professionally.
- // Define a wrapper to conform to bukkit stupidity (passing in events that don't match and wrapper exception) + // Define a wrapper to handle event type validation and exception wrapping for compatibility
1-381: Well-designed performance optimization for event handling.The implementation effectively uses ASM for bytecode generation and method handles for reflection, providing a significant performance improvement. The code is well-structured with proper synchronization and error handling.
Consider documenting the performance implications and benefits in the project's documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
build.gradle.kts(6 hunks)patches/api/0001-Convert-project-to-Gradle.patch(14 hunks)patches/api/0002-Build-system-changes.patch(1 hunks)patches/api/0003-Test-changes.patch(8 hunks)patches/api/0004-Code-Generation.patch(1 hunks)patches/api/0005-Add-FastUtil-to-Bukkit.patch(1 hunks)patches/api/0008-Use-ASM-for-event-executors.patch(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- patches/api/0005-Add-FastUtil-to-Bukkit.patch
- patches/api/0002-Build-system-changes.patch
🔇 Additional comments (15)
build.gradle.kts (3)
Line range hint 43-55: LGTM: Test configuration with Mockito agent
The test configuration properly integrates the Mockito agent and follows dependency management best practices by disabling transitive dependencies.
Line range hint 58-62: LGTM: Improved test parameter names configuration
Adding the -parameters compiler argument for test compilation improves the readability of parameterized test names in JUnit.
Line range hint 24-34: LGTM: Mockito agent configuration for newer Java versions
The configuration correctly addresses the changes introduced in JEP 451 for newer Java versions. The implementation follows Gradle best practices for JVM argument providers.
✅ Verification successful
Mockito agent configuration is correctly implemented
The verification confirms that the Mockito agent is properly configured across the codebase:
- Mockito agent configuration is present in both API and server patches
- Consistent version (5.14.1) is used across all configurations
- Proper implementation of
CommandLineArgumentProviderfor JVM arguments - Configuration follows the requirements for newer Java versions (JEP 451)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Mockito agent configuration and dependencies
# Check if mockito-core dependency is present and verify its version
rg -A 2 "mockito-core" | grep -E "version|implementation"
Length of output: 1028
Script:
#!/bin/bash
# Check for Mockito agent configuration in build files
rg -A 5 "mockitoAgent" build.gradle.kts subprojects/*/build.gradle.kts
# Check for JVM args configuration related to Mockito
rg -A 5 "jvmArgs" build.gradle.kts subprojects/*/build.gradle.kts
Length of output: 282
Script:
#!/bin/bash
# Find all gradle build files and check their content for Mockito configuration
fd -e gradle -e kts | xargs rg -A 5 "mockitoAgent|jvmArgs"
# Also check patch files for Mockito-related changes
fd -e patch | xargs rg -A 5 "mockitoAgent|jvmArgs"
Length of output: 7731
patches/api/0001-Convert-project-to-Gradle.patch (3)
49-50: LGTM: Updated Guava dependency
The Guava dependency has been updated to the latest stable version (33.3.1-jre).
66-69: LGTM: Updated test dependencies
Test dependencies have been updated to use JUnit Jupiter and latest versions of testing libraries.
105-108: LGTM: Updated Javadoc configuration
The Javadoc configuration has been updated with correct links to the latest API documentation.
patches/api/0003-Test-changes.patch (4)
Line range hint 71-102: LGTM: EmptyTag implementation
The EmptyTag implementation correctly provides a test utility for empty Tag instances with proper nullability annotations.
146-150: LGTM: Added @NullMarked class skipping
Correctly skips classes marked with @NullMarked to avoid redundant nullability checks.
191-198: LGTM: Enhanced TYPE_USE annotation support
Added support for TYPE_USE annotations with proper reference checking.
323-326: LGTM: Added test server mock implementations
Added proper mock implementations for getTag and getScoreboardCriteria methods.
patches/api/0004-Code-Generation.patch (3)
306-306: Consider thread safety for the static collection REGISTRY_KEYS
The REGISTRY_KEYS collection should be thread-safe as it could be accessed from multiple threads.
367-369: Add parameter validation in the factory method.
The create method should validate its parameters to prevent null values.
409-412: Add a comma after the last enum constant.
Add a trailing comma after MINECART_IMPROVEMENTS to maintain consistency and prevent syntax errors.
patches/api/0008-Use-ASM-for-event-executors.patch (2)
17-18: LGTM! Dependencies are appropriate.
The ASM dependencies are at a recent stable version (9.7.1) and include both necessary modules (core and commons) for bytecode manipulation.
49-52: Add null checks in constructor parameters.
The constructor parameters should be validated to prevent NullPointerException at runtime.
| + // Non-void event handlers will be unsupported in a future release. | ||
| + switch (Type.getType(m.getReturnType()).getSize()) { | ||
| + // case 0 is omitted because the only type that has size 0 is void - no pop instruction needed. | ||
| + case 1 -> methodGenerator.pop(); // handles reference types and most primitives | ||
| + case 2 -> methodGenerator.pop2(); // handles long and double | ||
| + } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding formal deprecation for non-void event handlers.
Since non-void event handlers will be unsupported in a future release, consider adding a formal deprecation notice using annotations and documentation.
+ /**
+ * @deprecated Event handlers should not return values. This will be enforced in a future release.
+ */
+ @Deprecated(forRemoval = true, since = "1.0.0")
switch (Type.getType(m.getReturnType()).getSize()) {
// case 0 is omitted because the only type that has size 0 is void - no pop instruction needed.
case 1 -> methodGenerator.pop(); // handles reference types and most primitives
case 2 -> methodGenerator.pop2(); // handles long and double
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| + // Non-void event handlers will be unsupported in a future release. | |
| + switch (Type.getType(m.getReturnType()).getSize()) { | |
| + // case 0 is omitted because the only type that has size 0 is void - no pop instruction needed. | |
| + case 1 -> methodGenerator.pop(); // handles reference types and most primitives | |
| + case 2 -> methodGenerator.pop2(); // handles long and double | |
| + } | |
| /** | |
| * @deprecated Event handlers should not return values. This will be enforced in a future release. | |
| */ | |
| @Deprecated(forRemoval = true, since = "1.0.0") | |
| switch (Type.getType(m.getReturnType()).getSize()) { | |
| // case 0 is omitted because the only type that has size 0 is void - no pop instruction needed. | |
| case 1 -> methodGenerator.pop(); // handles reference types and most primitives | |
| case 2 -> methodGenerator.pop2(); // handles long and double | |
| } |
* Switch Impl types to Holderable * Fix compile issues * more improvements * compile fixes * remove unneeded unwrapAndConvertHolder call --------- Co-authored-by: Bjarne Koll <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
patches/api/0003-Test-changes.patch (2)
Line range hint
203-222: Consider enhancing error message formattingThe error message builder could be more readable with String.format or text blocks (Java 15+).
- StringBuilder builder = new StringBuilder() - .append("There ") - .append(errors.size() != 1 ? "are " : "is ") - .append(errors.size()) - .append(" missing annotation") - .append(errors.size() != 1 ? "s:\n" : ":\n"); + String message = String.format(""" + There %s %d missing annotation%s: + %s""", + errors.size() != 1 ? "are" : "is", + errors.size(), + errors.size() != 1 ? "s" : "", + errors.stream().map(e -> "\t" + e).collect(joining("\n")) + );
330-331: Document reason for disabling the testConsider adding a comment explaining why this test class is being disabled to help future maintainers understand the context.
-@org.junit.jupiter.api.Disabled // Paper +@org.junit.jupiter.api.Disabled("Paper - TODO: Document why this test is disabled")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
patches/api/0003-Test-changes.patch(8 hunks)
🔇 Additional comments (4)
patches/api/0003-Test-changes.patch (4)
Line range hint 105-142: LGTM: Well-structured test utility class
The EmptyTag implementation is clean, immutable, and properly annotated. It serves its purpose as a test utility class perfectly.
Line range hint 105-198: LGTM: Enhanced annotation testing with TYPE_USE support
The implementation properly handles TYPE_USE annotations and provides comprehensive nullability checking.
342-346: LGTM: Extended mock server configuration
The additional mock configurations for tags and scoreboard criteria are properly implemented using the new EmptyTag utility class.
24-34: LGTM: Mockito agent configuration for newer Java versions
The configuration properly addresses the changes required by JEP 451 for newer Java versions. The implementation is clean and well-documented.
Let's verify the Mockito version compatibility:
Also applies to: 41-43
✅ Verification successful
Mockito agent configuration is correctly implemented
The implementation properly addresses JEP 451 requirements by:
- Using Mockito 5.14.1, which is compatible with newer Java versions
- Correctly configuring the Mockito agent with proper javaagent setup
- Including appropriate transitive dependency handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for latest stable Mockito version and its Java compatibility
curl -s "https://search.maven.org/solrsearch/select?q=g:org.mockito+AND+a:mockito-core&rows=1&wt=json" | jq -r '.response.docs[0].latestVersion'
Length of output: 153
Script:
#!/bin/bash
# Let's check the actual Mockito version being used in the project
rg -U "mockito.*version|mockito-core" --type gradle
Length of output: 202
Script:
#!/bin/bash
# Let's check build files for Mockito configuration
fd -e gradle -e kts | xargs cat
Length of output: 12769
Script:
#!/bin/bash
# Let's check the patch file directly to see the Mockito configuration
cat patches/api/0003-Test-changes.patch
Length of output: 15991
…roperties from original node (#11654)
Fixes #11649 - As noted in the issue, when CommandNodes are serialized they are used as the key in a Map. Their equals()/hashcode() should only match if they are equal nodes (name & command), but due to the erasure of the command field pre-serialization, nodes with different commands can be mapped onto the same value. This causes the client to interpret both nodes as the same, causing suggestions where they should not. This is fixed by creating a different no-op command for the erasure, instead of them holding the same lambda.
* Re-add shear methods from bukkit Shearable * Add back
…tions in ClientboundPlayerInfoUpdatePacket (#11731)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (2)
4-4: Format the bare URL as a markdown linkConvert the bare URL to a proper markdown link for better readability and markdown compliance.
-If you need history from before 1.21.4, please use the archive repo: https://github.com/PaperMC/Paper-archive +If you need history from before 1.21.4, please use the archive repo: [Paper Archive Repository](https://github.com/PaperMC/Paper-archive)🧰 Tools
🪛 Markdownlint (0.37.0)
4-4: null
Bare URL used(MD034, no-bare-urls)
71-71: Document the build command changeThe build command has changed from
createReobfBundlerJartocreateMojmapBundlerJar. Consider adding a brief explanation of this change and any implications for developers.Would you like me to help draft an explanation of the command change for the documentation?
Also applies to: 73-73
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(3 hunks)
🧰 Additional context used
🪛 Markdownlint (0.37.0)
README.md
4-4: null
Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (3)
README.md (3)
12-12: LGTM!
The updated project description better reflects the project's current status and purpose.
16-16: LGTM!
The simplified support channels focus on the most active platforms.
48-48: Verify compatibility with the new versions
The updates include significant changes:
- paper-api version bump to 1.21.4
- Java toolchain upgrade to 21
These changes might affect users' existing setups and plugin compatibility.
Also applies to: 61-61, 65-65
✅ Verification successful
Version and Java toolchain updates are properly documented
The version changes are consistently documented in the README.md:
- paper-api version 1.21.4-R0.1-SNAPSHOT is correctly specified in both Maven and Gradle dependency configurations
- Java toolchain version 21 requirement is clearly documented in:
- Gradle configuration section
- "How To (Compiling Jar From Source)" section which explicitly states "you need JDK 21"
The changes are well-aligned and properly communicated to both plugin developers and users compiling from source.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the version changes are consistently applied across the codebase
echo "Checking version references..."
rg "1\.20\.1|1\.21\.4" --type xml --type gradle --type properties
echo "Checking Java version references..."
rg "Java (17|21)|JDK (17|21)" --type md --type gradle --type properties
Length of output: 363
Script:
#!/bin/bash
# Check version references in pom.xml and build.gradle files
echo "Checking version references in build files..."
find . -type f -name "pom.xml" -o -name "build.gradle" | xargs cat
# Check for Java version configurations
echo -e "\nChecking Java toolchain configurations..."
find . -type f -name "*.gradle" -o -name "*.properties" -o -name "pom.xml" | xargs grep -l "sourceCompatibility\|targetCompatibility\|java.version\|maven.compiler"
# Check README for version information
echo -e "\nChecking README for version information..."
cat README.md
Length of output: 5168
A
Summary by CodeRabbit
New Features
WorldandPlayerinterfaces.permissions.yml.VersionFetcherinterface for version information management.Bug Fixes
Documentation
CONTRIBUTING.mdto reflect new Java version requirements and guidelines.Chores