Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feature wish] High-level function for performing a GET that follows redirects #76

Closed
protz opened this issue Jan 5, 2014 · 14 comments
Closed

Comments

@protz
Copy link

protz commented Jan 5, 2014

There's a whole bunch of ways to perform redirections: 301, other 3xx HTTP codes (temporarily moved, etc.), Location: header, etc. etc.. I kind of wish for an extra "follow_redirects" parameter for the Cohttp_lwt_unix.Client.get function, that doesn't require me to do that myself. Curl has that, and it's bloody convenient :-).

Cheers,

~ jonathan

@avsm
Copy link
Member

avsm commented Jan 5, 2014

Right, I've been reluctant to add this into the core protocol library since there's just a lot of logic lurking there, including things like upgrading/downgrading HTTP versions, proxies, and so forth.

I'd like to build a Http_client module that either uses Cohttp to perform this, or as an alternative implementation will use libcurl (via the OCaml bindings) to satisfy the same request. The former is useful for Mirage, and the latter for checking conformance.

On 5 Jan 2014, at 17:30, Jonathan Protzenko [email protected] wrote:

There's a whole bunch of ways to perform redirections: 301, other 3xx HTTP codes (temporarily moved, etc.), Location: header, etc. etc.. I kind of wish for an extra "follow_redirects" parameter for the Cohttp_lwt_unix.Client.get function, that doesn't require me to do that myself. Curl has that, and it's bloody convenient :-).

Cheers,

~ jonathan


Reply to this email directly or view it on GitHub.

@protz
Copy link
Author

protz commented Jan 5, 2014

... and I've been reluctant to implement it myself :-).

I agree that it's painful to implement. If you were to piggyback on
libcurl, could you still offer an Lwt interface for it?

On Sun 05 Jan 2014 06:32:52 PM CET, Anil Madhavapeddy wrote:

Right, I've been reluctant to add this into the core protocol library
since there's just a lot of logic lurking there, including things like
upgrading/downgrading HTTP versions, proxies, and so forth.

I'd like to build a Http_client module that either uses Cohttp to
perform this, or as an alternative implementation will use libcurl
(via the OCaml bindings) to satisfy the same request. The former is
useful for Mirage, and the latter for checking conformance.

On 5 Jan 2014, at 17:30, Jonathan Protzenko [email protected]
wrote:

There's a whole bunch of ways to perform redirections: 301, other
3xx HTTP codes (temporarily moved, etc.), Location: header, etc. etc..
I kind of wish for an extra "follow_redirects" parameter for the
Cohttp_lwt_unix.Client.get function, that doesn't require me to do
that myself. Curl has that, and it's bloody convenient :-).

Cheers,

~ jonathan


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub
https://github.com/avsm/ocaml-cohttp/issues/76#issuecomment-31609874.

@avsm
Copy link
Member

avsm commented Jan 5, 2014

Just imagine the payoff if you do implement it though!
All those eager users and bug reports...

My suggest for a Http_client module would also include a HTTP_CLIENT
module type that would be satisfied by either the curl or Lwt backend.
The curl version will probably require Lwt_preemptive, which is icky.

Perhaps more interesting is trying a Js_of_ocaml backend for the
HTTP_CLIENT, which should just involve mapping to xmlhttprequest.

-anil

On 5 Jan 2014, at 17:37, Jonathan Protzenko [email protected] wrote:

... and I've been reluctant to implement it myself :-).

I agree that it's painful to implement. If you were to piggyback on
libcurl, could you still offer an Lwt interface for it?

On Sun 05 Jan 2014 06:32:52 PM CET, Anil Madhavapeddy wrote:

Right, I've been reluctant to add this into the core protocol library
since there's just a lot of logic lurking there, including things like
upgrading/downgrading HTTP versions, proxies, and so forth.

I'd like to build a Http_client module that either uses Cohttp to
perform this, or as an alternative implementation will use libcurl
(via the OCaml bindings) to satisfy the same request. The former is
useful for Mirage, and the latter for checking conformance.

On 5 Jan 2014, at 17:30, Jonathan Protzenko [email protected]
wrote:

There's a whole bunch of ways to perform redirections: 301, other
3xx HTTP codes (temporarily moved, etc.), Location: header, etc. etc..
I kind of wish for an extra "follow_redirects" parameter for the
Cohttp_lwt_unix.Client.get function, that doesn't require me to do
that myself. Curl has that, and it's bloody convenient :-).

Cheers,

~ jonathan


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub
https://github.com/avsm/ocaml-cohttp/issues/76#issuecomment-31609874.


Reply to this email directly or view it on GitHub.

@hcarty
Copy link
Contributor

hcarty commented Jan 6, 2014

The latest release of ocurl does have event support in its Curl.Multi module which may be usable from Lwt.

@avsm avsm modified the milestone: 1.0 Stable API Nov 9, 2014
@avsm avsm added the Feature label Nov 9, 2014
@trevorsummerssmith
Copy link
Contributor

