抽离脚本引擎 + 添加 BTN 的脚本支持 #679
Conversation
WalkthroughThis pull request introduces significant enhancements to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (38)
src/main/java/com/ghostchu/peerbanhelper/module/RunnerAction.java (1)
3-6: LGTM! Consider adding documentation.The enum implementation is clean and follows Java conventions. However, to improve maintainability and API clarity, consider adding documentation:
package com.ghostchu.peerbanhelper.module; +/** + * Represents the outcome of a peer ban evaluation. + * Used by Runner implementations to indicate whether a peer should be banned. + * + * @since 1.0.0 + */ public enum RunnerAction { + /** + * Indicates that the peer matches ban criteria and should be banned. + */ HIT, + /** + * Indicates that the peer does not match ban criteria and should not be banned. + */ MISS }src/main/java/com/ghostchu/peerbanhelper/module/impl/pack/IPAddressPack.java (1)
5-8: Add documentation and consider parameter validation.While the record implementation is clean and appropriate for this use case, consider these improvements:
- Add JavaDoc to document the record's purpose and usage
- Consider adding validation in the canonical constructor to ensure ipMatcher is not null
Here's a suggested implementation:
+/** + * A record that encapsulates an IP address matcher for peer evaluation. + * This class is immutable and thread-safe. + */ public record IPAddressPack ( + /** + * The IP matcher used to evaluate IP addresses. + * Must not be null. + */ IPMatcher ipMatcher ){ + /** + * Creates a new IPAddressPack with the specified IP matcher. + * + * @param ipMatcher the IP matcher to use (must not be null) + * @throws IllegalArgumentException if ipMatcher is null + */ + public IPAddressPack { + if (ipMatcher == null) { + throw new IllegalArgumentException("ipMatcher must not be null"); + } + } }src/main/java/com/ghostchu/peerbanhelper/module/impl/pack/ClientNamePack.java (1)
8-8: Make the list immutable to prevent modification after construction.The
List<Rule>could be modified after the record is created, potentially leading to unexpected behavior. Consider making it immutable.- List<Rule> bannedPeers + List<Rule> bannedPeers +) { + public ClientNamePack(List<Rule> bannedPeers) { + this.bannedPeers = List.copyOf(bannedPeers); // Creates an unmodifiable copy + }src/main/java/com/ghostchu/peerbanhelper/module/Runner.java (2)
8-8: Remove unused importExecutorServiceThe
java.util.concurrent.ExecutorServiceimport is not used in this interface.-import java.util.concurrent.ExecutorService;
10-13: LGTM! Well-designed interface following SOLID principles.The interface is well-designed with:
- Clear single responsibility (determining ban actions)
- Strong null-safety with @NotNull annotations
- Descriptive method name and parameters
Some architectural considerations:
- Consider adding documentation to explain:
- The contract and expected behavior
- The significance of the RunnerAction return value
- Any exceptions that might be thrown
- Since this is part of a script engine integration, consider adding a method to validate scripts at load time to fail fast on configuration errors.
src/main/java/com/ghostchu/peerbanhelper/btn/BtnRule.java (1)
25-26: LGTM! Consider adding documentation for script rules.The new field follows the established pattern of rule storage in the class. However, some documentation would be helpful.
Add Javadoc to describe:
- Expected format and structure of scripts
- Valid keys for the Map
- Example usage
+ /** + * Map of script rules where key is the rule identifier and value is the script content. + * Scripts are used for dynamic rule evaluation. + * Example: + * { + * "checkPeer": "script content here" + * } + */ @SerializedName("script") private Map<String, String> scriptRules;src/main/java/com/ghostchu/peerbanhelper/module/impl/runner/ClientNameRunner.java (3)
17-17: Remove unused import.The
ExecutorServiceimport is not used in this class.-import java.util.concurrent.ExecutorService;
23-26: Consider constructor improvements.
- The explicit
super()call is unnecessary as it's implicitly called.- Consider adding
@Autowiredannotation for better Spring dependency injection clarity.+ @Autowired public ClientNameRunner(RunnerRegistryManager runnerRegistryManager) { - super(); this.runnerRegistryManager = runnerRegistryManager; }
28-36: Consider performance optimizations and monitoring.To improve the system's scalability and observability, consider:
- Caching the rules list with a TTL to reduce registry lookups
- Adding metrics to track:
- Rule match hit/miss ratios
- Rule matching latency
- Number of banned clients by rule
- Rate limiting rule checks for the same peer/client combination
Would you like me to provide implementation examples for any of these suggestions?
src/main/java/com/ghostchu/peerbanhelper/module/impl/runner/PeerIdRunner.java (2)
16-18: Remove unused imports.The following imports are not used in the class:
java.util.LinkedListjava.util.ExecutorService-import java.util.LinkedList; -import java.util.ExecutorService;
29-37: Consider adding defensive programming and logging.The implementation could benefit from the following improvements:
- Add null checks despite @NotNull annotations for defensive programming
- Add error handling for RuleParser.matchRule
- Add logging for ban decisions (especially hits) for audit purposes
- Consider caching rule matching results if performance becomes a concern
Here's a suggested improvement:
@Override public @NotNull RunnerAction shouldBanPeer(@NotNull Torrent torrent, @NotNull Peer peer, @NotNull Downloader downloader) { + Objects.requireNonNull(peer, "peer cannot be null"); + String peerId = peer.getPeerId(); + if (peerId == null) { + log.warn("Peer {} has null peerId, skipping rule check", peer); + return RunnerAction.MISS; + } + List<Rule> ruleList = runnerRegistryManager.get(getClass(), Rule.class); - RuleMatchResult matchResult = RuleParser.matchRule(ruleList, peer.getPeerId()); - if (matchResult.hit()) { - return RunnerAction.HIT; + try { + RuleMatchResult matchResult = RuleParser.matchRule(ruleList, peerId); + if (matchResult.hit()) { + log.info("Peer {} matched ban rule: {}", peer, matchResult.getMatchedRule()); + return RunnerAction.HIT; + } + return RunnerAction.MISS; + } catch (Exception e) { + log.error("Error matching rules for peer {}: {}", peer, e.getMessage(), e); + return RunnerAction.MISS; } - return RunnerAction.MISS; }src/main/java/com/ghostchu/peerbanhelper/module/impl/registry/RunnerRegistryManager.java (5)
14-14: Consider using more specific generic type bounds.The registry map's value type
List<Supplier<?>>is too broad. Consider usingList<Supplier<List<?>>>since all suppliers are expected to returnList<Pack>.- private final Map<Class<? extends Runner>, List<Supplier<?>>> registry = new ConcurrentHashMap<>(); + private final Map<Class<? extends Runner>, List<Supplier<List<?>>>> registry = new ConcurrentHashMap<>();
16-19: Utilize or remove the unused description parameter.The
descriptionparameter is not used in the method. Consider either:
- Using it for logging when registering suppliers
- Removing it if it's not needed
If logging is intended, here's a suggested implementation:
public <Pack> void register(String description, Class<? extends Runner> clazz, Supplier<List<Pack>> supplier) { + log.debug("Registering supplier for runner {}: {}", clazz.getSimpleName(), description); List<Supplier<?>> suppliers = registry.computeIfAbsent(clazz, k -> Collections.synchronizedList(new ArrayList<>())); suppliers.add(supplier); }
21-26: Consider adding logging and return value for unregister operation.The unregister operation could be more informative by:
- Adding debug logging
- Returning a boolean to indicate whether the supplier was actually removed
- public <Pack> void unregister(Class<? extends Runner> clazz, Supplier<List<Pack>> supplier) { + public <Pack> boolean unregister(Class<? extends Runner> clazz, Supplier<List<Pack>> supplier) { List<Supplier<?>> suppliers = registry.get(clazz); if (suppliers != null) { - suppliers.remove(supplier); + boolean removed = suppliers.remove(supplier); + log.debug("Unregistered supplier for runner {}: {}", clazz.getSimpleName(), removed); + return removed; } + return false; }
28-30: Add logging and return value for unregisterAll operation.Similar to the unregister method, consider adding logging and returning the removed suppliers for better observability.
- public void unregisterAll(Class<? extends Runner> clazz) { - registry.remove(clazz); + public List<Supplier<?>> unregisterAll(Class<? extends Runner> clazz) { + List<Supplier<?>> removed = registry.remove(clazz); + log.debug("Unregistered all suppliers for runner {}", clazz.getSimpleName()); + return removed != null ? removed : Collections.emptyList(); }
11-13: Add class-level documentation.Consider adding Javadoc to document:
- The purpose of this registry
- Thread-safety guarantees
- Expected usage patterns
@Component @Slf4j +/** + * A thread-safe registry for managing Runner suppliers. + * This class maintains a mapping between Runner classes and their associated suppliers, + * allowing dynamic registration and retrieval of Runner-specific configurations. + * <p> + * All operations are thread-safe, using ConcurrentHashMap for storage and synchronized lists + * for supplier collections. + */ public class RunnerRegistryManager {src/main/java/com/ghostchu/peerbanhelper/btn/ability/BtnAbilityRules.java (2)
43-46: Consider adding null validation for scriptEngine parameter.While the constructor changes look good, it's recommended to add null validation for the scriptEngine parameter to fail fast if null is passed.
public BtnAbilityRules(BtnNetwork btnNetwork, ScriptEngine scriptEngine, JsonObject ability) { + if (scriptEngine == null) { + throw new IllegalArgumentException("scriptEngine cannot be null"); + } this.btnNetwork = btnNetwork; this.scriptEngine = scriptEngine; this.interval = ability.get("interval").getAsLong();
Line range hint
38-116: Consider documenting the ScriptEngine integration.The addition of ScriptEngine introduces script execution capabilities to this class. Consider:
- Adding class-level documentation explaining the role of ScriptEngine in rule processing
- Documenting any script-related requirements or constraints for maintainers
src/main/java/com/ghostchu/peerbanhelper/btn/BtnNetwork.java (1)
112-114: Consider cleaning up commented code.The commented-out code block for 'submit_hitrate' should either be:
- Removed if it's no longer needed
- Implemented if it's planned functionality
- Documented with a TODO comment explaining why it's commented out
src/main/java/com/ghostchu/peerbanhelper/text/Lang.java (1)
423-423: LGTM! BTN script constants align with requirements.The BTN script-related constants follow the same pattern as other script-related constants and support the new BTN script functionality.
Consider maintaining alphabetical ordering of enum constants for better maintainability. These constants could be moved near other BTN-related constants.
src/main/resources/lang/messages_fallback.yml (1)
482-482: Add a newline at the end of the file.YAML files should end with a newline character to follow standard file formatting conventions.
Apply this diff to fix the issue:
BTN_RULES_SCRIPT_COMPILED: "已成功编译 {} 个脚本，用时 {}ms" +🧰 Tools
🪛 yamllint
[error] 482-482: no new line character at the end of file
(new-line-at-end-of-file)
src/main/resources/lang/en_us/messages.yml (1)
266-267: Consider enhancing the return value documentation.The message clearly lists valid return types but could be more user-friendly.
Consider this improved format:
- Script {} returned an invalid value. Valid return types are: - Boolean: [false=do nothing, true=ban peer] - Integer: [0=do nothing, 1=ban peer, 2=skip other rules] - com.ghostchu.peerbanhelper.module.PeerAction: [NO_ACTION, BAN, SKIP] - com.ghostchu.peerbanhelper.module.BanResult + Script {} returned an invalid value. Please return one of these valid types: + • Boolean: false (do nothing) or true (ban peer) + • Integer: 0 (do nothing), 1 (ban peer), or 2 (skip other rules) + • PeerAction enum: NO_ACTION, BAN, or SKIP + • BanResult objectsrc/main/java/com/ghostchu/peerbanhelper/module/impl/runner/IPAddressRunner.java (1)
51-54: Simplify return statement using a ternary operatorYou can simplify the return statement for better readability by using a ternary operator.
Apply this diff:
- if (mr) { - return RunnerAction.HIT; - } - return RunnerAction.MISS; + return mr ? RunnerAction.HIT : RunnerAction.MISS;src/main/java/com/ghostchu/peerbanhelper/btn/BtnRuleParsed.java (4)
36-44: Script compilation in constructor may affect performanceCompiling scripts within the constructor can lead to increased instantiation time, affecting performance, especially if there are many scripts to compile. Consider deferring the compilation to a separate initialization method or performing it asynchronously to improve efficiency.
47-47: Specify initial capacity forHashMapto optimize memory usageSince the number of scripts is known (
scriptRules.size()), initializing theHashMapwith an initial capacity can improve performance by reducing the need for resizing.Apply this diff to set the initial capacity:
- Map<String, CompiledScript> scripts = new HashMap<>(); + Map<String, CompiledScript> scripts = new HashMap<>(scriptRules.size());
57-57: Provide more context in error loggingWhen logging the exception for a failed script compilation, consider including additional information such as the script content or a snippet (if not sensitive) to aid in debugging.
60-60: Consider usingSystem.nanoTime()for more precise timingUsing
System.nanoTime()can provide more precise timing measurements compared toSystem.currentTimeMillis(), which is suitable for profiling the script compilation duration.Apply this diff to use
System.nanoTime():- long startAt = System.currentTimeMillis(); + long startAt = System.nanoTime();And update the log statement accordingly:
- log.info(tlUI(Lang.BTN_RULES_SCRIPT_COMPILED, scripts.size(), System.currentTimeMillis() - startAt)); + log.info(tlUI(Lang.BTN_RULES_SCRIPT_COMPILED, scripts.size(), (System.nanoTime() - startAt) / 1_000_000));src/main/java/com/ghostchu/peerbanhelper/scriptengine/ScriptEngine.java (2)
26-57: Remove unused commented-out code to improve maintainabilityThe block of commented-out code from lines 26 to 57 appears to be obsolete. Removing this code will enhance readability and reduce clutter in the codebase.
153-153: Include exception details in log message for better debuggingAt line 153, the exception
eis caught but not included in the log message. Including the exception in the log will provide stack trace information, aiding in troubleshooting.Apply this diff to include the exception:
- log.warn("Script Engine unable to compile the script: {}", fallbackName); + log.warn("Script Engine unable to compile the script: {}", fallbackName, e);src/main/java/com/ghostchu/peerbanhelper/module/impl/rule/ExpressionRule.java (4)
54-54: Consider using a concurrent collection instead ofCollections.synchronizedListUsing
Collections.synchronizedListcan be error-prone as it requires manual synchronization when iterating over the list. To improve thread safety and reduce the risk of concurrent modification issues, consider using a concurrent collection such asCopyOnWriteArrayList.Apply this change to use a thread-safe collection:
-private final List<CompiledScript> scripts = Collections.synchronizedList(new LinkedList<>()); +private final List<CompiledScript> scripts = new CopyOnWriteArrayList<>();
Line range hint
194-201: Synchronize when iterating overscriptsto ensure thread safetyIteration over a
Collections.synchronizedListis not thread-safe unless you manually synchronize on the list. To preventConcurrentModificationExceptionand ensure thread safety, wrap the iteration with a synchronized block.Apply this change to synchronize the iteration:
-public @NotNull CheckResult shouldBanPeer(@NotNull Torrent torrent, @NotNull Peer peer, @NotNull Downloader downloader, @NotNull ExecutorService ruleExecuteExecutor) { +public @NotNull CheckResult shouldBanPeer(@NotNull Torrent torrent, @NotNull Peer peer, @NotNull Downloader downloader, @NotNull ExecutorService ruleExecuteExecutor) { + synchronized (scripts) { AtomicReference<CheckResult> checkResult = new AtomicReference<>(pass()); try (ExecutorService exec = Executors.newVirtualThreadPerTaskExecutor()) { for (var compiledScript : scripts) { exec.submit(() -> { CheckResult expressionRun = runExpression(compiledScript, torrent, peer, downloader, ruleExecuteExecutor); if (expressionRun.action() == PeerAction.SKIP) { checkResult.set(expressionRun); // Early exit return; } if (expressionRun.action() == PeerAction.BAN) { if (checkResult.get().action() != PeerAction.SKIP) { checkResult.set(expressionRun); } } }); } } return checkResult.get(); + } }
Line range hint
258-277: Potential concurrent modification ofscriptswithout proper synchronizationThe
scriptslist is being cleared and modified inreloadConfig()which may run concurrently with other threads accessing or iterating overscripts. This can lead toConcurrentModificationExceptionor inconsistent behavior. Consider synchronizing modifications toscriptsor using a thread-safe collection designed for concurrent access.Apply this change to ensure thread-safe modification:
private void reloadConfig() throws IOException { + synchronized (scripts) { scripts.clear(); this.banDuration = getConfig().getLong("ban-duration", 0); initScripts(); log.info(tlUI(Lang.RULE_ENGINE_COMPILING)); long start = System.currentTimeMillis(); try (var executor = Executors.newVirtualThreadPerTaskExecutor()) { File scriptDir = new File(Main.getDataDirectory(), "scripts"); File[] scripts = scriptDir.listFiles(); if (scripts != null) { for (File script : scripts) { executor.submit(() -> { try { if (!script.getName().endsWith(".av") || script.isHidden()) { return; } try { String scriptContent = java.nio.file.Files.readString(script.toPath(), StandardCharsets.UTF_8); var compiledScript = scriptEngine.compileScript(script, script.getName(), scriptContent); if (compiledScript == null) return; this.scripts.add(compiledScript); } catch (IOException e) { log.error("Unable to load script file", e); } } catch (ExpressionSyntaxErrorException err) { log.error(tlUI(Lang.RULE_ENGINE_BAD_EXPRESSION), err); } }); } } } getCache().invalidateAll(); log.info(tlUI(Lang.RULE_ENGINE_COMPILED, scripts.size(), System.currentTimeMillis() - start)); + } }
Line range hint
261-289: Asynchronous script compilation may lead to incomplete initializationIn
reloadConfig(), scripts are compiled asynchronously using anExecutorService, but there is no mechanism to wait for these tasks to complete before proceeding. This can result inscriptsnot being fully populated when used elsewhere in the application. To ensure that all scripts are compiled before they are needed, consider usingexecutor.shutdown()andexecutor.awaitTermination()to wait for all tasks to finish.Apply this change to wait for task completion:
log.info(tlUI(Lang.RULE_ENGINE_COMPILING)); long start = System.currentTimeMillis(); try (var executor = Executors.newVirtualThreadPerTaskExecutor()) { // ... existing code to submit tasks ... + executor.shutdown(); + try { + if (!executor.awaitTermination(maxScriptExecuteTime, TimeUnit.MILLISECONDS)) { + log.warn("Script compilation did not finish within the expected time."); + } + } catch (InterruptedException e) { + log.error("Script compilation was interrupted.", e); + } } getCache().invalidateAll(); log.info(tlUI(Lang.RULE_ENGINE_COMPILED, scripts.size(), System.currentTimeMillis() - start));src/main/java/com/ghostchu/peerbanhelper/module/impl/rule/BtnNetworkOnline.java (2)
221-223: Avoid unnecessary synchronization onscript.expression()Synchronizing on
script.expression()may introduce a performance bottleneck if multiple scripts are executed concurrently, as it serializes the execution of scripts. Ifscript.expression().execute(env)is thread-safe, consider removing the synchronized block to improve concurrency.If thread safety is a concern, ensure that each script instance is thread-safe or that appropriate synchronization is applied without impacting overall performance.
225-230: Handle exceptions explicitly and log meaningful messagesCurrently, when a
TimeoutExceptionor a generalExceptionoccurs, the method returnspass()without notifying the caller. While the error is logged, the calling method might benefit from knowing that an exception occurred during script execution.Consider propagating exceptions or returning a specific
CheckResultindicating an error state. This can help in troubleshooting and provide more transparency to the system's behavior. Additionally, ensure that logged messages include sufficient context to aid in debugging.src/main/java/com/ghostchu/peerbanhelper/PeerBanHelperServer.java (3)
215-216: Remove commented-out code to improve cleanliness.The commented-out code at lines 215-216 might reduce code clarity and maintainability. Consider removing it if it's no longer needed.
Apply this diff to remove the commented-out code:
- // // 表达式允许序列化和反序列化 - // AviatorEvaluator.getInstance().setOption(Options.SERIALIZABLE, true);
207-230: Ensure consistency in comment language for better collaboration.The comments within the
setupScriptEngine()method are in Chinese. For better maintainability and collaboration, consider using English for code comments to ensure all team members can understand them.Apply this diff to translate the comments to English:
- // ASM 性能优先 + // Prioritize performance with ASM - // EVAL 性能优先 + // Prioritize performance with EVAL - // 降低浮点计算精度 + // Reduce floating-point calculation precision - // 启用变量语法糖 + // Enable property syntax sugar - // // 表达式允许序列化和反序列化 + // Allow expression serialization and deserialization - // 用户规则写糊保护 + // Protect against infinite loops in user rules - // 启用反射方法查找 + // Enable reflection method lookup - // 注册反射调用 + // Register reflection calls
38-38: Review package placement forInfoHashUtil.The
InfoHashUtilclass is imported from theutil.timepackage. IfInfoHashUtilis not related to time utilities, consider moving it to a more appropriate package to improve code organization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (24)
src/main/java/com/ghostchu/peerbanhelper/PeerBanHelperServer.java(6 hunks)src/main/java/com/ghostchu/peerbanhelper/btn/BtnConfig.java(3 hunks)src/main/java/com/ghostchu/peerbanhelper/btn/BtnNetwork.java(4 hunks)src/main/java/com/ghostchu/peerbanhelper/btn/BtnRule.java(1 hunks)src/main/java/com/ghostchu/peerbanhelper/btn/BtnRuleParsed.java(3 hunks)src/main/java/com/ghostchu/peerbanhelper/btn/ability/BtnAbilityRules.java(4 hunks)src/main/java/com/ghostchu/peerbanhelper/module/AbstractRunner.java(1 hunks)src/main/java/com/ghostchu/peerbanhelper/module/RuleFeatureModule.java(1 hunks)src/main/java/com/ghostchu/peerbanhelper/module/Runner.java(1 hunks)src/main/java/com/ghostchu/peerbanhelper/module/RunnerAction.java(1 hunks)src/main/java/com/ghostchu/peerbanhelper/module/impl/pack/ClientNamePack.java(1 hunks)src/main/java/com/ghostchu/peerbanhelper/module/impl/pack/IPAddressPack.java(1 hunks)src/main/java/com/ghostchu/peerbanhelper/module/impl/pack/PeerIdPack.java(1 hunks)src/main/java/com/ghostchu/peerbanhelper/module/impl/registry/RunnerRegistryManager.java(1 hunks)src/main/java/com/ghostchu/peerbanhelper/module/impl/rule/BtnNetworkOnline.java(5 hunks)src/main/java/com/ghostchu/peerbanhelper/module/impl/rule/ExpressionRule.java(8 hunks)src/main/java/com/ghostchu/peerbanhelper/module/impl/runner/ClientNameRunner.java(1 hunks)src/main/java/com/ghostchu/peerbanhelper/module/impl/runner/IPAddressRunner.java(1 hunks)src/main/java/com/ghostchu/peerbanhelper/module/impl/runner/PeerIdRunner.java(1 hunks)src/main/java/com/ghostchu/peerbanhelper/scriptengine/CompiledScript.java(1 hunks)src/main/java/com/ghostchu/peerbanhelper/scriptengine/ScriptEngine.java(1 hunks)src/main/java/com/ghostchu/peerbanhelper/text/Lang.java(2 hunks)src/main/resources/lang/en_us/messages.yml(3 hunks)src/main/resources/lang/messages_fallback.yml(4 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/main/java/com/ghostchu/peerbanhelper/module/AbstractRunner.java
- src/main/java/com/ghostchu/peerbanhelper/scriptengine/CompiledScript.java
🧰 Additional context used
🪛 yamllint
src/main/resources/lang/messages_fallback.yml
[error] 482-482: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (28)
src/main/java/com/ghostchu/peerbanhelper/module/impl/pack/IPAddressPack.java (1)
1-4: LGTM! Clean package structure and imports.
The package structure follows Java conventions, and the import is specific to the required functionality.
src/main/java/com/ghostchu/peerbanhelper/module/impl/pack/PeerIdPack.java (2)
1-6: LGTM! Clean package structure and imports.
The package structure follows Java conventions, and imports are minimal and specific to the requirements.
3-3: Verify Rule class compatibility and usage patterns.
Let's verify the Rule class implementation and how this record is being used in the codebase.
Also applies to: 8-8
✅ Verification successful
Let me gather more information about the Rule class and its usage pattern.
Let me check the Rule class implementation specifically.
Let me check one final thing about the Rule class itself.
Based on the gathered information, I can now provide the final response:
Import of Rule interface is compatible with the codebase
The Rule interface is a well-defined part of the codebase's rule system, used across various pack implementations including PeerIdPack, ClientNamePack, and IPAddressPack. The import is correctly referencing com.ghostchu.peerbanhelper.util.rule.Rule, which is a valid interface defining the contract for rule matching functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Rule class implementation and PeerIdPack usage
# Check Rule class implementation
echo "Checking Rule class implementation:"
ast-grep --pattern 'class Rule {
$$$
}'
# Check how PeerIdPack is being used
echo -e "\nChecking PeerIdPack usage:"
rg -l "PeerIdPack" --type java
# Check for similar pack implementations
echo -e "\nChecking for similar pack implementations:"
fd -e java "Pack.java$"
Length of output: 654
Script:
#!/bin/bash
# Check Rule class content and its usage
echo "Checking Rule class content:"
rg -A 10 "class Rule" --type java
echo -e "\nChecking Rule usage in Pack files:"
for file in $(fd -e java "Pack.java$"); do
echo -e "\nFile: $file"
rg "Rule" "$file" -A 3
done
echo -e "\nChecking Rule class imports:"
rg "import.*Rule" --type java
Length of output: 21166
Script:
#!/bin/bash
# Check Rule class implementation
echo "Checking Rule class implementation:"
rg -p "class Rule" --type java -B 2 -A 10
# Check Rule usage in PeerIdPack
echo -e "\nChecking PeerIdPack implementation:"
cat src/main/java/com/ghostchu/peerbanhelper/module/impl/pack/PeerIdPack.java
Length of output: 5403
Script:
#!/bin/bash
# Check Rule class definition
echo "Checking Rule class definition:"
rg -p "class Rule" --type java
rg -p "interface Rule" --type java
# Check Rule.java file if it exists
echo -e "\nChecking Rule.java content:"
fd -e java "Rule.java$" --exec cat {}
Length of output: 19238
src/main/java/com/ghostchu/peerbanhelper/btn/BtnRule.java (1)
25-26: Consider adding script content validation.
Since this field will store executable scripts, consider implementing validation mechanisms.
Let's check if there are any script validation mechanisms in place:
✅ Verification successful
Let's examine the content of the ScriptEngine.java file to understand if there are any validation mechanisms in place.
Let's check the CompiledScript class to fully understand the validation process.
Script validation is already implemented
The codebase already has script validation mechanisms in place:
- Scripts are validated using
AviatorEvaluator.getInstance().validate(scriptContent)before compilation - Scripts are compiled using a dedicated script engine (Aviator)
- Scripts include metadata validation for properties like @name, @author, @Cacheable, @Version, @threadsafe
- Return value validation is implemented in
handleResultmethod to ensure proper action types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for script validation or sanitization
rg -l "script.*valid|valid.*script|sanitize.*script|script.*sanitize"
Length of output: 143
Script:
#!/bin/bash
# Check the content of ScriptEngine.java
cat src/main/java/com/ghostchu/peerbanhelper/scriptengine/ScriptEngine.java
Length of output: 7142
Script:
#!/bin/bash
# Check the content of CompiledScript.java
fd -e java CompiledScript.java -x cat {}
Length of output: 354
src/main/java/com/ghostchu/peerbanhelper/module/RuleFeatureModule.java (1)
8-8: Verify if List import is needed
The java.util.List import appears to be unused in this interface. If it's not needed for the current implementation, consider removing it to keep the imports clean.
src/main/java/com/ghostchu/peerbanhelper/module/impl/runner/PeerIdRunner.java (2)
20-22: LGTM! Well-structured class declaration.
The class follows good practices with proper Spring annotation, immutable field, and clear inheritance hierarchy.
24-27: Verify AbstractRunner's constructor requirements.
The constructor calls super() without parameters. Let's verify that AbstractRunner's no-args constructor is intended to be used.
✅ Verification successful
Let me gather more information about the AbstractRunner class to verify its constructor requirements.
No constructor requirements in AbstractRunner - super() call is appropriate
The AbstractRunner class is a simple abstract class that implements Runner interface and has no explicit constructor. In this case, Java implicitly provides a no-args constructor, making the super() call in PeerIdRunner completely appropriate and correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check AbstractRunner's constructor to ensure no-args super() call is appropriate
# Search for AbstractRunner class definition and constructor
ast-grep --pattern 'class AbstractRunner {
$$$
}'
# Backup: Use ripgrep if ast-grep doesn't find it
rg -A 10 "class AbstractRunner"
Length of output: 282
Script:
#!/bin/bash
# Get the full content of AbstractRunner.java
cat src/main/java/com/ghostchu/peerbanhelper/module/AbstractRunner.java
Length of output: 176
src/main/java/com/ghostchu/peerbanhelper/btn/BtnConfig.java (3)
4-4: LGTM: Clean import addition
The ScriptEngine import is correctly placed and necessary for the new functionality.
26-27: LGTM: Proper dependency injection
The ScriptEngine field is correctly annotated with @Autowired and follows Spring's dependency injection pattern.
37-37: Verify BtnNetwork constructor parameter order
The constructor call looks correct, but since this is a significant change in the constructor signature, let's verify the parameter order matches the BtnNetwork class definition.
✅ Verification successful
Constructor parameter order is correct
The constructor call in BtnConfig.java perfectly matches the parameter order defined in BtnNetwork.java:
// Constructor definition
public BtnNetwork(PeerBanHelperServer server, ScriptEngine scriptEngine, String userAgent, String configUrl, boolean submit, String appId, String appSecret, ModuleMatchCache moduleMatchCache)
// Constructor call
new BtnNetwork(server, scriptEngine, userAgent, configUrl, submit, appId, appSecret, matchCache)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the BtnNetwork constructor parameter order matches the class definition
# Search for the BtnNetwork class constructor definition
ast-grep --pattern 'class BtnNetwork {
$$$
BtnNetwork($$$) {
$$$
}
$$$
}'
Length of output: 87
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find the BtnNetwork constructor
rg "BtnNetwork\(" -A 10 --type java
Length of output: 5434
src/main/java/com/ghostchu/peerbanhelper/btn/ability/BtnAbilityRules.java (3)
8-8: LGTM: Import added for ScriptEngine integration.
The ScriptEngine import is correctly placed and necessary for the new functionality.
38-38: LGTM: ScriptEngine field properly declared.
The field is correctly declared as private final and will be initialized through constructor injection.
61-61: LGTM: Consistent updates to BtnRuleParsed instantiation.
The scriptEngine parameter is consistently passed to BtnRuleParsed in both the cache loading and rule updating code paths.
Also applies to: 116-116
src/main/java/com/ghostchu/peerbanhelper/btn/BtnNetwork.java (4)
6-6: LGTM: Import statement correctly added for ScriptEngine.
The new import is properly placed and follows Java package naming conventions.
39-39: LGTM: ScriptEngine field properly declared.
The field is correctly declared as private and final, following good encapsulation practices.
57-59: LGTM: Constructor properly updated with ScriptEngine parameter.
The constructor changes are well-structured:
- Logical parameter ordering with core dependencies first
- Proper field assignment
- Maintains existing functionality
115-115: Verify BtnAbilityRules implementation with ScriptEngine.
The addition of scriptEngine parameter to BtnAbilityRules instantiation looks correct. However, since this is a significant architectural change:
Let's verify the BtnAbilityRules implementation:
✅ Verification successful
Let me gather more information about BtnAbilityRules specifically.
Let me check the BtnRuleParsed implementation to ensure proper integration.
ScriptEngine integration is properly implemented
The verification shows that:
- BtnAbilityRules correctly stores and initializes the ScriptEngine parameter
- It properly passes the ScriptEngine to BtnRuleParsed instances
- The integration chain (BtnNetwork → BtnAbilityRules → BtnRuleParsed) maintains consistent ScriptEngine usage
- Error handling is in place across the implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check BtnAbilityRules implementation to ensure it properly handles the ScriptEngine parameter
# Search for BtnAbilityRules class definition and constructor
ast-grep --pattern 'class BtnAbilityRules {
$$$
BtnAbilityRules($$$) {
$$$
}
$$$
}'
# Search for any script-related method calls within BtnAbilityRules
rg -A 5 'scriptEngine\.' --type java
Length of output: 3272
Script:
#!/bin/bash
# Search for BtnAbilityRules class implementation
rg -U "class BtnAbilityRules.*?\{.*?\}" --multiline --type java -A 50
# Search for any references to scriptEngine within BtnAbilityRules
rg "scriptEngine" --type java --glob "BtnAbilityRules.java"
Length of output: 839
Script:
#!/bin/bash
# Search for BtnRuleParsed class implementation
rg -U "class BtnRuleParsed.*?\{.*?\}" --multiline --type java -A 50
# Search for ScriptEngine usage in BtnRuleParsed
rg "ScriptEngine" --type java --glob "BtnRuleParsed.java"
Length of output: 494
src/main/java/com/ghostchu/peerbanhelper/text/Lang.java (1)
203-209: LGTM! Rule engine constants are well-structured.
The new rule engine constants follow a consistent naming pattern and appropriately replace the old MODULE_EXPRESSION_RULE_* constants.
src/main/resources/lang/messages_fallback.yml (2)
204-209: LGTM: Script engine messages have been properly renamed and organized.
The renaming from MODULE_EXPRESSION_RULE_* to RULE_ENGINE_* improves clarity and consistency. The new messages provide clear feedback for script compilation, execution, and error states.
Also applies to: 266-267, 311-312
480-482: LGTM: BTN script compilation messages added.
The new messages provide appropriate user feedback during BTN script compilation process.
🧰 Tools
🪛 yamllint
[error] 482-482: no new line character at the end of file
(new-line-at-end-of-file)
src/main/resources/lang/en_us/messages.yml (2)
204-209: LGTM! Clear and informative rule engine messages.
The messages provide good feedback about script compilation, execution, and error states. They cover essential scenarios like syntax errors, compilation progress, timeouts, and runtime errors.
311-312: LGTM! Concise script execution feedback messages.
The messages provide clear feedback about script execution and results.
src/main/java/com/ghostchu/peerbanhelper/btn/BtnRuleParsed.java (3)
15-15: Logging enabled with @Slf4j annotation
The addition of the @Slf4j annotation allows for efficient logging within the class, which is beneficial for monitoring and debugging.
26-26: ScriptEngine integration into BtnRuleParsed
Integrating the ScriptEngine via the scriptEngine field enhances the class's capability to compile and manage scripts dynamically.
52-56: Ensure compileScript handles null context appropriately
Passing null as the first argument to compileScript may lead to unexpected behavior if the method does not handle null values properly. Verify that the compileScript method is designed to accept a null context.
src/main/java/com/ghostchu/peerbanhelper/module/impl/rule/BtnNetworkOnline.java (2)
189-204: Verify the use of Executors.newVirtualThreadPerTaskExecutor()
The use of Executors.newVirtualThreadPerTaskExecutor() introduces virtual threads, which require a Java version that supports Project Loom (Java 19 or later). Ensure that the application's runtime environment is compatible with this feature.
Please confirm that the deployment environment supports virtual threads and that the use of this executor aligns with the project's requirements.
64-65: Confirm the successful injection of ScriptEngine
The scriptEngine is being autowired, but there is no null-check before its usage. If the ScriptEngine bean is not properly configured, this could lead to a NullPointerException.
Ensure that ScriptEngine is correctly defined and initialized in the application context. If there is a possibility of it being null, consider adding a null-check or using @Autowired(required = true) to enforce its presence.
src/main/java/com/ghostchu/peerbanhelper/PeerBanHelperServer.java (1)
140-141:
Initialize scriptEngine field to prevent potential NullPointerException.
The scriptEngine field is declared but never initialized. This could lead to a NullPointerException if accessed before being set. Ensure that scriptEngine is properly initialized, perhaps in the setupScriptEngine() method.
Run the following script to verify that scriptEngine is initialized and used appropriately:
This script will help you identify whether scriptEngine is being properly initialized and how it's used across the codebase.
src/main/java/com/ghostchu/peerbanhelper/module/impl/pack/PeerIdPack.java
Outdated
Show resolved
Hide resolved
src/main/java/com/ghostchu/peerbanhelper/module/impl/pack/ClientNamePack.java
Outdated
Show resolved
Hide resolved
src/main/java/com/ghostchu/peerbanhelper/module/impl/runner/ClientNameRunner.java
Outdated
Show resolved
Hide resolved
src/main/java/com/ghostchu/peerbanhelper/module/impl/registry/RunnerRegistryManager.java
Outdated
Show resolved
Hide resolved
src/main/java/com/ghostchu/peerbanhelper/module/impl/runner/IPAddressRunner.java
Outdated
Show resolved
Hide resolved
src/main/java/com/ghostchu/peerbanhelper/scriptengine/ScriptEngine.java
Outdated
Show resolved
Hide resolved
src/main/java/com/ghostchu/peerbanhelper/scriptengine/ScriptEngine.java
Outdated
Show resolved
Hide resolved
src/main/java/com/ghostchu/peerbanhelper/module/impl/rule/BtnNetworkOnline.java
Outdated
Show resolved
Hide resolved
…gine.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…ptengine-refactor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/main/java/com/ghostchu/peerbanhelper/scriptengine/ScriptEngine.java (2)
113-156: Add additional safety checks to script compilationWhile the method implements previous review suggestions, there are still some potential issues:
- No validation of metadata values (empty strings, malformed values)
- Potential infinite loop with empty files
- Missing maximum file size check
Consider these improvements:
@Nullable public CompiledScript compileScript(File file, String fallbackName, String scriptContent) { + if (scriptContent == null || scriptContent.isEmpty()) { + log.warn("Empty script content for: {}", fallbackName); + return null; + } + if (scriptContent.length() > MAX_SCRIPT_SIZE) { + log.warn("Script too large: {} ({} bytes)", fallbackName, scriptContent.length()); + return null; + } try (BufferedReader reader = new BufferedReader(new StringReader(scriptContent))) { String name = fallbackName; String author = "Unknown"; String version = "null"; boolean cacheable = true; boolean threadSafe = true; + int lineCount = 0; while (true) { + if (lineCount++ > MAX_HEADER_LINES) { + log.warn("Too many header lines in script: {}", fallbackName); + break; + } String line = reader.readLine(); if (line == null) { break; } if (line.startsWith("#")) { line = line.substring(1).trim(); + String value; if (line.startsWith("@NAME")) { - name = line.substring(5).trim(); + value = line.substring(5).trim(); + name = value.isEmpty() ? fallbackName : value; } else if (line.startsWith("@AUTHOR")) { - author = line.substring(7).trim(); + value = line.substring(7).trim(); + author = value.isEmpty() ? "Unknown" : value;
152-154: Improve error logging for script compilation failuresThe current error logging doesn't include the exception details, which could be valuable for debugging script issues.
- log.warn("Script Engine unable to compile the script: {}", fallbackName); + log.warn("Script Engine unable to compile the script: {}", fallbackName, e);src/main/java/com/ghostchu/peerbanhelper/module/impl/rule/BtnNetworkOnline.java (1)
177-213: Add method documentation and improve error handling.Consider adding JavaDoc to explain the method's purpose and behavior. Also, the error handling could be enhanced:
- The current error handling silently logs errors and continues. Consider if some errors should propagate up.
- The thread interruption should be re-interrupted to maintain the interrupt status.
+ /** + * Evaluates BTN script rules for the given peer. + * + * @param torrent The torrent being checked + * @param peer The peer to evaluate + * @param downloader The downloader instance + * @param ruleExecuteExecutor The executor service + * @return CheckResult indicating the evaluation result + */ private @NotNull CheckResult checkScript(Torrent torrent, Peer peer, Downloader downloader, ExecutorService ruleExecuteExecutor) { // ... existing code ... } catch (InterruptedException | ExecutionException e) { - log.error("Error executing script, skipping", e); + log.error("Error executing script", e); + if (e instanceof InterruptedException) { + Thread.currentThread().interrupt(); + } } // ... existing code ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/main/java/com/ghostchu/peerbanhelper/module/impl/rule/BtnNetworkOnline.java(5 hunks)src/main/java/com/ghostchu/peerbanhelper/scriptengine/ScriptEngine.java(1 hunks)
🔇 Additional comments (4)
src/main/java/com/ghostchu/peerbanhelper/scriptengine/ScriptEngine.java (1)
25-57: Consider implementing or removing the commented runExpression method
The commented-out runExpression method appears to be important functionality for executing scripts with proper thread safety and timeout handling. If this is planned functionality, it should be implemented. If it's no longer needed, it should be removed to maintain code clarity.
Let's check if this method is referenced elsewhere:
Would you like assistance in implementing the runExpression method with proper thread safety and timeout handling?
src/main/java/com/ghostchu/peerbanhelper/module/impl/rule/BtnNetworkOnline.java (3)
15-16: LGTM: Script engine integration looks good.
The imports and dependency injection for the script engine are properly configured.
Also applies to: 66-67
170-173: LGTM: Proper integration of script checks.
The script check is correctly placed in the validation chain, with appropriate early return behavior.
215-243: Consider performance optimization and add documentation.
- The synchronization on
script.expression()could become a bottleneck under high load. Consider using a more granular locking strategy or evaluating if synchronization is necessary. - Add JavaDoc to document the method's behavior, especially regarding caching and error handling.
+ /**
+ * Executes a compiled script with the given context.
+ * Results are cached based on the script's hash code and peer's cache key.
+ *
+ * @param script The compiled script to execute
+ * @param torrent The torrent context
+ * @param peer The peer being evaluated
+ * @param downloader The downloader instance
+ * @param ruleExecuteExecutor The executor service
+ * @return CheckResult indicating the script execution result
+ */
public @NotNull CheckResult runExpression(CompiledScript script, @NotNull Torrent torrent, @NotNull Peer peer, @NotNull Downloader downloader, @NotNull ExecutorService ruleExecuteExecutor) {
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores