Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure version files are created at same path as store #101

Merged
merged 1 commit into from
May 2, 2024

Conversation

JanTie
Copy link
Contributor

@JanTie JanTie commented Apr 28, 2024

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.

@xxfast
Copy link
Owner

xxfast commented Apr 28, 2024

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?

@xxfast xxfast self-requested a review April 28, 2024 21:24
@JanTie
Copy link
Contributor Author

JanTie commented Apr 29, 2024

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.

@xxfast
Copy link
Owner

xxfast commented Apr 29, 2024

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?

@JanTie
Copy link
Contributor Author

JanTie commented Apr 30, 2024

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.

@xxfast
Copy link
Owner

xxfast commented May 1, 2024

Hi @JanTie Can we also update the API? you can do this by running ./gradlew apiDump

@JanTie
Copy link
Contributor Author

JanTie commented May 1, 2024

updated the api

Copy link
Owner

@xxfast xxfast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done 👍 Thank you very much for your contribution

@xxfast xxfast merged commit bafbbf1 into xxfast:master May 2, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants