Fetching post params

Hello Everyone!

I swear I had this thing working before but now I’m stumped! I can’t seem to get to the POST params, any idea what I’m doing wrong?

params = HTTP::Params.parse((context.request.query || "") + "&" + (context.request.body.try &.gets_to_end || ""))
pp params # no posted param shows up! HTTP::Params(@raw_params={})

The form has an enctype set

<form method="post" enctype="application/x-www-form-urlencoded">

I think it would be super nice if the Request class had a params that took care of this! ;)

Thanks in advance!

Well, it looks like it certainly works in a simple setting like so:

require "http"

server = HTTP::Server.new do |context|
  params = HTTP::Params.parse((context.request.query || "") + "&" + (context.request.body.try &.gets_to_end || ""))
  pp params
  context.response.content_type = "text/plain"
  context.response.print "Hello world!"
end

address = server.bind_tcp 9090
puts "Listening on http://#{address}"
server.listen

same when using a handler BUT NOT when chaining a bunch of handlers, so this fails

module Foo
  class CustomHandler
    include HTTP::Handler

    def call(context)
      params = HTTP::Params.parse((context.request.query || "") + "&" + (context.request.body.try &.gets_to_end || ""))
      pp params
      context.response.content_type = "text/plain"
      context.response.print "Hello world!"

      call_next(context)
    end
  end

  class CustomHandler2
    include HTTP::Handler

    def call(context)
      params = HTTP::Params.parse((context.request.query || "") + "&" + (context.request.body.try &.gets_to_end || ""))
      pp params
      context.response.content_type = "text/plain"
      context.response.print "Hello world!"

      call_next(context)
    end
  end
end

server = HTTP::Server.new([
  HTTP::ErrorHandler.new,
  HTTP::LogHandler.new,
  HTTP::CompressHandler.new,

  Foo::CustomHandler.new,
  Foo::CustomHandler2.new,
  HTTP::StaticFileHandler.new("public"),
])

server.bind_tcp "127.0.0.1", 9090
server.listen

testing it like so:

curl -v --data 'id=2&email=foo' 'localhost:9090/'

outputs (pp)

HTTP::Params(@raw_params={"id" => ["2"], "email" => ["foo"]})
HTTP::Params(@raw_params={})

ie, one of the custom handlers doesn’t get any params because the body has been gets_to_end in the other. Now onto my question, is this by design or a bug because I’d expect every handler get a clean context/request environment to work with. If this is indeed by design, then is chaining more than one Handler not recommended? And if this is so, what happens when one in the chain (Error or Log or Compress for ex) consumes an IO that the others can’t get their hands on anymore?

Overall, I think, the model is slightly confusing and or broken, does anyone agree or am I completely mistaken here on how this thing works?

As a workaround, I decided to open and add a params to Request like so:

class HTTP::Request
  def params
    @params ||= HTTP::Params.parse((query || "") + "&" + (body.try &.gets_to_end || ""))
  end
end

module Foo
  class CustomHandler
    include HTTP::Handler

    def call(context)
    #   params = HTTP::Params.parse((context.request.query || "") + "&" + (context.request.body.try &.gets_to_end || ""))
      pp context.request.params
      context.response.content_type = "text/plain"
      context.response.print "Hello world!"

      call_next(context)
    end
  end

  class CustomHandler2
    include HTTP::Handler

    def call(context)
    #   params = HTTP::Params.parse((context.request.query || "") + "&" + (context.request.body.try &.gets_to_end || ""))
      pp context.request.params
      context.response.content_type = "text/plain"
      context.response.print "Hello world!"

      call_next(context)
    end
  end
end

and now testing with curl produces this:

HTTP::Params(@raw_params={"id" => ["2"], "email" => ["foo"]})
HTTP::Params(@raw_params={"id" => ["2"], "email" => ["foo"]})

Not sure if this approach is recommended but it certainly eliminates the problem I’ve been having. Can someone with more knowledge on this care to comment if this would end up causing trouble? Much thanks!

