Conversation
There was a problem hiding this comment.
Can you detail this function? Not sure how version and encryption are related.
There was a problem hiding this comment.
We are just trying to open the file, to make sure it is unencrypted, basically we only care if it's successful or not.
There was a problem hiding this comment.
I can think of others reason for this call to fail than just being unencrypted. Maybe we can find a better name? Or a safer method?
There was a problem hiding this comment.
Can we improve error handling? What if this fails ?
(I realize you didn't introduce this)
There was a problem hiding this comment.
In this case it's ok for us to fail, we want to detect unencrypted versions of realm, so we can reset them, once we remove the code I agree error handling should be made more robust.
There was a problem hiding this comment.
What about returning nil only and not logging then?
There was a problem hiding this comment.
you mean just remove the log statement? (we need the schema version if it we manage to decrypt it)
There was a problem hiding this comment.
Hopefully this function does not only return nil 😅
There was a problem hiding this comment.
Should this not be called outside of reset?
There was a problem hiding this comment.
not sure, it used to be there I have reintroduced it ( it seems resetting realm is a single purpouse thing, only applies for this release, should be throwaway code), but don't mind putting it outside.
There was a problem hiding this comment.
We might find other needs for this in the future. I would not expect that kind of side effects when calling a reset function.
There was a problem hiding this comment.
What will happen if this is fresh install without any previous schema - won't unencrypted? be true ?
If yes, will it work fine, or crash on the reset-realm schema (+ show confusing pop-up) ?
There was a problem hiding this comment.
that's interesting, it seems like you are right, I had tested it on my local android and worked fine, but will make sure it works.
There was a problem hiding this comment.
Weirdly enough it just works, I can't really say why from the code (possibly because opening an empty realm will succede and return a version of 0? ). But will make sure that is the case ,good shout.
There was a problem hiding this comment.
Yes, opening an empty realm does not fail, and return a schema version of -1
There was a problem hiding this comment.
Actually I know why, calling version on empty realm returns -1, I just remembered. It's probably worth documenting somewhere...
|
apk uploaded to https://i.diawi.com/s7XwyV for branch PR-4503 |
|
Can we take this opportunity to add some simple doc to |
|
ipa uploaded to https://i.diawi.com/dcHStq for branch PR-4503 |
|
@jeluard yes, I will test everything works ( is still wip, I needed an image to test on a real device), and add some docs to the namespace |
|
@jeluard updated with docstrings and some tests |
There was a problem hiding this comment.
is it fine to call migrate-schemas if encrypted-version is -1?
There was a problem hiding this comment.
I believe so, I haven't changed that logic + I have tested multiple times on clean installations
There was a problem hiding this comment.
I would add a comment though, because it looks like this case with -1 was just forgotten.
|
@cammellos Awesome thanks for the effort! |
|
apk uploaded to https://i.diawi.com/SMKUoS for branch PR-4503 |
|
ipa uploaded to https://i.diawi.com/yGqsQ7 for branch PR-4503 |
|
@cammellos all looks good 👍 |
Signed-off-by: Andrea Maria Piana <andrea.maria.piana@gmail.com>
6 participants
fixes #4409
Summary:
The code will try to open an encrypted realm.
If it fails will try to open an unencrypted realm.
If that is successful we reset the database and create an encrypted version.
We ensure it is unencrypted as we don't want to reset the database if a IO error occurred on step 1.
Testing notes (optional):
Should ask you to reset from 0.9.18, but update cleanly from 0.9.19/nightlies
status: ready