I am building client redirect-following functionality for my aws client and would like to get it upstreamed once it is in a ready state. Is there any more discussion or design thoughts about a redirect-following client? If not, can I create some gists once I am ready and get discussion going? Thanks! @rgrinberg

@avsm
Copy link
Member

avsm commented Apr 9, 2015

See https://GitHub.com/avsm/opam-mirror for code that does this -- needs to be pulled out into an ocurl lib

On 9 Apr 2015, at 06:59, Trevor Summers Smith [email protected] wrote:

I am building client redirect-following functionality for my aws client and would like to get it upstreamed once it is in a ready state. Is there any more discussion or design thoughts about a redirect-following client? If not, can I create some gists once I am ready and get discussion going? Thanks! @rgrinberg


Reply to this email directly or view it on GitHub.

@rgrinberg
Copy link
Member

@trevorsummerssmith would be great to have this. Let me know if you need anything from me.

I do think we have to refactor the client to be a little more friendly to such added functionality. E.g. a client should be able to generate get/post/put/delete/etc. just by providing his own call function. @avsm what do you think?

@trevorsummerssmith
Copy link
Contributor

@rgrinberg Thanks! I have a bunch of thoughts on this. I need to write some more code before they will be more together. I'm aiming to have something ready to talk about early next week.

@rgrinberg
Copy link
Member

-1 On this in retrospect. I'd much rather cohttp focus on getting the low level layer right and allow for people to implement this on their own.

@1hko
Copy link

1hko commented May 28, 2018

Just to illustrate @rgrinberg's point...

I was writing a simple Client.t for accessing a remote using basic auth. The remote sometimes responds with redirects and the client below is capable of following them. I wrote the cases that cover my needs but there's countless status codes that just fall through and could raise an UnhandledStatus exception if used on endpoints that return other statuses. It only supports GET requests, which again is all my project needed.

This is quite a bit of wiring but it was an enlightening experience for me. I now realize how difficult it would be to write something that handles all cases automatically while maintaining grace

The code here is part of my first project in Ocaml. I'm new to the language so I'm interested to see how other people solve this using cohttp

client.ml

open Core
open Async

exception TooManyRedirects [@@deriving sexp]
exception UnhandledStatus of Cohttp.Code.status_code [@@deriving sexp]

type t =
  string
[@@deriving sexp]

let make user pass =
  let cred = `Basic (user, pass) in
  Cohttp.Auth.string_of_credential cred

let rec get ?interrupt ?(max_redirects = 5) t uri =
  let headers = Cohttp.Header.of_list [ "authorization", t ] in
  let req = Cohttp.Request.make_for_client ~headers `GET uri in
  Cohttp_async.Client.request ?interrupt ~uri req
  >>= fun (res, body) ->
  return (req, res, body) (* my program needs access to each of these data members *)
  >>= follow_redirect t max_redirects

and follow_redirect t max_redirects (req, res, body) =
  if max_redirects = 0 then
    raise TooManyRedirects
  else
    let status = Cohttp.Response.status res in
    match status with
    | `OK -> return (req, res, body)
    | `Found 
    | `Moved_permanently
    | `Temporary_redirect -> (* wide variety of cases to handle *)
      handle_redirect t (req, res, body)
      >>= follow_redirect t (max_redirects - 1)
    | _ as s -> raise (UnhandledStatus s) (* many unhandled cases *)

and handle_redirect t (req, res, body) =
  let headers = Cohttp.Response.headers res in
  let location = Cohttp.Header.get headers "location" in
  match location with
  | None -> return (req, res, body) (* redirect without location header could signal an error *)
  | Some url -> get t (Uri.of_string url)

@brendanlong
Copy link
Contributor

I started working on a higher-level Cohttp-based client in https://github.com/brendanlong/blue-http which handles this (among other things).

The redirect logic is relatively simple if anyone wants to steal it, and then you can wrap any Cohttp_async.Client function with Redirect.with_redirects ?max_redirects uri (fun uri -> ...)

@hcarty
Copy link
Contributor

hcarty commented Aug 19, 2020

@brendanlong Maybe we can collaborate a bit? I have ezrest which seems like it's pretty similar - https://github.com/hcarty/ezrest

I've been using ezrest for years but haven't gotten around to making a formal release.

@brendanlong
Copy link
Contributor

Yeah I'd like to make a thing that usable for multiple people. I'm not sure how quickly I can get to it though. My version currently only supports Async and yours uses Lwt. I think the next step for me is breaking out the connection pool and making it work with both Async and Lwt. I'm just incredibly busy so I don't know if I can realistically collaborate at a useful speed :\

@mseri
Copy link
Collaborator

mseri commented Apr 18, 2021

I have added examples on how to handle redirects in the README, created a ticked to add it to the documentation, and pointed at this issue. I am going to close it for now.

@mseri mseri closed this as completed Apr 18, 2021
mseri added a commit to mseri/ocaml-cohttp that referenced this issue Jun 16, 2022
lib: remove use of codegen after dropping support for ocaml 4.02.3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants