Closed JanTie closed 6 months ago
Hi @JanTie Thanks for the contribution!
Little curious as to how this failed to create the version file before this patch. Could you share how the store was created?
What i tried to say in the original ticket is:
The current path would work fine, but only if the target directory is equal to the execution directory. Meaning on a local machine(desktop) for example, things will probably work perfectly fine in most cases. As long as the executing use has write-permissions in the same directory as the app that is being executed.
For example if we use a file path called test_migration.json
, just like in the KVersionedStoreTests, then currently the migration would be created under a file path of $test_migration.json.version
.
However, if we change the directory into a relative path, such as ./data/test_migration.json
then the migration file will still be saved to $test_migration.json.version
.
But things get a bit worse starting here, because using an absolute path for the store, such as /Users/MyUser/data/test_migration.json
will cause a path of $test_migration.json.version
for the version file.
As in all these cases there is no directory defined for the version path, the system will always try to create the file on the current PWD of the system. On Android devices however that directory will always be read-only, causing apps trying to make use of versioned KStores to crash immediately.
I must say i did not have the time to verify the results of this PR yet. Just wanted to point out the underlying issue for now.
But these changes should lead version files always being created at the same location as the original KStore, with just a .version
suffix. So i think this is the intended behavior that you wanted to achieve originally.
Hi @JanTie Thanks for the detailed explanation. It does make sense why it would fail in some scenarios. However, I'm unsure how the change to Path.name
from Path.toString()
could help this use case.
Perhaps a better way to handle this would be to let the codec manage the version file instead, so that you can create this file wherever it needs to go.
For example, something like
public class VersionedCodec<T: @Serializable Any>(
private val file: Path,
private val version: Int = 0,
private val json: Json,
private val serializer: KSerializer<T>,
private val migration: Migration<T>,
private val versionFile: Path = "$$file.version".toPath() // TODO: Save to file metadata instead
): Codec<T> { .. }
Any thoughts on this approach?
I added your feedback. I also provided the exact same default in the storeOf
function so it can actually be set from the caller side if needed.
I just had to change the $$file
to just $file
. Otherwise we would run into the exact same issues as before, because the first Dollar sign would not be escaped/replaced.
PS: Sry for minor reformats, i guess my autoformat kicked in. We may want to use some sort of code formatting tool at some point. I can revert these changes if needed.
Hi @JanTie Can we also update the API? you can do this by running ./gradlew apiDump
updated the api
Addresses #99
I did not have the time yet to verify the result. However, these changes should pretty much ensure that the version file will always be created at the same location as the store itself, having the ".version" suffix. Therefore, if the targeted directory is write-accessable on android, there shall be no more errors during the creation of such versioned stores.