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

Safari + where + modify = only one updated #594

Open
nicolashenry opened this Issue Oct 16, 2017 · 23 comments

Comments

Projects
None yet
3 participants
@nicolashenry
Contributor

nicolashenry commented Oct 16, 2017

I am doing somthing like this in an async function :

console.log((await table.where('objectProperty').equals(myValue).toArray()).length);
console.log(await table.where('objectProperty').equals(myValue).modify((object) => { object.anotherProperty = 'test'; }));

I have 3 objects matching 'myValue' for 'objectProperty'.
Chrome displays:

3
3

Safari displays:

3
1

So it modifies only one entry on Safari (v11 with Dexie 2.0.1). I tried multiple changes on my code to make it work, the only thing which works was to loop on each array value and update each entry one by one.

@dfahlander

This comment has been minimized.

Show comment
Hide comment
@dfahlander

dfahlander Oct 16, 2017

Owner

Thanks for the finding. Some questions just to nail it down:

  • Are you using binary keys (ArrayBuffer, etc)?
  • Are you using CRUD hooks?
Owner

dfahlander commented Oct 16, 2017

Thanks for the finding. Some questions just to nail it down:

  • Are you using binary keys (ArrayBuffer, etc)?
  • Are you using CRUD hooks?
@dfahlander

This comment has been minimized.

Show comment
Hide comment
@dfahlander

dfahlander Oct 16, 2017

Owner

Also, is Safari 11 in beta still? Do you see the same issue in Safari 10?

Owner

dfahlander commented Oct 16, 2017

Also, is Safari 11 in beta still? Do you see the same issue in Safari 10?

@nicolashenry

This comment has been minimized.

Show comment
Hide comment
@nicolashenry

nicolashenry Oct 16, 2017

Contributor

I am using a number key and I am using CRUD hooks.

Contributor

nicolashenry commented Oct 16, 2017

I am using a number key and I am using CRUD hooks.

@nicolashenry

This comment has been minimized.

Show comment
Hide comment
@nicolashenry

nicolashenry Oct 16, 2017

Contributor

I think Safari 11 is not in beta anymore, I never installed a beta version as far as I remember.

Contributor

nicolashenry commented Oct 16, 2017

I think Safari 11 is not in beta anymore, I never installed a beta version as far as I remember.

@nicolashenry

This comment has been minimized.

Show comment
Hide comment
@nicolashenry

nicolashenry Oct 16, 2017

Contributor

Note that it also happens on iOS Safari v11.

Contributor

nicolashenry commented Oct 16, 2017

Note that it also happens on iOS Safari v11.

@nicolashenry

This comment has been minimized.

Show comment
Hide comment
@nicolashenry

nicolashenry Oct 16, 2017

Contributor

I tried to not use CRUD hooks but it does not seems to change anything. I think I can't downgrade Safari to version 10 sadly.

Contributor

nicolashenry commented Oct 16, 2017

I tried to not use CRUD hooks but it does not seems to change anything. I think I can't downgrade Safari to version 10 sadly.

@dfahlander

This comment has been minimized.

Show comment
Hide comment
@dfahlander

dfahlander Oct 16, 2017

Owner

Ok thanks. I'll try repro it on Safari 10 later this evening.

Owner

dfahlander commented Oct 16, 2017

Ok thanks. I'll try repro it on Safari 10 later this evening.

dfahlander added a commit that referenced this issue Oct 16, 2017

dfahlander added a commit that referenced this issue Oct 16, 2017

Reverted the failing Safari test.
It is confirmed that issue #594 is a Safari issue that index cursors end prematurely if an item being iterated has been modified - either using cursor.update() or store.put() while still iterating the index.

We can't let the dexie suite fail because of this, but should announce a heads-up for it and a proposed workaround so far.
@dfahlander

This comment has been minimized.

Show comment
Hide comment
@dfahlander

dfahlander Oct 16, 2017

Owner

Confirmed there is an issue with Safari (10 and 11). If iterating an index (Dexie where()) the object being iterated must never be modified. This does not apply when iterating an entire object store (db.table.toCollection().modify()) - just when iterating an index (db.table.where(...).modify()).

We could work around this issue in Dexie by modifying the items first when the iteration has completed, but it would be be a larger effort and could possibly affect the behavior of Colletion.modify() in some rare use cases.

For now, we need to announce this and propose a workaround.

Owner

dfahlander commented Oct 16, 2017

Confirmed there is an issue with Safari (10 and 11). If iterating an index (Dexie where()) the object being iterated must never be modified. This does not apply when iterating an entire object store (db.table.toCollection().modify()) - just when iterating an index (db.table.where(...).modify()).

We could work around this issue in Dexie by modifying the items first when the iteration has completed, but it would be be a larger effort and could possibly affect the behavior of Colletion.modify() in some rare use cases.

For now, we need to announce this and propose a workaround.

@dfahlander

This comment has been minimized.

Show comment
Hide comment
@dfahlander

dfahlander Oct 16, 2017

Owner

Proposed workaround:

async function modify (collection, modifyerFn) {
  const objectsToModify = await collection.toArray();
  objectsToModify.forEach(obj => {
    modifyerFn(obj);
  });
  await table.bulkPut(objectsToModify);
  return objectsToModify.length;
}


// Example use:
const numModified = await db.transaction('rw', db.friends, ()=>modify(db.friends.where('age').above(64),
    friend => friend.retired = true
));
Owner

dfahlander commented Oct 16, 2017

Proposed workaround:

async function modify (collection, modifyerFn) {
  const objectsToModify = await collection.toArray();
  objectsToModify.forEach(obj => {
    modifyerFn(obj);
  });
  await table.bulkPut(objectsToModify);
  return objectsToModify.length;
}


// Example use:
const numModified = await db.transaction('rw', db.friends, ()=>modify(db.friends.where('age').above(64),
    friend => friend.retired = true
));
@dfahlander

This comment has been minimized.

Show comment
Hide comment
@dfahlander
Owner

dfahlander commented Oct 17, 2017

@nicolashenry

This comment has been minimized.

Show comment
Hide comment
@nicolashenry

nicolashenry Oct 17, 2017

Contributor

The workaround seems to work for now, thanks!

Contributor

nicolashenry commented Oct 17, 2017

The workaround seems to work for now, thanks!

@dfahlander

This comment has been minimized.

Show comment
Hide comment
@dfahlander

dfahlander Oct 24, 2017

Owner

I think we will need to fix a Safari-workaround for this issue in Dexie as Collection.modify() is a quite central part of the library. I'll see what I could do about it.

Owner

dfahlander commented Oct 24, 2017

I think we will need to fix a Safari-workaround for this issue in Dexie as Collection.modify() is a quite central part of the library. I'll see what I could do about it.

@jokamax

This comment has been minimized.

Show comment
Hide comment
@jokamax

jokamax Nov 2, 2017

Hi,
Horrible bug...
It concerned Chrome under IOS 11 too (webkit) ? It seem touch some users on my side with this configuration (not sure).

Do you think to integrate a workaround soon directly in dexie or you advise to use the workaround above (many and many modify in my application - hierarchical menu offline ;) )?

Thanks a lot,

Jonathan

jokamax commented Nov 2, 2017

Hi,
Horrible bug...
It concerned Chrome under IOS 11 too (webkit) ? It seem touch some users on my side with this configuration (not sure).

Do you think to integrate a workaround soon directly in dexie or you advise to use the workaround above (many and many modify in my application - hierarchical menu offline ;) )?

Thanks a lot,

Jonathan

@dfahlander

This comment has been minimized.

Show comment
Hide comment
@dfahlander

dfahlander Nov 2, 2017

Owner

Yes it's sad because in all other aspects Safari has got an almost a perfect IndexedDB implementation. My goal now is to implement a workaround as soon as possible, but can't promise when it will be released. If you do a workaround similar to the one mentioned in this issue, it should be almost identical to the workaround I'm planning to do with one caveat: If your Collection targets thousands of large objects, the workaround could suffer from large memory consumption. The final workaround that will be implemented in Dexie.js will try to avoid large memory consumption in those cases (by splitting into chunks). Otherwise, it will be almost identical.

Owner

dfahlander commented Nov 2, 2017

Yes it's sad because in all other aspects Safari has got an almost a perfect IndexedDB implementation. My goal now is to implement a workaround as soon as possible, but can't promise when it will be released. If you do a workaround similar to the one mentioned in this issue, it should be almost identical to the workaround I'm planning to do with one caveat: If your Collection targets thousands of large objects, the workaround could suffer from large memory consumption. The final workaround that will be implemented in Dexie.js will try to avoid large memory consumption in those cases (by splitting into chunks). Otherwise, it will be almost identical.

@jokamax

This comment has been minimized.

Show comment
Hide comment
@jokamax

jokamax Nov 2, 2017

Thanks for the quick reply.
I have to think about it...
(for the moment it concern around 20 users on 500, but I have to change the remote MySql copy every day by hand... Maybe I will ask them to use firefox for the moment)

jokamax commented Nov 2, 2017

Thanks for the quick reply.
I have to think about it...
(for the moment it concern around 20 users on 500, but I have to change the remote MySql copy every day by hand... Maybe I will ask them to use firefox for the moment)

@jokamax

This comment has been minimized.

Show comment
Hide comment
@jokamax

jokamax Nov 2, 2017

Hi (again),

I'm not a specialist of ES7/8 but it there a way to write a proposal workaround without async/await (Safari 9, Chome < 50 et FF < 50 compatible) ?

Thanks,

Jonathan

jokamax commented Nov 2, 2017

Hi (again),

