[CLA] [patch] Two QueryReadStore errors (#4597)

[guest]

The first error is actually a typo: the example HTML file contains the following line, in the PRE block:

request.query = {q:request.query.name, start:request.start, count:request.count}; 

while it should actually read

request.serverQuery = ...

The second error regards server-side pagination, which I believe is not working. I think the problem is that also simpleFetch is trying to do pagination, specifically where it says

subset = items.slice(startIndex, endIndex);

Removing that row from simpleFetch made pagination work fine - but clearly, it broke all the other components using it.

[jaredj]

Since this store is primarily yours, can you look at these issues and attach a patch for them?

[jaredj]

Obviously, you can't change simpleFetch to remove that subsetting line, as it is needed for all the other stores importing it. You may need to copy that function and modify it to work with your backend.

[wolfram]

this should fix it all, thx for reporting!

[BlueFire]

This includes wolfram's fix and support for the Identity API, thus allowing FilteringSelect? to use the store

[jaredj]

BlueFire?. Are you CLA approved?

[dante]

BlueFire? is under CCLA from UPW Innovative IT Losungen GMBH

[jaredj]

Ah, good to know, thanks. I'll review and try to get in today!

[jaredj]

The fetchItemByIdentity implementation doesn't conform to the Identity API as defined in the dojo 0.9 book at: http://dojotoolkit.org/book/dojo-book-0-9/part-3-programmatic-dijit-and-dojo/what-dojo-data/dojo-data-design-and-apis/ident

Nor to the API file defined at: dojo/data/api/Identity.js

So ... that isn't going to work with FilteringSelect?. :-)

[BlueFire]

Will have a look at that. For me it works so far as I got no errors from the FilteringSelect? and the data is transferred correctly to the server and autocompetition seems to work. I noticed yesterday there seems to be a problem with FilteringSelect?.setValue() which I will check.

[jaredj]

As for Wolfram's patch .... it doesn't pass UT.

PASSED test: testReadApi_fetch_client_paging

_AssertFailure: http://gambit.dyndns.org:8088/sinai/dojo_query/dojo/_base/_loader/bootstrap.js:281 doh._AssertFailure: assertEqual() failed: expected |Armed Forces Europe| but got |Alabama|

doh._AssertFailure

ERROR IN:

(function testReadApi_fetch_server_paging(t) {var store = dojox.data.tests.stores.QueryReadStore?.getStore();var lastRequestHash = null;var d = new doh.Deferred; function onComplete(items, request) {t.assertEqual(10, items.length);lastRequestHash = store.lastRequestHash;firstItems = items; function onComplete1(items, request) {t.assertEqual(5, items.length);t.assertTrue(lastRequestHash != store.lastRequestHash);t.assertEqual(store.getValue(firstItems[5], "name"), store.getValue(items[0], "name"));t.assertEqual(store.getValue(firstItems[6], "name"), store.getValue(items[1], "name"));t.assertEqual(store.getValue(firstItems[7], "name"), store.getValue(items[2], "name"));t.assertEqual(store.getValue(firstItems[8], "name"), store.getValue(items[3], "name"));t.assertEqual(store.getValue(firstItems[9], "name"), store.getValue(items[4], "name"));d.callback(true);} store.doClientPaging = false;store.fetch({start:5, count:5, onComplete:onComplete1, onError:onError});}

[jaredj]

BlueFire?,

The point where FilteringSelect? uses fetchItemByIdentity is their setValue function. Since your patch impl doesn't match the API, that's probably why it's failing.

[jaredj]

Wolfram,

Another side note ... is that if you're doing serverside paging, you should also probably do server-side sorting as well. As sorting should occur before the pages are generated.

Sincerely, -- Jared Jurkiewicz

[jaredj]

It doesn't look like your serverside query is actually passing the query to the server in that test: GET http://gambit.dyndns.org:8088/sinai/dojo_query/dojox/data/tests/stores/QueryReadStore.php (391ms)

Is what I see in Firebug, doesn't show query params.

[jaredj]

BlueFire?,

I think you need to do the following to get the serverside paging working right:

In _fetch:

// // The serverQuery contains more data than the query, so they might differ! //

var serverQuery = typeof requestserverQuery?=="undefined" ? request.query : request.serverQuery; //Need to add start and count if(!request.store.doClientPaging){

serverQuery = serverQuery{}; serverQuerystart? = request.start?request.start:0; serverQuerycount? = request.count?request.count:undefined;

} // Compare the last query and the current query by simply json-encoding them, // so we dont have to do any deep object compare ... is there some dojo.areObjectsEqual()??? if(this.doClientPaging && this._lastServerQuery!==null &&

dojo.toJson(serverQuery)==dojo.toJson(this._lastServerQuery) ){ fetchHandler(this._items, request);

}else{

[jaredj]

Bah the paste didn't work very well. I'm attaching a patch that includes my suggested change to the serverside paging. Please look at it and verify it is what you were intending to do.

[jaredj]

Patch to Wolfram's patch (cumulative), that I believe fixes serverside paging. wolfram to review and verify that was his intention with his serverside paging code.

[wolfram]

The (hopefully) final patch

[wolfram]

Thanks guys for enhancing the patch. I tested it in my current app and also fixed tiny formatting stuff. Now it could go in imho. Jared, if you would be so nice to commit it.

btw. the sorting has to happen on the server, yes. And i think the user has to send those data by hand to the server, so to say add them to serverQuery handish, such as any kind of data.

i assign it to you again jaredj, for commiting it. thx

a side note: something like git would really be helpful with many patches :-)

[jaredj]

Patch cannot be committed. Implementation of identity functions are wrong. Waiting on BlueFire? to provide a corrected patch for his addition. Only other option untilo the idenity issue is corrected is to strip out the identity add-in until it's correctly implemented.

[jaredj]

Make that: Only other option until the identity issue is corrected is to strip out the identity add-in until it's correctly implemented.

[BlueFire]

Taking care of the ID function is on my todo for tomorrow.

[jaredj]

Awesome. will check back on the patch tomorrow and hold off pulling out the identity support...

[BlueFire]

Hopefully that last patch fixes the fetchItemByIdentity issue. I didn't find regression tests for the Identity API, but I extended the QueryReadStore? test to include a little testing of FilteringSelect?.setValue() which seems to work fine.

[BlueFire]

Fixed fetchItemByIdentity

[jaredj]

Thanks for the provided patch. I did a quick scan and have a few more comments, I'm afraid. :-( That's not quite a correct implementation either. Stores that cannot find an item that matches an identity should call onItem() with a null. Not calling the callback ever on a non-match is bad. That, thankfully, is a trivial fix, though. Just remove the null check and let onItem be called with null. This is noted in the API docs for Identity: http://dojotoolkit.org/book/dojo-book-0-9/part-3-programmatic-dijit-and-dojo/what-dojo-data/dojo-data-design-and-apis/ident

And a minor nit ... shouldn't it also go back to the service to do the lookup in case there hasn't been a fetch that loaded the item yet? For example, someone does a fetchItemByIdentity without ever having called a fetch. It's a possible scenario.

[BlueFire]

I read that part about returning the returning null and then obviously failed to implement it... grrr

My intention was that it checks if the last load (if there was one) yields an item with the specified identifier. In this case I ignore that the server might have chosen to add/remove/change IDs on the items. Only if there is no matching item found locally, it goes back to the server.

I will have more time to work on the patch next monday if you are fine with that. The fixes you noted shouldn't be difficult.

A difference in behaviour between the QueryReadStore? and the ItemFileReadStore? is, that the QueryReadStore? doesn't at all serialize requests using a request queue. My app has quite a lot of combos and they fill up just fine, so I suppose it's not really a problem. How do you see that?

[jaredj]

Perfectly fine with that. I'm very grateful for all the work you are putting into it. I'm sorry I'm being so nitpicky about it. For what it's worth, I think the patch in general looks good with what you are targeting to do here for Identity (just need that null fix and a bit of work on fallback server lookup). Avoiding a trip to the server if possible is a great thing to be doing (check last query to see if you already have it); it just does probably need the fall back of 'if not in local request data, ask the server for it' to really be functionally complete with regards to the Identity API. That very scenario is why the fetchItemByIdentity API call is async to begin with. :-)

