Opened 9 years ago

Closed 7 years ago

Last modified 7 years ago

#13144 closed defect (fixed)

dojo.store.Observable fails to correctly notify with dojo.store.Store transaction API

Reported by: cjolif Owned by: Kris Zyp
Priority: high Milestone: 1.8
Component: Data Version: 1.6.1
Keywords: Cc:
Blocked By: Blocking:

Description

dojo.store.Store defines transaction API that can be implemented by stores.

No provided store does implement it even if it is fully documented.

As we could expect with a non implemented API testing is nonexistent and dojo.store.Observable does not work with a store providing that API. Indeed dojo.store.Observable directly hooks up on the calls to store.put,add,remove functions and notify the observer on each of this call even if the calls are right in the middle of a transaction in which case the observer must not be notified until the commit occurred (or never notified if the transaction is aborted).

That's for the defect.

Also more on a enhancement side, as we can see that with a transaction store we can get a batch of modifications on commit, maybe it would have been better to have observe function to potentially receive directly a batch of modifications instead of a single one.

I have attached a DOH test case that reproduces the issue with a very partial/limited implementation of a transaction store just made to highlight the issue.

Attachments (4)

MemoryTransaction.js (571 bytes) - added by cjolif 9 years ago.
MemoryTransactionTest.js (1.0 KB) - added by cjolif 9 years ago.
MemoryTransaction.2.js (575 bytes) - added by cjolif 9 years ago.
dojo.store.Observable.patch (2.0 KB) - added by cjolif 9 years ago.
tentative patch for the issue (IBM, CCLA)

Download all attachments as: .zip

Change History (14)

Changed 9 years ago by cjolif

Attachment: MemoryTransaction.js added

Changed 9 years ago by cjolif

Attachment: MemoryTransactionTest.js added

Changed 9 years ago by cjolif

Attachment: MemoryTransaction.2.js added

comment:1 Changed 9 years ago by cjolif

MemoryTransaction?.js does not load use second upload instead.

Changed 9 years ago by cjolif

Attachment: dojo.store.Observable.patch added

tentative patch for the issue (IBM, CCLA)

comment:2 Changed 9 years ago by bill

Owner: changed from Jared Jurkiewicz to Kris Zyp

comment:3 Changed 9 years ago by Kris Zyp

I am not sure I agree with the semantics that are being described here for transactions. Changes that occur within a transaction should be visible within that transaction, and that does not appear to be the case with the MemoryTransaction? store. For example:

transaction = store.transaction(); store.put({id:1, name:"New"}); at this point the put should *not* be visible outside this transaction, but it should be visible inside (here in this client): store.get(1).name -> "New" on a different client, store.get(1).name would not return "New"

now once we do the commit, the changed data should be visible everywhere transaction.commit();

Since this client is inside the transaction and the changes are visible immediately (prior to commit), it seems like it would also follow that Observable notifications should be consistent with the visibility and fire notification events immediately after put() and other operations.

comment:4 Changed 9 years ago by Kris Zyp

Trying again to put code in

transaction = store.transaction(); 
store.put({id:1, name:"New"}); 
// at this point the put should *not* be visible outside 
// this transaction, but it should be visible inside (here in this client): store.get(1).name -> "New" 
// on a different client, store.get(1).name would not return "New"

// now once we do the commit, the changed data should be visible everywhere transaction.commit();

comment:5 Changed 9 years ago by Kris Zyp

Trying again

transaction = store.transaction(); 
store.put({id:1, name:"New"}); 
// at this point the put should *not* be visible outside 
// this transaction, but it should be visible inside (here in this client): store.get(1).name -> "New" 
// on a different client, store.get(1).name would not return "New"

// now once we do the commit, the changed data should be visible everywhere 
transaction.commit()

comment:6 Changed 9 years ago by Kris Zyp

Trying again (this editor is pathetic)

transaction = store.transaction(); 
store.put({id:1, name:"New"}); 
// at this point the put should *not* be visible outside 
// this transaction, but it should be visible inside (here in this client): 
store.get(1).name -> "New" 
// on a different client, store.get(1).name would not return "New"

// now once we do the commit, the changed data should be visible everywhere 
transaction.commit()

comment:7 Changed 9 years ago by cjolif

Ok, you are indeed right it can also make sense to get notifications on the client while doing a transaction so this sound reasonable to keep that as-is. In that case however this means we should probably make clear that any implementer of transaction _must_ call remove or put (to remove newly added elements or put back elements before modification) when the transaction is aborted to notify the objects are rollbacked (instead of just trashing pending modifications). Otherwise the notification will be asymmetric.

comment:8 Changed 7 years ago by Kris Zyp

Resolution: fixed
Status: newclosed

I've updated the dojo/store docs to note this.

comment:9 Changed 7 years ago by Kris Zyp

Milestone: tbd1.8

comment:10 Changed 7 years ago by dg

I tend to agree with Christophe. I have a use case where I have 2 views and a store. In view 1, I display the content of the store (a list) In view 2, user is making modifications on the store. But I don't want to refresh view 1 which is hidden at each change (mobile device must use CPU wisely) So I could open a transaction in view2 startup and commit when showing view 1.

Note: See TracTickets for help on using tickets.