Schematic caching system for suggestions #2542

me4502 · 2024-06-01T04:33:04Z

This is a continuation of #2212, as the PR is abandoned by its original author

- Resolve first batch of review comments - Renamed Schematic to SchematicPath - Removed custom hashCode() and equals() implementations - Added unit-test to make sure it behaves as expected

Co-authored-by: Octavia Togami <[email protected]>

octylFractal · 2024-11-04T01:06:15Z

worldedit-core/src/main/java/com/sk89q/worldedit/WorldEdit.java

+    private static final java.util.regex.Pattern SAFE_FILENAME_REGEX = java.util.regex.Pattern.compile("^[A-Za-z0-9_\\- \\./\\\\'\\$@~!%\\^\\*\\(\\)\\[\\]\\+\\{\\},\\?]+\\.[A-Za-z0-9]+$");
+
    private boolean checkFilename(String filename) {
-        return filename.matches("^[A-Za-z0-9_\\- \\./\\\\'\\$@~!%\\^\\*\\(\\)\\[\\]\\+\\{\\},\\?]+\\.[A-Za-z0-9]+$");
+        return SAFE_FILENAME_REGEX.matcher(filename).matches();


This set of characters doesn't make sense, it's not safe for windows (contains ?, *, doesn't ban COM and others), and POSIX will generally take anything. It also excludes extended Unicode characters for some reason, despite those being perfectly fine.

Can we just drop checking this aside from "you must have an extension", i.e. ^.+\..+$?

octylFractal · 2024-11-04T01:15:16Z

worldedit-core/src/main/java/com/sk89q/worldedit/internal/util/RecursiveDirectoryWatcher.java

+    public static Optional<RecursiveDirectoryWatcher> create(Path root) throws IOException {
+        try {
+            WatchService watchService = root.getFileSystem().newWatchService();
+            return Optional.of(new RecursiveDirectoryWatcher(root, watchService));
+        } catch (UnsupportedOperationException ignored) {
+            return Optional.empty();
+        }
+    }


Let's not bother with the Optional path here. We're not currently dealing with non-default Path providers, and I don't expect to any time soon. Quote from the Javadocs of newWatchService:

This exception is not thrown by FileSystems created by the default provider.

So, let's remove the Optional and just throw.

Suggested change

public static Optional<RecursiveDirectoryWatcher> create(Path root) throws IOException {

try {

WatchService watchService = root.getFileSystem().newWatchService();

return Optional.of(new RecursiveDirectoryWatcher(root, watchService));

} catch (UnsupportedOperationException ignored) {

return Optional.empty();

}

}

public static RecursiveDirectoryWatcher create(Path root) throws IOException {

WatchService watchService;

try {

watchService = root.getFileSystem().newWatchService();

} catch (UnsupportedOperationException e) {

throw new IllegalArgumentException("Root must support file watching", new RecursiveDirectoryWatcher(root, watchService));

}

return new RecursiveDirectoryWatcher(root, watchService);

}

octylFractal · 2024-11-04T01:16:31Z

worldedit-core/src/main/java/com/sk89q/worldedit/internal/schematic/SchematicsManager.java

+    private void createFallbackBackend() {
+        LOGGER.warn("Failed to initialize file-monitoring based schematics backend. Falling back to polling.");
+        backend = PollingSchematicsBackend.create(schematicsDir);
+    }
+
+    private void setupBackend() {
+        try {
+            var fileWatcherBackend = FileWatcherSchematicsBackend.create(schematicsDir);
+            if (fileWatcherBackend.isPresent()) {
+                backend = fileWatcherBackend.get();
+            } else {
+                createFallbackBackend();
+            }
+        } catch (IOException e) {
+            createFallbackBackend();
+        }
+    }


Assuming that my Optional-removing suggestion is accepted, let's clean this up too:

Suggested change

private void createFallbackBackend() {

LOGGER.warn("Failed to initialize file-monitoring based schematics backend. Falling back to polling.");

backend = PollingSchematicsBackend.create(schematicsDir);

}

private void setupBackend() {

try {

var fileWatcherBackend = FileWatcherSchematicsBackend.create(schematicsDir);

if (fileWatcherBackend.isPresent()) {

backend = fileWatcherBackend.get();

} else {

createFallbackBackend();

}

} catch (IOException e) {

createFallbackBackend();

}

}

private void setupBackend() {

try {

backend = FileWatcherSchematicsBackend.create(schematicsDir);

} catch (IOException e) {

LOGGER.warn("Failed to initialize file-monitoring based schematics backend. Falling back to polling.", e);

backend = PollingSchematicsBackend.create(schematicsDir);

}

}

octylFractal · 2024-11-04T01:22:09Z

worldedit-core/src/main/java/com/sk89q/worldedit/internal/schematic/SchematicsManager.java

+            setupBackend();
+        } catch (IOException e) {
+            LOGGER.warn("Failed to create schematics directory", e);
+            backend = new DummySchematicsBackend(); //fallback to dummy backend


This comment isn't needed. It's obvious.

Suggested change

backend = new DummySchematicsBackend(); //fallback to dummy backend

backend = new DummySchematicsBackend();

octylFractal · 2024-11-04T01:23:38Z

worldedit-core/src/main/java/com/sk89q/worldedit/internal/schematic/SchematicsManager.java

+            schematicsDir = worldEdit.getWorkingDirectoryPath(worldEdit.getConfiguration().saveDir);
+            Files.createDirectories(schematicsDir);
+            schematicsDir = schematicsDir.toRealPath();


Let's not set this twice.

Suggested change

schematicsDir = worldEdit.getWorkingDirectoryPath(worldEdit.getConfiguration().saveDir);

Files.createDirectories(schematicsDir);

schematicsDir = schematicsDir.toRealPath();

Path schematicsDirByConfig = worldEdit.getWorkingDirectoryPath(worldEdit.getConfiguration().saveDir);

Files.createDirectories(schematicsDirByConfig);

schematicsDir = schematicsDirByConfig.toRealPath();

octylFractal · 2024-11-04T01:25:57Z

...re/src/main/java/com/sk89q/worldedit/internal/schematic/backends/DummySchematicsBackend.java

+/**
+ * A backend that never lists any files.
+ */
+public class DummySchematicsBackend implements SchematicsBackend {


I feel like this backend should throw instead. We're using it as the driving force behind //schem list now, so if it fails, people shouldn't just get an empty list out, they should get an error. Or possibly, we shouldn't have this at all and should crash out if we have an error setting up.

octylFractal · 2024-11-04T01:27:34Z

.../main/java/com/sk89q/worldedit/internal/schematic/backends/FileWatcherSchematicsBackend.java

+    private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
+    private final Set<Path> schematics = new HashSet<>();


Why are we not using a Sets.newConcurrentHashSet()? It would work much better since it doesn't need to lock the whole set.

octylFractal · 2024-11-04T01:30:18Z

worldedit-core/src/main/java/com/sk89q/worldedit/internal/util/RecursiveDirectoryWatcher.java

+        Path schematicRoot = WorldEdit.getInstance().getSchematicsManager().getRoot();
+
+        this.eventConsumer = eventConsumer;
+        watchThread = new Thread(() -> {


Just in case, we should check for an existing watchThread and kill it or throw. Killing requires us to have a means of communicating to it.

octylFractal · 2024-11-04T01:31:12Z

worldedit-core/src/main/java/com/sk89q/worldedit/internal/util/RecursiveDirectoryWatcher.java

+            }
+            LOGGER.debug("RecursiveDirectoryWatcher::EventConsumer exited");
+        });
+        watchThread.setName("RecursiveDirectoryWatcher");


At the very least we should identify ourselves fully. Perhaps we should also use a global counter.

Suggested change

watchThread.setName("RecursiveDirectoryWatcher");

watchThread.setName("WorldEdit-RecursiveDirectoryWatcher");

me4502 mentioned this pull request Jun 1, 2024

SchematicsManager: Handling and caching list of known schematics, providing cli suggestions #2212

Closed

seijikun and others added 10 commits June 1, 2024 14:45

Implement SchematicsManager to cache list of known schematics

ec1c7f6

Add command suggestions for schematic filenames

e5f9212

Rename Schematic to SchematicPath and cleanup

0551ff6

- Resolve first batch of review comments - Renamed Schematic to SchematicPath - Removed custom hashCode() and equals() implementations - Added unit-test to make sure it behaves as expected

Remove unncessary (un)init log points

efb6304

Apply suggestions from code review

64d8e39

Co-authored-by: Octavia Togami <[email protected]>

Resolve many of octy's review notes

4f3ad0c

Further review fixes

0c7f5f6

Further review improvements

92a3ad2

Fix symlink check

05b229a

PR notes

ff6393e

me4502 force-pushed the feature/schematic-manager branch from cf27d60 to ff6393e Compare June 1, 2024 04:45

me4502 added this to the 7.4.0 milestone Jun 8, 2024

octylFractal requested changes Nov 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Schematic caching system for suggestions #2542

Schematic caching system for suggestions #2542

me4502 commented Jun 1, 2024

octylFractal Nov 4, 2024

octylFractal Nov 4, 2024

octylFractal Nov 4, 2024

octylFractal Nov 4, 2024

octylFractal Nov 4, 2024

octylFractal Nov 4, 2024

octylFractal Nov 4, 2024

octylFractal Nov 4, 2024

octylFractal Nov 4, 2024

	backend = new DummySchematicsBackend(); //fallback to dummy backend
	backend = new DummySchematicsBackend();

		private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
		private final Set<Path> schematics = new HashSet<>();

	watchThread.setName("RecursiveDirectoryWatcher");
	watchThread.setName("WorldEdit-RecursiveDirectoryWatcher");

Schematic caching system for suggestions #2542

Are you sure you want to change the base?

Schematic caching system for suggestions #2542

Conversation

me4502 commented Jun 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment