Opened 8 years ago
Closed 5 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)
Change History (16)
Changed 8 years ago by
Attachment: | 16291.patch added |
---|
comment:1 Changed 8 years ago by
Summary: | lang::exists returning false positives → [patch] [cla] lang::exists returning false positives |
---|
Changed 8 years ago by
Attachment: | 16291_2.patch added |
---|
Changes to getProps and setObject - added to existing tests
comment:2 Changed 8 years ago by
Milestone: | tbd → 1.9 |
---|---|
Owner: | set to Kris Zyp |
Status: | new → assigned |
comment:3 Changed 8 years ago by
Milestone: | 1.9 → 1.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 7 years ago by
Priority: | undecided → low |
---|
comment:5 Changed 7 years ago by
Owner: | changed from Kris Zyp to dylan |
---|
I'll create a pull request from this asap.
comment:6 Changed 7 years ago by
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 7 years ago by
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 7 years ago by
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().
comment:9 Changed 7 years ago by
I'm going to check in a highly modified version of the original patch. About your tests:
delete window.foo
(anddelete dojo.global.foo
throw an exception on IE8, so I removed that testing code- I also removed the checks that
getObject()
andsetObject()
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 7 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:12 Changed 6 years ago by
Milestone: | 1.10 |
---|---|
Resolution: | fixed |
Status: | closed → reopened |
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 6 years ago by
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 5 years ago by
Milestone: | → 1.10 |
---|---|
Resolution: | → fixed |
Status: | reopened → closed |
lang exists/setObject update for false positive bug