Opened 10 years ago

Closed 10 years ago

Last modified 8 years ago

#9023 closed defect (fixed)

FilteringSelect: forces unnecessary loadItem during _setValueAttr

Reported by: Jaanus Heeringson Owned by: bill
Priority: high Milestone: 1.4
Component: Dijit - Form Version: 1.3.0rc2
Keywords: filteringSelect, isItemLoaded, jsonRestStore Cc: Jared Jurkiewicz
Blocked By: Blocking:

Description

Even if the stores (jsonRestStore) partially loaded items hold the neceessary data for the filteringSelect (identity & label) it triggers isItemLoaded and store.loadItem during _setValueAttr. Looking at the source I really can't see any good reason for it since no additional data is required.

By adding an additional check to see if the returned item contains id & label properties this before checking isItemLoaded we avoid unnecessary trips to the serverside datastore.

Change History (12)

comment:1 Changed 10 years ago by bill

I'm not sure what you mean. AFAIK there's no concept of partially loaded items in the dojo.data spec. Either an item is [fully] loaded or it isn't, and if it isn't [fully] loaded then we can't assume or check that some of the attributes are loaded.

If you are talking about a feature specific to jsonRestStore then FilteringSelect can't use that, since it must use the standard interface so it can connect to any data store.

See source:dojo/trunk/data/api/Read.js.

comment:2 Changed 10 years ago by Jaanus Heeringson

What I'm referring to as a partially loaded item is an item that contains some data fields and a json reference to itself:

{id:1,label:'somelabel',$ref:1}

In these cases, even if all the data needed for filteringSelect is present (id,label), filteringSelect still forces the store to load the data even if it's unneccesary.

A user case for this is when using stores that potentially contain lots of data fields with filteringSelect. You then won't need to download the entire store initially - a small part of it would still fulfill your needs.

comment:3 Changed 10 years ago by bill

Cc: Jared Jurkiewicz added
Summary: FilteringSelect forces unnecessary loadItem during _setValueAttrFilteringSelect: forces unnecessary loadItem during _setValueAttr

OK, I haven't worked with JsonRestStore enough to be familiar with that store-specific feature (about a single item being split into two parts, one of which is lazy loaded), but I don't see any way to take advantage of that from FilteringSelect, since, as I said above, FilteringSelect must use the standard interface so it can connect to any data store. The dojo.data API would need to have an isPropertyLoaded() method.

Having said that though, the loadItem() call in ource:dijit/trunk/form/FilteringSelect.js#L133 seems unnecessary since it comes after a fetchItemByIdentity() call, which, according to the spec, already guarantees that the item has been loaded:

//	The *onItem* parameter.
//		Function(item)
//		The onItem parameter is the callback to invoke **when the item has been loaded**.  It takes only one
//		parameter, the item located, or null if none found.

That loadItem() call was added in [9352] but I don't understand why. Jared, can you explain? ISTM that JsonRestStore is breaking the spec by returning a half-loaded item and that isn't something that FilteringSpec should be worrying about.

comment:4 Changed 10 years ago by bill

comment:5 Changed 10 years ago by bill

See also #9059, should fix both of these at the same time.

comment:6 Changed 10 years ago by Jarrod Carlson

This brings up another issue, then. If the Identity API implies that fetchItemByIdentity will guarantee an item is returned for which subsequent calls to store.isItemLoaded(item) will return true, then there might be a bug in dojox.data.ServiceStore#fetchItemByIdentity?.

Take a look at:source:dojox/data/ServiceStore.js#L362. If the item requested is in cache, loaded or not, it is returned as is. The issue is that if you make a call to fetchItemByIdentity and do not supply an onItem handler, the item is forcibly loaded from the server, regardless of whether the item is already loaded or not.

FilteringSelect? provides an onItem handler, which means that the FilteringSelect? cannot assume the returned item will be fully loaded. Thus, it cannot be assumed that the returned item will have the attribute necessary to populate the display value. Remember that the displayed value may be an attribute other than the item's label by setting FilteringSelect?'s labelAttr property.

I'm going to register this as a new defect, just because I think it's unclear what the correct behavior for the ServiceStore? would be.

comment:7 Changed 10 years ago by Jarrod Carlson

See #9061

comment:8 Changed 10 years ago by Jared Jurkiewicz

From vague memory (2 years ago!) that call to isItemLoaded/loadItem was added as a paranoia check in the case some store decided to not return a fully inflated item (such as a cached unloaded handle). In reality, it turns out this was a good idea as ServiceStore? actually did just that.

comment:9 Changed 10 years ago by Jarrod Carlson

Well +2 points for ridiculously smart forward-thinking...

comment:10 Changed 10 years ago by bill

Milestone: tbd1.4
Owner: set to bill
Status: newassigned

Unfortunately (from your perspective) #9061 changed ServiceStore so that fetchItemByIdentity() returns a fully loaded item. This is actually obeying the dojo.data API contract. So, I'm going to remove the loadItem() call from FilteringSelect but it won't help you.

I think there's a larger issue that data stores are queried at all during page load. It would be much better if we only needed to query them when a user starts typing into a field. Or if they cached the needed data on page load... anyway, some way so that there aren't a slew of XHR requests on page load for pages that have FilteringSelect widgets with non-blank values.

comment:11 Changed 10 years ago by bill

Resolution: fixed
Status: assignedclosed

loadItem() call removed in [17225]. Marking this as "fixed" although that may be a misnomer; see above comment.

comment:12 Changed 8 years ago by bill

Component: DijitDijit - Form
Note: See TracTickets for help on using tickets.