-
Notifications
You must be signed in to change notification settings - Fork 81
don't try deserializing short blobs #249
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #249 +/- ##
==========================================
- Coverage 83.19% 83.16% -0.03%
==========================================
Files 5 5
Lines 607 606 -1
==========================================
- Hits 505 504 -1
Misses 102 102
Continue to review full report at Codecov.
|
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.
This LGTM; I think there was also an open issue about not deserializing non-serialized blobs. Because I think it's still an issue if you just serialize some random bytes as a blob and it's longer than sizeof(SERIALIZATION)
. But we can fix that later if you want. Thanks for this! If you wouldn't mind tweaking the test to fix the only
issue, we can merge after that.
Actually, I'm just going to bump the Julia compat for the package up to 1.3, so that should solve the |
|
What exactly do you mean here? Like, if starting bytes of a blob coincide with those of I would really like to have a way to completely opt-out of serialization/deserialization in SQLite.jl (or even make it opt-in). It's arguably pretty rare to have an sqlite database that is to be accessed from julia alone, and even more so - from an exactly matching version of julia with the same package versions. Serialization docs say:
I was surprised by this behavior when unexpectedly ended up with serialized blobs in my database that resulted from An error would be a much safer approach. |
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.
Thanks @aplavin!
fixes #248