[4.1 RC1] Models and idgen

[westy]

Hi,

I don't think the function generated by buildRecordDataExtractor is advanced enough, since it blindly sets the idProperty to defaultValue rather than seeing if it's already been set.

As such, in extractData you create the model instance and your id generator assigns the correct id, then convertRecordData gets called, and overwrites the value with the default, empty string in most cases.

Is anyone using id generators successfully with 4.1?


I think buildRecordDataExtractor should be doing an applyIf - I'll try and sort an override out tomorrow, and a test case for a bug report, but thought would post of gather some thoughts before I head home for the evening...

Example generated code:

var me = this,
    fields = me.model.prototype.fields,
    value, internalId, __field0 = fields.get("id"),
    __field1 = fields.get("config"),
    __field2 = fields.get("isCategoryNode"),
    __field3 = fields.get("entityId"),
    __field4 = fields.get("entityType"),
    __field5 = fields.get("text"),
    __field6 = fields.get("additionalData"),
    __field7 = fields.get("parentId"),
    __field8 = fields.get("index"),
    __field9 = fields.get("depth"),
    __field10 = fields.get("expanded"),
    __field11 = fields.get("expandable"),
    __field12 = fields.get("checked"),
    __field13 = fields.get("leaf"),
    __field14 = fields.get("cls"),
    __field15 = fields.get("iconCls"),
    __field16 = fields.get("icon"),
    __field17 = fields.get("root"),
    __field18 = fields.get("isLast"),
    __field19 = fields.get("isFirst"),
    __field20 = fields.get("allowDrop"),
    __field21 = fields.get("allowDrag"),
    __field22 = fields.get("loaded"),
    __field23 = fields.get("loading"),
    __field24 = fields.get("href"),
    __field25 = fields.get("hrefTarget"),
    __field26 = fields.get("qtip"),
    __field27 = fields.get("qtitle"),
    __field28 = fields.get("children");
return function(dest, source, record) {
    dest["id"] = (source["id"] === undefined) ? __field0.defaultValue : source["id"];
    dest["config"] = source["config"];
    dest["isCategoryNode"] = __field2.convert((source["isCategoryNode"] === undefined) ? __field2.defaultValue : source["isCategoryNode"], record);
    dest["entityId"] = __field3.convert((source["entityId"] === undefined) ? __field3.defaultValue : source["entityId"], record);
    dest["entityType"] = __field4.convert((source["entityType"] === undefined) ? __field4.defaultValue : source["entityType"], record);
    dest["text"] = __field5.convert((source["text"] === undefined) ? __field5.defaultValue : source["text"], record);
    dest["additionalData"] = source["additionalData"];
    dest["parentId"] = (source["parentId"] === undefined) ? __field7.defaultValue : source["parentId"];
    dest["index"] = __field8.convert((source["index"] === undefined) ? __field8.defaultValue : source["index"], record);
    dest["depth"] = __field9.convert((source["depth"] === undefined) ? __field9.defaultValue : source["depth"], record);
    dest["expanded"] = __field10.convert((source["expanded"] === undefined) ? __field10.defaultValue : source["expanded"], record);
    dest["expandable"] = __field11.convert((source["expandable"] === undefined) ? __field11.defaultValue : source["expandable"], record);
    dest["checked"] = (source["checked"] === undefined) ? __field12.defaultValue : source["checked"];
    dest["leaf"] = __field13.convert((source["leaf"] === undefined) ? __field13.defaultValue : source["leaf"], record);
    dest["cls"] = __field14.convert((source["cls"] === undefined) ? __field14.defaultValue : source["cls"], record);
    dest["iconCls"] = __field15.convert((source["iconCls"] === undefined) ? __field15.defaultValue : source["iconCls"], record);
    dest["icon"] = __field16.convert((source["icon"] === undefined) ? __field16.defaultValue : source["icon"], record);
    dest["root"] = __field17.convert((source["root"] === undefined) ? __field17.defaultValue : source["root"], record);
    dest["isLast"] = __field18.convert((source["isLast"] === undefined) ? __field18.defaultValue : source["isLast"], record);
    dest["isFirst"] = __field19.convert((source["isFirst"] === undefined) ? __field19.defaultValue : source["isFirst"], record);
    dest["allowDrop"] = __field20.convert((source["allowDrop"] === undefined) ? __field20.defaultValue : source["allowDrop"], record);
    dest["allowDrag"] = __field21.convert((source["allowDrag"] === undefined) ? __field21.defaultValue : source["allowDrag"], record);
    dest["loaded"] = __field22.convert((source["loaded"] === undefined) ? __field22.defaultValue : source["loaded"], record);
    dest["loading"] = __field23.convert((source["loading"] === undefined) ? __field23.defaultValue : source["loading"], record);
    dest["href"] = __field24.convert((source["href"] === undefined) ? __field24.defaultValue : source["href"], record);
    dest["hrefTarget"] = __field25.convert((source["hrefTarget"] === undefined) ? __field25.defaultValue : source["hrefTarget"], record);
    dest["qtip"] = __field26.convert((source["qtip"] === undefined) ? __field26.defaultValue : source["qtip"], record);
    dest["qtitle"] = __field27.convert((source["qtitle"] === undefined) ? __field27.defaultValue : source["qtitle"], record);
    dest["children"] = (source["children"] === undefined) ? __field28.defaultValue : source["children"];
    if (record && (internalId = source["phantomId"])) {
        record.internalId = internalId;
    }
};

