Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#16480 closed defect (fixed)

Observable async stores could not be used with StoreRefController

Reported by: Johnathon Harris Owned by: Ed Chatelain
Priority: undecided Milestone: 1.8.4
Component: DojoX MVC Version: 1.8.2rc1
Keywords: Cc:
Blocked By: Blocking:

Description

The StoreRefController? does some copying of attributes at the end of queryStore so that Observable results are maintained. Unfortunately this does not work if the result it modifies is a Promise (e.g. if you are using an Observable JsonRest? store).

The error will be that the results eventually generated will no longer have the observe method.

A simple fix is to move the logic which copies the Observable attributes into the callback for dojo/when invoked on the promised results.

Attachments (3)

dojo-1.8.2_StoreRefController_ObservableAsyncStoreFix_ticket16480.patch (1004 bytes) - added by Johnathon Harris 7 years ago.
Fix for this ticket.
ticket16480-testcase.patch (2.5 KB) - added by Johnathon Harris 7 years ago.
ticket16480-testcase-StoreRefController.patch (3.1 KB) - added by Johnathon Harris 7 years ago.
Demonstrates that StoreRefController?? fails to make observe() available if the store returns a Promise, even after using dojo/when. Note that before using when() the Promise returned doesn't have observable() either.

Download all attachments as: .zip

Change History (25)

Changed 7 years ago by Johnathon Harris

Fix for this ticket.

comment:1 Changed 7 years ago by Johnathon Harris

Corporate CLA is on file for Certus Technology.

comment:2 Changed 7 years ago by cjolif

jmharris do you have a doh test case as well, so that we can check regressions in the future?

Thanks.

comment:3 Changed 7 years ago by Ed Chatelain

Thanks for the patch. I am on vacation until early January so I will not be able to work on this until I return.

comment:4 Changed 7 years ago by Akira Sudoh

Looks that this will be fixed by [30552], so we can close this ticket?

comment:5 Changed 7 years ago by Ed Chatelain

Resolution: fixed
Status: newclosed

Thanks asudoh, I will close it as fixed, it was fixed by #16656.

comment:6 Changed 7 years ago by bill

Milestone: tbd1.9

comment:7 Changed 7 years ago by Johnathon Harris

Hi, the patch for Ticket #16656 does not address the problem I reported. Can this ticket be re-opened?

Will the patch I provided not be applied until a doh test case is provided as requested by cjolif?

comment:8 Changed 7 years ago by Ed Chatelain

If a testcase had been provided we could have used the test to tell if the patch for #16656 actually fixed it or not. So if you can provide the test we will take another look, and see if it should be reopened.

Changed 7 years ago by Johnathon Harris

Attachment: ticket16480-testcase.patch added

comment:9 Changed 7 years ago by Johnathon Harris

Thanks edchat, please see newly attached test case.

observeQueryResults passes as you would expect.

observeDeferredQueryResults fails and shows the problem I reported regarding async stores.

Applying the dojox/mvc/StoreRefController patch originally submitted will allow observeDeferredQueryResults to pass.

Applying the patch for ticket #16656 makes no difference to the test case.

comment:10 Changed 7 years ago by Akira Sudoh

I found a bug in EditStoreRefController? not doing decoration for Obsrevable, which is fixed in: https://github.com/edchat/dojox_mvc/pull/77

Given dojo/store/Observable:52-61 tells us that it sets observe() directly to query() result no matter it's promise or value, what application needs to do is calling observe() of what queryStore() returns, instead of trying to call observe() of the resolved value.

comment:11 Changed 7 years ago by Johnathon Harris

Hi asudoh, your patch does not appear to resolve the problem I'm reporting (although I'm not completely sure if you were claiming it does).

I have provided a test case and bug fix patch for this problem- is there anything outstanding?

comment:12 Changed 7 years ago by Akira Sudoh

