1. 39
  1. 5

    I am not too familiar with Go, however, in the sample code,

      if err := requests.URL("http://foo/t.json").ToJSON(&t).Fetch(ctx); err == nil {
        return nil, err
      }
    

    err is defined as the result of the requests call, and then compared against nil. If it is nil, then nil, err are both returned.

    Is that not just returning nil, nil? How is err a useful response value here, it seems functionally useless.


    On a separate point, I find it interesting that you decided to declare headers as a method call on the existing class, as opposed to a single header method that can be chained or accept Go’s varargs equivalent. Do you have a generic header method? If not, you might consider that for the future, as currently this doesn’t support custom headers that some web apis might need, and means it is less future-compatible.

    1. 2

      you’re missing the &t part which essentially acts as an ‘out param’ that writes into the var t T declared before. with that in place, the actual return value can be just an (optional) err value

      1. 12

        No, they’re talking about the ‘err’ in ‘nil, err’. Which does appear to be nil. Presumably that’s supposed to say ‘err != nil’? I’m taking this as further evidence of Go’s error handling being poor do to lack of ADTs, that mistake would have been much more obvious with a Result type…

        1. 6

          oops, yeah, i guess that condition should’ve read err != nil instead. which kinda proves your point: it’s very easy to accidentally make mistakes. on top of that, i think that the golang habit to put statements in between the if keyword and the actual condition only adds to the confusion. case in point: by the time i reached the end of that jam-packed line i got so tired that it didn’t stand out that the condition was accidentally flipped 🙃

      2. 1

        Typo fixed, thanks.

      3. 5

        (this is a bit off topic or more focused on what this is in response to, so less a comment on the library and more to the fact that stdlib gets replaced more frequently as well as some thoughts on the linked problems laid out by Brad Fitzpatrick. I hope you can ignore if that’s too off topic. Great library though!)

        I think that status codes should be handled by the consumer. That’s by HTTP’s design.

        Why is only 2xx valid? Both 3xx and 1xx are clearly not errors. Why not use 201 and 204 for additional confirmation? What about handing redirects, caching, etc. in the 3xx range? 404 and other parts of 4xx can be very valid non error cases depending on the application.

        What I mean with “by HTTP’s design” is that it has the problem of mixing layers. A great example is APIs doing something like sending JSON, probuf, etc. responses indicating the equivalent of Not Found, Internal Error or Forbidden. I think that’s a mismatch between HTTP’s design and how it’s used today.

        And then there’s the philosophical/bike shedding argument on whether an HTTP library should return an error when everything worked perfectly on the HTTP side, when there’s just a different status code than the application side expects. That can get even more philosophical, when arguing on differences between 4xx and 5xx, which are clearer “hard errors”.

        While I see the benefit of having no boiler code on a small/demo project I think overoptimizing for it can do a lot of harm, because you’ll start fighting the defaults.

        Same if you in the case it net/http actually benefit from manually closing the body.

        For every bigger project you will very likely end up with some form of wrapper anyways. And be it just to deal with things like authentication, setting more meaningful user agent strings, doing additional verification, adding custom headers, etc.

        That’s not an argument against this library, but more an argument for having clearer defined abstraction layers and building blocks so that projects like these could be easier to create. I think net/http and actually both the server and the client suffers a bit from having had different goals over its lifetime. At least that’s what it looks like to me. It seems like the goal was “an HTTP library” but not making clear what that implies.

        I’m also seeing that in the context of a recent thread of the rest example using an external library/framework (and containing hugs) in place of the perfectly suitable net/http. I think the problem here too is whether the library would give you building blocks or more a standard way to do things on a higher level, so closer to what people call frameworks.

        Of course it can do both and the fact that people are able to extend net/http and other standard libraries show that. It also shows that people clearly have interest in using the standard library instead of wrappers and reimplantations, but it seems from rsc’s response on the linked topic that the Go project wants to push external libraries more. So in that case it might make sense to like other language see the standard library more as a building blocks provider (like must b other stdlib libraries, like io, sql, strings, bytes, etc. do).

        Of course that might not make people happy, who like Go for offering more than that and therefore go through great lengths to make things integrate well with net/http (and sql and io). I think the biggest problem is when people feel forced to skip stdlib completely and rewrite things from scratch, maybe copying parts. Because at that point even having providing a standard library option that needs to be maintained starts to become pointless. That would be kind of sad, because it used to be a selling point of go.

        1. 1

          One nice thing with requests is you change what statuses (if any) are considered errors and test for those errors downstream. Here’s a case where I change my message to users based on the status returned by a request to Google Docs: https://github.com/spotlightpa/nkotb/blob/a61016068e9d31378aa05da49eadafd12b31c4bd/pkg/clis/nkotbweb_routes.go#L198-L206

          1. 1

            Yes exactly what I meant. Great example.

            Just meant to say that one always has to either check the status code or remember what is an error. But since people in one way or another end up wrapping HTTP requests outside of examples and quick and dirty scripts it’s not usually the thing that causes a problem.

            As in one still needs to look which statues are errors and how to handle them.

            In some APIs you also have the problem that you still need b the body from “status code errors” to know what’s actually going on. But that’s all very API-dependent.

        2. 3

          Cool! I love seeing the Builder pattern here. I remember (maybe incorrectly) it coming up and being criticized in a Go Time podcast. Though we do see similar patterns in other languages as well as some usages in Go (https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/config#LoadOptionsFunc). I’m not saying this library proves that it’s the ideal pattern, but does show that it has its place.

          Wish you the best with this library!

          1. 2

            Personally, normal struct literal syntax looks the cleanest and clearest to me:

            req := Request{URL: url, Handler: ToJSON(&t)}
            

            There’s probably a time and a place for the builder pattern, but it seems pretty rare IMO. Would like to hear your opinion.

            1. 3

              A simple struct literal alone doesn’t work well when the default isn’t a natural “empty” value.

              With a builder, you can do validation in the With or Build method to ensure you have a properly constructed object at all times. You can also indicate which related parameters belong together.

              req, err := RequestBuilder.
                  WithUrl(url).
                  WithBasicAuth(user, pass).
                  WithSerializer(customFn).
                  Build()    // sets certain non-nil default values, like a custom DNS resolver
              
              1. 2

                I’m not sure what you mean? How does the builder pattern address the lack of a natural “empty” value (specifically with a “Do()” or “Run()” method filling in the default values as necessary)? From your example, which field or fields would have an unnatural empty value?

                1. 1

                  Assume Serializer must be a non-nil pointer. Perhaps it satisfies an interface of

                  type Serializer func(t interface{}) err
                  

                  And you want the built “request” to use some default serializer, but you can configure it with a parameter. The Build method can look through the fields of the RequestBuilder object and choose between the default one or the one passed by WithSerializer.

              2. 1

                It can’t be Request{URL: url, Handler: ToJSON(&t), ...} because it needs to be imported and executed, so it ends up as

                err := requests.Do(ctx, requests.Options{
                    URL: url, 
                    Handler: requests.ToJSON(&t), 
                    ...
                })
                

                which is a little bit ugly, IMO. Tastes vary though. I like that for simple requests you end up only having a single package name invocation.

                1. 1

                  Yeah, I agree that the package name qualifier degrades the ergonomics. I usually import under a short name (e.g., pz “github com/weberc2/httpeasy”) which makes things better but not great. It probably wouldn’t be the end of the world to import it as . either. Not a big deal either way.

            2. 3

              This is a nitpick, but I would like the API more if it would have allowed for passing a body read function instead of hardcoded ToJson, ToString, etc

              If you’re interested I can see if I can put together a PR.

              1. 4

                Unless I’m totally misunderstanding, this already exists but wasn’t spelled out in the article. When you say body, do you mean request body or response body?

                1. 3

                  It’s not in the post, but it already allows passing a function with Handle. In fact ToJson, ToString are implemented using it.

                  1. 1

                    Nice. :)

                2. 2

                  At the risk of sounding self-promotion-y, the aesthetic of your HTTP client library reminds me of my HTTP server library, especially the resemblance between your BodyGetter abstraction and my Serializer (flip sides of the same coin–BodyGetter returns a request body reader and Serializer returns a response body writer). Certainly your quote describes the philosophy that guided the design of my library, although I’m not satisfied that I attained it:

                  A good tool should be able to work with requests in general, whether simple or complex, and make all of them more declarative and easier to understand.

                  That said, I do have a couple of criticisms with some of the rationalizations that you gave for using the builder pattern (or perhaps you’re seeing something I’m not seeing and I’ll learn something from the exchange):

                  But given the way that the parts of a request interact, this ends up being a poor fit. In a POST request, do you have separate fields for BodyBytes vs. BodyJSON? What happens if they both get set?

                  This argument seems a little suspicious. With a normal configuration struct, I would expect that you would have a single Body field which can be satisfied by both a BodyBytes() or a BodyJSON(). If I’m not mistaken, this is what your Builder does; however, contrary to the implied claim, I don’t think your Builder pattern provides any safety guarantees with respect to providing both a BodyBytes() and a BodyJSON() or even providing multiple calls of BodyBytes() with the same arguments.

                  For example:

                  // compiler error
                  Request{
                      URL: url,
                      Body: BodyJSON(...),
                      Body: BodyBytes(...),
                  }
                  
                  // compiler error
                  Request{
                      URL: url,
                      Body: BodyBytes(foo),
                      Body: BodyBytes(bar),
                  }
                  
                  // no compiler error
                  URL(url).BodyBytes(...).BodyJSON(...)
                  
                  // no compiler error
                  URL(url).BodyBytes(goodData).BodyBytes(badData)
                  

                  What about the parts of a URL (host, path, query)? How do you build a URL using a config struct? Should you break it down into a URL helper struct that feeds into a request helper?

                  Why not:

                  type Request struct {
                      URL string
                  
                      // ... overrides the scheme from `URL`
                      Scheme string
                  
                      // ... overrides the host from `URL`
                      Host string
                  
                      // ... overrides the path from `URL`
                      Path string
                  }
                  

                  This seems like exactly the same behavior as your builder pattern provides (but without the aforementioned “same field provided multiple times” issue).

                  Personally, if I really wanted to support the ability to compose a URL from distinct parts, I’d just do something like this:

                  Request {
                      URL: &url.URL{Scheme: "https", Host: "example.org"},
                  }
                  
                  Request {
                      URL: MustParse("https://example.org"),
                  }
                  

                  Lastly and most importantly, the quote on your blog is hilarious and I hadn’t heard it before:

                  It should be noted that no ethically-trained software engineer would ever consent to write a DestroyBaghdad procedure. Basic professional ethics would instead require him to write a DestroyCity procedure, to which Baghdad could be given as a parameter.

                  1. 1

                    There’s nothing stopping you from calling Builder.Body() twice, but that’s deliberate, because the last call wins, so you can override a default you inherit from a parent. Similarly for the paths, queries, headers, and validators they stack, so you can add multiple if you want without requiring an ugly Path: []string{"/foo"}, for the single case. A decision that I waffled on is that the b.Query("foo", "bar").Query("foo", "baz") results in just foo=baz and if you want foo=bar&foo=baz you have to do b.Query("foo", "bar", "baz"). I dunno, at a certain point, it’s a matter of personal taste, and you probably could make a really nice option struct version, but I like how the builder version turned out.

                    1. 1

                      I agree with all of this. I happen to prefer the struct version a bit more, at least in Go (builder feels a bit less natural as opposed to Rust where method chains are more common), but I never feel tempted to make a fuss one way or another. On at least one occasion I’ve used something similar to builder because the struct pattern failed me, and that was with respect to an outer struct which wrapped an inner struct alongside a bitmap representing which of the wrapped struct’s fields were actually set. In this case, encapsulation is important or else a caller might set a field and forget to update the bitmap or vice versa—given that consistency constraint the builder pattern was natural and strictly better than the struct initializer approach.

                  2. 2

                    I wonder if it is useful to have something like ToFile, so the following code block:

                      f, err := os.Create(destPath)
                      if err != nil {
                        return err
                      }
                      defer f.Close()
                    
                      return requests.
                        URL(url).
                        Client(&app.hc).
                        ToWriter(f).
                        Fetch(context.Background())
                    

                    can be written as:

                      return requests.
                        URL(url).
                        Client(&app.hc).
                        ToFile(destPath).
                        Fetch(context.Background())
                    
                    1. 2

                      I avoided that because I didn’t want to impose my own choices about how the file should be written (atomically or not etc) and what permissions, but thinking about it more, maybe I should bite the bullet and do it.

                      1. 2

                        maybe I should bite the bullet and do it.

                        I created a PR for it: https://github.com/carlmjohnson/requests/pull/12.

                    2. 2

                      You should really contribute some of these ideas to the discussion in the Go community about the HTTP client!

                      1. 1

                        I participated in the discussion when it was new, but I don’t think it’s going to come back to life unless BradFitz feels like it. Seemed like a labor of love and then he got busy with other stuff.

                      2. 2

                        maybe it is ~10yrs of familiarity with net/http, but I find the net/http to be FAR more readable.

                        The method chaining in the examples leave me scratching my head wonder what it does. I’ve no idea what ToJSON would do in the circumstances of those method chains. The lack of an HTTP verb makes me wonder is that doing a GET and serializing the response JSON? Or is it doing a POST and sending the JSON. I can’t immediately tell and that suggests to me that it is not explicit enough for me.

                        I’m sure I’d get used to it as I use the library. Compared to net/http it isn’t as immediately obvious.

                        Edit: having looked at the source: https://github.com/carlmjohnson/requests/blob/main/builder.go I now see the title is a lie. This isn’t “my own go http client” this is a layer on top of net/http.

                        1. 3

                          I changed the first paragraph to “HTTP client helper library for Go” to clear up this misunderstanding, which someone also asked about on Reddit. Making your own HTTP parser is a thing people do, but much more work, and not really what I want to do unless I get paid for it.

                          For the record, the famous Requests Python library also does not parse HTTP. It’s a wrapper around urllib3. It’s very common to use “http client” to just mean wrapper around some HTTP library.