yhirose / cpp-httplib

A C++ header-only HTTP/HTTPS server and client library
MIT License
13.04k stars 2.3k forks source link

Params option missing from client Get calls ? #831

Closed al3ph closed 3 years ago

al3ph commented 3 years ago

Hi, Nice library :)

But it seems there is no support for passing Params using the client Get calls.

As far as I can tell this is a valid use of GET. Am I missing something ?

PixlRainbow commented 3 years ago

Params refers to the html form POST formats, which can be either application/x-www-form-urlencoded or multipart/form-data. As GET requests should not have a request body, these Params cannot be used.

You are perhaps thinking of using URL-embedded parameters (where parameters are part of the path string)?

al3ph commented 3 years ago

Yep, URL-embedded, does GET support passing them as a structure (like params), or do I manually have to build the string and url encode it.

ta

yhirose commented 3 years ago

@al3ph, as you can see from the @PixlRainbow's comment, GET requests currently need URL-embedded parameters. I know it requires tedious work to concatenate query parameters on our side... If other libraries like curl support it and you think it's a reasonable enhancement, I am ok to look into it.

al3ph commented 3 years ago

It'd be nice if it did handle it for you, not so much the concatenation, but the url encoding, or does that already happen when you pass the path string ?

yhirose commented 3 years ago

@al3ph, you can see a number of examples of Get requests using the url encoding in test/test.cc like the following: https://github.com/yhirose/cpp-httplib/blob/8d9a477edb3e9ae962a9a7aeb20bf70b571093a1/test/test.cc#L367-L369 Hope it helps.

yhirose commented 3 years ago

@al3ph, I had to make a number of changes in test/test.cc after I added Result Get(const char *path, const Params &params). https://github.com/yhirose/cpp-httplib/pull/835/files

It's because the compiler gets confused with Result Get(const char *path, const Headers &headers). Here is an example:

  //auto res = cli_.Get("/streamed", {{make_range_header({{2, 3}})}}); // Current
  auto res = cli_.Get("/streamed", Headers{{make_range_header({{2, 3}})}}); // New code. It requires `Headers`

I would not like to break any existing users codebase with this change. So I'll ponder over this change if it's worth to do it. Thanks for your understanding.

ghost commented 3 years ago

@yhirose Maybe... introduce something new, like Server::get_with_params? Doesn't sound ideal though.

yhirose commented 3 years ago

@00ff0000red, we are talking about Client::Get, not Server.

yhirose commented 3 years ago

@00ff0000red, I ended up adding the following API which doesn't break any users' existing codes.

Result Get(const char *path, const Params &params, const Headers &headers, Progress progress = nullptr);

The only downside of this approach is that you have to pass Hears{} even if you don't add any extra HTTP header.

cli.Get("/path", {{"key1", "val1"},{"key2","val2"}}, Headers{});

Hope you enjoy the new APi.

al3ph commented 3 years ago

Great, thanks!! I'll give it a spin.