As you can see, this will always set id to something, either what has been sent back in the data (source), or the defaultValue.

No good enough IMO.
Would welcome any insights.

Cheers,
Westy

[dongryphon]

Sounds like you are correct ... how are you getting that data? Is this on a Model instance save or a store save?

[westy]

Neither, it's from readRecords; reading data from a server response into a store, creating a model instance for each record.

[westy]

Might also be worth mentioning that have also seen issues with using defaultValue's of undefined.

This used to mean that the resultant record would not have the member (which is often nice, probably due to using apply or applyIf I assume), where as now it does always have a member with a value of undefined.

Not a problem, now I know it's happening, but a change none the less.

[westy]

Hmm, pretty complex this isn't it; clever no doubt, but a pain to follow

Not sure whether to override Ext.data.reader.Reader.buildRecordDataExtractor or Ext.data.reader.Json.createFieldAccessExpression...

Suspect the latter, although the former probably easier.


Edit: Hmm, not sure attacking it from createFieldAccessExpression possible, since it hasn't got access to the dest to see what's already there. Will tinker with buildRecordDataExtractor...

[westy]

Ok, got an override that works for me, for now, but suspect there's probably a more efficient way of sorting it out.

Ext.require('Ext.data.reader.Reader',
    function() {
        Ext.override(Ext.data.reader.Reader, {
            /**
             * Override of this method to get around issues of overwriting generated ids.
             * See: http://www.sencha.com/forum/showthread.php?192649-4.1-RC1-Models-and-idgen
             */
             buildRecordDataExtractor: function() {
                var me = this,
                    modelProto = me.model.prototype,
                    clientIdProp = modelProto.clientIdProperty,
                    fields = modelProto.fields.items,
                    numFields = fields.length,
                    fieldVarName = [],
                    prefix = '__field',
                    varName,
                    i = 0,
                    field,
                    code = [
                        'var me = this,\n',
                        '    fields = me.model.prototype.fields,\n',
                        '    value,\n',
                        '    internalId'
                    ];


                for (; i < numFields; i++) {
                    field = fields[i];
                    fieldVarName[i] = '__field' + i;
                    code.push(',\n    ', fieldVarName[i], ' = fields.get("', field.name, '")');
                }
                code.push(';\n\n    return function(dest, source, record) {\n');


                for (i = 0; i < numFields; i++) {
                    field = fields[i];
                    varName = fieldVarName[i];
                    // createFieldAccessExpression must be implemented in subclasses to extract data from the source object in the correct way.
                    // <WestyFix>
                    // code.push('        dest["' + field.name + '"]', ' = ', me.createFieldAccessExpression(field, varName, 'source'), ';\n');
                    code.push('        Ext.applyIf(dest, {\n');
                    code.push('            ' + field.name + ': ', me.createFieldAccessExpression(field, varName, 'source'), '\n');
                    code.push('        });\n');
                    // </WestyFix>
                }


                // set the client id as the internalId of the record.
                // clientId handles the case where a client side record did not previously exist on the server,
                // so the server is passing back a client id that can be used to pair the server side record up with the client record
                if (clientIdProp) {
                    code.push('        if (record && (internalId = ' + me.createFieldAccessExpression({mapping: clientIdProp}, null, 'source') + ')) {\n');
                    code.push('            record.internalId = internalId;\n        }\n');
                }


                code.push('    };');


                // Here we are creating a new Function and invoking it immediately in the scope of this Reader
                // It declares several vars capturing the configured context of this Reader, and returns a function
                // which, when passed a record data object, a raw data row in the format this Reader is configured to read,
                // and the record which is being created, will populate the record's data object from the raw row data.
                return Ext.functionFactory(code.join('')).call(me);
            }
        });
    }
);