I'm not a specialist of ES7/8 but it there a way to write a proposal workaround without async/await (Safari 9, Chome < 50 et FF < 50 compatible) ?

Thanks,

Jonathan

@nicolashenry

This comment has been minimized.

Show comment
Hide comment
@nicolashenry

nicolashenry Nov 2, 2017

Contributor

You can try this:

function modify(collection, modifyerFn) {
  return collection.toArray()
  .then(function (objectsToModify) {
    objectsToModify.forEach(function(obj) {
      modifyerFn(obj);
    });
    return table.bulkPut(objectsToModify)
    .then(function() {
      return objectsToModify.length;
    });
  });
}
Contributor

nicolashenry commented Nov 2, 2017

You can try this:

function modify(collection, modifyerFn) {
  return collection.toArray()
  .then(function (objectsToModify) {
    objectsToModify.forEach(function(obj) {
      modifyerFn(obj);
    });
    return table.bulkPut(objectsToModify)
    .then(function() {
      return objectsToModify.length;
    });
  });
}
@jokamax

This comment has been minimized.

Show comment
Hide comment
@jokamax

jokamax Nov 3, 2017

It works like a charm !

Question :
Collection.modify(function () {delete this.value;});

Is this case of the modify is impacted (partial delete) ?
I haven't Safari, then I can't test :'(

Exemple :

  draft.where('pj_id').equals(pj_id).and(function (draftFilter) {
    return (draftFilter.bg >= targetItem.bg && draftFilter.bd <= targetItem.bd);
  })
  .modify(function(value) {              
    if(value.df_id) {
     trashed_sync.add({pj_id: pj_id, df_id: value.df_id, revsync:value.revsync});
    }
    //-- Remove elt     
    delete this.value;
  })

jokamax commented Nov 3, 2017

It works like a charm !

Question :
Collection.modify(function () {delete this.value;});

Is this case of the modify is impacted (partial delete) ?
I haven't Safari, then I can't test :'(

Exemple :

  draft.where('pj_id').equals(pj_id).and(function (draftFilter) {
    return (draftFilter.bg >= targetItem.bg && draftFilter.bd <= targetItem.bd);
  })
  .modify(function(value) {              
    if(value.df_id) {
     trashed_sync.add({pj_id: pj_id, df_id: value.df_id, revsync:value.revsync});
    }
    //-- Remove elt     
    delete this.value;
  })
@dfahlander

This comment has been minimized.

Show comment
Hide comment
@dfahlander

dfahlander Nov 4, 2017

Owner

There's another workaround that enables the use of Collection.modify().

table.where(...).primaryKeys().then(function (keys) {
    return table.where(':id').anyOf(keys).modify (...)
} 

This version is not as fast as the other workaround but it should support using existing callbacks to modify. I've not tested it though but if someone with a Mac could verify it that would be awesome ;)

Owner

dfahlander commented Nov 4, 2017

There's another workaround that enables the use of Collection.modify().

table.where(...).primaryKeys().then(function (keys) {
    return table.where(':id').anyOf(keys).modify (...)
} 

This version is not as fast as the other workaround but it should support using existing callbacks to modify. I've not tested it though but if someone with a Mac could verify it that would be awesome ;)

@dfahlander

This comment has been minimized.

Show comment
Hide comment
@dfahlander

dfahlander Mar 3, 2018

Owner

@beidson Just want to ping you again for an update of new Safari-related issue and found that two of them stands out more than the others (imho); this one (#594) but also #656. Just for your info. The webkit bug report for this one is still at state NEW. Would be awesome if you could have a look ;)

Kind regards, David

Owner

dfahlander commented Mar 3, 2018

@beidson Just want to ping you again for an update of new Safari-related issue and found that two of them stands out more than the others (imho); this one (#594) but also #656. Just for your info. The webkit bug report for this one is still at state NEW. Would be awesome if you could have a look ;)

Kind regards, David

@jokamax

This comment has been minimized.

Show comment
Hide comment
@jokamax

jokamax Apr 16, 2018

@dfahlander just a question.
This bug concern "modify".
No problem with Table.Where(toto: toto).delete() ? I ask because in the documention delete can be written with modify.

Thanks by advance,

Jonathan

jokamax commented Apr 16, 2018

@dfahlander just a question.
This bug concern "modify".
No problem with Table.Where(toto: toto).delete() ? I ask because in the documention delete can be written with modify.

Thanks by advance,

Jonathan

@dfahlander

This comment has been minimized.

Show comment
Hide comment
@dfahlander

dfahlander Apr 16, 2018

Owner

delete() does not use modify anymore, so no, it should not be affected.

Owner

dfahlander commented Apr 16, 2018

delete() does not use modify anymore, so no, it should not be affected.

@jokamax

This comment has been minimized.

Show comment
Hide comment
@jokamax

jokamax Apr 16, 2018

Thanks a lot

jokamax commented Apr 16, 2018

Thanks a lot

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