Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#14163 closed defect (fixed)

StatefulModel does not handle falsy values

Reported by: ben hockey Owned by: rahul
Priority: high Milestone: 1.7.1
Component: DojoX MVC Version: 1.7.0
Keywords: Cc:
Blocked By: Blocking:

Description

this simple case shows how StatefuleModel? changes null values to empty strings. the null should be retained.

var d = {prop: null};
var m = new dojox.mvc.StatefulModel({data: d});
console.log(d.prop, m.toPlainObject().prop);

fyi - this is in the 1.7.0 release.

Attachments (2)

mvc-14163.patch (6.1 KB) - added by Ed Chatelain 8 years ago.
14163.diff (6.2 KB) - added by ben hockey 8 years ago.
stricter assertions

Download all attachments as: .zip

Change History (17)

comment:1 Changed 8 years ago by ben hockey

Version: 1.7.0b11.7.0

comment:2 Changed 8 years ago by ben hockey

the same problem occurs for each of the properties with the following data:

var d = {
  u: undefined,
  z: 0,
  n: null,
  f: false
};
var m = new dojox.mvc.StatefulModel({data: d});
console.log(m.toPlainObject());

comment:3 Changed 8 years ago by ben hockey

a possible fix is this (UPDATED):

  • StatefulModel.js

     
    370370                       //              the data structure.
    371371                       // tags:
    372372                       //              private
    373                        var data = (args && args.data) || this.data;
    374                        if(data){
    375                                this._createModel(data);
    376                        }
     373                       var data = (args && "data" in args) ? args.data : this.data;
     374                       this._createModel(data);
    377375               },
    378376
    379377               //////////////////////// PRIVATE METHODS ////////////////////////
     
    385383                       //              The input for the model, as a plain JavaScript object.
    386384                       // tags:
    387385                       //              private
    388                        if(lang.isObject(obj) && !(obj instanceof Date) && !(obj instanceof RegExp)){
     386                       if(lang.isObject(obj) && !(obj instanceof Date) && !(obj instanceof RegExp) && obj !== null){
    389387                               for(var x in obj){
    390388                                       var newProp = new StatefulModel({ data : obj[x] });
    391389                                       this.set(x, newProp);

i would have added some tests but wasn't sure where to put them

Last edited 8 years ago by ben hockey (previous) (diff)

comment:4 Changed 8 years ago by ben hockey

Summary: StatefulModel does not handle null valuesStatefulModel does not handle falsy values

comment:5 Changed 8 years ago by ben hockey

just updated the diff in comment:3

comment:6 Changed 8 years ago by Ed Chatelain

I will update a testcase for this and create a patch.

Changed 8 years ago by Ed Chatelain

Attachment: mvc-14163.patch added

comment:7 Changed 8 years ago by Ed Chatelain

Ben, Thanks for finding the problem and providing the fix. I just added the test into the date test where I had the test for the regexp. I also made an adjustment to the order that the robot tests were run, because this order seems to work more consistently for me.

comment:8 Changed 8 years ago by ben hockey

ed, thanks for the patch.

i added one more test - for an empty string. since this case was not failing i had not mentioned it but i think it should be included in the tests so that the tests will reveal unexpected regressions.

once you can confirm that you're happy with the patch, i can commit it if you like.

comment:9 Changed 8 years ago by ben hockey

hmm... just noticed a typo in my diff

doh.is("", test5_model.f.get('value'),"test5_model.s.get(value) should be an empty string");

should be

// was testing test5_model.f before!!!
doh.is("", test5_model.s.get('value'),"test5_model.s.get(value) should be an empty string");

the tests passed without any error so we might be getting false positives from doh so i'm going to look into changing the tests. i'll update my diff once i've changed the tests.

Changed 8 years ago by ben hockey

Attachment: 14163.diff added

stricter assertions

comment:10 Changed 8 years ago by ben hockey

ok, http://dojotoolkit.org/reference-guide/util/doh.html#doh-assertequal-obj1-obj2 says that doh.assertEqual (doh.is is an alias for doh.assertEqual) uses a fairly loose equality. i've updated the tests to use stricter equality.

comment:11 Changed 8 years ago by Ed Chatelain

Hi Ben,

Your update looks good, it is also good to know about the loose equality for assertEqual, I will have to remember that. If you could commit it that would be great.

comment:12 Changed 8 years ago by ben hockey

Resolution: fixed
Status: newclosed

In [26940]:

add support for StatefulModel? to handle falsy values. includes tests.
fixes #14163 !strict

comment:13 Changed 8 years ago by bill

Milestone: tbd1.8

comment:14 Changed 8 years ago by cjolif

In [27169]:

Fixes #14163 in 1.7.1 for edchat.

comment:15 Changed 8 years ago by cjolif

Milestone: 1.81.7.1
Note: See TracTickets for help on using tickets.