At the moment this is expected due to the fact the request body is an IO which gets consumed using .gets_to_end. I created an issue about this a little while ago, of which I did essentially the same thing as you, but I’m not sold on if it’s the “right” long term fix, or more of a workaround.

1 Like

Perfect! Thanks @Blacksmoke16

Yikes! This came back to bite me. Again! I think request.params is the last place anyone would expect surprises! And oddly this time, my patch (above) doesn’t even work! I get an empty hash with the same exact code that worked a few months ago! Can we please please have a fix real soon?

What do you suggest to fix this? If someone reads the request body before calling params, it will stop working, right?

@asterite Yes that’s right. It would be lovely if we had a request.params like above and also a request.uploads while we’re at it because that seems to be not so straightforward too.

To be clear I don’t think it would be as simple as adding that method. Yes it would solve for the case of allowing multiple handlers access to the params, but as you pointed out if another handler not using #request consumes the request body, it’ll make all future calls to that method return an empty params object.

In the end I don’t think there is a clean solution to this problem. Plus it’s simple enough to monkey patch that method in if you really need it. Maybe it’s worth asking why do you have multiple handlers trying to consume the same request data?

@Blacksmoke16 I’m not sure I understand, are you saying there’s no permanent fix other than monkey patching this problem? Moreover, the very patch that worked for me earlier (0.35 I think) now refuses to work for me, my handlers have remained the same during and after Crystal upgrade to 1.0.

I don’t understand the reason why you say you can monkey patch it in “if you really need it”. I’d think getting the params or uploads is an absolute need, for anyone building a web app!

Also, I don’t understand what you mean by multiple handlers trying to consume request data – are you suggesting that the current model only allows one handler per server block? The following was adapted from the sample on the docs. Aren’t Error, Log and Compress handlers too? What’s to stop them from peeking at the request params before we get a chance to?

May I’ve got this completely wrong but how do I rewrite the following so I don’t run into this issue again? Because if I understood you correctly you are asking me to restrict myself to CustomHandler and drop CustomHandler2 in the following example, right? If that’s so, that would be a severe limitation of the current model.

require "http"

class HTTP::Request
  def params
    @params ||= HTTP::Params.parse((query || "") + "&" + (body.try &.gets_to_end || ""))
  end
end

module Foo
  class CustomHandler
    include HTTP::Handler

    def call(context)
    #   params = HTTP::Params.parse((context.request.query || "") + "&" + (context.request.body.try &.gets_to_end || ""))
      pp context.request.params
      context.response.content_type = "text/plain"
      context.response.print "Hello world!"

      call_next(context)
    end
  end

  class CustomHandler2
    include HTTP::Handler

    def call(context)
    #   params = HTTP::Params.parse((context.request.query || "") + "&" + (context.request.body.try &.gets_to_end || ""))
      pp context.request.params
      context.response.content_type = "text/plain"
      context.response.print "Hello world!"

      call_next(context)
    end
  end
end

server = HTTP::Server.new([
  HTTP::ErrorHandler.new,
  HTTP::LogHandler.new,
  HTTP::CompressHandler.new,

  Foo::CustomHandler.new,
  Foo::CustomHandler2.new,
  HTTP::StaticFileHandler.new("public"),
])

server.bind_tcp "127.0.0.1", 9090
server.listen

All in all, IMO, while I understand it may be tricky to implement we need a reasonable and permanent fix for this issue to save others from the frustration and the time needed to discover, understand and patch (which may or may not work!).

I mean that if request.params was introduced, there are still cases where it wouldn’t work. I.e. if the request body has already been consumed before you call request.params for the first time. I.e. via gets_to_end or something. I.e. it fixes one problem, but also introduces other problems.

My thinking is this isn’t actually a big deal or others would have been complaining about it as well. For my use case I ended up making my own Request object that exposes this functionality, which works well for me.

What request are you making to the server? I did: curl -X post -d 'foo=bar&bar=baz' localhost:9090?id=1 and got the following output:

URI::Params(@raw_params={"id" => ["1"], "foo" => ["bar"], "bar" => ["baz"]})
URI::Params(@raw_params={"id" => ["1"], "foo" => ["bar"], "bar" => ["baz"]})
2021-05-30T04:28:25.646322Z   INFO - http.server: 127.0.0.1 - post /?id=1 HTTP/1.1 - 404 (125.88µs)

So seems fine to me?

Yes, those are handlers, but they’re not consuming the request body because they have no reason to do so. The main problem you’re having is that you can’t consume a request body more than once. Can you share some more info/code about why you need two separate handlers that are expected to both consume the request body and do different things? Why couldn’t they be combined into one handler that consumes the body then does something with it? In that case you wouldn’t even need this method as the consumption of the request body would be centralized and only happen once.

EDIT:

What would you propose that fix be tho? As I mentioned earlier it would seem that not too many people are actually running into this problem anyway. It’s kinda just how IOs work.

@Blacksmoke16 As someone who has heavily invested in building for their startup due to merits seen in the Crystal community I must say I’m very disheartened by that last line. A problem is a problem irrespective of how many complained about it as long as it’s legitimate and valid. But feelings aside –

How do you propose I write an application that serves regular users and admin users without resorting to over engineering and introducing micro services? I’d imagine from my past experience with other stacks that it would be like so:

### sinatra

#regular users
namespace("/quux/"){}

# admin users
namespace("/quux/admin/"){}

or

// Go
func userHandler(w http.ResponseWriter, r *http.Request){}
func adminHandler(w http.ResponseWriter, r *http.Request){}

func init(){
   http.HandleFunc("/quux/", userHandler)
   http.HandleFunc("/quux/admin/", adminHandler)
}

I could go on and I’m sure all of them use some kind of IO to process form data and uploads and yet they don’t seem to have this same issue! That tells me the model chosen by Crystal is broken and needs fixing.

My point with the CustomHandler and CustomHandler2 was to do the same, one handles regular user logic and the other admin and I wanted to keep them separate and not create one massive case block.

Will you be able to share this please?

It seems what you need is a router? The router will choose which logic to use, the user one or the admin one. That’s not something you usually do with handlers.

1 Like

I would agree there. I don’t think the usage of handlers ever envisioned each of them checking if the path is correct and then handling a request or passing it onwards. If that’s what you’re trying to do.

My perspective here is that handlers are never necessary for anything. You can very reasonably put everything into one method, but that method can, of course, delegate to other code.

(As an extension of my perspective, I would say that handlers are beneficial only when they’re generic and reusable, i.e. if you could reasonably extract it into a library.)

and I wanted to keep them separate and not create one massive case block.

The case block wouldn’t need to have any logic immediately in it; it would just be the selection logic.
And then if you don’t like the look of that, you indeed replace it with a router library.

And router libraries can vary: basic invocations might just be direct replacements to a case block, while more advanced uses might colocate the declarations of what paths are supported near the methods that handle them.


My example:
I use handlers only for very generic things, and that is followed by delegating all the core logic to one method, which invokes a router (delegating to another sub-method) and possibly does generic error handling:

Example how my homegrown router colocates path handling:

(and I have to stress the point that these annotations are indeed merely colocated; the actual logic is still generated in a centralized location by collecting those annotations)

2 Likes

As others have said, you probably want a router that given a path invokes a specific set of logic. Might also be worth looking into a framework, which would provide helpful abstractions for this sort of thing and would be a bit more robust than homegrown framework.

But aren’t these two handles mutually exclusive? If the request for a regular user, the user handler would be invoked, and if it’s for an admin the admin handler would be invoked. I.e. why is the user handler AND the admin handler BOTH consuming the request body? Shouldn’t you be checking the path and skipping that handler’s logic if it’s the wrong “type” of request? Because in that case, this would also avoid the problem.

https://github.com/athena-framework/athena/blob/master/src/request.cr#L104 Essentially the same as the monkey patch, just no longer within HTTP::Request itself.

1 Like

@oprypin Thank you for the background Oleh! That explains it and the troubles I’ve been having with this model as that’s exactly what I’ve been trying to do – replicate a style I know from other stacks using handlers. I’ll look at the options you and the others have mentioned. One last question, is there any chance such a router will make it into the core?