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

Fix attachments in websql #1210

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@nolanlawson
Copy link
Member

nolanlawson commented Jan 5, 2014

Currently, test.attachments.js breaks in Safari (and websql in general) at this test. I also see this in the Android stock browser (4.4 KitKat).

The issue is really subtle and annoying: it's that the blob data gets converted to UTF8 (!) before being encoded to binary. I have a solution, but it has the following limitations:

  1. Assumes blobSupport in websql.
  2. Doesn't work in leveldb yet. (@daleharvey, @calvinmetcalf: how do you guys debug npm test? Can't seem to get node-inspector to work...)

Details for those who are interested: the little png gets UTF8-encoded, so all the high ASCII characters get corrupted, and it turns into:

wolQTkcNChoKAAAADUlIRFIAAAAQAAAAEAgDAAAAKC0PUwAAADBQTFRFw77Dt8Oow77DtMOgw77DrsOSw73DpsK6w73DnsKkw7zDkcKAw7vDh2LDusK+ScO6wrMsw7nCqRTDuMKiBMO4wp8Aw7jCngDDuMKdAMO4wp0Aw7jCnQDDk8OadMO/AAAAcElEQVQYw5NNwo5RAsOEQARDw4NQw7XDocO+w5fDnTDCnW3Ds8OnwoUEw6ojwpUqwrxjwpYjPyAAwoXDhR/DnMOmC8Ktw5dHJMOsOsOAwpbCk2A9GcKuw7AJeUDDtm3CksOobgnClVwQwpLCnD9ibGtSA2gywozCsQcEwowfw7XCpwM8wrlyd8OLFsOvwp0raXnCgMOLw648YsOGJcOdf8O0A8KiBQjChMKCTMOOwqYAAAAASUVORMKuQmDCgg==

Undoubtedly this is related to #895 and #1008; we'll probably be able to close all three when this is fixed.


