[bug] incorrectly implemented ComboBoxItem._shouldFetchMissingValue()

[antanas.arvasevicius]

Hi,
tested: with SmartClient 10.x 2016-01-15

In ComboBoxItem._shouldFetchMissingValue(), there is a bug then this method lookups inside pickList,
if it found a record and then call _addDataToDisplayFieldCache it must also call updateDisplayValueMap(true); to recalculate _displayValueMap according to newly added "recordFromPickList"
otherwise ComboBoxItem will display "ID" instead of real value.

Snippet from ComboBoxItem.js and where to add a line:

    _shouldFetchMissingValue : function (value, fieldName) {

        if (this.fetchMissingValues == false) return false;
        var ods = this.getOptionDataSource();
        if (ods == null) return false;

        var superShouldFetch = this.Super("_shouldFetchMissingValue", arguments);
        if (superShouldFetch == false) return false;

        if (value != null && ods) {


            var recordFromPickList = this.pickList && this.getPickListRecordForValue(value, fieldName);

            if (recordFromPickList != null) {
                // Store the record from the pickList in our displayField cache

                this._addDataToDisplayFieldCache([recordFromPickList]);
                this._updateSelectedRecord();
                this.updateDisplayValueMap(true); // <- this line must be added
                return false;
            }
        }
        return superShouldFetch;
    },

[antanas.arvasevicius]

Current work around for this, is disable all pick list caching:

isc.PickList.addProperties({cachePickListResults:false});
isc.SelectItem.addProperties({cachePickListResults:false});
isc.ComboBoxItem.addProperties({cachePickListResults:false});

[Isomorphic]

If we find the record in the pickList data set, that record should *already* have been used to update the displayMap, so your call should not be necessary.

Can you show a situation where the displayMap is not updated properly?

[antanas.arvasevicius]

Hi, Isomorphic,
that was not so easy to reproduce but here you go: I've reproduced here exactly how our production application builds up an internal states of SmartClient's components. It's not a 1:1 how our code looks,
but by inspecting in debugger I can exactly create a states as in our production application.

And what it happens shortly:
Two components shares the same PickList , and they couples with the `PickList.formItem` field, if component A shows picklist then PickList.formItem = A; if B, then PickList.formItem = B;

and there is a method `_translateValueFieldValue` which uses `getPickListResultSet` to obtain a cache of pick list only if PickList.formItem == this! so they are not the same (for example someone else opened picklist) then it cannot obtain a resultset.
then fall back is to look thru a bound data source object for cache, and look up there, but if it fails too it return null; (can fail, because submitted different criteria, operation id, or someone invalidated)

I can see that `_translateValueFieldValue` was a solution, but it would be more simpler to just update display value map in all cases then display cache gets updated, and thats all.

PS. I've seen more places where you use `addDataToDisplayFieldCache` but do not updating a display value map.

PPS. I do not know how much do you pay for your developers that they do not run away from that kind of global shared state of unresolvable maze of interactions of SelectItem/ComboBoxItem "heaven".

proof of concept:

    var listGrid;

    var testData = new Array(50).map(function (v, i) {
        return {id: "" + (i + 1), title: "title:" + (i+1)}
    });

    var ds = isc.DataSource.create({
        ID: "ds",
        clientOnly: true,
        fields: [{name: 'id', primaryKey: true}, {name: 'title', type: 'text'}],
        testData: testData
    });

    var form;

    var layout = isc.VLayout.create({
        autoDraw: true,
        members: [
            form = isc.DynamicForm.create({
                fields: [
                    {
                        name: "orderOwner",
                        type: "integer",
                        editorType: "ComboBoxItem",
                        optionDataSource: "ds",
                        valueField: "id",
                        foreignDisplayField: "title"
                    }
                ]
            })
        ]
    });

    var item = form.getItem('orderOwner');

    // * here we simulate value set and dropdown
    item.setValue(1);
    var record = item.getSelectedRecord(); // this method will create a pickList if it's null


    var form2;

    // * simulating in timer as our application will use dialog for new form.
    isc.Timer.setTimeout(function () {
        layout.addMember(
            form2 = isc.DynamicForm.create({
                fields: [
                    {
                        name: "orderOwner",
                        type: "integer",
                        editorType: "ComboBoxItem",
                        optionDataSource: "ds",
                        valueField: "id",
                        foreignDisplayField: "title"
                    }
                ]
            })
        );

        var item2 = form2.getItem('orderOwner');

        // * here we simulate a selection,
        item2.setValue(2);
        var record = item2.getSelectedRecord(); // this will create pickList for second item.


        // * this code will make cached pickList's `formItem` to refere to first ComboBoxItem, so
        // * that way we disable `_translateValueFieldValue` method to work corectly because it won't get a picklist result set.
        // * see the this.getPickListResultSet(): return this.pickList && this.pickList.formItem == this && ... ? list : null;
        item.showPickList();
        item.pickList.hide();


        isc.Timer.setTimeout(function () {

            // * this line will clear cache, as _translateValueFieldValue second pass if getPickListResultSet() fails it trys
            // * to use datasource cache as a backup ( you know datasource cache can be different, due different criteria fetch etc.)
            ds.invalidateCache();

            // * seting the value
            item2.setValue(20);

            // RESULT: boom, failed. cannot map 20 to correct display value.

        }, 1000);

    }, 2000);

[Isomorphic]

Thanks for the work on creating this test case. It's very strange code, especially the trick to force creation of a pickList before the user has accessed the FormItem. However, we'll take your word for it that this can somehow arise in a real application that has normal usage, even though it's never been reported before.

About the "maze" of shared pickLists - we had no choice but to share pickLists because we first implemented this when browsers were just too slow for each SelectItem or ComboBoxItem to have it's own pickList widget. We plan to get ride of sharing pickLists in favor of a data-level caching mechanism. However, while this will be *less* complicated, there's still a lot of inherent complexity here.

[Isomorphic]

Actually, hang on, on review, you've got two calls using internal, unsupported APIs

   item.showPickList();
   item.pickList.hide();

showPickList is not documented, and selectItem.pickList is not a documented autoChild. In both cases we intentionally did not document these APIs precisely because manipulations of this kind could mess up our pickList sharing logic.

If you have calls like these in your application, try eliminating them and see if that solves the problem.

If you don't have calls like these, unfortunately, we need a test case that doesn't use undocumented APIs. If this really can be reproduced without using undocumented APIs, maybe you could substitute a manual action for that step (like having the end user click in the first form field).

[antanas.arvasevicius]

Hi,

no problem, there are many ways to exploit this, e.g. you can just create one more component and do getSelectedRecord(); as this method internally creates a pickList property.


Just to be clear here is our real case in application:
1) user opens orders page
2) filter orders by customer
3) open record details (its dialog with +10 tab's)
4) customer field shows ID instead of customer name.
---
* customer datasource is used extensively.
** also using getSelectedRecord() as we need a full selected object
===


here is code without any internal API:

    var listGrid;

    var testData = new Array(50).map(function (v, i) {
        return {id: "" + (i + 1), title: "title:" + (i+1)}
    });

    var ds = isc.DataSource.create({
        ID: "ds",
        clientOnly: true,
        fields: [{name: 'id', primaryKey: true}, {name: 'title', type: 'text'}],
        testData: testData
    });

    var form;

    var layout = isc.VLayout.create({
        autoDraw: true,
        members: [
            form = isc.DynamicForm.create({
                fields: [
                    {
                        name: "orderOwner",
                        type: "integer",
                        editorType: "ComboBoxItem",
                        optionDataSource: "ds",
                        valueField: "id",
                        foreignDisplayField: "title"
                    },
                    {
                        name: "orderOwner3",
                        type: "integer",
                        editorType: "ComboBoxItem",
                        optionDataSource: "ds",
                        valueField: "id",
                        foreignDisplayField: "title"
                    }
                ]
            })
        ]
    });

    var item = form.getItem('orderOwner');
    var item3 = form.getItem('orderOwner3');

    // * here we simulate value set and dropdown
    item.setValue(1);
    var record = item.getSelectedRecord(); // this method will create a pickList if it's null


    var form2;

    // * simulating in timer as our application will use dialog for new form.
    isc.Timer.setTimeout(function () {
        layout.addMember(
            form2 = isc.DynamicForm.create({
                fields: [
                    {
                        name: "orderOwner",
                        type: "integer",
                        editorType: "ComboBoxItem",
                        optionDataSource: "ds",
                        valueField: "id",
                        foreignDisplayField: "title"
                    }
                ]
            })
        );

        var item2 = form2.getItem('orderOwner');

        // * here we simulate a selection,
        item2.setValue(2);
        var record = item2.getSelectedRecord(); // this will create pickList for second item.


        // * this code will make cached pickList's `formItem` to refere to first ComboBoxItem, so
        // * that way we disable `_translateValueFieldValue` method to work corectly because it won't get a picklist result set.
        // * see the this.getPickListResultSet(): return this.pickList && this.pickList.formItem == this && ... ? list : null;
        item3.setValue(3);
        item3.getSelectedRecord(); // this will create a pickList internally for item 3 (so that statement above will be false later)


        isc.Timer.setTimeout(function () {

            // * this line will clear cache, as _translateValueFieldValue second pass if getPickListResultSet() fails it trys
            // * to use datasource cache as a backup ( you know datasource cache can be different, due different criteria fetch etc.)
            ds.invalidateCache();

            // * seting the value
            item2.setValue(20);

            // RESULT: boom, failed. cannot map 20 to correct display value.

        }, 1000);

    }, 2000);

