Opened 7 years ago

Closed 4 years ago

#16291 closed defect (fixed)

[patch][cla] lang::exists returning false positives

Reported by: mccormack Owned by: bill
Priority: low Milestone: 1.10
Component: Core Version: 1.8.1
Keywords: Cc:
Blocked By: Blocking:

Description

For example dojo.exists("foo.bar", {foo:0}) === true

Please see attached patch and test cases.

Attachments (2)

16291.patch (2.2 KB) - added by mccormack 7 years ago.
lang exists/setObject update for false positive bug
16291_2.patch (4.5 KB) - added by mccormack 7 years ago.
Changes to getProps and setObject - added to existing tests

Download all attachments as: .zip

Change History (16)

Changed 7 years ago by mccormack

Attachment: 16291.patch added

lang exists/setObject update for false positive bug

comment:1 Changed 7 years ago by bill

Summary: lang::exists returning false positives[patch] [cla] lang::exists returning false positives

Changed 7 years ago by mccormack

Attachment: 16291_2.patch added

Changes to getProps and setObject - added to existing tests

comment:2 Changed 7 years ago by bill

Milestone: tbd1.9
Owner: set to Kris Zyp
Status: newassigned

comment:3 Changed 7 years ago by bill

Milestone: 1.91.10

Bumping this ticket since we are past the deadline for the 1.9RC. The fix can be put into 1.9.1 too, if desired.

comment:4 Changed 6 years ago by Kris Zyp

Priority: undecidedlow

comment:5 Changed 6 years ago by dylan

Owner: changed from Kris Zyp to dylan

I'll create a pull request from this asap.

comment:6 Changed 6 years ago by dylan

Owner: changed from dylan to bill
Summary: [patch] [cla] lang::exists returning false positives[patch][cla] lang::exists returning false positives

Cleaned this up a bit and tried to reduce the size of the various comparison checks, pull request at https://github.com/dojo/dojo/pull/81

comment:7 Changed 6 years ago by bill

As for fixing the bug with exists(), that's fine.

As for the patch, there's a philosophical question, because the patch affects not just exists() but modifies the behavior of getObject() and setObject(), and then it adds unit tests to confirm that getObject() and setObject() behave in IMO an unintuitive way.

Given an object like:

var test = {
	foo : 0
};

Then what should

lang.getObject("foo.bar", true, test);

or

lang.setObject("foo.bar", "hi", test);

do?

In order to create test.foo.bar, you have to overwrite test.foo from 0 to {}. IMO that's beyond the spec of getObject() which says:

Objects will be created at any point along the 'path' that is *undefined*.

As for setObject(), it's spec is vague, it just says:

Objects are created as needed along path.

Since it doesn't mention overwriting existing values, I figure it isn't supposed to, but the spec admittedly isn't explicit.

comment:8 Changed 6 years ago by bill

After realizing this ticket is about a corner case (where the path reference primitive properties (numbers, booleans, etc.), it seems much less important.

But I made an alternate patch anyway, see https://github.com/dojo/dojo/pull/82.

I modified the tests from the original patch to only check the corner case for exists().

Last edited 6 years ago by bill (previous) (diff)

comment:9 Changed 6 years ago by bill

I'm going to check in a highly modified version of the original patch. About your tests:

  1. delete window.foo (and delete dojo.global.foo throw an exception on IE8, so I removed that testing code
  2. I also removed the checks that getObject() and setObject() overwrite existing properties if they are primitive types; I'd rather just say that the behavior is undefined in that corner case.

comment:10 Changed 6 years ago by Bill Keese <bill@…>

Resolution: fixed
Status: assignedclosed

In 84cbaf9144b7c5ce1ef309f8297e8426066de0eb/dojo:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:11 Changed 6 years ago by Bill Keese <bill@…>

In 2957d1b090ae44f2e821e98ea754c99700672d5a/dojo:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:12 Changed 4 years ago by dylan

Milestone: 1.10
Resolution: fixed
Status: closedreopened

Per Claudio Procida on twitter:

Undocumented change: dojo.getObject("a.b", false, {a:null}) 1.9 null dojo.getObject("a.b", false, {a:null}) 1.10 undefined

Changed behavior is a side effect of the fix for this ticket

He suggests that

lang.getObject("a.b", false, {a:null}) should throw TypeError?: Cannot read property 'b' of null instead of returning undefined or null.

comment:13 Changed 4 years ago by bill

Undocumented change: dojo.getObject("a.b", false, {a:null}) 1.9 null dojo.getObject("a.b", false, {a:null}) 1.10 undefined

That's a bug fix. Although the API doc is unspecific about the return value when the path doesn't exist, 1.9 returns undefined in general:

lang.getObject("a.b", false, {}) undefined

Therefore it should return undefined in this corner case too.


He suggests that lang.getObject("a.b", false, {a:null}) should throw ... instead of returning undefined or null

We can't make such a drastic change because it breaks backwards compatibility and relatedly will break existing applications.

comment:14 Changed 4 years ago by bill

Milestone: 1.10
Resolution: fixed
Status: reopenedclosed
Note: See TracTickets for help on using tickets.