tj / node-querystring

querystring parser for node and the browser - supporting nesting (used by Express, Connect, etc)
MIT License
455 stars 66 forks source link

Parse '=' should return {} #59

Closed ikokostya closed 10 years ago

ikokostya commented 11 years ago

Now qs.parse('=') returns { '=': '' }.

tjwebb commented 10 years ago

why should it return an object?

buschtoens commented 10 years ago

All those edge cases... According to the very loose spec, a = may not be part of a name or value. Technically this isn't a valid query string, so throwing or returning null would be the closest to a right behaviour.

However to not break code an empty object is the easy way out.

rlidwka commented 10 years ago

I believe, it should return { '': '' }. As far as I remember, qs never throws and always return an object, so breaking that won't really be right.

tj commented 10 years ago

I'd definitely consider that invalid from the client perspective, but if you can't tweak the client to not send that i think this is ok, either {} or { '': '' }

buschtoens commented 10 years ago

Does { '': '' } really make sense for anyone? It's kinda the equivalent to ?=, that's true, but ?= doesn't contain any information of which anyone could make any sense.

So I'll +1 for the {}. I also feel like we should take this as the default solution, if there is nothing for qs to make sense of, i.e. empty string, strings that only consist of illegal characters,

rlidwka commented 10 years ago

Does { '': '' } really make sense for anyone?

Yes, because '='.split('=') equals to [ '', '' ]. First element is key (''), second element is a value (''). Sounds reasonable enough?

Also, require('querystring').parse('=') equals to { '': '' } as well.

buschtoens commented 10 years ago

@rlidwka Well, querystring is querystring. qs is qs. ;)

According to the very little spec we got, the query string should always be a combination of key and value pairs. ?key1&key2 is already weird. But just a = is total non-sense. If you're really interested in this nothingness of information do a plain equality check like if(query === "="). You don't need a lib for that.

rlidwka commented 10 years ago

No, it's not fixed. My opinion, nothing more than that.

  1. foo=bar -> {foo: 'bar'}, right? Which means that foo= -> {foo: ''}. Empty value is just a ordinary value here.
  2. For the same reasons, =bar -> {'': 'bar'}. Empty key is just a ordinary key here.
  3. But empty key having an empty value is represented by... guess what
rlidwka commented 10 years ago

By the way, ?key1&key2 has a standard meaning. It means that both key1 and key2 are true.

buschtoens commented 10 years ago

Yeah, I know. ?key1&key2 is the de facto standard. AFAIK this is not noted in the spec. The only thing the spec have for us is the key value pairing. Somewhere we have to draw a line between "actually used in the wild" and "just plain broken".

Let's go the other way around, in what use case do you want to use =? It feels broken to me.

(And that "are you implying that" part was my fault. I misread your answer.)

rlidwka commented 10 years ago

Let's go the other way around, in what use case do you want to use =? It feels broken to me.

I am using empty values very often, I don't remember using empty keys though. = looks like an empty key with an empty value.

All right, let me ask you this. How do you think these cases should be parsed:

  1. =12345
  2. []=12345
  3. foo=bar&=12345
tj commented 10 years ago

my question is why would anyone care how =123 etc is parsed, that's not really useful for an app, it shouldn't throw or anything obviously but certainly not useful, we're just wasting time on non-issues

buschtoens commented 10 years ago

I use empty values myself quite often too (though per spec they should be illegal, but who cares). But empty values are like: Here's some data. Don't know where it's from or what it should mean.

It's kinda like: Deliver this package to this address: "" wat do nao?

buschtoens commented 10 years ago

And to answer your question ;)

  1. =12345 - Illegal -> {}
  2. []=12345 - Illegal -> {}
  3. foo=bar&=12345 - Partially readable -> { "foo": "bar" }
rlidwka commented 10 years ago

Well, built-in querystring seem to do be able to produce this automatically:

> querystring.stringify({'':''})
'='
> querystring.stringify({'':'123'})
'=123'

But since qs doesn't, I agree, it's not an issue.

buschtoens commented 10 years ago

It's really a thing of perspective and personal favour. As long as it's consistent, it's ok.