Create cache parent directories if they don't exist

[wzieba]

Description

This PR adds a behavior of creating parent dirs for a cache file for ExPlat module if needed. This way, client's don't have to worry if directories they provide exist.

Testing

Checkout a06f9cf and run added test (to confirm it fails). Or comment out

Automattic-Tracks-Android/experimentation/src/main/java/com/automattic/android/experimentation/local/FileBasedCache.kt

Line 50 in dae98ab

assignmentsFile.parentFile?.mkdirs()

and see that test fails.

[MiSikora]

It might be safer to use the TemporaryFolder rule, like this:

@get:Rule val tempDir = TemporaryFolder()

@Test
fun test() {
  val cacheDir = File(tempDir.newFolder(), "cache")
}

This way, you won't have to manually clean up the test directory with cacheDir.deleteRecursively(). If the test fails, the cleanup might not execute, which could cause subsequent test failures due to the existence of cacheDir.

[wzieba]

You make a good point of manual clean up, but using tempDir.newFolder defats the purpose of the test, because it will create a new folder (and we want to verify the case of non-existing folder).

I propose a change using setup/teardown methods: f451396

[MiSikora]

val cacheDir = File(tempDir.newFolder(), "cache") doesn't create the cacheDir. Only the parent newFolder() folder. If you want to test recursive creation you can use something like "deep/nested/cache" instead of "cache".

[wzieba]

Ah right, thanks 👍 addressed in 4bb70bc

[MiSikora]

I often struggle with reviewing these. The mkdirs() method can return false (or even throw an exception), but there’s no way to inform the caller since saveAssignments() doesn’t return anything. On the other hand it’s a common issue that results of such calls are frequently ignored in applications by devs. However, at the library level, I believe it should be communicated somehow. I think it would be best to address this in a separate change that adds return results, rather than including it in this PR.

[wzieba]

If mkdirs returns false, it's all good - the directory might've already existed. If it throws the SecurityException (the only possible), the client wouldn't be gracefully informed. But I think it's a rare possibility. We could cover from this if you think it's valuable by making saveAssignments return kotlin.Result and propagate it down the invocation chain, so it'll be eventually logged.

[MiSikora]

You're right about the SecurityException. I wouldn't expect the library or anyone to handle it.

The mkdirs() method can return false also if OS fails to create the file.

I probably should've made my comment more about file handling than mkdirs(). In general, working with files can result in IOExceptions and failures signaled with false.

[wzieba]

I see - but I still think false, next to signaling a failure, can be a completely fine result if the provided cacheDir exist.

I'm proposing a change like this: 6ba5a8e to cover against any failure, but at the same time I'm not entirely sure if this is what you meant or if this is necessary. But I'd also apply a safe-programming strategy for this SDK, so I think it's fine. Eager to hear your feedback!

[MiSikora]

I see - but I still think false, next to signaling a failure, can be a completely fine result if the provided cacheDir exist.

Yes, I agree; it’s completely fine. My point was that it should be handled.

I think 6ba5a8e is a good compromise without changing the API. Though I'm not sure if failFast is necessary. This will crash anyway at some point if the library fails to create a file. I think making it configurable is unnecessary maintenance complexity. Personally, I'd fail fast. But this is a design decision. If I were a maintainer I would just make it one or the other.

Ideally, saveAssignments() should return something like Result<Unit> or Result<Boolean>. Result<Unit> would be suitable if we don’t need to differentiate between the failure of any operation and a false return value. Result<Boolean> would be preferable if this distinction is important.

The main reason is that exceptions don’t integrate well with Kotlin, and I’m sure most developers wouldn’t instinctively wrap it in try/catch or runCatching(). This could lead to app crashes. Having an explicit result type is more transparent and helps convey what to expect.

[MiSikora]

It is probably my personal preference but I think it makes more sense to use !!. operator instead of ?.. If the parentFile is null it is a code error in the library since we know what assignmentsFile is exactly. Alternatively it might be requireNotNull(assignmentsFile.parentFile) { "error message" } to provide more context.

[wzieba]

I'd reduce a chance of crashing the client app because of non existing parent file. Do you think a log in case of parentFile=null be sufficient for this case?

[MiSikora]

In this specific case, the crash will happen anyway because file.writeText() is called later in saveAssignments(). If the file doesn’t exist, this method will fail. Having the crash occur earlier would indicate the problem at a more appropriate point in the code.

As I mentioned before, this is likely my personal design approach. parentFile must exist here. We know this because it's defined as part of assignmentsFile, and it’s correct for the code to crash if parentFile doesn’t exist. Ignoring this condition leads to unpredictability in the library. If the code neither crashes nor returns an error, the consumer might remain unaware of the issue for a long time. Although in this case, the issue will be detected quickly due to the file.writeText() call.

I'm not expecting a change here but I wanted to offer my 2 cents about the library design. Having it logged doesn't change much in my opinion but it's better than nothing. It's also worth mentioning that the code is covered with a test so it's more of a philosophical than practical point.

[wzieba]

Thank for the insightful review @MiSikora ! I think we agreed on suggestions, I'm going to merge this PR and release a new version.