Gives something like:

var me = this,
    fields = me.model.prototype.fields,
    value, internalId, __field0 = fields.get("id"),
    __field1 = fields.get("config"),
    __field2 = fields.get("isCategoryNode"),
    __field3 = fields.get("entityId"),
    __field4 = fields.get("entityType"),
    __field5 = fields.get("text"),
    __field6 = fields.get("additionalData"),
    __field7 = fields.get("parentId"),
    __field8 = fields.get("index"),
    __field9 = fields.get("depth"),
    __field10 = fields.get("expanded"),
    __field11 = fields.get("expandable"),
    __field12 = fields.get("checked"),
    __field13 = fields.get("leaf"),
    __field14 = fields.get("cls"),
    __field15 = fields.get("iconCls"),
    __field16 = fields.get("icon"),
    __field17 = fields.get("root"),
    __field18 = fields.get("isLast"),
    __field19 = fields.get("isFirst"),
    __field20 = fields.get("allowDrop"),
    __field21 = fields.get("allowDrag"),
    __field22 = fields.get("loaded"),
    __field23 = fields.get("loading"),
    __field24 = fields.get("href"),
    __field25 = fields.get("hrefTarget"),
    __field26 = fields.get("qtip"),
    __field27 = fields.get("qtitle"),
    __field28 = fields.get("children");
return function(dest, source, record) {
    Ext.applyIf(dest, {
        id: (source["id"] === undefined) ? __field0.defaultValue : source["id"]
    });
    Ext.applyIf(dest, {
        config: source["config"]
    });
    Ext.applyIf(dest, {
        isCategoryNode: __field2.convert((source["isCategoryNode"] === undefined) ? __field2.defaultValue : source["isCategoryNode"], record)
    });
    Ext.applyIf(dest, {
        entityId: __field3.convert((source["entityId"] === undefined) ? __field3.defaultValue : source["entityId"], record)
    });
    Ext.applyIf(dest, {
        entityType: __field4.convert((source["entityType"] === undefined) ? __field4.defaultValue : source["entityType"], record)
    });
    Ext.applyIf(dest, {
        text: __field5.convert((source["text"] === undefined) ? __field5.defaultValue : source["text"], record)
    });
    Ext.applyIf(dest, {
        additionalData: source["additionalData"]
    });
    Ext.applyIf(dest, {
        parentId: (source["parentId"] === undefined) ? __field7.defaultValue : source["parentId"]
    });
    Ext.applyIf(dest, {
        index: __field8.convert((source["index"] === undefined) ? __field8.defaultValue : source["index"], record)
    });
    Ext.applyIf(dest, {
        depth: __field9.convert((source["depth"] === undefined) ? __field9.defaultValue : source["depth"], record)
    });
    Ext.applyIf(dest, {
        expanded: __field10.convert((source["expanded"] === undefined) ? __field10.defaultValue : source["expanded"], record)
    });
    Ext.applyIf(dest, {
        expandable: __field11.convert((source["expandable"] === undefined) ? __field11.defaultValue : source["expandable"], record)
    });
    Ext.applyIf(dest, {
        checked: (source["checked"] === undefined) ? __field12.defaultValue : source["checked"]
    });
    Ext.applyIf(dest, {
        leaf: __field13.convert((source["leaf"] === undefined) ? __field13.defaultValue : source["leaf"], record)
    });
    Ext.applyIf(dest, {
        cls: __field14.convert((source["cls"] === undefined) ? __field14.defaultValue : source["cls"], record)
    });
    Ext.applyIf(dest, {
        iconCls: __field15.convert((source["iconCls"] === undefined) ? __field15.defaultValue : source["iconCls"], record)
    });
    Ext.applyIf(dest, {
        icon: __field16.convert((source["icon"] === undefined) ? __field16.defaultValue : source["icon"], record)
    });
    Ext.applyIf(dest, {
        root: __field17.convert((source["root"] === undefined) ? __field17.defaultValue : source["root"], record)
    });
    Ext.applyIf(dest, {
        isLast: __field18.convert((source["isLast"] === undefined) ? __field18.defaultValue : source["isLast"], record)
    });
    Ext.applyIf(dest, {
        isFirst: __field19.convert((source["isFirst"] === undefined) ? __field19.defaultValue : source["isFirst"], record)
    });
    Ext.applyIf(dest, {
        allowDrop: __field20.convert((source["allowDrop"] === undefined) ? __field20.defaultValue : source["allowDrop"], record)
    });
    Ext.applyIf(dest, {
        allowDrag: __field21.convert((source["allowDrag"] === undefined) ? __field21.defaultValue : source["allowDrag"], record)
    });
    Ext.applyIf(dest, {
        loaded: __field22.convert((source["loaded"] === undefined) ? __field22.defaultValue : source["loaded"], record)
    });
    Ext.applyIf(dest, {
        loading: __field23.convert((source["loading"] === undefined) ? __field23.defaultValue : source["loading"], record)
    });
    Ext.applyIf(dest, {
        href: __field24.convert((source["href"] === undefined) ? __field24.defaultValue : source["href"], record)
    });
    Ext.applyIf(dest, {
        hrefTarget: __field25.convert((source["hrefTarget"] === undefined) ? __field25.defaultValue : source["hrefTarget"], record)
    });
    Ext.applyIf(dest, {
        qtip: __field26.convert((source["qtip"] === undefined) ? __field26.defaultValue : source["qtip"], record)
    });
    Ext.applyIf(dest, {
        qtitle: __field27.convert((source["qtitle"] === undefined) ? __field27.defaultValue : source["qtitle"], record)
    });
    Ext.applyIf(dest, {
        children: (source["children"] === undefined) ? __field28.defaultValue : source["children"]
    });
    if (record && (internalId = source["phantomId"])) {
        record.internalId = internalId;
    }
};

