Custom file handler (Also workaround for issue with mixin paths in mixin configs) #66
Conversation
…bility, and align version string format.
…iguration for publishing
It can be useful for making handlers for files that need special handling like Mixin which doesn't store full paths.
WalkthroughThreads a map of file-pattern -> CustomFileHandler through relocation APIs. Adds Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Config as ForgixConfiguration
participant Facade as Forgix / MergeJarsTask
participant Relocator
participant Handler as CustomFileHandler
User->>Config: mixin(...) / fileHandler(...)
Config-->>Config: store pattern -> handler
User->>Facade: mergeLoaders(jars, out, config.getCustomFileHandlers())
Facade->>Relocator: relocate(relocationConfigs, customFileHandlers)
Relocator->>Relocator: relocateClasses(...)
Relocator->>Relocator: relocateResources(..., customFileHandlers, ...)
loop per resource entry
Relocator->>Relocator: match resource name against patterns
alt matched pattern
Relocator->>Handler: handle(name, content, replacementMap)
Handler-->>Relocator: transformed content
Relocator->>Relocator: write transformed resource
else no match
Relocator->>Relocator: apply mapping-based replacements
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
src/main/java/io/github/pacifistmc/forgix/core/RelocationConfig.java (1)
3-3: Unused import.The
ForgixConfigurationimport doesn't appear to be used anywhere in this file.Apply this diff to remove the unused import:
-import io.github.pacifistmc.forgix.plugin.configurations.ForgixConfiguration; -src/main/java/io/github/pacifistmc/forgix/core/filehandlers/MixinFileHandler.java (1)
36-37: Remove unused variables.The
mixinPluginandmixinRefvariables are read but never used.Apply this diff to remove the dead code:
- JsonPrimitive mixinPlugin = mixinJson.getAsJsonPrimitive("plugin"); - JsonPrimitive mixinRef = mixinJson.getAsJsonPrimitive("refmap"); -src/main/java/io/github/pacifistmc/forgix/plugin/configurations/ForgixConfiguration.java (1)
3-3: Unused import.The
Gsonimport doesn't appear to be used in this file. It may have been added in anticipation of future use or by mistake.Apply this diff to remove the unused import:
-import com.google.gson.*;src/main/java/io/github/pacifistmc/forgix/core/Relocator.java (7)
24-28: Static tempDir creation on class load can fail opaquelyCreating the temp directory in a static field initializer means any
IOExceptionduring class loading will surface as anExceptionInInitializerErrorand take the whole tool down without context. Consider lazy-initializingtempDirinsidegenerateMappings(or a small helper) with explicit error handling/logging, and optionally recreating it if it was deleted between runs.
46-73: mappingsSnapshot lifecycle and keying by JarFile may leak state across runs
mappingsSnapshotis static, never cleared, and keyed byJarFileidentity. Over multiple invocations ofRelocator.relocateyou can accumulate stale mappings keyed by closedJarFileinstances, and those are never reclaimed until process exit. Also, if a future refactor reuses the sameRelocatorin a long-lived process, stale mappings could accidentally be observed.Two concrete suggestions:
- Clear
mappingsSnapshotonce the outerrelocateClasses(...)call finishes all passes (after the final restoration of mappings).- Consider keying by the underlying jar path (
relocationConfig.jarFile.getName()orFile) instead ofJarFileto avoid tying state to object identity.
49-60: RelocateClasses recursive multi-pass lacks hard cap and could recurse deeplyThe recursive
relocateClasses(relocationConfigs, true);at the end relies on the early-return condition that no mappings end with.classto terminate. If for any reason a new set of.classconflicts keeps appearing each pass (e.g., due to unforeseen mapping interactions), this becomes unbounded recursion and risks stack overflow.A safer structure would:
- Replace the recursive call with an explicit loop (e.g.,
while (true)with a break) and track whether the last pass introduced any new.classmappings.- Optionally guard with a reasonable maximum number of passes and fail fast with a clear error if exceeded.
This keeps the behavior but makes termination guarantees explicit and protects against pathological inputs.
Also applies to: 102-118
49-50: Use of List.getFirst() raises the minimum Java version
relocationConfigs.getFirst()relies on the JDK 21+List#getFirstAPI. If the project’s intended baseline is Java 17 (common in the Minecraft ecosystem) or earlier, this will not compile without additional tooling.If you still need to support JDK < 21, consider switching to the conventional
relocationConfigs.get(0)with an explicit empty-list check at the public entry point(s).Also applies to: 127-128
147-162: CustomFileHandler is invoked per-conflict, not per-file, and ignores predicatesThe current handler invocation pattern:
- Builds
conflictsas a map from predicates to singleton replacement maps.- For each resource entry, loops over all
conflicts, and when a handler is present callshandler.handle(entry.getName(), content, conflict.value)for each conflict, without checkingconflict.getKey().test(content).Effects:
- Handlers are called multiple times per file with one replacement mapping at a time instead of once with all applicable mappings, which is both less efficient and surprising given the API shape (
replacementPathssounds like “all replacements for this file”).- Handlers receive mappings that may not be relevant to the current file because the predicate is ignored in the handler path.
- For larger
mappings, this becomesO(#conflicts * size(content))work per file.A more predictable design would be:
- First, collect the union of all
conflictvalue maps that actually apply to a given file (respecting their predicates).- If a handler exists, call it once per file with
replacementPathsequal to that aggregated map.- If no handler exists, apply the aggregated replacements generically.
Sketch:
Map<String, String> applicableReplacements = new HashMap<>(); for (var conflict : conflicts.entrySet()) { if (conflict.getKey().test(content)) { applicableReplacements.putAll(conflict.getValue()); } } if (handler != null) { content = handler.handle(entry.getName(), content, applicableReplacements); } else { for (var replacement : applicableReplacements.entrySet()) { content = content.replace(replacement.getKey(), replacement.getValue()); } } contentMapping.put(entry, content);This keeps the flexibility of custom handlers while aligning better with the advertised API and improving performance.
Also applies to: 164-190
167-175: PathMatcher allocation for every resource/pattern pair is unnecessarily expensiveInside the resource loop you do:
CustomFileHandler handler = customFileHandlers.entrySet() .stream() .filter(entry1 -> { var matcher = FileSystems.getDefault().getPathMatcher("glob:${entry1.key}"); return matcher.matches(Paths.get(entry.getName())); }) ...This constructs a new
PathMatcherfor each(resource, pattern)combination. With many resources or patterns this becomes a hotspot.Consider precomputing the matchers once when the configuration is built or at the start of
relocateResources, e.g. turnMap<String, CustomFileHandler>intoMap<PathMatcher, CustomFileHandler>(or a small helper holder) so you only allocate matchers once per pattern.
209-264: generateMappings/mapConflicts logic is subtle; consider clearer append semantics and cleanupThe
appendflag drives several behaviors at once:
- Whether to reuse
file.jarFileor reopen vianew JarFile(file.jarFile.getName()).- Whether to clear all existing
RelocationConfig.mappings.- Whether new conflicts overwrite vs. accumulate across passes and between classes/resources.
This is hard to reason about, especially in combination with
mappingsSnapshotand the multi-pass logic inrelocateClasses/relocateResources.Two concrete improvements:
- Separate concerns: introduce two flags or an enum to distinguish “re-scan jars using current state” from “discard in-memory mappings but append to tiny file”. That will make the control flow far easier to follow.
- After the outer relocate workflow completes, clear any temporary or pass-specific state (e.g.,
filesByPathlocal is fine, butmappingsSnapshotand any temp tiny files intempDirmay warrant cleanup) to avoid surprising behavior on repeated invocations.Not urgent, but refactoring this will make future changes to conflict mapping less error‑prone.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/java/io/github/pacifistmc/forgix/Forgix.java(3 hunks)src/main/java/io/github/pacifistmc/forgix/core/Multiversion.java(1 hunks)src/main/java/io/github/pacifistmc/forgix/core/RelocationConfig.java(1 hunks)src/main/java/io/github/pacifistmc/forgix/core/Relocator.java(2 hunks)src/main/java/io/github/pacifistmc/forgix/core/filehandlers/CustomFileHandler.java(1 hunks)src/main/java/io/github/pacifistmc/forgix/core/filehandlers/MixinFileHandler.java(1 hunks)src/main/java/io/github/pacifistmc/forgix/plugin/configurations/ForgixConfiguration.java(3 hunks)src/main/java/io/github/pacifistmc/forgix/plugin/tasks/MergeJarsTask.java(1 hunks)src/test/java/io/github/pacifistmc/forgix/tests/CoreTest.java(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
src/main/java/io/github/pacifistmc/forgix/plugin/configurations/ForgixConfiguration.java (1)
src/main/java/io/github/pacifistmc/forgix/core/filehandlers/MixinFileHandler.java (1)
MixinFileHandler(7-41)
src/main/java/io/github/pacifistmc/forgix/Forgix.java (2)
src/main/java/io/github/pacifistmc/forgix/plugin/configurations/ForgixConfiguration.java (1)
ForgixConfiguration(19-354)src/main/java/io/github/pacifistmc/forgix/core/Relocator.java (1)
Relocator(23-265)
src/main/java/io/github/pacifistmc/forgix/core/RelocationConfig.java (1)
src/main/java/io/github/pacifistmc/forgix/plugin/configurations/ForgixConfiguration.java (1)
ForgixConfiguration(19-354)
src/main/java/io/github/pacifistmc/forgix/plugin/tasks/MergeJarsTask.java (1)
src/main/java/io/github/pacifistmc/forgix/Forgix.java (1)
Forgix(17-145)
src/main/java/io/github/pacifistmc/forgix/core/Multiversion.java (1)
src/main/java/io/github/pacifistmc/forgix/core/Relocator.java (1)
Relocator(23-265)
src/test/java/io/github/pacifistmc/forgix/tests/CoreTest.java (2)
src/main/java/io/github/pacifistmc/forgix/core/filehandlers/MixinFileHandler.java (1)
MixinFileHandler(7-41)src/main/java/io/github/pacifistmc/forgix/core/Relocator.java (1)
Relocator(23-265)
src/main/java/io/github/pacifistmc/forgix/core/Relocator.java (2)
src/main/java/io/github/pacifistmc/forgix/plugin/configurations/ForgixConfiguration.java (1)
ForgixConfiguration(19-354)src/main/java/io/github/pacifistmc/forgix/utils/JAR.java (1)
JAR(25-644)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (8)
src/main/java/io/github/pacifistmc/forgix/plugin/tasks/MergeJarsTask.java (1)
77-77: LGTM!The integration of custom file handlers into the merge operation is correct and aligns with the new API.
src/main/java/io/github/pacifistmc/forgix/core/Multiversion.java (1)
51-51: LGTM!Passing an empty map for custom file handlers is appropriate for the multiversion use case and correctly adapts to the new API signature.
src/main/java/io/github/pacifistmc/forgix/core/filehandlers/CustomFileHandler.java (1)
1-8: LGTM!Clean interface design with appropriate functional interface annotation. The signature provides the necessary flexibility for custom file handling during relocation.
src/main/java/io/github/pacifistmc/forgix/plugin/configurations/ForgixConfiguration.java (1)
336-346: LGTM!The custom file handler API is well-designed with a clean separation of concerns. The
mixinconvenience method provides good developer ergonomics for the common use case.src/test/java/io/github/pacifistmc/forgix/tests/CoreTest.java (3)
193-193: LGTM!Correctly updated to use the new
Relocator.relocateAPI with an empty map for custom file handlers.
251-251: LGTM!Correctly updated to use the new
Relocator.relocateAPI with an empty map for custom file handlers.
316-414: LGTM!Comprehensive test coverage for the custom file handling feature. The test verifies that mixin class paths are correctly remapped in the JSON configuration files for both jars.
src/main/java/io/github/pacifistmc/forgix/Forgix.java (1)
22-34: LGTM!The API extension maintains backward compatibility with the default parameter and correctly integrates custom file handlers into the relocation pipeline.
src/main/java/io/github/pacifistmc/forgix/core/filehandlers/MixinFileHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/pacifistmc/forgix/core/filehandlers/MixinFileHandler.java
Outdated
Show resolved
Hide resolved
| public static void relocateResources(List<RelocationConfig> relocationConfigs, Map<String, CustomFileHandler> customFileHandlers, boolean anotherPass=false) { | ||
| // Generate mappings if they don't exist or this is another pass | ||
| if (anotherPass || relocationConfigs.getFirst().tinyFile == null) generateMappings(relocationConfigs, !anotherPass); | ||
|
|
||
| // Return if there are no new conflicts | ||
| // if (anotherPass && relocationConfigs.stream().allMatch(config -> config.mappings.isEmpty())) return; | ||
| // This will return if there are no conflicts at all (empty mappings) or there are only class and/or META-INF conflicts which we're ignoring | ||
| if (anotherPass && relocationConfigs.stream() | ||
| .flatMap(config -> config.mappings.keySet().stream()) | ||
| .noneMatch(mapping -> !mapping.endsWith(".class") && !mapping.startsWith("META-INF/")) // Checking to see if there's any resource that isn't a class or META-INF, if there isn't then return | ||
| ) return; | ||
|
|
||
| AtomicBoolean doAnotherPass = new AtomicBoolean(false); | ||
|
|
||
| // Process each JAR file in parallel | ||
| relocationConfigs.parallelStream().forEach(relocationConfig -> { | ||
| JarFile jarFile = JAR.isClosed(relocationConfig.jarFile) ? new JarFile(relocationConfig.jarFile.getName()) : relocationConfig.jarFile; // Reopen the JAR file if it's closed (it can be closed by the `relocateClasses` method) | ||
|
|
||
| Set<JarEntry> resources = JAR.getResources(jarFile); | ||
| Map<JarEntry, String> contentMapping = new HashMap<>(); | ||
|
|
||
| // Create a map of conflicts with path alterations. | ||
| Map<Predicate<String>, Map<String, String>> conflicts = new HashMap<>(); | ||
| Map<String, String> fileConflicts = new HashMap<>(); // Keep track of mixins to handle them specially | ||
| relocationConfig.mappings.forEach((originalPath, relocatedPath) -> { | ||
| if (originalPath.endsWith("META-INF/MANIFEST.MF")) return; // Skip manifest | ||
| if (!originalPath.endsWith(".class") && !originalPath.startsWith("META-INF/services/")) { // Is a regular file conflict | ||
| fileConflicts.put(originalPath, relocatedPath); | ||
| } | ||
| // replacing with `removeExtension()` would make the ones with extensions be replaced which is what we want | ||
| conflicts.put(content -> content.contains(originalPath.removeExtension()), Map.of(originalPath.removeExtension(), relocatedPath.removeExtension())); // Add the original path without the .class extension // the predicate should be _ -> true but for some reason that doesn't work | ||
| conflicts.put(_ -> originalPath.contains("/"), Map.of(originalPath.removeExtension().replace('/', '.'), relocatedPath.removeExtension().replace('/', '.'))); // If it's in a directory, add the original path without the .class extension and with dots instead of slashes | ||
| conflicts.put(_ -> originalPath.contains("/"), Map.of(originalPath.removeExtension().replace('/', '\\'), relocatedPath.removeExtension().replace('/', '\\'))); // If it's in a directory, add the original path without the .class extension and with backslashes instead of slashes | ||
| if (originalPath.endsWith(".class")) { // Is a class conflict | ||
| conflicts.put(content -> content.contains("\"${originalPath.getPath().replace('/', '.')}\""), // look for the path with dots and in quotes (for mixins), example: "com.example.mod.mixins" | ||
| Map.of("\"${originalPath.getBaseName().removeExtension()}\"", "\"${relocatedPath.getBaseName().removeExtension()}\"")); // Just the filename without the path & extension and in quotes (for mixins) | ||
| } | ||
| }); | ||
|
|
||
| resources.parallelStream().forEach(entry -> { | ||
| String content = JAR.getResource(jarFile, entry); | ||
| for (var conflict : conflicts.entrySet()) { | ||
| // TODO: Smart replace | ||
| // If a file has "com.example.Meow" and "com.example.Meow2" and we're only replacing "com.example.Meow" then only replace all instances of "com.example.Meow" but not "com.example.Meow2" | ||
| if (conflict.getKey().test(content)) { | ||
| for (var replacement : conflict.getValue().entrySet()) { | ||
| content = content.replace(replacement.getKey(), replacement.getValue()); | ||
| } | ||
| } | ||
| } | ||
| contentMapping.put(entry, content); | ||
| }); | ||
|
|
||
| doAnotherPass.set(JAR.writeResources(jarFile, contentMapping)); | ||
| jarFile.close(); | ||
| JAR.renameResources(jarFile.getName(), fileConflicts); | ||
| }); | ||
|
|
||
| // Do a multiple passes to handle conflicts that were created by the previous pass | ||
| if (doAnotherPass.get()) relocateResources(relocationConfigs, true); | ||
| } | ||
|
|
||
| /** | ||
| * Sets up the mappings for conflicting files in JARs. | ||
| * @param relocationConfigs The relocationConfigs to process | ||
| * @param append Isn't a good name, but we set it to false to check if we have any new conflicts, | ||
| * setting it to false will ignore previous mappings and overwrite them, | ||
| * but it will still append the mappings to the tiny file | ||
| */ | ||
| public static void generateMappings(List<RelocationConfig> relocationConfigs, boolean append = true) { | ||
| mapConflicts(relocationConfigs, append); | ||
| TinyClassWriter.write(relocationConfigs, tempDir); | ||
| } | ||
|
|
||
| /** | ||
| * Maps conflicting entries to their relocated paths. | ||
| * @param relocationConfigs The list of relocationConfigs to process | ||
| * @param append Whether to append to the existing mappings | ||
| */ | ||
| private static void mapConflicts(List<RelocationConfig> relocationConfigs, boolean append = true) { | ||
| record FileInfo(String path, byte[] hash, RelocationConfig source) { } | ||
|
|
||
| // Map to store all relocationConfigs and their hashes grouped by path | ||
| Map<String, List<FileInfo>> filesByPath = new ConcurrentHashMap<>(); | ||
|
|
||
| // Process each JAR file in parallel | ||
| relocationConfigs.parallelStream().forEach(file -> { | ||
| var jarFile = append ? file.jarFile : new JarFile(file.jarFile.getName()); // If we're not appending, we need to reopen the JAR file | ||
| var entries = jarFile.entries(); | ||
| while (entries.hasMoreElements()) { | ||
| var entry = entries.nextElement(); | ||
| if (!entry.isDirectory()) { | ||
| // Normalize the path to use forward slashes (JAR standard) | ||
| String path = FilenameUtils.normalize(entry.getName(), true); | ||
| byte[] hash = JAR.computeHash(jarFile, entry); | ||
|
|
||
| filesByPath.compute(path, (_, existing) -> { | ||
| var list = existing == null ? new ArrayList<FileInfo>() : existing; | ||
| // Only add if the hash is different | ||
| if (list.stream().noneMatch(info -> Arrays.equals(info.hash, hash))) | ||
| list.add(new FileInfo(path, hash, file)); | ||
| return list; | ||
| }); | ||
| } | ||
| } | ||
| if (!append) jarFile.close(); // Close the jar if we opened it | ||
| }); | ||
|
|
||
| if (!append) { // remove all mappings from the relocation configs as we're not appending | ||
| relocationConfigs.forEach(config -> config.setMappings(new HashMap<>())); | ||
| } | ||
|
|
||
| // Create mappings for conflicts | ||
| filesByPath.forEach((_, fileInfos) -> { | ||
| // Return if there are no conflicts | ||
| if (fileInfos.size() <= 1) return; | ||
|
|
||
| // Create mappings for all relocationConfigs | ||
| fileInfos.forEach(fileInfo -> | ||
| fileInfo.source.mappings.putIfAbsent(fileInfo.path, fileInfo.path.addPrefixExtension(fileInfo.source.conflictPrefix)) | ||
| ); | ||
| }); | ||
| } | ||
| // This will return if there are no conflicts at all (empty mappings) or there are only class and/or META-INF conflicts which we're ignoring | ||
| if (anotherPass && relocationConfigs.stream() | ||
| .flatMap(config -> config.mappings.keySet().stream()) | ||
| .noneMatch(mapping -> !mapping.endsWith(".class") && !mapping.startsWith("META-INF/")) // Checking to see if there's any resource that isn't a class or META-INF, if there isn't then return | ||
| ) return; | ||
|
|
||
| AtomicBoolean doAnotherPass = new AtomicBoolean(false); | ||
|
|
||
| // Process each JAR file in parallel | ||
| relocationConfigs.parallelStream().forEach(relocationConfig -> { | ||
| JarFile jarFile = JAR.isClosed(relocationConfig.jarFile) ? new JarFile(relocationConfig.jarFile.getName()) : relocationConfig.jarFile; // Reopen the JAR file if it's closed (it can be closed by the `relocateClasses` method) | ||
|
|
||
| Set<JarEntry> resources = JAR.getResources(jarFile); | ||
| Map<JarEntry, String> contentMapping = new HashMap<>(); | ||
|
|
There was a problem hiding this comment.
Data races in relocateResources: shared HashMap and AtomicBoolean in parallel streams
There are two concurrency issues in relocateResources:
contentMappingis a plainHashMapwritten fromresources.parallelStream().forEach(...). Concurrentputcalls onHashMapare undefined and can corrupt internal state.doAnotherPass.set(JAR.writeResources(jarFile, contentMapping));is executed inside aparallelStreamoverrelocationConfigs, so multiple threads race to set the sameAtomicBoolean. A laterfalseresult can overwrite an earliertrue, causing the second-pass recursion to be skipped even when at least one jar wrote changes.
Concrete fix:
- AtomicBoolean doAnotherPass = new AtomicBoolean(false);
+ AtomicBoolean doAnotherPass = new AtomicBoolean(false);
relocationConfigs.parallelStream().forEach(relocationConfig -> {
...
- Map<JarEntry, String> contentMapping = new HashMap<>();
+ Map<JarEntry, String> contentMapping = new ConcurrentHashMap<>();
...
- resources.parallelStream().forEach(entry -> {
+ resources.forEach(entry -> { // or use a sequential stream
...
contentMapping.put(entry, content);
});
-
- doAnotherPass.set(JAR.writeResources(jarFile, contentMapping));
+ if (JAR.writeResources(jarFile, contentMapping)) {
+ doAnotherPass.set(true); // once true, never reset to false
+ }This removes the data race on contentMapping and ensures doAnotherPass reflects the OR of all jar results rather than the last one.
Also applies to: 192-199
🤖 Prompt for AI Agents
In src/main/java/io/github/pacifistmc/forgix/core/Relocator.java around lines
125-145 (and similarly at 192-199), the code uses a plain HashMap
(contentMapping) written to from a parallelStream and sets an AtomicBoolean by
overwriting it from multiple threads, causing data races and lost updates;
change contentMapping to a thread-safe map (e.g., ConcurrentHashMap) or collect
results in a thread-local structure and merge, and replace the overwrite of
doAnotherPass with an atomic OR operation (e.g., compute the boolean result from
JAR.writeResources(...) into a local variable then call
doAnotherPass.accumulateAndGet(result, (a,b)->a||b) or
doAnotherPass.updateAndGet(v -> v || result)) so multiple threads cannot corrupt
the map and a single false never wipes out a prior true.
…xinFileHandler.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
This seems more of a workaround than an actual solution. I had an idea for a fix a few months ago |
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/io/github/pacifistmc/forgix/core/filehandlers/MixinFileHandler.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (java-kotlin)
| public String handle(String fileName, String fileContent, Map<String, String> replacementPaths) { | ||
| Gson gson = new GsonBuilder().setPrettyPrinting().create(); | ||
| JsonObject mixinJson = gson.fromJson(fileContent, JsonObject.class); | ||
| String packagePath = mixinJson.get("package").getAsString(); |
There was a problem hiding this comment.
Add null safety check for the "package" field.
The code will throw a NullPointerException if the "package" field is missing or null in the mixin configuration JSON. While this field should always be present in valid mixin configs, defensive coding requires validation.
Apply this diff to add validation:
- String packagePath = mixinJson.get("package").getAsString();
+ JsonElement packageElement = mixinJson.get("package");
+ if (packageElement == null || !packageElement.isJsonPrimitive()) {
+ throw new IllegalArgumentException("Mixin config '" + fileName + "' is missing required 'package' field");
+ }
+ String packagePath = packageElement.getAsString();📝 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.
| String packagePath = mixinJson.get("package").getAsString(); | |
| JsonElement packageElement = mixinJson.get("package"); | |
| if (packageElement == null || !packageElement.isJsonPrimitive()) { | |
| throw new IllegalArgumentException("Mixin config '" + fileName + "' is missing required 'package' field"); | |
| } | |
| String packagePath = packageElement.getAsString(); |
🤖 Prompt for AI Agents
In
src/main/java/io/github/pacifistmc/forgix/core/filehandlers/MixinFileHandler.java
around line 12, the code directly reads mixinJson.get("package").getAsString()
which will throw a NullPointerException if the "package" member is missing or
null; change this to first verify mixinJson.has("package") and that
mixinJson.get("package") is not JsonNull, then read getAsString(); if the field
is missing or null either throw a clear IllegalArgumentException (with the
file/context) or use a sensible default value, and ensure callers handle that
exception accordingly.
| JsonArray commonMixinPaths = mixinJson.getAsJsonArray("mixins"); | ||
| if (commonMixinPaths != null) { | ||
| JsonArray newCommonMixinPaths = new JsonArray(); | ||
| for (JsonElement mixinPath : commonMixinPaths) { | ||
| String fullPath = packagePath + "." + mixinPath.getAsString(); | ||
| if (replacementPaths.containsKey(fullPath)) | ||
| newCommonMixinPaths.add(replacementPaths.get(fullPath).substring(packagePath.length() + 1)); | ||
| else | ||
| newCommonMixinPaths.add(mixinPath); | ||
| } | ||
| mixinJson.add("mixins", newCommonMixinPaths); | ||
| } |
There was a problem hiding this comment.
Fix potential StringIndexOutOfBoundsException and incorrect path handling.
The substring operation at line 20 assumes the replacement path is longer than packagePath.length() + 1 and starts with the package prefix. This will throw StringIndexOutOfBoundsException if the replacement path is shorter, or produce incorrect results if the replacement uses a different package structure.
Example failure case:
packagePath = "com.example.mixin"(18 chars)fullPath = "com.example.mixin.MyMixin"replacement = "org.other.New"(13 chars)substring(19)throws exception
Apply this diff to safely handle replacement paths:
JsonArray newCommonMixinPaths = new JsonArray();
for (JsonElement mixinPath : commonMixinPaths) {
String fullPath = packagePath + "." + mixinPath.getAsString();
- if (replacementPaths.containsKey(fullPath))
- newCommonMixinPaths.add(replacementPaths.get(fullPath).substring(packagePath.length() + 1));
- else
+ if (replacementPaths.containsKey(fullPath)) {
+ String replacement = replacementPaths.get(fullPath);
+ String prefix = packagePath + ".";
+ if (replacement.startsWith(prefix) && replacement.length() > prefix.length()) {
+ newCommonMixinPaths.add(replacement.substring(prefix.length()));
+ } else {
+ // Replacement is in a different package or format; use the class name only
+ int lastDot = replacement.lastIndexOf('.');
+ newCommonMixinPaths.add(lastDot >= 0 ? replacement.substring(lastDot + 1) : replacement);
+ }
+ } else {
newCommonMixinPaths.add(mixinPath);
+ }
}📝 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.
| JsonArray commonMixinPaths = mixinJson.getAsJsonArray("mixins"); | |
| if (commonMixinPaths != null) { | |
| JsonArray newCommonMixinPaths = new JsonArray(); | |
| for (JsonElement mixinPath : commonMixinPaths) { | |
| String fullPath = packagePath + "." + mixinPath.getAsString(); | |
| if (replacementPaths.containsKey(fullPath)) | |
| newCommonMixinPaths.add(replacementPaths.get(fullPath).substring(packagePath.length() + 1)); | |
| else | |
| newCommonMixinPaths.add(mixinPath); | |
| } | |
| mixinJson.add("mixins", newCommonMixinPaths); | |
| } | |
| JsonArray commonMixinPaths = mixinJson.getAsJsonArray("mixins"); | |
| if (commonMixinPaths != null) { | |
| JsonArray newCommonMixinPaths = new JsonArray(); | |
| for (JsonElement mixinPath : commonMixinPaths) { | |
| String fullPath = packagePath + "." + mixinPath.getAsString(); | |
| if (replacementPaths.containsKey(fullPath)) { | |
| String replacement = replacementPaths.get(fullPath); | |
| String prefix = packagePath + "."; | |
| if (replacement.startsWith(prefix) && replacement.length() > prefix.length()) { | |
| newCommonMixinPaths.add(replacement.substring(prefix.length())); | |
| } else { | |
| // Replacement is in a different package or format; use the class name only | |
| int lastDot = replacement.lastIndexOf('.'); | |
| newCommonMixinPaths.add(lastDot >= 0 ? replacement.substring(lastDot + 1) : replacement); | |
| } | |
| } else { | |
| newCommonMixinPaths.add(mixinPath); | |
| } | |
| } | |
| mixinJson.add("mixins", newCommonMixinPaths); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/io/github/pacifistmc/forgix/core/filehandlers/MixinFileHandler.java
around lines 14 to 25, the code assumes replacementPaths.get(fullPath) always
begins with packagePath + "." and is long enough for
substring(packagePath.length()+1), which can cause
StringIndexOutOfBoundsException or incorrect results; fix by retrieving the
replacement into a local variable, then if the replacement
startsWith(packagePath + ".") and its length is greater than
packagePath.length()+1 use replacement.substring(packagePath.length()+1),
otherwise add the replacement value as-is (no substring), preserving the
original mixinPath fallback if replacement is null.
| JsonArray clientMixinPaths = mixinJson.getAsJsonArray("client"); | ||
| if (clientMixinPaths != null) { | ||
| JsonArray newClientMixinPaths = new JsonArray(); | ||
| for (JsonElement mixinPath : clientMixinPaths) { | ||
| String fullPath = packagePath + "." + mixinPath.getAsString(); | ||
| if (replacementPaths.containsKey(fullPath)) | ||
| newClientMixinPaths.add(replacementPaths.get(fullPath).substring(packagePath.length() + 1)); | ||
| else | ||
| newClientMixinPaths.add(mixinPath); | ||
| } | ||
| mixinJson.add("client", newClientMixinPaths); | ||
| } |
There was a problem hiding this comment.
Fix potential StringIndexOutOfBoundsException and incorrect path handling (client mixins).
The same substring issue from lines 14-25 exists here for client mixins. Line 33 will throw StringIndexOutOfBoundsException if the replacement path is shorter than packagePath.length() + 1 or produce incorrect results with different package structures.
Apply this diff to safely handle replacement paths:
JsonArray newClientMixinPaths = new JsonArray();
for (JsonElement mixinPath : clientMixinPaths) {
String fullPath = packagePath + "." + mixinPath.getAsString();
- if (replacementPaths.containsKey(fullPath))
- newClientMixinPaths.add(replacementPaths.get(fullPath).substring(packagePath.length() + 1));
- else
+ if (replacementPaths.containsKey(fullPath)) {
+ String replacement = replacementPaths.get(fullPath);
+ String prefix = packagePath + ".";
+ if (replacement.startsWith(prefix) && replacement.length() > prefix.length()) {
+ newClientMixinPaths.add(replacement.substring(prefix.length()));
+ } else {
+ // Replacement is in a different package or format; use the class name only
+ int lastDot = replacement.lastIndexOf('.');
+ newClientMixinPaths.add(lastDot >= 0 ? replacement.substring(lastDot + 1) : replacement);
+ }
+ } else {
newClientMixinPaths.add(mixinPath);
+ }
}
src/main/java/io/github/pacifistmc/forgix/core/filehandlers/MixinFileHandler.java
Show resolved
Hide resolved
src/main/java/io/github/pacifistmc/forgix/core/filehandlers/MixinFileHandler.java
Show resolved
Hide resolved
|
Its the best solution i can think of. Also you are right, not a fix but a workaround. |
ThePandaOliver
changed the title
ThePandaOliver
force-pushed
the
custom-file-handler
branch
from
fd5e8d8 to
1e6b535
Compare
2 participants
While thinking of a fix for the mixin issue where the mixin class paths in the mixin configs aren't being remapped to their new name, i thought of just implementing a custom file handling API for files that need special handling as a workaround.
It can be used like this (The example is written in Kotlin DSL)
forgix { // Mixin handler preset mixinHandler("**.mixins.json") // Custom file handler fileHandler("**/something.*.xml") { fileName, fileContent, replacementPaths -> return // The modified file content } }Im not sure how to implement this with CLI, but it will probably only be capable of using preset handlers.
Summary by CodeRabbit
New Features
Tests