Download artifacts using HTTP

Question

The public code is SaveAsFile(). It takes a URL, checksum and authentication info, and downloads the artifact and its hash using HTTP. It saves the content as a file and returns the file's path on success, otherwise an error.

As I'm fairly new to Go I would like to get some feedback on the error handling and on any ways to write this code more elegantly?

https://go.dev/play/p/8i_xZ-EVTvP

import (
    "io"
    "io/ioutil"
    "net/http"
    "os"
    "strings"
    "time"

    "github.com/pkg/errors"
)

var errCode = errors.New("bad response status code")
var errFileNotWritten = errors.New("file not written")
var errDigest = errors.New("digest are not identical")

const (
    statusMsg        = `code: %d, response body:\n%s`
    notWritten       = `unable to write file %s from Response body`
    msgDigestFailure = `target: \"%s\", file: \"%s\")`
    msgFileNotClosed = `unable to close file %s`
)

func respBody(url string, user string, pass string) (io.ReadCloser, error) {
    client := &http.Client{
        Timeout: time.Second * 10,
    }
    req, err := http.NewRequest("GET", url, nil)
    if err != nil {
        return nil, err
    }

    if user != "" && pass != "" {
        req.SetBasicAuth(user, pass)
    }
    resp, err := client.Do(req)
    if err != nil {
        return nil, err
    }
    if resp.StatusCode < 200 || resp.StatusCode >= 300 {
        return nil, errors.Wrapf(errCode, statusMsg, resp.StatusCode, resp.Body)
    }
    return resp.Body, nil
}

func classify(url string, user string, pass string) (string, error) {
    const suffix = ".sha512"
    rb, err := respBody(url+suffix, user, pass)
    if err != nil {
        return "", err
    }
    defer rb.Close()
    bodyBytes, err := ioutil.ReadAll(rb)
    if err != nil {
        return "", err
    }

    return string(bodyBytes), nil
}

func downFile(url string, user string, pass string) (*os.File, error) {
    responseBody, err := respBody(url, user, pass)
    if err != nil {
        return nil, err
    }
    defer responseBody.Close()

    file, err := ioutil.TempFile("", "*")
    if err != nil {
        return nil, err
    }
    written, err := io.Copy(file, responseBody)
    if err != nil {
        return nil, errors.Wrapf(err, notWritten, file.Name())
    } else if written <= 0 {
        return nil, errors.Wrapf(errFileNotWritten, notWritten, file.Name())
    }

    if err := file.Close(); err != nil {
        return nil, errors.Wrapf(err, msgFileNotClosed, file.Name())
    }

    return file, nil
}


func SaveAsFile(url string, digest string, user string, pass string) (string, error) {
    file, err := downFile(url, user, pass)
    if err != nil {
        return "", err
    }
    if digest != "" {
        targetDigest, err := classify(url, user, pass)
        if err != nil {
            return "", err
        }
        if strings.Compare(targetDigest, digest) != 0 {
            return "", errors.Wrapf(errDigest, msgDigestFailure, targetDigest, digest)
        }
    }
    return file.Name(), nil
}

Answer 1

First I want to say that your code is very well written and idiomatic for someone who is new to Go.

Here are a few suggestions:

• Prefer standard library tools for error handling rather than using an external library. errors.Wrapf can be replaced by fmt.Errorf.
• The %q format verb will print a string in quotes. Prefer using %q instead of \"%s\".
• Call file.Close with defer, so that if writing to the file fails it will still get closed. The documentation for file.Close specifies that it will only return an error if the file is already closed, so you could consider just ignoring any error from file.Close.
• Also if writing to the temp file fails, downFile should probably delete the file to avoid leaving extra empty files.
• This is kind of subjective, but I think downFile should return the file name instead of an *os.File because the file is already closed, so any attempt to read or write it would fail.
• Strings can be compared directly with !=, no need to call strings.Compare.
• SaveAsFile has a lot of string arguments, and some of them are optional. If a caller calls SaveAsFile(url, digest, "", ""), it's not obvious that the empty strings are the username and password.

What you can use for this last point is the option pattern, I would define SaveAsFile as follows:

type opts struct {
    user string
    pass string
    digest string
}

type SaveAsFileOption func(*opts)

func Auth(user, pass string) SaveAsFileOption {
    return func(o *opts) {
        o.user = user
        o.pass = pass
    }
}

func Digest(digest string) SaveAsFileOption {
    return func(o *opts) {
        o.digest = digest
    }
}

func SaveAsFile(url string, options SaveAsFileOption...) {
    var o opts
    for _, opt := range options {
        opt(&o)
    }
    // Then you can the options through o.user, o.pass, etc.
}

Callers can call SaveAsFile either with SaveAsFile(url), or with SaveAsFile(url, Digest(digest), Auth(user, pass)). This also gives you the flexibility to define more options in the future.