Hi jmharris, if this makes it more clear: You cannot expect the resolved value (in other words, that data coming from server/store) always has observe() method. observe() method is attached whichever queryStore() returns, either promise or value. Please refer to my newly added test case how to use observe() with queryStore() returning a promise.

comment:13 Changed 7 years ago by Johnathon Harris

Ok, you are saying that it is the expected behaviour that if I use Observable with a store I get observe() on the results regardless of whether the store defers results or not.. but when I use that store with a StoreRefController? it's fine that no longer works?

That seems odd to me- especially as StoreRefController? specifically has code to copy attributes injected by Observable (which it specifically mentions) over. I'm only suggesting that existing code be moved.

comment:14 Changed 7 years ago by Akira Sudoh

My newly added test case shows that store returning promise does work with StoreRefController?. There was a bug store returning promise does not work with EditStoreRefController?, which I fixed in my recent commit. I guess what you hit is the latter as your test case refers to EditStoreRefListController?, which is an inheritance of EditStoreRefController?.

Though moving the decoration code to when() callback may improve things in some scenarios, as far as I see it delays observe() being set if store returns a promise. Given dojo/store/Observable sets observe() to what query() returns right then, I chose to do the same thing.

comment:15 Changed 7 years ago by Johnathon Harris

Thank you for your insight into this problem asudoh, but EditStoreRefController? and EditStoreRefListController? are inconsequential- my test case only uses those because I've extended some existing test code for the StoreRefController?. I will attach a revised test case to demonstrate this.

You say a 'store returning promise does work with StoreRefController?'. If by 'work' you mean the observe function is available in the results, then no, it does not work. Since this problem can be demonstrated with the plain StoreRefController? I fail to see how your patches to EditStoreRefController? are relevant.

The code in StoreRefController? which tries to copy values onto the result specifically to support dojo/store/Observable (at the end of queryStore) does not work if the store returns a Promise- the observe function cannot be set on a Promise object (running on Chrome 25 at least)!

If you don't want to set observe() on the results during when(), and its demonstrable that they cannot be set before, how would this work otherwise?

Changed 7 years ago by Johnathon Harris

Demonstrates that StoreRefController?? fails to make observe() available if the store returns a Promise, even after using dojo/when. Note that before using when() the Promise returned doesn't have observable() either.

comment:16 Changed 7 years ago by Johnathon Harris

Just attached a refined version of the test case which uses StoreRefController? only. Note that it now demonstrates the Promise object returned does /not/ have the observe() function.

If you debug the code in StoreRefController?.queryStore you can see that while it attempts to copy the functions that Observable added to the Promise, they will not be available.


What more can I do to help here? Do you require more evidence that this is a problem, or do you accept that it is a problem but disagree with the proposed solution?

comment:17 Changed 7 years ago by Akira Sudoh

If by 'work' you mean the observe function is available in the results

No, (to reiterate my early comment to this ticket) for async case the observe() function should be in the promise, instead of in the resolved value (results). Please refer to My newly added test case for how JsonRest/Observable? combination should be used with StoreRefController?.

comment:18 Changed 7 years ago by Johnathon Harris

Thank you for your continued attention to this ticket asudoh, now I can understand where you are coming from. So as I see it the facts are:

While we might disagree on the approach to fixing the problem, it is now clear we agreed this was a problem and there is some solution.

comment:19 Changed 7 years ago by Ed Chatelain

Thanks jmharris and asudoh, I will backport those fixes to 1.8 as soon as I can.

comment:20 Changed 7 years ago by Ed Chatelain

In [30922]:

refs #16480 EditStoreRefController?'s query() supporting Observable. !strict Thanks Akira Sudoh (IBM, CCLA).

comment:21 Changed 7 years ago by Ed Chatelain

In [30923]:

refs #16480 backport to 1.8 EditStoreRefController?'s query() supporting Observable. !strict Thanks Akira Sudoh (IBM, CCLA).

comment:22 Changed 7 years ago by Ed Chatelain

Milestone: 1.91.8.4
Note: See TracTickets for help on using tickets.