Suggestion for fix:

could you add logic into `_modifyDataInDisplayFieldCache` method to reflect changes to _displayFieldValueMap instantly? Then where will be no need to call updateDisplayValueMap() everythere.

[antanas.arvasevicius]

PS. as I have said, test case was used to simulate an internal state of our application, and we are not using undocumented API's in production.

[Isomorphic]

Thanks, we'll take a look at this.

In your understanding of the issue, is it necessary to call getSelectedRecord() on a SelectItem / ComboBoxItem where the pickList has never been shown in order to create the problem?

[antanas.arvasevicius]

Yes, as getSelectedRecord() will create (new or will grab from shared pool) "this.pickList" property , and it will set that shared pick list "formItem" to that component.

That way then we call "item2.setValue(20);", it will go thru _translateValueFieldValue(), and it calls getPickListResultSet() which returns an null (because shared picklist refers to not this element), so it will try to lookup in
datasource cache, and if it is also null, then issue with a test case occurs.


We'll use getSelectedRecord() sparingly in our application, because we need a full object of selected record (some logic is based on selected customer type and other properties).

I think you could simulate that also without getSelectedRecord() but only with manual interaction (as with showPickList())

As I said before, it seems that you could remove _translateValueFieldValue competely if you'll maintain _displayValueMap in tact of display cache changes ( standard mapKey() will do a job)

[Isomorphic]

Right, we understand the mechanics, we were just trying to figure out how you're the first to hit this :) It sounds like possibly it's related to the timing of your getSelectedRecord() calls, but it's not clear. Either way, we'll fix the case you showed and think through related cases.

[antanas.arvasevicius]

Hi, thank you!

I can suspect that you are right it's due getSelectedRecord() timing, I can share with you our implementation of one utility class which we are using to unify behaviour.
Reason for this, that we actually wanted a records not an ID then SelectItem/ComboBoxItem changed by user or changed by record bound to form.
So we've implemented this solution for these cases: http://pastebin.com/kEd8Gfgf
As developers do not need to worry how exactly to get selected record.
So this utility will use getSelectedRecord() excessively. And btw this is a kind of getter, so by convention it must be side effect free function. Who could know that it must be called by proper timing or at some conditions only :)

[Isomorphic]

Ah, great, thanks for the extra context. A helper method like that makes perfect sense, and that definitely explains why you would be the only one to find this issue. We'll have it resolved shortly.

[Isomorphic]

We've made basically the change you suggested here of calling 'updateDisplayValueMap' after adding the record to the display-field-cache, and adjusted another case where addDataToDisplayFieldCache was called without doing this.
A deeper rework of this whole area (including possibly dropping the shared pickList altogether and moving to an approach based around avoiding unnecessary fetches via more sophisticated data cacheing logic only) will probably occur at some point, but this is not a priority right now.

Thanks for the help here pinning down exactly what you were seeing and showing us clear test cases / talking through your usage.

You should see the fix in builds from 5.1 and 6.0 dated Jan 26 or above

Regards
Isomorphic Software

[antanas.arvasevicius]

Thanks!
Is there any advantages nowadays to use shared picklist and what drawbacks could occur if we'll use cachePickListResults=false; to disable it everywhere?