-
Notifications
You must be signed in to change notification settings - Fork 714
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
Android: Column xxxx is not unique. #232
Comments
This is the correct behavior. In case of any doubt you can try the same thing on a Web SQL database opened with windows.openDatabase(). |
Hi @brodybits , this.instance = window.sqlitePlugin.openDatabase({name: "DB-v2"});
insert_user_bulk= function (context, data, callback) {
var counter = 0;
this.instance.transaction(function (tx) {
for (var i = 0; i < data.length; i++) {
var sql = "insert into users(user_id) values (?)";
tx.executeSql(sql,[data[i]], function () {
counter++;
if (counter === data.length) {
callback.call(context);
}
}, function () {
//Here should be fired if row could not be inserted
counter++;
if (counter === data.length) {
callback.call(context);
}
});
}
}, function(){
console.log("transaction error");// this is fired when i try to insert a row with existing user_id.
});
}; |
This issue is due to a misunderstanding of the W3 database (draft) specification in PR #170. Will be fixed later this week. (I do see the executeSql error callback get fired.) As a workaround: in the executeSql error callback, if you return false the transaction error callback will not be fired. |
The spec says:
The last step in the overall steps is:
So, if you don't want the transaction to fail, you need to provide an error callback and explicitly return false. Unless I misinterpreted something, it sounds to me like the plugin is behaving correctly and when I added PR #170 I updated the tests for this. |
I would agree with you according to the spec, however this Web SQL seems to behave differently (at least in Android and iOS) as shown by the old tests. I will add some more tests to show this in the next few days. So the question becomes whether we should follow the spec more strictly at the risk of breaking apps that are working in the browser (web) SQL. My preference, at least in this case, is to match the behavior of Web SQL as we see in the browser to avoid breaking apps. @nolanlawson do you have any comments? |
Hm, that's weird, never seen it behave differently myself. If you don't want the transaction to fail, you need to pass in an error callback that returns false. In fact, we rely on this behavior in the PouchDB test suite, and the test passes on PhantomJS/Android/Chrome/iOS/Safari, so that would be one way to confirm it. :) |
Prior to PR #170 the plugin would continue the transaction if a handler was either missing or returned non-true (which includes no return value) as opposed to explicit false, which I don't believe matches the spec or a desirable behavior. In that PR, I corrected the plugin to match the spec above which not only simplifies the error handling code you have to write, it allows you to detect issues you might not have been aware of otherwise, such as the root cause for the transaction failure in this bug report. If Android and iOS don't implement the logic above, I would lean towards filing bugs in WebKit to get it corrected rather than making the plugin bug compatible. |
Agreed with @aarononeal and @nolanlawson and covered in test suite. |
when I try to insert a row having an index already exists in the table where this index is unique, an exception occurred which prevent me to continue using the plugin.
_Note:_ I was using an old version of this plugin and this issue never happened
Thank you.
sample code:
and this is the error log:
The text was updated successfully, but these errors were encountered: