Skip to content

Handle partially parsed objects (w/ querystrings as keys) #27

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Handle partially parsed objects (w/ querystrings as keys) #27

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
583a95b
Handle partially parsed objects (w/ querystrings as keys)
kainosnoema Dec 17, 2011
b6e8767
add test for arrays, concat() instead of push() in case val is an array
kainosnoema Dec 19, 2011
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 8 additions & 13 deletions lib/querystring.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,7 @@ function parse(parts, parent, key, val) {
var part = parts.shift();
// end
if (!part) {
if (Array.isArray(parent[key])) {
parent[key].push(val);
} else if ('object' == typeof parent[key]) {
parent[key] = val;
} else if ('undefined' == typeof parent[key]) {
parent[key] = val;
} else {
parent[key] = [parent[key], val];
}
set(parent, key, val);
// array
} else {
var obj = parent[key] = parent[key] || [];
Expand Down Expand Up @@ -95,8 +87,8 @@ function merge(parent, key, val){
* Parse the given obj.
*/

function parseObject(obj){
var ret = { base: {} };
function parseObject(obj, base){
var ret = { base: base || {} };
Object.keys(obj).forEach(function(name){
merge(ret, name, obj[name]);
});
Expand Down Expand Up @@ -232,10 +224,13 @@ function stringifyObject(obj, prefix) {

function set(obj, key, val) {
var v = obj[key];
if (undefined === v) {
if ('undefined' == typeof v) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need this? where's v coming from that we need typeof

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh it's right above haha, we dont need typeof then

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just consistency really, and because undefined can be redefined in Javascript, meaning the safest way to check for undefined is with typeof. It's a habit of mine...

EDIT: I knew this, but just tried it... blows my mind:

> typeof undefined
'undefined'
> {}['foo'] === undefined
true
> undefined = 'foo';
'foo'
> typeof undefined
'string'
> {}['foo'] === undefined
false

EDIT 2: Apparently this behavior was eliminated in newer versions of V8? The above was in Node v0.4.8, v0.6.2 doesn't allow it... thank goodness.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah gotcha, personally I avoid defensive programming whenever possible to delegate the issue. plus there are a ton of retarded things you can do in js, people just shouldn't do them haha

obj[key] = val;
} else if (Array.isArray(v)) {
v.push(val);
obj[key] = v.concat(val);
} else if (('object' == typeof v) && ('[object Object]' == toString.call(val))) {
// partially parsed object
parseObject(val, v);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, putting this recursive call inside set() seems a very odd to me given that its not actually setting anything. The only reason is to keep things a little cleaner since you'd otherwise have to check and handle this case in both the parse() and merge() functions. That said, if you have a better idea I'll be glad to make changes.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

meh yeah it's, the whole lib is pretty obscure, it's a strange thing to parse, as long as we have strong test cases we should be fine

} else {
obj[key] = [v, val];
}
Expand Down
23 changes: 23 additions & 0 deletions test/parse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,29 @@ module.exports = {
'test malformed uri': function(){
qs.parse('{%:%}').should.eql({ '{%:%}': '' });
qs.parse('foo=%:%}').should.eql({ 'foo': '%:%}' });
},

'test partially parsed objects': function(){
qs.parse({ 'foo[0]': 'bar', 'foo[1]': 'baz' })
.should.eql({ foo: ['bar', 'baz'] });

qs.parse({ 'foo[items]': [], foo: { items: ['bar'] } })
.should.eql({ foo: { items: ['bar'] } })

qs.parse({ foo: { items: ['bar'] }, 'foo[items]': [] })
.should.eql({ foo: { items: ['bar'] } })

qs.parse({ 'foo[base64]': 'RAWR', 'foo[64base]': 'RAWR' })
.should.eql({ foo: { base64: 'RAWR', '64base': 'RAWR' } });

qs.parse({ 'user[name][first]': ['tj', 'TJ'] })
.should.eql({ user: { name: { first: ['tj', 'TJ'] } } });

qs.parse({ 'user[name]': { first: 'tj' }, 'user': { name: { last: 'holowaychuk' } } })
.should.eql({ user: { name: { first: 'tj', last: 'holowaychuk' } } });

qs.parse({ 'user': { name: { last: 'holowaychuk' } }, 'user[name]': { first: 'tj' } })
.should.eql({ user: { name: { last: 'holowaychuk', first: 'tj' } } });
}

// 'test complex': function(){
Expand Down