Hope my ramblings make sense... feel free to move this to the bugs forum, although I've obviously not come up with a test case

Cheers,
Westy

[dongryphon]

Neither, it's from readRecords; reading data from a server response into a store, creating a model instance for each record.

In this case, shouldn't the idProperty be in the data? The role of idgen is to generate id's for new records...

Still though, the reader mechanism needs to have a read-and-apply-defaults flavor and a read-only-what-is-present flavor.

[westy]

I'm using idgen to give unique ids for tree nodes, since if they're not unique you get all kinds of issues because of the use of hashing to increase performance.

My trees contains entities from many database tables so the ids can clash. Not only that but some nodes can appear in several places in the hierarchy.

Cheers

[Animal]

OK, this rapidly turned into "a maze of twisty little passages, all alike"!

Of course an idProperty should not get a fixed default value shoved into it if it's data is undefined. That's never a valid thing to do. Whether the lack of the corresponding property in the data is a problem is a matter of application semantics.

Also, when receiving updated record values back from a 'create' or 'update' operation, defaults should not be applied. The data returned should be just copied into the client side record,

Another thing I fixed is the ability to null out convert functions for your standard data types like 'int', and 'float', so that if you know your server produces correct JSON, the type coercion routines can be bypassed.

Also, you can configure a Model's fields with a defaultValue of undefined which explicitly eliminates any defaulting code from the conversion.

So the generated data extraction function for examples/grid/array-grid.html looks like this now which is as lean as can be:

(function() {
var me = this,
    fields = me.model.prototype.fields,
    value,
    internalId,
    __field0 = fields.get("company"),
    __field1 = fields.get("price"),
    __field2 = fields.get("change"),
    __field3 = fields.get("pctChange"),
    __field4 = fields.get("lastChange");

    return function(dest, source, record) {
        value = source[0];
        if (value !== undefined) {
            dest["company"] = value;
        }
        value = source[1];
        if (value !== undefined) {
            dest["price"] = value;
        }
        value = source[2];
        if (value !== undefined) {
            dest["change"] = value;
        }
        value = source[3];
        if (value !== undefined) {
            dest["pctChange"] = value;
        }
        value = source[4];
        if (value !== undefined) {
            dest["lastChange"] = __field4.convert(value, record);
        }
    };
})

The idProperty is "company", so there's no default processing on that field now.

The other fields have been explicitly configured with

defaultValue: undefined

So they do not do any default processing either.

All this will mean that data reading in 4.1 can be configured to be much faster than in 3.x and 4.0. So repeated polling of data will perform better.

Might also be worth mentioning that have also seen issues with using defaultValue's of undefined.

This used to mean that the resultant record would not have the member (which is often nice, probably due to using apply or applyIf I assume), where as now it does always have a member with a value of undefined.

Not a problem, now I know it's happening, but a change none the less.

Yes, I fixed that. If a field does not have a value, or a default value, then it does not set the property in the resulting Record's data object.