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

enable callers to handle Client.Download errors #341

Closed
ryepup opened this issue Oct 29, 2024 · 2 comments · Fixed by #343
Closed

enable callers to handle Client.Download errors #341

ryepup opened this issue Oct 29, 2024 · 2 comments · Fixed by #343

Comments

@ryepup
Copy link
Contributor

ryepup commented Oct 29, 2024

I'm using the golang client https://pkg.go.dev/github.com/maxmind/geoipupdate/[email protected]/client#Client.Download, and would like a better way to differentiate between permanent errors and transient errors.

Right now my code looks something like:

client := client.New(accID, licenseKey)

backoff.Retry(func() error {
	resp, err := client.Download(ctx, edition, hash)
	if err != nil {
		// is this an error that won't be fixed by a retry?
		if (strings.Contains(err.Error(), "AUTHORIZATION_INVALID") ||
			strings.Contains(err.Error(), "LICENSE_KEY_INVALID")) {
			return backoff.Permanent(err)
		}
		// retry
		return err
	}
	// <rest omitted for brevity>
}, backoff.BackOff(...))

I don't love relying on the Error() containing certain strings to decide if I should retry or not.

Would you consider providing some mechanism for inspecting the error so I could retry as needed based on https://dev.maxmind.com/geoip/docs/web-services/responses/#errors?

Some implementation ideas:

  • export the underlying github.com/maxmind/geoipupdate/v7/internal/HTTPError so I can use that with errors.As; a type alias in the package client should do it type HTTPError = internal.HTTPError
  • export the internal.IsPermanentError func
@oschwald
Copy link
Member

oschwald commented Nov 1, 2024

Exporting the HTTPError error does make sense to me. We'd be happy to accept a PR with that change.

@ryepup ryepup mentioned this issue Nov 5, 2024
@ryepup
Copy link
Contributor Author

ryepup commented Nov 5, 2024

Great! opened #343.

oschwald added a commit that referenced this issue Nov 18, 2024
7.1.0

* Allow the `Host` configuration directive and the `GEOIPUPDATE_HOST`
  environment variable to accept a value with the scheme set. If not set, it
  will continue to default to `https://`. Pull request by Gabe Cook. GitHub
  #310.
* Export `HTTPError` to enable fine-grained error handling for users of
  `github.com/maxmind/geoipupdate/client`. Pull request by Ryan Davis. GitHub
  #341.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants