HTTP::Params#encode won't accept anything but String as hash value

[HCLarsen]

Unless I'm reading the documentation wrong, it implies that the value of the hash argument can be any value, but the encode method throws an error if the values in the hash are anything but a string.

Error in openWeatherMap.cr:19: instantiating 'HTTP::Params:Class#encode(Hash(String, Array(String)))'

puts params = HTTP::Params.encode(hash)
                           ^~~~~~

in /opt/crystal/src/http/params.cr:83: instantiating 'Hash(String, Array(String))#each()'

        hash.each do |key, value|
             ^~~~

in /opt/crystal/src/http/params.cr:83: instantiating 'Hash(String, Array(String))#each()'

        hash.each do |key, value|
             ^~~~

in /opt/crystal/src/http/params.cr:84: instantiating 'HTTP::Params::Builder#add(String, Array(String))'

          builder.add key, value
                  ^~~

in /opt/crystal/src/http/params.cr:329: no overload matches 'HTTP::Params.encode_www_form_component' with types Array(String), IO
Overloads are:
 - HTTP::Params.encode_www_form_component(string : String, io : IO)

        Params.encode_www_form_component value, @io if value

[luislavena]

Hello @HCLarsen, it appears the limitation is being introduced by HTTP::Params.encode_www_form_component signature.

A minimal example that shows this problem:

require "http/params"

data = {"key" => 100, "foo" => "bar"}

puts HTTP::Params.encode(data)

Error in 2.cr:5: instantiating 'HTTP::Params:Class#encode(Hash(String, Int32 | String))'

puts HTTP::Params.encode(data)
                  ^~~~~~

in /opt/crystal/src/http/params.cr:83: instantiating 'Hash(String, Int32 | String)#each()'

        hash.each do |key, value|
             ^~~~

in /opt/crystal/src/http/params.cr:83: instantiating 'Hash(String, Int32 | String)#each()'

        hash.each do |key, value|
             ^~~~

in /opt/crystal/src/http/params.cr:84: instantiating 'HTTP::Params::Builder#add(String, (Int32 | String))'

          builder.add key, value
                  ^~~

in /opt/crystal/src/http/params.cr:329: no overload matches 'HTTP::Params.encode_www_form_component' with types (Int32 | String), IO
Overloads are:
 - HTTP::Params.encode_www_form_component(string : String, io : IO)
Couldn't find overloads for these types:
 - HTTP::Params.encode_www_form_component(string : Int32, io : File::PReader)
 - HTTP::Params.encode_www_form_component(string : Int32, io : IO::ARGF)
 - HTTP::Params.encode_www_form_component(string : Int32, io : IO::Delimited)
 - HTTP::Params.encode_www_form_component(string : Int32, io : IO::FileDescriptor)
 - HTTP::Params.encode_www_form_component(string : Int32, io : IO::Hexdump)
 - HTTP::Params.encode_www_form_component(string : Int32, io : IO::Memory)
 - HTTP::Params.encode_www_form_component(string : Int32, io : IO::MultiWriter)
 - HTTP::Params.encode_www_form_component(string : Int32, io : IO::Sized)
 - HTTP::Params.encode_www_form_component(string : Int32, io : String::Builder)

        Params.encode_www_form_component value, @io if value
               ^~~~~~~~~~~~~~~~~~~~~~~~~

[luislavena]

@HCLarsen following up on this, I believe simply coercing value to be a string should be enough:

Params.encode_www_form_component value.to_s, @io if value

But definitely needs more testing (and specs 😉)

Cheers.

[HCLarsen]

Thank you @luislavena, this functionality will be key to improving a library that I'm building right now.

[asterite]

Maybe that's intentional. I don't know if we want to support encoding any value. For example if you have:

{"foo" => [1, 2, 3]}

what would you expect? If we change it to invoke to_s you will get:

foo=[1,2,3]

but maybe you wanted this?

foo=1&foo=2&foo=3

It might be confusing. I think we should just restrict the Hash to be Hash(String, String). I'd also like to avoid that method being instantiated for every possible Hash you throw at it.

[luislavena]

Good points from @asterite, coercing to_s is not a solution. Apologies for the rush to suggest it, definitely didn't spent enough time thinking about it.

Cheers.

[HCLarsen]

@asterite here's an example of an API that's expecting what is essentially an array of numbers:

http://api.openweathermap.org/data/2.5/group?id=524901,703448,2643743&units=metric

While this is only one example, I doubt that it's the only API in the world that does this. This is why I think it'll be useful to make #encode work like the documentation suggests.

[luislavena]

@HCLarsen can you point us where in the documentation suggests that works?

I don't see that in HTTP::Params.encode or HTTP::Params::Builder.

Thank you.

[HCLarsen]

def self.encode(hash : Hash(String, _))

Doesn't the underscore mean any class?

[luislavena]

Correct, but as @asterite pointed out, that might be an overlook of the method signature.

The naive change I suggested will not produce the id field as you indicated but instead produce something different.

Making automatic coercion is not the solution on this case 😞

[HCLarsen]

No, a simple to_s wouldn't do it for an array.

The current solution I'm using in my code is: .to_s.lchop.rchop.split.join

[asterite]

Exactly. We should force the parameter to be Hash(String, String). It's a bug in the type restriction. Well, actually not quite, it doesn't compile, so it works as expected. It's just that the error you get is not nice.

In general, when a type restriction doesn't exist it doesn't mean the method can accept any type. It just means we are lazy to add the type restriction.

[HCLarsen]

No offense, but that's rather sloppy. You should always add a type restriction where one exists. The alternative is very unfriendly to newcomers to the language.

[asterite]

And how do we force that?

The only solution is forcing type restrictions everywhere. I think that's something good, actually. But nobody will use Crystal like that.

[RX14]

No type restriction means exactly the same as it does in a dynamic language. That some types may work and others wont.

The difference is that in crystal the wrong types will get you a compile error instead of a runtime error in a dynamic language. It'll just be a more messy error than if you'd thought about types and manually annotated it. In crystal you have the choice to think about types or not, or be flexible with types. I don't think thats sloppy, just different to other languages.

[HCLarsen]

I'm with you on the first part of that. I agree that it would be a good thing, but I do think that people would use Crystal like that. Other statically typed languages have type restrictions in every method argument. The alternative is exactly what happened in this example. The method takes an argument of a non-expected class, and crashes. It's like having a hidden type restriction as opposed to an obvious one. You might as well make it obvious.

[asterite]

It doesn't crash. You get a compile error saying exactly why it doesn't work. Somewhere a String was expected. If we add a type restriction you will also get a compiler error, slightly more informative.

It's impossible to have added the type restriction because there's no way to travel to the past. We can start adding type restrictions everywhere we find a need, though. It's maybe not Crystal's way... or at least it wasn't when we began the language.

[luislavena]

Working on a PR with type restriction fixes for this.

[HCLarsen]

Exactly, the error is more informative if you add a type restriction. As I said, the restriction is still there, it's just a few method calls deep.

I think that going in an adding type restrictions everywhere that you find a need would be great for the future of the language. It is a statically typed language after all, and type restrictions are part of static typing. All my Crystal code so far uses type restrictions, and I fully intend to keep doing it that way.

[luislavena]

Hello @HCLarsen, seems we all are on the same page then. I believe the confusion is the wording around the request.

For me (a non-native english speaker), it appears your request was a complaint about not supporting any other type than String and the confusion around the error message (or the type system).

Either way. I've sent #5184 and hopefully that will reduce confusion for others under the same situation.

Cheers.

[HCLarsen]

Either way. I was more remarking that the documentation and functionality don't match. Your PR will definitely fix this. Thank you for creating it.

[asterite]

Note that we still need to fix the type restrictions in HTTP::Client and probably other places. And there's no way to restrict named tuples to have string keys, so you'll also get the same cryptic error there. I guess we should remove all NamedTuple overloads as it makes little sense to pass named tuples there (plus it will instantiate that method for every named tuple type you pass it, leading to a lot of code bloat).

[HCLarsen]

I'd help with this, but I'm still waiting for my first PR to be merged.

[RX14]

I agree that in the stdlib, and public shards, using type restrictions as much as possible is a good idea for documentation and better compiler errors. However, the moment we force people to use type restrictions is the moment I can no longer say with faith that crystal feels like a dynamic language to program in insofar that I don't have to really think about types.

When i'm working on small projects, just playing around, or just getting a fast prototype out I don't want to go around putting type restrictions on everything. And we should keep it that way.

[HCLarsen]

Ok, I'm confused now. I didn't understand that Crystal is supposed to feel like a dynamic language.

[asterite]

@HCLarsen That's the only reason Crystal came to existence. The idea is that you could program almost similar as in Ruby, without caring much about type annotations, but you'd get errors at compile time instead of runtime. So not writing type restrictions is actually part of the language philosophy to make things more DRY and less tedious.

Now, we all agree that when there's a chance to specify a type restriction in a public API that should be done. But we came to that conclusion later in the development process.

[HCLarsen]

Really? I thought the reason Crystal came to existence was that people were sick of choosing between having great syntax and being able to compile code? That's why I'm so interested in Crystal.

[luislavena]

@HCLarsen different people might have discovered, liked or disliked Crystal as a language for different reasons.

One of the initial ideas (as explained by asterite) was to be able to write code and don't be forced to define types for every method or variable. Others might see Crystal as a compiled Ruby, which is far from the reality (as Crystal is not Ruby), but that is what floats their boat. 😁

I (personally) believe Crystal's raison d'être is described in the documentation:

• Have a syntax similar to Ruby (but compatibility with it is not a goal).
• Be statically type-checked, but without having to specify the type of variables or method arguments.
• Be able to call C code by writing bindings to it in Crystal.
• Have compile-time evaluation and generation of code, to avoid boilerplate code.
• Compile to efficient native code.

Anyway, what we should conclude from this is that Crystal's standard library (stdlib) needs better type signatures to provide improved error messages, avoiding the issue originally reported here.

Have a nice day.
❤️ ❤️ ❤️