Opened 12 years ago

Closed 12 years ago

#4761 closed defect (fixed)

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

Reported by: guest Owned by: Jared Jurkiewicz
Priority: high Milestone:
Component: DojoX Data Version: 0.9
Keywords: QueryReadStore Cc: BlueFire, wolfram
Blocked By: Blocking:

Description

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.

Attachments (7)

fix4761.patch (11.0 KB) - added by wolfram 12 years ago.
this should fix it all, thx for reporting!
dojoQueryReadStore-head-fix4761.patch (15.4 KB) - added by BlueFire 12 years ago.
This includes wolfram's fix and support for the Identity API, thus allowing FilteringSelect? to use the store
dojox.data.QueryReadStore_20071023.patch (12.1 KB) - added by Jared Jurkiewicz 12 years ago.
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.
dojox.data.QueryReadStore_20071024.patch (15.0 KB) - added by wolfram 12 years ago.
The (hopefully) final patch
dojox.data.QueryReadStore_20071026.patch (17.6 KB) - added by BlueFire 12 years ago.
Fixed fetchItemByIdentity
dojox.data.QueryReadStore_20071029.patch (18.2 KB) - added by BlueFire 12 years ago.
Changed code a little and added comments
dojox.data.QueryReadStore_20071030.patch (18.5 KB) - added by Jared Jurkiewicz 12 years ago.
Patch to the patch to fix a test error.

Download all attachments as: .zip

Change History (36)

comment:1 Changed 12 years ago by Jared Jurkiewicz

Owner: changed from Jared Jurkiewicz to wolfram

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

comment:2 Changed 12 years ago by Jared Jurkiewicz

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.

Changed 12 years ago by wolfram

Attachment: fix4761.patch added

this should fix it all, thx for reporting!

Changed 12 years ago by BlueFire

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

comment:3 Changed 12 years ago by BlueFire

Cc: BlueFire added

comment:4 Changed 12 years ago by Jared Jurkiewicz

BlueFire?. Are you CLA approved?

comment:5 Changed 12 years ago by dante

BlueFire? is under CCLA from UPW Innovative IT Losungen GMBH

comment:6 Changed 12 years ago by wolfram

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

comment:7 Changed 12 years ago by Jared Jurkiewicz

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

comment:8 Changed 12 years ago by Jared Jurkiewicz

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?. :-)

comment:9 Changed 12 years ago by 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.

comment:10 Changed 12 years ago by Jared Jurkiewicz

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});}

comment:11 Changed 12 years ago by Jared Jurkiewicz

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.

comment:12 Changed 12 years ago by Jared Jurkiewicz

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

comment:13 Changed 12 years ago by Jared Jurkiewicz

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.

comment:14 Changed 12 years ago by Jared Jurkiewicz

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{

comment:15 Changed 12 years ago by Jared Jurkiewicz

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.

Changed 12 years ago by Jared Jurkiewicz

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.

Changed 12 years ago by wolfram

The (hopefully) final patch

comment:16 Changed 12 years ago by wolfram

Owner: changed from wolfram to Jared Jurkiewicz

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 :-)

comment:17 Changed 12 years ago by Jared Jurkiewicz

Cc: wolfram added

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.

comment:18 Changed 12 years ago by Jared Jurkiewicz

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

comment:19 Changed 12 years ago by BlueFire

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

comment:20 Changed 12 years ago by Jared Jurkiewicz

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

comment:21 Changed 12 years ago by 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.

Changed 12 years ago by BlueFire

Fixed fetchItemByIdentity

comment:22 Changed 12 years ago by Jared Jurkiewicz

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.

comment:23 Changed 12 years ago by 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?

comment:24 Changed 12 years ago by Jared Jurkiewicz

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.

Changed 12 years ago by BlueFire

Changed code a little and added comments

comment:25 Changed 12 years ago by 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.

comment:26 Changed 12 years ago by Jared Jurkiewicz

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.

comment:27 Changed 12 years ago by Jared Jurkiewicz

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!).

comment:28 Changed 12 years ago by Jared Jurkiewicz

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.

Changed 12 years ago by Jared Jurkiewicz

Patch to the patch to fix a test error.

comment:29 Changed 12 years ago by Jared Jurkiewicz

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.