From c677596aa30f234a6957fd850f45d8888850adfc Mon Sep 17 00:00:00 2001 From: Manoel Aranda Neto Date: Mon, 27 Jul 2020 13:23:34 +0200 Subject: [PATCH 1/5] feat: rotate cache folder --- .../io/sentry/core/cache/CacheStrategy.java | 97 +++++++++++++++++++ .../java/io/sentry/core/cache/DiskCache.java | 72 ++++---------- .../io/sentry/core/cache/SessionCache.java | 52 ++-------- .../io/sentry/core/cache/CacheStrategyTest.kt | 91 +++++++++++++++++ 4 files changed, 213 insertions(+), 99 deletions(-) create mode 100644 sentry-core/src/main/java/io/sentry/core/cache/CacheStrategy.java create mode 100644 sentry-core/src/test/java/io/sentry/core/cache/CacheStrategyTest.kt diff --git a/sentry-core/src/main/java/io/sentry/core/cache/CacheStrategy.java b/sentry-core/src/main/java/io/sentry/core/cache/CacheStrategy.java new file mode 100644 index 000000000..0280536c3 --- /dev/null +++ b/sentry-core/src/main/java/io/sentry/core/cache/CacheStrategy.java @@ -0,0 +1,97 @@ +package io.sentry.core.cache; + +import static io.sentry.core.SentryLevel.ERROR; + +import io.sentry.core.ISerializer; +import io.sentry.core.SentryLevel; +import io.sentry.core.SentryOptions; +import io.sentry.core.util.Objects; +import java.io.File; +import java.nio.charset.Charset; +import java.util.Arrays; +import org.jetbrains.annotations.NotNull; + +abstract class CacheStrategy { + + @SuppressWarnings("CharsetObjectCanBeUsed") + protected static final Charset UTF_8 = Charset.forName("UTF-8"); + + protected final @NotNull SentryOptions options; + protected final @NotNull ISerializer serializer; + protected @NotNull File directory; + protected int maxSize; + + CacheStrategy( + final @NotNull SentryOptions options, + final @NotNull String directoryPath, + final int maxSize) { + this.options = Objects.requireNonNull(options, "SentryOptions is required."); + this.serializer = Objects.requireNonNull(options.getSerializer(), "Serializer is required."); + + Objects.requireNonNull(directoryPath, "Directory is required."); + this.directory = new File(directoryPath); + + this.maxSize = maxSize; + } + + /** + * Check if a Dir. is valid, has write and read permission + * + * @return true if valid and has permission or false otherwise + */ + protected boolean isDirectoryValid() { + if (!directory.isDirectory() || !directory.canWrite() || !directory.canRead()) { + options + .getLogger() + .log( + ERROR, + "The directory for caching files is inaccessible.: %s", + directory.getAbsolutePath()); + return false; + } + return true; + } + + /** + * Sort files from oldest to the newest using the lastModified method + * + * @param files the Files + */ + void sortFilesOldestToNewest(@NotNull File[] files) { + // just sort it if more than 1 file + if (files.length > 1) { + // sort by the oldest to the newest + Arrays.sort(files, (f1, f2) -> Long.compare(f1.lastModified(), f2.lastModified())); + } + } + + /** + * Rotates the caching folder if full, deleting the oldest files first + * + * @param files the Files + */ + protected void rotateCacheIfNeeded(final @NotNull File[] files) { + final int length = files.length; + if (length >= maxSize) { + options + .getLogger() + .log(SentryLevel.WARNING, "Cache folder if full (respecting maxSize). Rotating files"); + final int totalToBeDeleted = (length - maxSize) + 1; + + sortFilesOldestToNewest(files); + + // delete files from the top of the Array as its sorted by the oldest to the newest + for (int i = 0; i < totalToBeDeleted; i++) { + final File file = files[i]; + // sanity check if the file actually exists. + if (file.exists()) { + if (!file.delete()) { + options + .getLogger() + .log(SentryLevel.WARNING, "File can't be deleted: %s", file.getAbsolutePath()); + } + } + } + } + } +} diff --git a/sentry-core/src/main/java/io/sentry/core/cache/DiskCache.java b/sentry-core/src/main/java/io/sentry/core/cache/DiskCache.java index 2054c4f80..637c6ae24 100644 --- a/sentry-core/src/main/java/io/sentry/core/cache/DiskCache.java +++ b/sentry-core/src/main/java/io/sentry/core/cache/DiskCache.java @@ -5,11 +5,8 @@ import static io.sentry.core.SentryLevel.WARNING; import static java.lang.String.format; -import io.sentry.core.ISerializer; import io.sentry.core.SentryEvent; -import io.sentry.core.SentryLevel; import io.sentry.core.SentryOptions; -import io.sentry.core.util.Objects; import java.io.BufferedReader; import java.io.BufferedWriter; import java.io.File; @@ -22,7 +19,6 @@ import java.io.OutputStreamWriter; import java.io.Reader; import java.io.Writer; -import java.nio.charset.Charset; import java.util.ArrayList; import java.util.Iterator; import java.util.List; @@ -34,39 +30,19 @@ * configured directory. */ @ApiStatus.Internal -public final class DiskCache implements IEventCache { +public final class DiskCache extends CacheStrategy implements IEventCache { /** File suffix added to all serialized event files. */ public static final String FILE_SUFFIX = ".sentry-event"; - @SuppressWarnings("CharsetObjectCanBeUsed") - private static final Charset UTF_8 = Charset.forName("UTF-8"); - - private final File directory; - private final int maxSize; - private final ISerializer serializer; - private final SentryOptions options; - - public DiskCache(SentryOptions options) { - Objects.requireNonNull(options.getCacheDirPath(), "Cache dir. path is required."); - this.directory = new File(options.getCacheDirPath()); - this.maxSize = options.getCacheDirSize(); - this.serializer = options.getSerializer(); - this.options = options; + public DiskCache(final @NotNull SentryOptions options) { + super(options, options.getCacheDirPath(), options.getCacheDirSize()); } @Override - public void store(SentryEvent event) { - if (getNumberOfStoredEvents() >= maxSize) { - options - .getLogger() - .log( - SentryLevel.WARNING, - "Disk cache full (respecting maxSize). Not storing event {}", - event); - return; - } + public void store(final @NotNull SentryEvent event) { + rotateCacheIfNeeded(allEventFiles()); - File eventFile = getEventFile(event); + final File eventFile = getEventFile(event); if (eventFile.exists()) { options .getLogger() @@ -92,8 +68,8 @@ public void store(SentryEvent event) { } @Override - public void discard(SentryEvent event) { - File eventFile = getEventFile(event); + public void discard(final @NotNull SentryEvent event) { + final File eventFile = getEventFile(event); if (eventFile.exists()) { options .getLogger() @@ -107,34 +83,17 @@ public void discard(SentryEvent event) { } } - private int getNumberOfStoredEvents() { - return allEventFiles().length; - } - - private boolean isDirectoryValid() { - if (!directory.isDirectory() || !directory.canWrite() || !directory.canRead()) { - options - .getLogger() - .log( - ERROR, - "The directory for caching Sentry events is inaccessible.: %s", - directory.getAbsolutePath()); - return false; - } - return true; - } - - private File getEventFile(SentryEvent event) { + private @NotNull File getEventFile(final @NotNull SentryEvent event) { return new File(directory.getAbsolutePath(), event.getEventId().toString() + FILE_SUFFIX); } @Override public @NotNull Iterator iterator() { - File[] allCachedEvents = allEventFiles(); + final File[] allCachedEvents = allEventFiles(); - List ret = new ArrayList<>(allCachedEvents.length); + final List ret = new ArrayList<>(allCachedEvents.length); - for (File f : allCachedEvents) { + for (final File f : allCachedEvents) { try (final Reader reader = new BufferedReader(new InputStreamReader(new FileInputStream(f), UTF_8))) { @@ -159,10 +118,13 @@ private File getEventFile(SentryEvent event) { return ret.iterator(); } - private File[] allEventFiles() { + private @NotNull File[] allEventFiles() { if (isDirectoryValid()) { // TODO: we need to order by oldest to the newest here - return directory.listFiles((__, fileName) -> fileName.endsWith(FILE_SUFFIX)); + File[] files = directory.listFiles((__, fileName) -> fileName.endsWith(FILE_SUFFIX)); + if (files != null) { + return files; + } } return new File[] {}; } diff --git a/sentry-core/src/main/java/io/sentry/core/cache/SessionCache.java b/sentry-core/src/main/java/io/sentry/core/cache/SessionCache.java index ce45f601e..f10de3ab5 100644 --- a/sentry-core/src/main/java/io/sentry/core/cache/SessionCache.java +++ b/sentry-core/src/main/java/io/sentry/core/cache/SessionCache.java @@ -7,7 +7,6 @@ import static java.lang.String.format; import io.sentry.core.DateUtils; -import io.sentry.core.ISerializer; import io.sentry.core.SentryEnvelope; import io.sentry.core.SentryEnvelopeItem; import io.sentry.core.SentryItemType; @@ -33,7 +32,6 @@ import java.io.OutputStreamWriter; import java.io.Reader; import java.io.Writer; -import java.nio.charset.Charset; import java.util.ArrayList; import java.util.Date; import java.util.Iterator; @@ -46,7 +44,7 @@ import org.jetbrains.annotations.Nullable; @ApiStatus.Internal -public final class SessionCache implements IEnvelopeCache { +public final class SessionCache extends CacheStrategy implements IEnvelopeCache { /** File suffix added to all serialized envelopes files. */ static final String SUFFIX_ENVELOPE_FILE = ".envelope"; @@ -55,37 +53,17 @@ public final class SessionCache implements IEnvelopeCache { static final String SUFFIX_CURRENT_SESSION_FILE = ".json"; static final String CRASH_MARKER_FILE = ".sentry-native/last_crash"; - @SuppressWarnings("CharsetObjectCanBeUsed") - private static final Charset UTF_8 = Charset.forName("UTF-8"); - - private final @NotNull File directory; - private final int maxSize; - private final @NotNull ISerializer serializer; - private final @NotNull SentryOptions options; - private final @NotNull Map fileNameMap = new WeakHashMap<>(); public SessionCache(final @NotNull SentryOptions options) { - Objects.requireNonNull(options.getSessionsPath(), "sessions dir. path is required."); - this.directory = new File(options.getSessionsPath()); - this.maxSize = options.getSessionsDirSize(); - this.serializer = options.getSerializer(); - this.options = options; + super(options, options.getSessionsPath(), options.getSessionsDirSize()); } @Override public void store(final @NotNull SentryEnvelope envelope, final @Nullable Object hint) { Objects.requireNonNull(envelope, "Envelope is required."); - if (getNumberOfStoredEnvelopes() >= maxSize) { - options - .getLogger() - .log( - SentryLevel.WARNING, - "Disk cache full (respecting maxSize). Not storing envelope {}", - envelope); - return; - } + rotateCacheIfNeeded(allEnvelopeFiles()); final File currentSessionFile = getCurrentSessionFile(); @@ -185,7 +163,7 @@ public void store(final @NotNull SentryEnvelope envelope, final @Nullable Object * @param markerFile the marker file * @return the timestamp as Date */ - private Date getTimestampFromCrashMarkerFile(final @NotNull File markerFile) { + private @Nullable Date getTimestampFromCrashMarkerFile(final @NotNull File markerFile) { try (final BufferedReader reader = new BufferedReader(new InputStreamReader(new FileInputStream(markerFile), UTF_8))) { final String timestamp = reader.readLine(); @@ -299,23 +277,6 @@ public void discard(final @NotNull SentryEnvelope envelope) { } } - private int getNumberOfStoredEnvelopes() { - return allEnvelopeFiles().length; - } - - private boolean isDirectoryValid() { - if (!directory.isDirectory() || !directory.canWrite() || !directory.canRead()) { - options - .getLogger() - .log( - ERROR, - "The directory for caching Sentry envelopes is inaccessible.: %s", - directory.getAbsolutePath()); - return false; - } - return true; - } - /** * Returns the envelope's file path. If the envelope has no eventId header, it generates a random * file name to it. @@ -378,7 +339,10 @@ private boolean isDirectoryValid() { private @NotNull File[] allEnvelopeFiles() { if (isDirectoryValid()) { // lets filter the session.json here - return directory.listFiles((__, fileName) -> fileName.endsWith(SUFFIX_ENVELOPE_FILE)); + File[] files = directory.listFiles((__, fileName) -> fileName.endsWith(SUFFIX_ENVELOPE_FILE)); + if (files != null) { + return files; + } } return new File[] {}; } diff --git a/sentry-core/src/test/java/io/sentry/core/cache/CacheStrategyTest.kt b/sentry-core/src/test/java/io/sentry/core/cache/CacheStrategyTest.kt new file mode 100644 index 000000000..a3844b9ac --- /dev/null +++ b/sentry-core/src/test/java/io/sentry/core/cache/CacheStrategyTest.kt @@ -0,0 +1,91 @@ +package io.sentry.core.cache + +import com.nhaarman.mockitokotlin2.mock +import io.sentry.core.DateUtils +import io.sentry.core.SentryOptions +import java.nio.file.Files +import kotlin.test.AfterTest +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +class CacheStrategyTest { + + private class Fixture { + val dir = Files.createTempDirectory("sentry-disk-cache-test").toAbsolutePath().toFile() + val options = SentryOptions().apply { + setSerializer(mock()) + } + + fun getSUT(maxSize: Int = 5): CacheStrategy { + return CustomCache(options, dir.absolutePath, maxSize) + } + } + + private val fixture = Fixture() + + @Test + fun `isDirectoryValid returns true if a valid directory`() { + val sut = fixture.getSUT() + + // sanity check + assertTrue(fixture.dir.isDirectory) + + // this test assumes that the dir. has write/read permission. + assertTrue(sut.isDirectoryValid) + } + + @Test + fun `Sort files from the oldest to the newest`() { + val sut = fixture.getSUT() + + val f1 = Files.createTempFile(fixture.dir.toPath(), "f1", ".json").toFile() + f1.setLastModified(DateUtils.getDateTime("2020-03-27T08:52:58.015Z").time) + + val f2 = Files.createTempFile(fixture.dir.toPath(), "f2", ".json").toFile() + f2.setLastModified(DateUtils.getDateTime("2020-03-27T08:52:59.015Z").time) + + val f3 = Files.createTempFile(fixture.dir.toPath(), "f3", ".json").toFile() + f3.setLastModified(DateUtils.getDateTime("2020-03-27T08:53:00.015Z").time) + + val files = arrayOf(f3, f2, f1) + + sut.sortFilesOldestToNewest(files) + + assertEquals(files[0].absolutePath, f1.absolutePath) + assertEquals(files[1].absolutePath, f2.absolutePath) + assertEquals(files[2].absolutePath, f3.absolutePath) + } + + @Test + fun `Rotate cache folder to save new file`() { + val sut = fixture.getSUT(3) + + val f1 = Files.createTempFile(fixture.dir.toPath(), "f1", ".json").toFile() + f1.setLastModified(DateUtils.getDateTime("2020-03-27T08:52:58.015Z").time) + + val f2 = Files.createTempFile(fixture.dir.toPath(), "f2", ".json").toFile() + f2.setLastModified(DateUtils.getDateTime("2020-03-27T08:52:59.015Z").time) + + val f3 = Files.createTempFile(fixture.dir.toPath(), "f3", ".json").toFile() + f3.setLastModified(DateUtils.getDateTime("2020-03-27T08:53:00.015Z").time) + + val files = arrayOf(f3, f2, f1) + + sut.rotateCacheIfNeeded(files) + + assertFalse(f1.exists()) + assertTrue(f2.exists()) + assertTrue(f3.exists()) + } + + @AfterTest + fun shutdown() { + fixture.dir.listFiles()?.forEach { + it.deleteRecursively() + } + } + + private class CustomCache(options: SentryOptions, path: String, maxSize: Int) : CacheStrategy(options, path, maxSize) +} From cfd4cb05f23569fdab13f5d0ca205d479e52e058 Mon Sep 17 00:00:00 2001 From: Manoel Aranda Neto Date: Mon, 27 Jul 2020 13:36:53 +0200 Subject: [PATCH 2/5] ref tests --- .../io/sentry/core/cache/CacheStrategy.java | 6 +-- .../java/io/sentry/core/cache/DiskCache.java | 3 +- .../io/sentry/core/cache/SessionCache.java | 3 +- .../io/sentry/core/cache/CacheStrategyTest.kt | 54 +++++++++---------- 4 files changed, 32 insertions(+), 34 deletions(-) diff --git a/sentry-core/src/main/java/io/sentry/core/cache/CacheStrategy.java b/sentry-core/src/main/java/io/sentry/core/cache/CacheStrategy.java index 0280536c3..db2c0a538 100644 --- a/sentry-core/src/main/java/io/sentry/core/cache/CacheStrategy.java +++ b/sentry-core/src/main/java/io/sentry/core/cache/CacheStrategy.java @@ -26,7 +26,7 @@ abstract class CacheStrategy { final @NotNull String directoryPath, final int maxSize) { this.options = Objects.requireNonNull(options, "SentryOptions is required."); - this.serializer = Objects.requireNonNull(options.getSerializer(), "Serializer is required."); + this.serializer = options.getSerializer(); Objects.requireNonNull(directoryPath, "Directory is required."); this.directory = new File(directoryPath); @@ -35,9 +35,9 @@ abstract class CacheStrategy { } /** - * Check if a Dir. is valid, has write and read permission + * Check if a Dir. is valid and have write and read permission * - * @return true if valid and has permission or false otherwise + * @return true if valid and has permissions or false otherwise */ protected boolean isDirectoryValid() { if (!directory.isDirectory() || !directory.canWrite() || !directory.canRead()) { diff --git a/sentry-core/src/main/java/io/sentry/core/cache/DiskCache.java b/sentry-core/src/main/java/io/sentry/core/cache/DiskCache.java index 637c6ae24..fd91f3085 100644 --- a/sentry-core/src/main/java/io/sentry/core/cache/DiskCache.java +++ b/sentry-core/src/main/java/io/sentry/core/cache/DiskCache.java @@ -120,8 +120,7 @@ public void discard(final @NotNull SentryEvent event) { private @NotNull File[] allEventFiles() { if (isDirectoryValid()) { - // TODO: we need to order by oldest to the newest here - File[] files = directory.listFiles((__, fileName) -> fileName.endsWith(FILE_SUFFIX)); + final File[] files = directory.listFiles((__, fileName) -> fileName.endsWith(FILE_SUFFIX)); if (files != null) { return files; } diff --git a/sentry-core/src/main/java/io/sentry/core/cache/SessionCache.java b/sentry-core/src/main/java/io/sentry/core/cache/SessionCache.java index f10de3ab5..1318a10e2 100644 --- a/sentry-core/src/main/java/io/sentry/core/cache/SessionCache.java +++ b/sentry-core/src/main/java/io/sentry/core/cache/SessionCache.java @@ -339,7 +339,8 @@ public void discard(final @NotNull SentryEnvelope envelope) { private @NotNull File[] allEnvelopeFiles() { if (isDirectoryValid()) { // lets filter the session.json here - File[] files = directory.listFiles((__, fileName) -> fileName.endsWith(SUFFIX_ENVELOPE_FILE)); + final File[] files = + directory.listFiles((__, fileName) -> fileName.endsWith(SUFFIX_ENVELOPE_FILE)); if (files != null) { return files; } diff --git a/sentry-core/src/test/java/io/sentry/core/cache/CacheStrategyTest.kt b/sentry-core/src/test/java/io/sentry/core/cache/CacheStrategyTest.kt index a3844b9ac..2ff2c9a4d 100644 --- a/sentry-core/src/test/java/io/sentry/core/cache/CacheStrategyTest.kt +++ b/sentry-core/src/test/java/io/sentry/core/cache/CacheStrategyTest.kt @@ -3,6 +3,7 @@ package io.sentry.core.cache import com.nhaarman.mockitokotlin2.mock import io.sentry.core.DateUtils import io.sentry.core.SentryOptions +import java.io.File import java.nio.file.Files import kotlin.test.AfterTest import kotlin.test.Test @@ -40,44 +41,28 @@ class CacheStrategyTest { fun `Sort files from the oldest to the newest`() { val sut = fixture.getSUT() - val f1 = Files.createTempFile(fixture.dir.toPath(), "f1", ".json").toFile() - f1.setLastModified(DateUtils.getDateTime("2020-03-27T08:52:58.015Z").time) - - val f2 = Files.createTempFile(fixture.dir.toPath(), "f2", ".json").toFile() - f2.setLastModified(DateUtils.getDateTime("2020-03-27T08:52:59.015Z").time) - - val f3 = Files.createTempFile(fixture.dir.toPath(), "f3", ".json").toFile() - f3.setLastModified(DateUtils.getDateTime("2020-03-27T08:53:00.015Z").time) + val files = createTempFiles() + val reverseFiles = files.reversedArray() - val files = arrayOf(f3, f2, f1) + sut.sortFilesOldestToNewest(reverseFiles) - sut.sortFilesOldestToNewest(files) - - assertEquals(files[0].absolutePath, f1.absolutePath) - assertEquals(files[1].absolutePath, f2.absolutePath) - assertEquals(files[2].absolutePath, f3.absolutePath) + assertEquals(files[0].absolutePath, reverseFiles[0].absolutePath) + assertEquals(files[1].absolutePath, reverseFiles[1].absolutePath) + assertEquals(files[2].absolutePath, reverseFiles[2].absolutePath) } @Test fun `Rotate cache folder to save new file`() { val sut = fixture.getSUT(3) - val f1 = Files.createTempFile(fixture.dir.toPath(), "f1", ".json").toFile() - f1.setLastModified(DateUtils.getDateTime("2020-03-27T08:52:58.015Z").time) + val files = createTempFiles() + val reverseFiles = files.reversedArray() - val f2 = Files.createTempFile(fixture.dir.toPath(), "f2", ".json").toFile() - f2.setLastModified(DateUtils.getDateTime("2020-03-27T08:52:59.015Z").time) + sut.rotateCacheIfNeeded(reverseFiles) - val f3 = Files.createTempFile(fixture.dir.toPath(), "f3", ".json").toFile() - f3.setLastModified(DateUtils.getDateTime("2020-03-27T08:53:00.015Z").time) - - val files = arrayOf(f3, f2, f1) - - sut.rotateCacheIfNeeded(files) - - assertFalse(f1.exists()) - assertTrue(f2.exists()) - assertTrue(f3.exists()) + assertFalse(files[0].exists()) + assertTrue(files[1].exists()) + assertTrue(files[2].exists()) } @AfterTest @@ -88,4 +73,17 @@ class CacheStrategyTest { } private class CustomCache(options: SentryOptions, path: String, maxSize: Int) : CacheStrategy(options, path, maxSize) + + private fun createTempFiles(): Array { + val f1 = Files.createTempFile(fixture.dir.toPath(), "f1", ".json").toFile() + f1.setLastModified(DateUtils.getDateTime("2020-03-27T08:52:58.015Z").time) + + val f2 = Files.createTempFile(fixture.dir.toPath(), "f2", ".json").toFile() + f2.setLastModified(DateUtils.getDateTime("2020-03-27T08:52:59.015Z").time) + + val f3 = Files.createTempFile(fixture.dir.toPath(), "f3", ".json").toFile() + f3.setLastModified(DateUtils.getDateTime("2020-03-27T08:53:00.015Z").time) + + return arrayOf(f1, f2, f3) + } } From 2807010222cd6fd267ad1a424bc39fa999734f10 Mon Sep 17 00:00:00 2001 From: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com> Date: Tue, 28 Jul 2020 08:45:33 +0200 Subject: [PATCH 3/5] Update sentry-core/src/main/java/io/sentry/core/cache/CacheStrategy.java Co-authored-by: Bruno Garcia --- .../src/main/java/io/sentry/core/cache/CacheStrategy.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sentry-core/src/main/java/io/sentry/core/cache/CacheStrategy.java b/sentry-core/src/main/java/io/sentry/core/cache/CacheStrategy.java index db2c0a538..f3eb31bb5 100644 --- a/sentry-core/src/main/java/io/sentry/core/cache/CacheStrategy.java +++ b/sentry-core/src/main/java/io/sentry/core/cache/CacheStrategy.java @@ -25,10 +25,10 @@ abstract class CacheStrategy { final @NotNull SentryOptions options, final @NotNull String directoryPath, final int maxSize) { + Objects.requireNonNull(directoryPath, "Directory is required."); this.options = Objects.requireNonNull(options, "SentryOptions is required."); + this.serializer = options.getSerializer(); - - Objects.requireNonNull(directoryPath, "Directory is required."); this.directory = new File(directoryPath); this.maxSize = maxSize; From a2f1fac7620daf919497fb5c5c1c507a59041782 Mon Sep 17 00:00:00 2001 From: Manoel Aranda Neto Date: Tue, 28 Jul 2020 08:57:56 +0200 Subject: [PATCH 4/5] code review --- .../io/sentry/core/cache/CacheStrategy.java | 19 ++++++++----------- .../io/sentry/core/cache/CacheStrategyTest.kt | 10 +++++----- 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/sentry-core/src/main/java/io/sentry/core/cache/CacheStrategy.java b/sentry-core/src/main/java/io/sentry/core/cache/CacheStrategy.java index f3eb31bb5..9aac59a0d 100644 --- a/sentry-core/src/main/java/io/sentry/core/cache/CacheStrategy.java +++ b/sentry-core/src/main/java/io/sentry/core/cache/CacheStrategy.java @@ -19,7 +19,7 @@ abstract class CacheStrategy { protected final @NotNull SentryOptions options; protected final @NotNull ISerializer serializer; protected @NotNull File directory; - protected int maxSize; + private int maxSize; CacheStrategy( final @NotNull SentryOptions options, @@ -27,7 +27,7 @@ abstract class CacheStrategy { final int maxSize) { Objects.requireNonNull(directoryPath, "Directory is required."); this.options = Objects.requireNonNull(options, "SentryOptions is required."); - + this.serializer = options.getSerializer(); this.directory = new File(directoryPath); @@ -35,7 +35,7 @@ abstract class CacheStrategy { } /** - * Check if a Dir. is valid and have write and read permission + * Check if a dir. is valid and have write and read permission * * @return true if valid and has permissions or false otherwise */ @@ -57,10 +57,9 @@ protected boolean isDirectoryValid() { * * @param files the Files */ - void sortFilesOldestToNewest(@NotNull File[] files) { + private void sortFilesOldestToNewest(@NotNull File[] files) { // just sort it if more than 1 file if (files.length > 1) { - // sort by the oldest to the newest Arrays.sort(files, (f1, f2) -> Long.compare(f1.lastModified(), f2.lastModified())); } } @@ -84,12 +83,10 @@ protected void rotateCacheIfNeeded(final @NotNull File[] files) { for (int i = 0; i < totalToBeDeleted; i++) { final File file = files[i]; // sanity check if the file actually exists. - if (file.exists()) { - if (!file.delete()) { - options - .getLogger() - .log(SentryLevel.WARNING, "File can't be deleted: %s", file.getAbsolutePath()); - } + if (!file.delete()) { + options + .getLogger() + .log(SentryLevel.WARNING, "File can't be deleted: %s", file.getAbsolutePath()); } } } diff --git a/sentry-core/src/test/java/io/sentry/core/cache/CacheStrategyTest.kt b/sentry-core/src/test/java/io/sentry/core/cache/CacheStrategyTest.kt index 2ff2c9a4d..4d7e7c109 100644 --- a/sentry-core/src/test/java/io/sentry/core/cache/CacheStrategyTest.kt +++ b/sentry-core/src/test/java/io/sentry/core/cache/CacheStrategyTest.kt @@ -39,12 +39,12 @@ class CacheStrategyTest { @Test fun `Sort files from the oldest to the newest`() { - val sut = fixture.getSUT() + val sut = fixture.getSUT(3) - val files = createTempFiles() + val files = createTempFilesSortByOldestToNewest() val reverseFiles = files.reversedArray() - sut.sortFilesOldestToNewest(reverseFiles) + sut.rotateCacheIfNeeded(reverseFiles) assertEquals(files[0].absolutePath, reverseFiles[0].absolutePath) assertEquals(files[1].absolutePath, reverseFiles[1].absolutePath) @@ -55,7 +55,7 @@ class CacheStrategyTest { fun `Rotate cache folder to save new file`() { val sut = fixture.getSUT(3) - val files = createTempFiles() + val files = createTempFilesSortByOldestToNewest() val reverseFiles = files.reversedArray() sut.rotateCacheIfNeeded(reverseFiles) @@ -74,7 +74,7 @@ class CacheStrategyTest { private class CustomCache(options: SentryOptions, path: String, maxSize: Int) : CacheStrategy(options, path, maxSize) - private fun createTempFiles(): Array { + private fun createTempFilesSortByOldestToNewest(): Array { val f1 = Files.createTempFile(fixture.dir.toPath(), "f1", ".json").toFile() f1.setLastModified(DateUtils.getDateTime("2020-03-27T08:52:58.015Z").time) From 696da07b51cd18adc409298f16dd03c44a47d425 Mon Sep 17 00:00:00 2001 From: Manoel Aranda Neto Date: Tue, 28 Jul 2020 13:27:25 +0200 Subject: [PATCH 5/5] make fields final --- .../src/main/java/io/sentry/core/cache/CacheStrategy.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sentry-core/src/main/java/io/sentry/core/cache/CacheStrategy.java b/sentry-core/src/main/java/io/sentry/core/cache/CacheStrategy.java index 9aac59a0d..e175d7712 100644 --- a/sentry-core/src/main/java/io/sentry/core/cache/CacheStrategy.java +++ b/sentry-core/src/main/java/io/sentry/core/cache/CacheStrategy.java @@ -18,8 +18,8 @@ abstract class CacheStrategy { protected final @NotNull SentryOptions options; protected final @NotNull ISerializer serializer; - protected @NotNull File directory; - private int maxSize; + protected final @NotNull File directory; + private final int maxSize; CacheStrategy( final @NotNull SentryOptions options,