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

(Windows) added fix to preserve embedded nulls within text data #709

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

spacepope
Copy link

Hi,
as stated in the docs this plugin has this known issue on Windows:

Truncation issue with UNICODE \u0000 character (same as \0)

I ran into this issue using PouchDB with SQLite Adapter in an Cordova/Ionic App and noticed, that in a Windows UWP App index databases generated by PouchDB were not successfully filled. After digging deeper into the PouchDB source code i saw that index document id's are generated by concatenating multiple values as strings with \u0000 as separator.

Because the current implementation of ColumnText and BindText are using automatic string length estimation (which means looking for the first null char), the data text gets truncated and is not fully written/read.

On my search i stumbled upon a bug report on Web SQL implementation of Chromium, which addresses exactly this issue.
So i did nearly the same patch on ColumnText and pass the string length calculated from the number of text bytes reported by SQLite into the string constructor.

Looking a bit deeper into the SQLite3 docs i saw also the description of the third parameter of sqlite3_bind_text16(), which could be set to the number of bytes the string has.
So for BindText i did the opposite of the fix above and pass the number of bytes calculated from the string length.

This seems to work and the index id column is updated and fetched with embedded nulls (SQLiteBrowser reports this as a BLOB and doesn't show a string representation any more, but the binary data are exactly as expected..)

As i am not that C++ guy, i am a bit concerned, whether the string length or byte count calculation is always correct, regardless of things like encoding, etc..
So can you please double check this on testing my PR?

Thanks so far, i hope we can fix this issue with my PR and prevent others from running into these problems..

@spacepope
Copy link
Author

Hmm, i don't see why travis builds are failing..
But it seems like this is not an issue with the code itself but the travis configuration?

brodycj pushed a commit to brodycj/Cordova-sqlite-storage-common-dev that referenced this pull request Nov 7, 2017
added fix for unwanted truncation on ColumnText and BindText to preserve embedded nulls.

With minor changes by @brodybits:
- indentation consistent with rest of Windows C++ code
- use const declaration for intermediate results
- update test cases & docs
- mark common version, to be merged into multiple plugin versions
- drop extra question

ref: storesafe#709
@brodycj
Copy link
Contributor

brodycj commented Dec 8, 2017

Thanks @spacepope for the nice contribution, my apologies for the delay. The changes definitely look OK to me. The Windows platform version with your changes will pass u0000 character tests in the test suite. I hope to integrate in an upcoming minor release (#685), otherwise will be part of the next major release (#687).

P.S. No worries about the Travis build, will probably be removed.

@brodycj brodycj mentioned this pull request Dec 8, 2017
32 tasks
@spacepope
Copy link
Author

Glad that i could help.
So maybe next time i get a humongous discount for the commercial license 😉 😃

@brodycj
Copy link
Contributor

brodycj commented Dec 12, 2017

@spacepope I would definitely be happy to give you a discount with credit for your work on the next commercial license, will discuss by email whenever you are ready. Thanks again for your contributions.

sergii-trotsiuk pushed a commit to sergii-trotsiuk/Cordova-sqlite-storage that referenced this pull request Apr 3, 2018
@brodycj brodycj changed the base branch from storage-master to dev February 18, 2019 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants