Skip to content

Commit ad50b2a

Browse files
npomaroliplyhun
npomaroli
and
plyhun
authored
SUP 17176: Fix accessing files in the binary image cache to not create folders of old structure (#1643)
* Fix accessing files in the binary image cache to not create folders of the old structure. Fix the image cache migration to delete all folders of the old structure * Fix test cleanup after some folders were manually removed * Update LTS-CHANGELOG.adoc --------- Co-authored-by: Serhii Plyhun <S.Plyhun@gentics.com> Co-authored-by: Serhii Plyhun (commercial) <snuk182@live.com>
1 parent f6523ae commit ad50b2a

File tree

6 files changed

+205
-76
lines changed

6 files changed

+205
-76
lines changed

‎LTS-CHANGELOG.adoc‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,14 @@ include::content/docs/variables.adoc-include[]
1717
The LTS changelog lists releases which are only accessible via a commercial subscription.
1818
All fixes and changes in LTS releases will be released the next minor release. Changes from LTS 1.4.x will be included in release 1.5.0.
1919

20+
[[v1.10.37]]
21+
== 1.10.37 (15.01.2025)
22+
23+
icon:check[] Core: The node deletion rules has been strictened, to avoid internal deletion API misusage.
24+
25+
icon:check[] Cache: The Image Cache refactoring, which was done in a previous hotfix release introduced an error which caused creation of empty folders in the old structure as well. This has been fixed.
26+
Also the migration process has been fixed to really remove all folders of the old structure, even if they are empty or contain cache files of binaries that were deleted before.
27+
2028
[[v1.10.36]]
2129
== 1.10.36 (23.10.2024)
2230

‎changelog/src/changelog/entries/2024/12/8095.SUP-17176.bugfix‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
Cache: The Image Cache refactoring, which was done in a previous hotfix release introduced an error which caused creation of empty folders in the old structure as well. This has been fixed.
2+
Also the migration process has been fixed to really remove all folders of the old structure, even if they are empty or contain cache files of binaries that were deleted before.

‎common/src/main/java/com/gentics/mesh/core/image/spi/AbstractImageManipulator.java‎

Lines changed: 31 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -122,38 +122,40 @@ protected Single<CacheFileInfo> getCacheFilePathOld(String sha512sum, ImageManip
122122
String baseFolder = Paths.get(options.getImageCacheDirectory(), buffer.toString()).toString();
123123
String baseName = "image-" + parameters.getCacheKey();
124124

125-
return fs.rxMkdirs(baseFolder)
126-
// Vert.x uses Files.createDirectories internally, which will not fail when the folder already exists.
127-
// See https://github.com/eclipse-vertx/vert.x/issues/3029
128-
.andThen(fs.rxReadDir(baseFolder, baseName + "(\\..*)?"))
129-
.map(foundFiles -> {
130-
int numFiles = foundFiles.size();
131-
if (numFiles == 0) {
132-
String retPath = Paths.get(baseFolder, baseName).toString();
133-
if (log.isDebugEnabled()) {
134-
log.debug("No cache file found for base path {" + retPath + "}");
135-
}
136-
return new CacheFileInfo(maybeNewPath.orElse(retPath), false);
137-
}
125+
return fs.rxExists(baseFolder).flatMap(exists -> {
126+
if (exists) {
127+
return fs.rxReadDir(baseFolder, baseName + "(\\..*)?").flatMap(foundFiles -> {
128+
int numFiles = foundFiles.size();
129+
if (numFiles == 0) {
130+
String retPath = Paths.get(baseFolder, baseName).toString();
131+
if (log.isDebugEnabled()) {
132+
log.debug("No cache file found for base path {" + retPath + "}");
133+
}
134+
return Single.just(new CacheFileInfo(maybeNewPath.orElse(retPath), false));
135+
}
138136

139-
if (numFiles > 1) {
140-
String indent = System.lineSeparator() + " - ";
141-
142-
log.warn(
143-
"More than one cache file found:"
144-
+ System.lineSeparator() + " hash: " + sha512sum
145-
+ System.lineSeparator() + " key: " + parameters.getCacheKey()
146-
+ System.lineSeparator() + " files:"
147-
+ indent
148-
+ String.join(indent, foundFiles)
149-
+ System.lineSeparator()
150-
+ "The cache directory {" + options.getImageCacheDirectory() + "} should be cleared");
151-
}
137+
if (numFiles > 1) {
138+
String indent = System.lineSeparator() + " - ";
139+
140+
log.warn(
141+
"More than one cache file found:"
142+
+ System.lineSeparator() + " hash: " + sha512sum
143+
+ System.lineSeparator() + " key: " + parameters.getCacheKey()
144+
+ System.lineSeparator() + " files:"
145+
+ indent
146+
+ String.join(indent, foundFiles)
147+
+ System.lineSeparator()
148+
+ "The cache directory {" + options.getImageCacheDirectory() + "} should be cleared");
149+
}
152150

153-
if (log.isDebugEnabled()) {
154-
log.debug("Using cache file {" + foundFiles.size() + "}");
151+
if (log.isDebugEnabled()) {
152+
log.debug("Using cache file {" + foundFiles.size() + "}");
153+
}
154+
return Single.just(new CacheFileInfo(foundFiles.get(0), true));
155+
});
156+
} else {
157+
return Single.just(new CacheFileInfo(baseName, false));
155158
}
156-
return new CacheFileInfo(foundFiles.get(0), true);
157159
});
158160
}
159161

‎mdm/common/src/main/java/com/gentics/mesh/core/jobs/ImageCacheMigrationProcessor.java‎

Lines changed: 64 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -4,15 +4,19 @@
44
import static com.gentics.mesh.core.rest.job.JobStatus.FAILED;
55

66
import java.io.File;
7-
import java.io.FileNotFoundException;
87
import java.io.IOException;
98
import java.nio.file.DirectoryNotEmptyException;
9+
import java.nio.file.FileVisitResult;
10+
import java.nio.file.FileVisitor;
1011
import java.nio.file.Files;
1112
import java.nio.file.NoSuchFileException;
1213
import java.nio.file.Path;
14+
import java.nio.file.attribute.BasicFileAttributes;
1315

1416
import javax.inject.Inject;
1517

18+
import org.apache.commons.lang3.StringUtils;
19+
1620
import com.gentics.mesh.core.data.binary.HibBinary;
1721
import com.gentics.mesh.core.data.dao.BinaryDao;
1822
import com.gentics.mesh.core.data.job.HibJob;
@@ -52,41 +56,68 @@ public Completable process(HibJob job) {
5256
return db.asyncTx(() -> {
5357
log.info("Image cache migration started");
5458
Path imageCachePath = Path.of(options.getImageOptions().getImageCacheDirectory());
55-
Files.walk(imageCachePath)
56-
.filter(path -> path.getFileName().toString().startsWith("image-") && Files.isRegularFile(path))
57-
.map(path -> {
58-
String sha512Hash = imageCachePath.relativize(path.getParent()).toString().replace("/", "").replace("\\", "");
59-
HibBinary binary = null;
60-
if (sha512Hash.length() == 128 && (binary = dao.findByHash(sha512Hash).runInExistingTx(Tx.get())) != null) {
61-
String uuid = binary.getUuid();
62-
String segments = getSegmentedPath(uuid);
63-
Path segmentsPath = Path.of(options.getImageOptions().getImageCacheDirectory(), segments);
64-
try {
65-
Files.createDirectories(segmentsPath);
66-
Files.move(path, segmentsPath.resolve(uuid + "-" + path.getFileName().toString().replace("image-", "")));
67-
} catch (IOException e) {
68-
log.error("Could not copy old cached file " + path, e);
69-
}
70-
} else {
71-
log.error("Not a SHA512 hash or binary not found: " + sha512Hash);
59+
60+
// walk the whole tree
61+
Files.walkFileTree(imageCachePath, new FileVisitor<Path>() {
62+
63+
@Override
64+
public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) throws IOException {
65+
// continue from the root directory
66+
if (dir.equals(imageCachePath)) {
67+
return FileVisitResult.CONTINUE;
7268
}
73-
return path;
74-
}).forEach(path -> {
75-
while (path != null && !path.equals(imageCachePath)) {
76-
try {
77-
Files.delete(path);
78-
} catch (DirectoryNotEmptyException e) {
79-
// fair
80-
return;
81-
} catch (NoSuchFileException e) {
82-
// fair, but continue with parent
83-
} catch (IOException e) {
84-
log.error("Could not delete image cache " + path, e);
85-
return;
69+
// continue with directories, which are of the old structure (identifiable by directory names with 8 characters)
70+
return StringUtils.length(dir.toFile().getName()) == 8 ? FileVisitResult.CONTINUE : FileVisitResult.SKIP_SUBTREE;
71+
}
72+
73+
@Override
74+
public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException {
75+
String fileName = file.toFile().getName();
76+
// files that start with "image-" and are located in the old structure need to be migrated
77+
if (fileName.startsWith("image-")) {
78+
String sha512Hash = imageCachePath.relativize(file.getParent()).toString().replace("/", "").replace("\\", "");
79+
HibBinary binary = null;
80+
if (sha512Hash.length() == 128 && (binary = dao.findByHash(sha512Hash).runInExistingTx(Tx.get())) != null) {
81+
String uuid = binary.getUuid();
82+
String segments = getSegmentedPath(uuid);
83+
Path segmentsPath = Path.of(options.getImageOptions().getImageCacheDirectory(), segments);
84+
try {
85+
Files.createDirectories(segmentsPath);
86+
Files.move(file, segmentsPath.resolve(uuid + "-" + fileName.replace("image-", "")));
87+
} catch (IOException e) {
88+
log.error("Could not copy old cached file " + file, e);
89+
}
90+
} else if (sha512Hash.length() == 128) {
91+
log.info("Binary not found: " + sha512Hash + " deleting file " + file);
92+
Files.delete(file);
93+
} else {
94+
log.info("Not a SHA512: " + sha512Hash);
8695
}
87-
path = path.getParent();
8896
}
89-
});
97+
98+
return FileVisitResult.CONTINUE;
99+
}
100+
101+
@Override
102+
public FileVisitResult visitFileFailed(Path file, IOException exc) throws IOException {
103+
return FileVisitResult.CONTINUE;
104+
}
105+
106+
@Override
107+
public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException {
108+
try {
109+
Files.delete(dir);
110+
} catch (DirectoryNotEmptyException e) {
111+
// fair
112+
} catch (NoSuchFileException e) {
113+
// fair
114+
} catch (IOException e) {
115+
log.error("Could not delete image cache " + dir, e);
116+
}
117+
return FileVisitResult.CONTINUE;
118+
}
119+
});
120+
90121
log.info("Image cache migration finished successfully");
91122
});
92123
}).doOnComplete(() -> {

‎tests/tests-core/src/main/java/com/gentics/mesh/core/field/binary/BinaryFieldEndpointTest.java‎

Lines changed: 90 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -27,10 +27,13 @@
2727
import java.util.List;
2828
import java.util.Optional;
2929
import java.util.Random;
30+
import java.util.function.Predicate;
3031

3132
import org.apache.commons.codec.digest.DigestUtils;
3233
import org.apache.commons.io.FileUtils;
3334
import org.apache.commons.io.IOUtils;
35+
import org.apache.commons.lang3.RandomStringUtils;
36+
import org.apache.commons.lang3.StringUtils;
3437
import org.junit.Before;
3538
import org.junit.Test;
3639

@@ -72,6 +75,22 @@ public class BinaryFieldEndpointTest extends AbstractFieldEndpointTest {
7275

7376
private static final String FIELD_NAME = "binaryField";
7477

78+
/**
79+
* Predicate to filter out directories belonging to the old binaryImageCache structure
80+
*/
81+
private static final Predicate<Path> IS_OLD_STRUCTURE = p -> {
82+
File f = p.toFile();
83+
return f.isDirectory() && StringUtils.length(f.getName()) == 8;
84+
};
85+
86+
/**
87+
* Predicate to filter out directories belonging to the new binaryImageCache structure
88+
*/
89+
private static final Predicate<Path> IS_NEW_STRUCTURE = p -> {
90+
File f = p.toFile();
91+
return f.isDirectory() && StringUtils.length(f.getName()) == 2;
92+
};
93+
7594
/**
7695
* Update the schema and add a binary field.
7796
*
@@ -362,33 +381,33 @@ public void testCacheMigration() throws IOException {
362381
}
363382
tx(tx -> {
364383
tx.binaryDao().findAll().runInExistingTx(tx).forEach(binary -> {
365-
String sha512sum = binary.getSHA512Sum();
366-
String[] parts = sha512sum.split("(?<=\\G.{8})");
367-
StringBuffer buffer = new StringBuffer();
368-
buffer.append(File.separator);
369-
for (String part : parts) {
370-
buffer.append(part + File.separator);
371-
}
372-
String baseFolder = Paths.get(imageCache, buffer.toString()).toString();
373-
String baseName = "image-" + randomImageManipulation().getCacheKey() + ".jpg";
374384
try {
375-
Files.createDirectories(Path.of(baseFolder));
376-
try (InputStream i = binary.openBlockingStream().get(); OutputStream o = new BufferedOutputStream(new FileOutputStream(new File(Paths.get(baseFolder, baseName).toString())))) {
385+
Path baseFolder = createOldBinaryImageCacheStructure(imageCache, binary.getSHA512Sum());
386+
String baseName = "image-" + randomImageManipulation().getCacheKey() + ".jpg";
387+
try (InputStream i = binary.openBlockingStream().get(); OutputStream o = new BufferedOutputStream(new FileOutputStream(new File(baseFolder.toFile(), baseName).toString()))) {
377388
i.transferTo(o);
378389
}
379390
} catch (Exception e) {
380391
throw new RuntimeException(e);
381392
}
382393
});
394+
395+
// also create an empty folder structure (old structure)
396+
createOldBinaryImageCacheStructure(imageCache, createRandomSha512Sum());
397+
398+
// and create a folder structure for a binary, which does not exist
399+
Path baseFolder = createOldBinaryImageCacheStructure(imageCache, createRandomSha512Sum());
400+
String baseName = "image-" + randomImageManipulation().getCacheKey() + ".jpg";
401+
new File(baseFolder.toFile(), baseName).createNewFile();
383402
});
384403
assertThat(Files.walk(Path.of(imageCache)).filter(path -> {
385404
File file = new File(path.toString());
386405
return file.exists() && file.isFile() && file.getName().startsWith("image-");
387-
}).count()).isEqualTo(4);
406+
}).count()).isEqualTo(5);
388407
assertThat(Files.walk(Path.of(imageCache)).filter(path -> {
389408
File file = new File(path.toString());
390409
return file.exists() && file.isFile() && file.getName().endsWith(".jpg");
391-
}).count()).isEqualTo(4);
410+
}).count()).isEqualTo(5);
392411

393412
grantAdmin();
394413
HibJob job = tx(tx -> { return tx.jobDao().enqueueImageCacheMigration(user()); });
@@ -408,6 +427,64 @@ public void testCacheMigration() throws IOException {
408427
File file = new File(path.toString());
409428
return file.exists() && file.isFile() && file.getName().endsWith(".jpg");
410429
}).count()).isEqualTo(4);
430+
431+
assertThat(Files.walk(Path.of(imageCache)).filter(IS_OLD_STRUCTURE)).as("Directories/Files in binaryImageCache of the old structure").isEmpty();
432+
}
433+
434+
/**
435+
* Create a random sha512sum
436+
* @return sha512sum
437+
*/
438+
protected String createRandomSha512Sum() {
439+
Buffer dummyContent = Buffer.buffer(RandomStringUtils.random(100));
440+
return com.gentics.mesh.util.FileUtils.hash(dummyContent).blockingGet();
441+
}
442+
443+
/**
444+
* Create the old folder structure for the given sha512sum
445+
* @param imageCache base image cache directory
446+
* @param sha512sum sha512sum
447+
* @return path to the deepest folder
448+
* @throws IOException
449+
*/
450+
protected Path createOldBinaryImageCacheStructure(String imageCache, String sha512sum) throws IOException {
451+
String[] parts = sha512sum.split("(?<=\\G.{8})");
452+
StringBuffer buffer = new StringBuffer();
453+
buffer.append(File.separator);
454+
for (String part : parts) {
455+
buffer.append(part + File.separator);
456+
}
457+
Path dir = Paths.get(imageCache, buffer.toString());
458+
Files.createDirectories(dir);
459+
return dir;
460+
}
461+
462+
/**
463+
* Test that getting an image variant of an existing binary will put the file into the new structure of the binaryImageCache, but not the old one
464+
* @throws IOException
465+
*/
466+
@Test
467+
public void testImageCacheUsage() throws IOException {
468+
String imageCache = options().getImageOptions().getImageCacheDirectory();
469+
470+
// assert that image cache directory is empty
471+
assertThat(Files.list(Path.of(imageCache))).as("Directories/Files in binaryImageCache").isEmpty();
472+
473+
// first create an image
474+
byte[] bytes = getBinary("/pictures/blume.jpg");
475+
NodeResponse nodeResponse = createBinaryNode();
476+
call(() -> client().updateNodeBinaryField(PROJECT_NAME, nodeResponse.getUuid(), "en", nodeResponse.getVersion(), "binary",
477+
new ByteArrayInputStream(bytes), bytes.length, "blume.jpg","image/jpg"));
478+
479+
// assert that image cache directory is still empty
480+
assertThat(Files.list(Path.of(imageCache))).as("Directories/Files in binaryImageCache").isEmpty();
481+
482+
// get a resized variant of the image
483+
call(() -> client().downloadBinaryField(PROJECT_NAME, nodeResponse.getUuid(), "en", "binary", randomImageManipulation()));
484+
485+
// assert that the image cache contains exactly two (nested) directories of the new structure, but none of the old structure
486+
assertThat(Files.walk(Path.of(imageCache)).filter(IS_OLD_STRUCTURE)).as("Directories/Files in binaryImageCache of the old structure").isEmpty();
487+
assertThat(Files.walk(Path.of(imageCache)).filter(IS_NEW_STRUCTURE)).as("Directories/Files in binaryImageCache of the new structure").hasSize(2);
411488
}
412489

413490
private ImageManipulationParameters randomImageManipulation() {

‎tests/tests-core/src/main/java/com/gentics/mesh/test/context/MeshTestContext.java‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
import java.io.File;
99
import java.io.IOException;
10+
import java.nio.file.NoSuchFileException;
1011
import java.security.cert.CertificateException;
1112
import java.time.Duration;
1213
import java.util.ArrayList;
@@ -524,7 +525,15 @@ private void stopJobWorker() throws Exception {
524525

525526
private void cleanupFolders() throws IOException {
526527
for (File folder : tmpFolders) {
527-
FileUtils.deleteDirectory(folder);
528+
try {
529+
FileUtils.deleteDirectory(folder);
530+
} catch (IOException e) {
531+
if (e instanceof NoSuchFileException || (e.getCause() instanceof NoSuchFileException)) {
532+
LOG.debug("Suppressing inexisting directory deletion error", e);
533+
} else {
534+
throw e;
535+
}
536+
}
528537
}
529538
if (meshDagger != null && meshDagger.permissionCache() != null) {
530539
meshDagger.permissionCache().clear(false);

0 commit comments

Comments
 (0)