-
-
Notifications
You must be signed in to change notification settings - Fork 20
GH-874 Expand Random Teleport configuration. #874
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5a71df8
fc68d41
cbb9d2e
7d66781
a7d1c16
a270f2b
5e17616
5914aaf
f13cfdf
6ea96f7
2b271e8
6c78d2a
8d9a7a9
2b40148
49d94c3
3be765c
bd81e1a
2c8f248
f190e79
ef9752c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,18 +5,24 @@ | |||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| import java.time.Duration; | ||||||||||||||||||||||||||||||||||
| import java.time.Instant; | ||||||||||||||||||||||||||||||||||
| import java.util.function.Supplier; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| public class Delay<T> { | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| private final Cache<T, Instant> delays; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| private final DelaySettings delaySettings; | ||||||||||||||||||||||||||||||||||
| private final Supplier<Duration> delaySettings; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| @Deprecated | ||||||||||||||||||||||||||||||||||
| public Delay(DelaySettings delaySettings) { | ||||||||||||||||||||||||||||||||||
| this.delaySettings = delaySettings; | ||||||||||||||||||||||||||||||||||
| this((Supplier<Duration>) () -> delaySettings.delay()); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| public Delay(Supplier<Duration> delayProvider) { | ||||||||||||||||||||||||||||||||||
| this.delaySettings = delayProvider; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| this.delays = CacheBuilder.newBuilder() | ||||||||||||||||||||||||||||||||||
| .expireAfterWrite(delaySettings.delay()) | ||||||||||||||||||||||||||||||||||
| .expireAfterWrite(delayProvider.get()) | ||||||||||||||||||||||||||||||||||
| .build(); | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+21
to
26
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add null checks for the delay provider The constructor should validate the supplier and its provided duration to prevent runtime errors. Apply these changes: public Delay(Supplier<Duration> delayProvider) {
+ Objects.requireNonNull(delayProvider, "delayProvider cannot be null");
this.delaySettings = delayProvider;
+ Duration initialDelay = Objects.requireNonNull(delayProvider.get(),
+ "delay duration cannot be null");
this.delays = CacheBuilder.newBuilder()
- .expireAfterWrite(delayProvider.get())
+ .expireAfterWrite(initialDelay)
.build();
}📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
@@ -25,7 +31,7 @@ public void markDelay(T key, Duration delay) { | |||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| public void markDelay(T key) { | ||||||||||||||||||||||||||||||||||
| this.markDelay(key, this.delaySettings.delay()); | ||||||||||||||||||||||||||||||||||
| this.markDelay(key, this.delaySettings.get()); | ||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification 🛠️ Refactor suggestion Null check is needed, but duration consistency is by design The null check suggestion is valid, but the concern about duration consistency is incorrect. The cache expiration is set only once during construction ( Here's the recommended implementation: public void markDelay(T key) {
- this.markDelay(key, this.delaySettings.get());
+ Duration delay = Objects.requireNonNull(this.delaySettings.get(),
+ "delay duration cannot be null");
+ this.markDelay(key, delay);
}🔗 Analysis chainAdd null check and consider duration consistency The method should validate the duration from the supplier. Also, there's a potential inconsistency if the supplier returns different durations over time, as the cache expiration might not match the marked delay. Consider this implementation: public void markDelay(T key) {
- this.markDelay(key, this.delaySettings.get());
+ Duration delay = Objects.requireNonNull(this.delaySettings.get(),
+ "delay duration cannot be null");
+ this.markDelay(key, delay);
}Let's check if the supplier is used consistently across the codebase: 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for potential inconsistent usage of the delay supplier
ast-grep --pattern 'delaySettings.get()'
Length of output: 42 Script: #!/bin/bash
# Let's try a different approach to find usages of delaySettings
rg -A 2 "delaySettings"
# Also search for the Delay class implementation
fd "Delay.java" --exec cat {}
Length of output: 2709 |
||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| public void unmarkDelay(T key) { | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
| import java.time.Duration; | ||
|
|
||
| @Deprecated | ||
| public interface DelaySettings { | ||
|
|
||
| Duration delay(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| package com.eternalcode.core.feature.randomteleport; | ||
|
|
||
| public interface RandomTeleportRadius { | ||
| int maxZ(); | ||
| int minZ(); | ||
| int maxX(); | ||
| int minX(); | ||
|
|
||
| static RandomTeleportRadius of(int minX, int maxX, int minZ, int maxZ) { | ||
| return new SimpleRandomTeleportRadius(minX, maxX, minZ, maxZ); | ||
| } | ||
vLuckyyy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| static RandomTeleportRadius of(int radius) { | ||
| return new SimpleRandomTeleportRadius(-radius, radius, -radius, radius); | ||
| } | ||
Rollczi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| package com.eternalcode.core.feature.randomteleport; | ||
|
|
||
| record SimpleRandomTeleportRadius(int minX, int maxX, int minZ, int maxZ) implements RandomTeleportRadius { | ||
| public SimpleRandomTeleportRadius { | ||
| if (minX > maxX) { | ||
| throw new IllegalArgumentException("minX cannot be greater than maxX"); | ||
| } | ||
| if (minZ > maxZ) { | ||
| throw new IllegalArgumentException("minZ cannot be greater than maxZ"); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| package com.eternalcode.core.configuration.migration; | ||
|
|
||
| public interface Migration { | ||
|
|
||
| boolean migrate(); | ||
|
|
||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,38 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| package com.eternalcode.core.configuration.migration; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.eternalcode.core.configuration.ConfigurationManager; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.eternalcode.core.configuration.ReloadableConfig; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.eternalcode.core.injector.annotations.Inject; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.eternalcode.core.injector.annotations.component.Controller; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.eternalcode.core.publish.Subscribe; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.eternalcode.core.publish.Subscriber; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.eternalcode.core.publish.event.EternalInitializeEvent; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.logging.Logger; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Controller | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class MigrationController implements Subscriber { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final MigrationService migrationService; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final ConfigurationManager configurationManager; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final Logger logger; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Inject | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| MigrationController(MigrationService migrationService, ConfigurationManager configurationManager, Logger logger) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.migrationService = migrationService; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.configurationManager = configurationManager; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.logger = logger; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+19
to
+24
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add null checks for injected dependencies. To prevent potential NullPointerExceptions, consider adding null validation in the constructor. @Inject
MigrationController(MigrationService migrationService, ConfigurationManager configurationManager, Logger logger) {
+ if (migrationService == null || configurationManager == null || logger == null) {
+ throw new IllegalArgumentException("Dependencies cannot be null");
+ }
this.migrationService = migrationService;
this.configurationManager = configurationManager;
this.logger = logger;
}📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Subscribe | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| void onMigration(EternalInitializeEvent event) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (ReloadableConfig config : configurationManager.getConfigs()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| boolean wasMigrated = migrationService.migrate(config); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (wasMigrated) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| configurationManager.save(config); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.info("Configuration " + config.getClass().getSimpleName() + " was migrated and saved."); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+26
to
+36
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance error handling and logging implementation. The current implementation has several areas for improvement:
Consider implementing this enhanced version: @Subscribe
void onMigration(EternalInitializeEvent event) {
+ int totalConfigs = configurationManager.getConfigs().size();
+ int migratedCount = 0;
for (ReloadableConfig config : configurationManager.getConfigs()) {
+ String configName = config.getClass().getSimpleName();
+ try {
boolean wasMigrated = migrationService.migrate(config);
if (wasMigrated) {
configurationManager.save(config);
- logger.info("Configuration " + config.getClass().getSimpleName() + " was migrated and saved.");
+ logger.info(String.format("Configuration '%s' was migrated and saved successfully", configName));
+ migratedCount++;
}
+ } catch (Exception e) {
+ logger.severe(String.format("Failed to migrate configuration '%s': %s", configName, e.getMessage()));
+ }
}
+ logger.info(String.format("Migration completed: %d/%d configurations processed", migratedCount, totalConfigs));
}📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,37 @@ | ||||||||||||||||||||||||||
| package com.eternalcode.core.configuration.migration; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| import com.eternalcode.core.configuration.ReloadableConfig; | ||||||||||||||||||||||||||
| import com.eternalcode.core.injector.annotations.component.Service; | ||||||||||||||||||||||||||
| import com.eternalcode.core.util.ReflectUtil; | ||||||||||||||||||||||||||
| import java.lang.reflect.Field; | ||||||||||||||||||||||||||
| import net.dzikoysk.cdn.entity.Contextual; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| @Service | ||||||||||||||||||||||||||
| class MigrationService { | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| public <T extends ReloadableConfig> boolean migrate(T config) { | ||||||||||||||||||||||||||
| return reflectMigrate(config); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| private <T> boolean reflectMigrate(T config) { | ||||||||||||||||||||||||||
| boolean isMigrated = false; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| for (Field declaredField : ReflectUtil.getAllSuperFields(config.getClass())) { | ||||||||||||||||||||||||||
| Class<?> fieldType = declaredField.getType(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if (Migration.class.isAssignableFrom(fieldType)) { | ||||||||||||||||||||||||||
| Migration migration = ReflectUtil.getFieldValue(declaredField, config); | ||||||||||||||||||||||||||
| boolean wasMigrationSuccessful = migration.migrate(); | ||||||||||||||||||||||||||
| isMigrated |= wasMigrationSuccessful; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
Comment on lines
+22
to
+26
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handle potential If a field implementing Apply this fix to handle the if (Migration.class.isAssignableFrom(fieldType)) {
Migration migration = ReflectUtil.getFieldValue(declaredField, config);
+ if (migration != null) {
boolean wasMigrationSuccessful = migration.migrate();
isMigrated |= wasMigrationSuccessful;
+ }
}📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if (fieldType.isAnnotationPresent(Contextual.class)) { | ||||||||||||||||||||||||||
| Object fieldValue = ReflectUtil.getFieldValue(declaredField, config); | ||||||||||||||||||||||||||
| isMigrated |= reflectMigrate(fieldValue); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
Comment on lines
+28
to
+31
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct the annotation check for Currently, the code checks if the field's type is annotated with Apply this fix to ensure the annotation check is performed on the field: - if (fieldType.isAnnotationPresent(Contextual.class)) {
+ if (declaredField.isAnnotationPresent(Contextual.class)) {
Object fieldValue = ReflectUtil.getFieldValue(declaredField, config);
+ if (fieldValue != null) {
isMigrated |= reflectMigrate(fieldValue);
+ }
}📝 Committable suggestion
Suggested change
Comment on lines
+29
to
+31
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handle potential When accessing the value of a field annotated with Apply this fix to handle the if (declaredField.isAnnotationPresent(Contextual.class)) {
Object fieldValue = ReflectUtil.getFieldValue(declaredField, config);
+ if (fieldValue != null) {
isMigrated |= reflectMigrate(fieldValue);
+ }
}
|
||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| return isMigrated; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
Comment on lines
+16
to
+35
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider exception handling for reflective operations Reflective operations such as Wrap the reflective calls in try-catch blocks and handle exceptions appropriately: for (Field declaredField : ReflectUtil.getAllSuperFields(config.getClass())) {
Class<?> fieldType = declaredField.getType();
if (Migration.class.isAssignableFrom(fieldType)) {
+ try {
Migration migration = ReflectUtil.getFieldValue(declaredField, config);
if (migration != null) {
boolean wasMigrationSuccessful = migration.migrate();
isMigrated |= wasMigrationSuccessful;
}
+ } catch (Exception e) {
+ // Handle exception or log error
+ }
}
if (declaredField.isAnnotationPresent(Contextual.class)) {
try {
Object fieldValue = ReflectUtil.getFieldValue(declaredField, config);
if (fieldValue != null) {
isMigrated |= reflectMigrate(fieldValue);
}
} catch (Exception e) {
// Handle exception or log error
}
}
}
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add proper deprecation documentation and improve type safety
The deprecated constructor should include proper Javadoc with
@deprecatedand@sincetags explaining the migration path. Also, the cast toSupplier<Duration>is unsafe and could be improved.Consider this safer implementation:
📝 Committable suggestion