put request doesn't respect uniqueness

ct-pink commented 10 years ago

when "update_history_item" is performed, the putUnique request doesn't respect the unicity of the key > the historyItem is overwritten.

ct-pink commented 10 years ago

By modifying the incoming item id from int to string it works.

no int/string recognition, we may have to deal with it ourselves
it should still not be a problem, as the url is also unique. I don't understand this part. Still inquiring

moutard commented 10 years ago

You talk about update unique in the indexeddb database or in the pool ? (I think it's in the pool because there is no "key" in indexeddb) Can you give an example of entry to reproduce ?

Expected behavior: For indexeddb the behavior of put unique should be -> if no error -> put if error -> find the existing entry and use merge function. (it seems to be the behavior you describe)

For pool: find if an entry corresponds given the key and the value yes: replace using merge no: put

ps: modifying the id using string instead of int can have a lot of side effects.

ct-pink commented 10 years ago

It's in database.

try to log something in the putUnique, in the first put request "onsuccess" Then open a webpage and see the logs. Because the pages updates with the same historyItem, we should always have errors. But on the update_visit_item we always have SUCCESS.

I agree with you I don't want to modify the id. But if you try converting the id to a string before the put, then it works. (just to give more elements)

putUnique: function(sObjectStoreName, dItem, mMerge, mOnSaveCallback) {
var self = this;

var oTransaction = this._oDb.transaction([sObjectStoreName],
    "readwrite");
var oStore = oTransaction.objectStore(sObjectStoreName);
var oPutRequest = oStore.put(dItem);

oPutRequest.onsuccess = function(oEvent) {
  console.log("SUCCESS");
  mOnSaveCallback.call(self, oEvent.target.result);
};

oPutRequest.onerror = function(oEvent) {
 console.log('MERGE ITEMS');

[...]

ct-pink commented 10 years ago

Sorry when I said "key" in the first place I meant that during a put, the db is supposed to check if the for unique indexes (url & id for historyItems) the values for the item are already existing in the db. If they do then there is an error. Here the error is not thrown. For the id, it looks like it's related to the type int vs string but I thought that if the url is already existing in the base it would still throw the error. Looks like not :S I reckon now that you need all the unique indexes to be equal in the base for the request to throw the error.

moutard commented 10 years ago

If it's true that a big problem of the database. I will check if we can find another solution.

So you confirm that it's not link with the type of the id.

On Thu, Oct 24, 2013 at 11:21 AM, ct-pink notifications@github.com wrote:

Sorry when I said "key" in the first place I meant that during a put, the db is supposed to check if the for unique indexes (url & id for historyItems) the values for the item are already existing in the db. If they do then there is an error. Here the error is not thrown. For the id, it looks like it's related to the type int vs string but I thought that if the url is already existing in the base it would still throw the error. Looks like not :S I reckon now that you need all the unique indexes to be equal in the base for the request to throw the error.

— Reply to this email directly or view it on GitHubhttps://github.com/cottonTracks/extension/issues/158#issuecomment-26977391 .

Raphaël Moutard | Co-founder cottonTracks

Phone: +33 6 50 54 55 64 (France) cottonTracks.com http://cottontracks.com/ Facebook https://www.facebook.com/CottonTracks Twitterhttps://twitter.com/cottontracks LinkedIn https://www.linkedin.com/company/2696411 AngelListhttps://angel.co/cottontracks

ct-pink commented 10 years ago

when I tried a "find" with a string id instead of an integer, the DB returns null. So the id is correctly stored as int, not string.

However past that I seriously don't understand what's happening, why it doesn't/does throw an error. Maybe I just didn't get the way it was supposed to work. Best would be to have a call a bout it.

grr sorry for the closed/reopened, I keep clicking on the wrong button >:(

ct-pink commented 10 years ago

so:

let's say we have a history item (A) in base with id, and url (which are the "unique" indexkeys) And we putUnique (supposedly = update, in our mind) the same historyItem B. What we expect : if either the id or the url of B is the same as in A.

What I get for the put request:

if id(B) == id(A) and url(B) == url(A) > success and overwrite (we expect error)
if id(B) != id(A) and url(B) == url(A) > error (ok)
if id(B) == id(A) and url(B) != url(A) > success and overwrite (we expect error)
if id(B) != id(A) and url(B) != url(A) > success and new item in base (ok)

What I don't understand: the 2 unique keys don't behave symetrically What I suspect on top of it: the first request we do may have to be an "add" and not "put". Because from what I read "put" updates no matter what, even if there is already an item in base.

@moutard I'm really lost here, but hope it gives you a bit of insight.

moutard commented 10 years ago

There is also the case where id is not setted that should react differently.

moutard commented 10 years ago

I just think I find the error in the translator.

history_item_translator:32 dDbRecord.id = oHistoryItem.id(); this sould be broken during minification :(

ct-pink commented 10 years ago

Do you mean minification during the prod compilation? Because I also in dev and there is already the issue.

moutard commented 10 years ago

sorry I wanted to say I find an error (not the error)

ct-pink commented 10 years ago

What bothers me with the non-symetrical results between id and url is that it could seem legit to have the id more important but it doesn't seem in the specs that a key 'id' is required! have you read anything like this?

moutard commented 10 years ago

the key 'id' is not required, but has we decided to use a nosql db as a sqldb we decided to always fixed an unique id.

On Sat, Oct 26, 2013 at 11:53 AM, ct-pink notifications@github.com wrote:

What bothers me with the non-symetrical results between id and url is that it could seem legit to have the id more important but it doesn't seem in the specs that a key 'id' is required! have you read anything like this?

— Reply to this email directly or view it on GitHubhttps://github.com/cottonTracks/extension/issues/158#issuecomment-27143175 .

Raphaël Moutard | Co-founder cottonTracks

Phone: +33 6 50 54 55 64 (France) cottonTracks.com http://cottontracks.com/ Facebook https://www.facebook.com/CottonTracks Twitterhttps://twitter.com/cottontracks LinkedIn https://www.linkedin.com/company/2696411 AngelListhttps://angel.co/cottontracks

ct-pink commented 10 years ago

fyi @moutard I had this warning when highlighting a quote on a page (I think) "event.returnValue is deprecated. Please use the standard event.preventDefault() instead." But I couldn't find anny occurence in the code of 'returnValue' :S It's only a warning but I wanted to let you know. (logs below)

http://www.economist.com/news/leaders/21589883-responding-disaster-essential-so-preparing-next-stress-test background.min.js:5939

Event bubbles: true cancelBubble: false cancelable: true clipboardData: undefined currentTarget: null defaultPrevented: false eventPhase: 0 returnValue: true srcElement: IDBRequest target: IDBRequest timeStamp: 1384614309958 type: "error" proto: Event background.min.js:2754

event.returnValue is deprecated. Please use the standard event.preventDefault() instead.

cottonTracks / extension

put request doesn't respect uniqueness #158