Backup save related files outside of save folder #178

DarkShadow44 · 2025-01-22T16:04:23Z

This includes:

journeymap data
NEI data
TCNodeTracker data
visualprospecting data
schematics
blueprints

Fixes #120

Note that I don't know what exactly is stored inside blueprints, I never used it

src/main/java/serverutils/task/backup/ThreadBackup.java

Lyfts · 2025-01-22T17:08:06Z

Have you tested how restoring a backup from the in-game menu interacts with this?

YannickMG · 2025-01-22T17:23:58Z

I feel like this feature could help enable seamless in-place pack updates.

src/main/java/serverutils/task/backup/ThreadBackup.java

DarkShadow44 · 2025-01-23T19:30:51Z

I pushed a rework, addressing the mentioned issues. I still need to test it more before I consider it ready, though.

Lyfts · 2025-01-23T20:24:46Z

Windows doesn't seem to like those wildcards

java.nio.file.InvalidPathException: Illegal char <*> at index 54: journeymap/data/sp/New World-------------------------/**
	at java.base/sun.nio.fs.WindowsPathParser.normalize(WindowsPathParser.java:199) ~[?:?]
	at java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:175) ~[?:?]
	at java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:77) ~[?:?]
	at java.base/sun.nio.fs.WindowsPath.parse(WindowsPath.java:92) ~[?:?]
	at java.base/sun.nio.fs.WindowsFileSystem.getPath(WindowsFileSystem.java:231) ~[?:?]
	at java.base/java.nio.file.Path.of(Path.java:148) ~[?:?]
	at java.base/java.nio.file.Paths.get(Paths.java:69) ~[?:?]
	at Launch//serverutils.task.backup.ThreadBackup.addBaseFolderFiles(ThreadBackup.java:77) ~[ThreadBackup.class:?]
	at Launch//serverutils.task.backup.ThreadBackup.doBackup(ThreadBackup.java:97) [ThreadBackup.class:?]
	at Launch//serverutils.task.backup.ThreadBackup.run(ThreadBackup.java:67) [ThreadBackup.class:?]

Lyfts · 2025-01-23T20:25:51Z

src/main/java/serverutils/ServerUtilitiesConfig.java

+                "saves/NEI/global/**", "saves/NEI/local/$WORLDNAME/**", "visualprospecting/client/*/$WORLDNAME_*/**",
+                "visualprospecting/server/$WORLDNAME_*/**", "visualprospecting/veintypesLUT", "schematics/**",
+                "blueprints/**" })
+        public String[] additional_backup_files;


These are fine as GTNH defaults, but I don't see why most of these need to be mod defaults.
(also veintypesLUT is gone after 2.7.x)

Which ones would you leave in? Just NEI, or nothing?

The NEI/local/ subfolder is fine to keep imo, but restoring the NEI/global/ folder can cause some unintended consequences if the player restores a backup from an old world.
The trailing ** aren't necessary since FileUtils.listFiles() will include the entire subdirectory anyways.

The trailing ** are needed for the PathMatcher to work.

Regarding NEI/global: But on the other hand it would make sense to backup and restore in case it got corrupted. Not sure how I should handle this?

Regarding NEI/global: But on the other hand it would make sense to backup and restore in case it got corrupted. Not sure how I should handle this?

We can't safely restore data that isn't related to the world.

src/main/java/serverutils/task/backup/BackupTask.java

Lyfts · 2025-01-23T20:37:11Z

src/main/java/serverutils/client/gui/GuiRestoreBackup.java

+        for (String pattern : backups.additional_backup_files) {
+            pattern = pattern.replace("$WORLDNAME", worldName);
+
+            String rootFolder = String.valueOf(Paths.get(pattern).getName(0));
+
+            File original = new File(rootFolder);
+            if (original.isDirectory()) {
+                File copy = new File(rootFolder + "_old");
+                while (copy.exists()) {
+                    copy = new File(copy.getName() + "_old");
+                }
+                original.renameTo(copy); // Rename original and don't change that again
+
+                try {
+                    org.apache.commons.io.FileUtils.copyDirectory(copy, original);
+                } catch (IOException e) {
+                    e.printStackTrace();
+                }
+
+                // Delete all matching files from directory, so we can cleanly restore the backup
+                List<File> fileCandidates = FileUtils.listTree(original);
+                PathMatcher matcher = FileSystems.getDefault().getPathMatcher("glob:" + pattern);
+
+                for (File file : fileCandidates) {
+                    if (matcher.matches(file.toPath())) {
+                        file.delete();
+                    }


I don't think it's a good idea to leave _old versions of all the files stored outside of the saves folder, it's only done with the world folder because the copy is directly visual in the world select menu. This risks creating potentially big copies in folders that players very rarely have any reason to look in.

Is there a good alternative? Deleting all old data without a backup sounds dangerous.

Edit: I also found another issue where it created more than one _old folder, but I didn't fix that yet since it might get removed

Idk what the alternative could be, maybe someone else can weigh in on that.
Personally I think it's a very bad idea to duplicate all this data in various folders every time a backup is restored.

src/main/java/serverutils/task/backup/ThreadBackup.java

This includes by default: - journeymap data - NEI data - TCNodeTracker data - visualprospecting data - schematics - blueprints

DarkShadow44 · 2025-01-23T21:03:52Z

Windows doesn't seem to like those wildcards

java.nio.file.InvalidPathException: Illegal char <*> at index 54: journeymap/data/sp/New World-------------------------/**
	at java.base/sun.nio.fs.WindowsPathParser.normalize(WindowsPathParser.java:199) ~[?:?]
	at java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:175) ~[?:?]
	at java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:77) ~[?:?]
	at java.base/sun.nio.fs.WindowsPath.parse(WindowsPath.java:92) ~[?:?]
	at java.base/sun.nio.fs.WindowsFileSystem.getPath(WindowsFileSystem.java:231) ~[?:?]
	at java.base/java.nio.file.Path.of(Path.java:148) ~[?:?]
	at java.base/java.nio.file.Paths.get(Paths.java:69) ~[?:?]
	at Launch//serverutils.task.backup.ThreadBackup.addBaseFolderFiles(ThreadBackup.java:77) ~[ThreadBackup.class:?]
	at Launch//serverutils.task.backup.ThreadBackup.doBackup(ThreadBackup.java:97) [ThreadBackup.class:?]
	at Launch//serverutils.task.backup.ThreadBackup.run(ThreadBackup.java:67) [ThreadBackup.class:?]

Seems like Windows java doesn't like "*" in the paths, I pushed an update that should fix that. Although I can't fully test since I currently can't test on Windows.

Lyfts · 2025-01-23T23:24:39Z

Stripping the path of all * makes the VP subfolder end up as $WORLDNAME_ which isn't gonna match up with any directory.

Also all those force pushes makes it very hard to track what was actually changed.

DarkShadow44 · 2025-01-24T15:29:27Z

Stripping the path of all * makes the VP subfolder end up as $WORLDNAME_ which isn't gonna match up with any directory.

I only remove the * for the String rootFolder = String.valueOf(Paths.get(cleaned_pattern).getName(0));, this is just to get the root folder, which shouldn't have wildcards to begin with.

Also all those force pushes makes it very hard to track what was actually changed.

Sorry, I'm used to gitlab where that is no problem, I will make additional commits from now on.

Lyfts · 2025-01-25T10:33:52Z

I only remove the * for the String rootFolder = String.valueOf(Paths.get(cleaned_pattern).getName(0));, this is just to get the root folder, which shouldn't have wildcards to begin with.

If you're only using the PathMatcher to resolve the wildcards it would be much more efficient to only resolve the part of the path that contains a wildcard, instead of trying to match every file in the root folder against the pattern.

Doing it this way means that it'll iterate through potentially thousands of files, since all content of the subfolders is included, to only add a few of those files to the backup.
For the NEI folders in my dev instance it iterated through the entire saves folder which was over 3000 files and it only ended up adding 1 file to the backup.
I also tried checking a more extreme example by adding ./options.txt to the additional backup files and this ended up iterating through > 10000 files to only add a single one.

DarkShadow44 · 2025-01-25T13:07:31Z

I only remove the * for the String rootFolder = String.valueOf(Paths.get(cleaned_pattern).getName(0));, this is just to get the root folder, which shouldn't have wildcards to begin with.

If you're only using the PathMatcher to resolve the wildcards it would be much more efficient to only resolve the part of the path that contains a wildcard, instead of trying to match every file in the root folder against the pattern.

Doing it this way means that it'll iterate through potentially thousands of files, since all content of the subfolders is included, to only add a few of those files to the backup. For the NEI folders in my dev instance it iterated through the entire saves folder which was over 3000 files and it only ended up adding 1 file to the backup. I also tried checking a more extreme example by adding ./options.txt to the additional backup files and this ended up iterating through > 10000 files to only add a single one.

Thanks for your in-depth review, appreciate it! I pushed a fix that only iterates as many files as needed.

Could you please open conversations for future issue? I find it to be easier to follow, since it groups.

(cherry picked from commit cff0889)

DarkShadow44 mentioned this pull request Jan 22, 2025

Add Journeymap folder to backup #120

Open

Dream-Master requested review from Lyfts and a team January 22, 2025 16:19

Lyfts requested changes Jan 22, 2025

View reviewed changes

src/main/java/serverutils/task/backup/ThreadBackup.java Outdated Show resolved Hide resolved

Lyfts reviewed Jan 23, 2025

View reviewed changes

src/main/java/serverutils/task/backup/ThreadBackup.java Outdated Show resolved Hide resolved

DarkShadow44 force-pushed the master branch from c1c363a to 90eddd1 Compare January 23, 2025 19:29

Lyfts requested changes Jan 23, 2025

View reviewed changes

Backup save related files outside of save folder

e2aaaba

This includes by default: - journeymap data - NEI data - TCNodeTracker data - visualprospecting data - schematics - blueprints

DarkShadow44 force-pushed the master branch from 90eddd1 to e2aaaba Compare January 23, 2025 21:01

Dream-Master added 2 commits January 24, 2025 19:46

Merge branch 'master' into master

4725bcb

Merge branch 'master' into master

1e202ec

Iterate over as few files as possible

50d6b7d

sa

cdfa1c6

(cherry picked from commit cff0889)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backup save related files outside of save folder #178

Backup save related files outside of save folder #178

DarkShadow44 commented Jan 22, 2025

Lyfts commented Jan 22, 2025

YannickMG commented Jan 22, 2025

DarkShadow44 commented Jan 23, 2025

Lyfts commented Jan 23, 2025

Lyfts Jan 23, 2025

DarkShadow44 Jan 23, 2025

Lyfts Jan 23, 2025

DarkShadow44 Jan 24, 2025

Lyfts Jan 25, 2025

Lyfts Jan 23, 2025

DarkShadow44 Jan 23, 2025 •

edited

Loading

Lyfts Jan 25, 2025

DarkShadow44 commented Jan 23, 2025

Lyfts commented Jan 23, 2025

DarkShadow44 commented Jan 24, 2025

Lyfts commented Jan 25, 2025

DarkShadow44 commented Jan 25, 2025 •

edited

Loading

Backup save related files outside of save folder #178

Are you sure you want to change the base?

Backup save related files outside of save folder #178

Conversation

DarkShadow44 commented Jan 22, 2025

Lyfts commented Jan 22, 2025

YannickMG commented Jan 22, 2025

DarkShadow44 commented Jan 23, 2025

Lyfts commented Jan 23, 2025

Lyfts Jan 23, 2025

Choose a reason for hiding this comment

DarkShadow44 Jan 23, 2025

Choose a reason for hiding this comment

Lyfts Jan 23, 2025

Choose a reason for hiding this comment

DarkShadow44 Jan 24, 2025

Choose a reason for hiding this comment

Lyfts Jan 25, 2025

Choose a reason for hiding this comment

Lyfts Jan 23, 2025

Choose a reason for hiding this comment

DarkShadow44 Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Lyfts Jan 25, 2025

Choose a reason for hiding this comment

DarkShadow44 commented Jan 23, 2025

Lyfts commented Jan 23, 2025

DarkShadow44 commented Jan 24, 2025

Lyfts commented Jan 25, 2025

DarkShadow44 commented Jan 25, 2025 • edited Loading

DarkShadow44 Jan 23, 2025 •

edited

Loading

DarkShadow44 commented Jan 25, 2025 •

edited

Loading