Add type coercion for parsing querystrings.
Opened this issue · 31 comments
Would turn 'true' and 'false' into booleans; ints into ints; and floats into floats.
cool thanks, I'll ask around to see what people think, I'm +1 in general
Makes sense to me.
I like it
a +1 one and "pragmatic" on twitter, haha so it's looking like a go
Kinda works with jQuery data. Just don't go insane with it I guess. I guess it gets hairy when you have strings like 0x21 and then it coerces to a number instead of a string.
Maybe make it opt-in?
the implementation will need a bit of tweaking and i'll add tests but it sounds like in general it's "enable by default for express 3.0" and make configurable otherwise. thanks everyone
Need to handle strings that start with numbers and also it seems like because parseInt() on a float in a string is valid, it will never get to the parseFloat branch?
e.g coerce("32 fake st") = 32
e.g coerce("32.123") = 32
-1 see @kennethkufluk comments in the diff. There may be more edge cases like this (as @tbranyen points out). If a user wants some specific type I say let them do it.
I like it ... but opt-in as an option would definitely be the best way of doing it I think. :) I agree with tbranyen.
I like it! Just as long as you don't end up with surprises like 0x41 being 0 or another number.
yeah i was thinking base 10 only
@micmcg dont worry about the implementation, just talking about the idea
I think the idea is great then :-)
👍
started here with test cov: https://github.com/visionmedia/node-querystring/tree/coerce
@shtylman yeah he brings up a good point. the int test regexp could definitely test for things like that but you're right. That's certainly the downside is that you can't count on it just simply being a string, tough call. Myself as a user with few query-string special-cases I would definitely like this, I can easily make it a setting in Express since connect.query() is used by default
Yeah, implementation needed some work. There's a reason this wasn't a pull request, heh. @visionmedia, make sure you're checking for overflows in conversion to ints/floats; I don't see that in there now.
+1
I think this is good as a default but having the option to "opt-out". I'm wondering how parse failures will be handled as if it is implemented nicely then it could remove/reduce the need for express-validator. If no errors aren't thrown or the errors aren't extensible then the option is kind of pointless IMO because you will still need to do validation and sanitization on your own.
@ekryski not sure what you mean. We likely wont be throwing anything at all, aside from "natural" errors
I guess as long as there is an error thrown it will be okay. Currently in my apps I find myself generating an error for each invalid value type that is sent to the server and then reporting those errors in a batch to the client. As long as the coercion doesn't obscure invalid values then it should be fine but you'll have to look over those edge cases like the ones defined by others above. An example could be what if you receive a '7452823' for a value and the coercion coverts this to an int but for that field you actually want strings because you could have '7458ab34' sometimes as well, maybe better yet you wanna throw an error if the value is an int.
I guess what I'm getting at is the same as @tbranyen and @kennethkufluk. There could be a lot of edge cases that crop up and therefore if someone is willing to throw an error on each edge case then it might be okay but this could prove tricky. As implementations go It would be good to know which param caused an error, or better yet return an errors object/array containing an error for each param that fails as opposed to simply throwing a single error on any failure. If the latter implementation is the case as soon as I start getting errors (which could happen frequently given the edge cases described already) the feature is useless.
@ekryski gotcha, yeah I see what you're saying. definitely gets iffy if you have ids or hashes that are both numeric/alpha-numeric at times, that's a good point as well. Maybe we should default to disabled, if the user opts-in it's their own deal :p haha but even with third-party middleware etc that might get sketchy
This library should definitely support it, but Express/Connect is a different issue
ya your call. The way I see it is it is kind of an all or nothing. Even if it is opt-in then whenever someone runs into a case that isn't handled (which could be often) then they have to opt-out and do it all themselves anyway. I like the idea especially if it covers 90% of the use cases (twitter ids and so on) it just depends on where people want to spend their time. I don't want to discourage anyone from implementing this as long as there is going to be support for newly discovered edge cases.
yeah, I can see that being somewhat painful having to potentially revert a bunch of code you already finished.
As I understand it, this would be within the req.query params object, or some other key/value object, correct?
If so, I would say it makes sense to convert the exact string 'true' to boolean true, pure integers, '123' to 123, and pure floats to floats. This adds some nice elegance, especially when doing something like if (params.someBoolean)
If there is any number/letter mix, or anything beyond these 3 scenarios, it should stay a string. Urls are strings in the first place, so this is most expected.
...But what about all the javascript types: Array, Boolean, Date, Function, Number, Object, RegExp, and String?
Arrays: see objects below
Dates: It'd be very messy/pointless to try and convert to date objects, I'd immagine timestamp ints are much more commonplace.
Functions: Of course not. However, these could be wrapped in json.
Objects: Could attempt a JSON.parse in a try block if the first char is a '{' or '[', but I think most developers expect to have to do this. (With arrays, don't forget they are also json) I vote this shouldn't be attempted.
Regex's...? leave it.
As far as existing code that is based off of strings, it's not hard to do a simple regex search for integers/floats to convert them from strings to ints, and just searching 'true' and replacing with true can fix this for booleans. No need to have split functionality or feature cruft.
TL;DR: Convert 'true' to boolean true, '123' to 123, floats to floats, but not a single thing more. Number/letter mixes stay strings.
@visionmedia Ultimately it's you call TJ, I would do what you personally prefer.
@devinrhode2 Thats why I prefer base64 encoded JSON for complex datastructures on the querystring.
?data=eyJuYW1lIjoibWljaGFlbCIsImlzQXdlc29tZSI6dHJ1ZSwiYWdlIjozMH0=
I know this isn't the same, but I do sometimes wonder about frameworks doing magical things to your params for you (for example the Rails problems they've had recently). It always seems to me that I want to do this myself so that I know for sure what validation or conversions have already happened.
I'm not sure this is a good feature but if it does get landed, it must be opt in. :)
It would be nice to add boolean coercion, at least.
No Boolean coercion yet? I have been looking around for an elegant solution to this. I cant believe people parse their booleans by hand. It seems I'm wrong :)
The demand is still high. Release it please.
+1 it would be nice addition.