Support multi client storage #290
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |
| import java.io.File; | ||
| import java.io.FileWriter; | ||
| import java.io.Writer; | ||
| import java.nio.file.Paths; | ||
| import java.util.ArrayList; | ||
| import java.util.Arrays; | ||
| import java.util.Comparator; | ||
|
|
@@ -16,8 +17,11 @@ abstract class FileStore<T extends JsonStream.Streamable> { | |
|
|
||
| @NonNull | ||
| protected final Configuration config; | ||
|
|
||
| @Nullable | ||
| final String oldDirectory; | ||
|
|
||
| File storageDir; | ||
|
There was a problem hiding this comment. Why should this property be renamed? There was a problem hiding this comment. I wanted it to be clear that there is a difference between the old directory we previously used, and the client-specific directory that we will use in future. Maybe the nomenclature is a bit off here? |
||
| private final int maxStoreCount; | ||
| private final Comparator<File> comparator; | ||
|
|
||
|
|
@@ -29,31 +33,30 @@ abstract class FileStore<T extends JsonStream.Streamable> { | |
|
|
||
| String path; | ||
| try { | ||
| File baseDir = new File(appContext.getCacheDir().getAbsolutePath(), folder); | ||
| path = baseDir.getAbsolutePath(); | ||
| storageDir = getStorageDir(path, config); | ||
|
|
||
| if (!storageDir.exists()) { | ||
| Logger.warn("Could not prepare file storage directory"); | ||
| path = null; | ||
| } | ||
| } catch (Exception exception) { | ||
| Logger.warn("Could not prepare file storage directory", exception); | ||
| path = null; | ||
| } | ||
| this.oldDirectory = path; | ||
| } | ||
|
|
||
| @Nullable | ||
| String write(@NonNull T streamable) { | ||
| if (storageDir == null) { | ||
| return null; | ||
| } | ||
|
|
||
| // Limit number of saved errors to prevent disk space issues | ||
| if (storageDir.isDirectory()) { | ||
| File[] files = storageDir.listFiles(); | ||
|
There was a problem hiding this comment. This needs fixing as it doesn't take the previous directory into account. |
||
| if (files != null && files.length >= maxStoreCount) { | ||
| // Sort files then delete the first one (oldest timestamp) | ||
| Arrays.sort(files, comparator); | ||
|
|
@@ -86,22 +89,50 @@ String write(@NonNull T streamable) { | |
| return null; | ||
| } | ||
|
|
||
| @NonNull | ||
| abstract String getFilename(T streamable); | ||
|
|
||
| List<File> findStoredFiles() { | ||
| List<File> files = new ArrayList<>(); | ||
|
|
||
| if (oldDirectory != null) { | ||
| File dir = new File(oldDirectory); | ||
| addStoredFiles(dir, files); | ||
| } | ||
| if (storageDir != null) { | ||
| addStoredFiles(storageDir, files); | ||
| } | ||
| return files; | ||
| } | ||
|
|
||
| void addStoredFiles(File dir, List<File> files) { | ||
| if (!dir.exists() || !dir.isDirectory()) { | ||
| return; | ||
| } | ||
| File[] values = dir.listFiles(); | ||
|
|
||
| if (values != null) { | ||
| for (File value : values) { | ||
| if (value.isFile()) { | ||
| files.add(value); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // support multiple clients in the same app by using a unique directory path | ||
|
|
||
| private File getStorageDir(String path, @NonNull Configuration config) { | ||
|
There was a problem hiding this comment. If a project rotates API keys or the on-premise endpoint changes, will stored files ever be sent (or cleared)? There was a problem hiding this comment. Not currently. Is this something we need to address? There was a problem hiding this comment. Yeah - in the worst case, files shouldn't be left on disk indefinitely. In the best case, everything is delivered according to the configuration at the time the report was captured. Splitting directories by endpoint and key works until either component needs to change. A better approach would be to write the entire request to disk including the endpoint and the headers. For example, a (nearly) raw HTTP request (format amended to encode protocol and port): POST https://notify.bugsnag.com
Bugsnag-Api-Key: aaaabaaaabaaaabaaaa
Content-Type: application/json
{"events": [{"severity": "warning", "exceptions":[]}]on premise example for contrast: POST https://bugsnag.example.internal:79002
Bugsnag-Api-Key: aaaabaaaabaaaabaaaa
Content-Type: application/json
{"events": [{"severity": "warning", "exceptions":[]}]Which could then be parsed until encountering the double newline for the endpoint and headers, then stream the rest as the body directly. This should:
There was a problem hiding this comment. I think this approach makes sense. There was a problem hiding this comment. It's also worth mentioning that we use the cache directory when storing files on Android, so Files may be deleted if they are left forever and the system is low on disk space. Reading through the docs it looks like we may want to perform a check on our quota when writing files - I'll create a separate ticket for that. I think it still makes sense to have a limit of around 128 reports/sessions on disk at any time. |
||
| String apiKey = "" + config.getApiKey().hashCode(); | ||
| String endpoint = "" + config.getEndpoint().hashCode(); | ||
|
|
||
| File apiDir = new File(path, apiKey); | ||
| apiDir.mkdirs(); | ||
|
|
||
| File dir = new File(apiDir, endpoint); | ||
| dir.mkdirs(); | ||
|
|
||
| return dir; | ||
| } | ||
|
|
||
| } | ||
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.
If there are multiple clients, will all attempt to find and send files from the "old" directory?
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.
Yes. I've added a mazerunner scenario which covers this.