The request queue in ItemFile?*Store came about because it does pull everything over locally then process it, so we had to be careful about staging the fetch so that only one server request happens and no data clobbering occurs (The queue only really does anything on initial load of store data). Since this store normally asks the server to do all the querying anyway, you shouldn't need the queue. In that sense, this store is much simpler to implement than ItemFile?*Store was. :-) You don't have to do as much async queuing. What you're doing now should be quite fine, I do believe.

Again, thank you for all the work and putting up with my nitpicks. If all is good next Monday on your followup changes, it'll get committed that day.

[BlueFire]

Changed code a little and added comments

[BlueFire]

Hi Jared,

I went through the patch again respecting your comments. I believe the last patch already did not have any of the problems you mentioned, but I changed the code a little and added comments to explain.

Fire "null" on not found:

This happened in the last version as well as now. The check for "null" in the last patch was only in the part where

we looked up if we find something in the _itemsById index. If nothing was found (item == null), then we went on to fetching the data from the backend. If nothing was found there we fire with null.

Initial fetch when looking up by id:

Already happened before as well. We check if _itemsById exists and only try to look something up in it if it does (which it only will after a fetch). If it does not exist, we fall through to initiating a fetch.

[jaredj]

This patch looks a lit better and is passing UT cleanly. I hit another bug in non-QueryStore? code I'll fix, and there's an error in the QuertReadStore?.html test file with the fetch button failing with the error:

GET http://gambit.dyndns.org:8088/sinai/dojo_query/dojox/data/tests/stores/QueryReadStore.php?id= (187ms)bootstrap.js (line 776) dojo.query("#fetchForm")[0].pagingStart has no properties [Break on this error] request.start = dojo.query("#fetchForm")[0].pagingStart.value; I think this should be an easy fix, will look at it.

[jaredj]

I found the problem. When you added in your filteringSelect widget test, you copied the form tag used below for the other tests and forgot to change the ID. This in turn caused the dojo.query() API to get the wrong form, then the accessors of the fields of course failed.

Will post a patch that has all your fixed, plus this. I'm also going to do a small disablement of the QueryReadStore? UT if the file is loaded from disk (local load testing), as that will naturally fail (no PHP server!).

[jaredj]

Unit tested on:

Firefox 2.0.0.8 IE 6 IE 7 Safari B3 Seamonkey 1.1.2

TODOs still needed at some point: Adding in UT for Identity.

[jaredj]

Patch to the patch to fix a test error.

[jaredj]

This was committed in: http://trac.dojotoolkit.org/changeset/11233