// partially taken from http://ecmanaut.blogspot.ca/2006/07/encoding-decoding-utf8-in-javascript.html
function decodeUtf8(str) {
var result

This comment has been minimized.

@calvinmetcalf

calvinmetcalf Jan 5, 2014

Member

this is why it's failing the build

@calvinmetcalf

This comment has been minimized.

Copy link
Member

calvinmetcalf commented Jan 5, 2014

debugging node is annoying but you can enable it via this method

@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented Jan 5, 2014

Missing semicolon. That's what I get for disabling jshint in webstorm. 😉

Your debug method works, but now there's another bug in Node and one in Safari. I'm on the case!

nolanlawson added a commit to nolanlawson/pouchdb that referenced this pull request Jan 16, 2014

(pouchdb#1210) - Fix attachments in websql/Safari
Fixes problems we encounter with unexpected
encoding coercions for non-text attachment
data.

nolanlawson added a commit to nolanlawson/pouchdb that referenced this pull request Jan 17, 2014

(pouchdb#1210) - Fix attachments in websql/Safari
Fixes problems we encounter with unexpected
encoding coercions for non-text attachment
data.
@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented Jan 17, 2014

OK, this is no longer a WIP. I can confirm that Chrome/Firefox/Node have not regressed, and that Safari 6.1.1 (8537.73.11) breaks on the test.attachments.js test before this commit, but works afterwards. (There were actually several broken tests; they're all good now.)

Limitations: it still doesn't work with WebSQL browsers lacking blob support, e.g. Android 2.3 and below. Also, this doesn't fix all the problems in Safari; there are still plenty (see #1068).

@@ -631,17 +667,65 @@ function webSqlPouch(opts, callback) {
utils.call(callback, null);
};

This comment has been minimized.

@nolanlawson

nolanlawson Jan 17, 2014

Member

Using the hex() function in SQLite was the only way I could find to avoid UTF-8 coercion problems for blob data. Sadly, it seems we may have well just used base64 strings instead of the native blob type; it's not doing us any favors.

I didn't find the hex() solution mentioned anywhere, so it's either a clever hack or something we should research more and try to replace.

This comment has been minimized.

@daleharvey

daleharvey Jan 22, 2014

Member

So we are doing type conversions plus navigating and modifying entire binaries every time they get read or written as an attachment?

That sounds worse than using base64, why dont we just use that?

This comment has been minimized.

@daleharvey

daleharvey Jan 22, 2014

Member

but if there is a reason to not use base64, then +1 on the changes, they all look good, congrats on tracking them down, some of them sound insane

}
return result;
}

This comment has been minimized.

@nolanlawson

nolanlawson Jan 17, 2014

Member

Yet another encoding weirdness, this time due to Safari rather than SQLite.

@calvinmetcalf

This comment has been minimized.

Copy link
Member

calvinmetcalf commented Jan 18, 2014

@nolanlawson is this ready to take a look ?

@daleharvey

This comment has been minimized.

Copy link
Member

daleharvey commented Jan 19, 2014

@nolanlawson also daleharvey@d493e3f started getting a bit towards fixing the tests in safari, still fairly broken though

This is looking good, will review with a rebase though just to make sure im looking at the right changes

nolanlawson added a commit to nolanlawson/pouchdb that referenced this pull request Jan 19, 2014

(pouchdb#1210) - Fix attachments in websql/Safari
Fixes problems we encounter with unexpected
encoding coercions for non-text attachment
data.
@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented Jan 19, 2014

Yep, this is good to go; I rebased it as well. Let me know if you need any help reproing errors in Safari; the tests in general are still broken, but ?testFiles=test.attachments.js was working consistently for me after this fix.

@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented Jan 23, 2014

Sorry @daleharvey, I just now noticed your other questions.

I had the same thoughts myself about base64. We're losing all the benefits of the blob type in SQL, plus javascript Blob isn't even well-supported across all browsers so we're going into contortions just to please everybody. I kinda wish we could just change the putAttachment api to use base64 strings (and then use those everywhere), but it may be too late for that. Thoughts?

@daleharvey

This comment has been minimized.

Copy link
Member

daleharvey commented Jan 24, 2014

We want to use blobs where they exist, this isnt a matter of the api, people should be able to store an opaque type, but if opaque type support sucks on the platform we can use base64 internally

@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented Jan 25, 2014

That's essentially what we're doing now. No big deal, and I suppose blobs are a better way to future-proof the API anyway.

@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented Jan 31, 2014

Ping @daleharvey: I know this uglies up the code a lot, but even if we switch to storing base64 in the database, we'll have to run a migration script that does the exact same code in order to preserve existing user data.

Plus apparently people are running into storage limitations on iOS and Android (#1260), so perhaps it was a good idea after all to store blobs, since presumably they take up less space than base64.

@daleharvey

This comment has been minimized.

Copy link
Member

daleharvey commented Jan 31, 2014

@nolanlawson yeh you are right, I had forgotten this was ready to merge and just started relooking at it after ^

@ghost

This comment has been minimized.

Copy link

ghost commented Feb 1, 2014

For backward compatibility, can you leave blob and provide another set of put attachment and get attachment functions that use base64 instead of blob? On Android 4.3 I can use blob and store the data in pouchDB as binary, so if that was the only platform I was wanting to run on the existing api is fine.

nolanlawson added a commit to nolanlawson/pouchdb that referenced this pull request Feb 3, 2014

(pouchdb#1210) - Fix attachments in websql/Safari
Fixes problems we encounter with unexpected
encoding coercions for non-text attachment
data.

nolanlawson added a commit to nolanlawson/pouchdb that referenced this pull request Feb 3, 2014

(pouchdb#1210) - Fix attachments in websql/Safari
Fixes problems we encounter with unexpected
encoding coercions for non-text attachment
data.
@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented Feb 3, 2014

OK, rebased to master since there were a few changes to websql.js since I wrote this. I still think it's good to go, test.attachments.js passes in Safari (other stuff is still broken, natch), and Chrome is 100% unbroken with preferredAdapters reordered to put websql first.

@briankj Yeah, we have lots of open issues about the attachments API. I think going forward the solution is going to be to expose PouchDB.utils.createBlob plus Blob.js for shims. I agree it's not the most user-friendly solution, but unfortunately blob support is just hairy in Javascript at the moment.

Alternatively, did you know you can include the base64 strings in the documents themselves, then just save those docs? It's called "inline attachments," and it doesn't require you to create Blobs.

@ghost

This comment has been minimized.

Copy link

ghost commented Feb 3, 2014

Nolan, thanks for the tip. I tried inline attachments and got a message
that said "attachments must be base64 encoded", but the body was already
encoded. So, since each document needs both an image and audio attachment
I simply created a javascript object with both the encoded image and the
encoded audio, called JSON.stringafy on the object, converted it to blob
and attached it.

On Sun, Feb 2, 2014 at 7:58 PM, Nolan Lawson notifications@github.comwrote:

OK, rebased to master since there were a few changes to websql.js since I
wrote this. I still think it's good to go, test.attachments.js passes in
Safari (other stuff is still broken, natch), and Chrome is 100% unbroken
with preferredAdapters reordered to put websql first.

@briankj https://github.com/briankj Yeah, we have lots of open issues
about the attachments API. I think going forward the solution is going to
be to expose PouchDB.utils.createBlob plus Blob.js for shims. I agree
it's not the most user-friendly solution, but unfortunately blob support is
just hairy in Javascript at the moment.

Alternatively, did you know you can include the base64 strings in the
documents themselves, then just save those docs? It's called "inline
attachments," and it doesn't require you to create Blobs.

Reply to this email directly or view it on GitHubhttps://github.com/daleharvey/pouchdb/pull/1210#issuecomment-33921808
.

http://www.alpinetechsolutions.com

@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented Feb 3, 2014

That's strange. I think if you get that error, then it's because atob failed on the encoded string.

Next time, could you try using the unminified pouchdb.js file and then step through in the debugger? Or if you're testing on mobile exclusively, there's Weinre, which doesn't have a debugger, but does have a console. It would be nice to squash these bugs, so that the next person doesn't have to go through so much effort.

@daleharvey

This comment has been minimized.

Copy link
Member

daleharvey commented Feb 8, 2014

So this is another problem with travis reporting success, this still fails node tests

Possibly unhandled TypeError: Cannot call method 'toString' of undefined
at Object.testUtils.readBlob (/Users/daleharvey/src/pouchdb/tests/test.utils.js:106:19)
at /Users/daleharvey/src/pouchdb/tests/test.attachments.js:515:25

(#1210) - Fix attachments in websql/Safari
Fixes problems we encounter with unexpected
encoding coercions for non-text attachment
data.
@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented Feb 8, 2014

Sorry about that, this is the pull request that never ends 😛. Added the following to fixBinary() in test.utils.js:

if (!process.browser) {
  // don't need to do this in Node
  return bin;
}

Also rebased. Tested in Safari, Firefox, Chrome, and Node, and the tests in test.attachments.js all pass.

nolanlawson added a commit to nolanlawson/pouchdb that referenced this pull request Feb 8, 2014

(pouchdb#1210) - Fix attachments in websql/Safari
Fixes problems we encounter with unexpected
encoding coercions for non-text attachment
data.

nolanlawson added a commit that referenced this pull request Feb 8, 2014

(#1210) - Fix attachments in websql/Safari
Fixes problems we encounter with unexpected
encoding coercions for non-text attachment
data.
@daleharvey

This comment has been minimized.

Copy link
Member

daleharvey commented Feb 8, 2014

Finally merged, sorry about the delayed reviews, but yay

daleharvey@0dbfd2a

@daleharvey daleharvey closed this Feb 8, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment