switch to http client for pushover notifier #1490
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,24 @@ | ||
| package pushover | ||
|
|
||
| import ( | ||
| "context" | ||
| "encoding/json" | ||
| "net/http" | ||
| "net/url" | ||
| "strconv" | ||
| "strings" | ||
| "time" | ||
|
|
||
| "github.com/crazy-max/diun/v4/internal/model" | ||
| "github.com/crazy-max/diun/v4/internal/msg" | ||
| "github.com/crazy-max/diun/v4/internal/notif/notifier" | ||
| "github.com/crazy-max/diun/v4/pkg/utl" | ||
| "github.com/pkg/errors" | ||
| "github.com/rs/zerolog/log" | ||
| ) | ||
|
|
||
| const pushoverAPIURL = "https://api.pushover.net/1/messages.json" | ||
|
|
||
| // Client represents an active Pushover notification object | ||
| type Client struct { | ||
| *notifier.Notifier | ||
|
|
@@ -38,11 +46,15 @@ func (c *Client) Send(entry model.NotifEntry) error { | |
| token, err := utl.GetSecret(c.cfg.Token, c.cfg.TokenFile) | ||
| if err != nil { | ||
| return errors.Wrap(err, "cannot retrieve token secret for Pushover notifier") | ||
| } else if token == "" { | ||
| return errors.New("Pushover API token cannot be empty") | ||
| } | ||
|
|
||
| recipient, err := utl.GetSecret(c.cfg.Recipient, c.cfg.RecipientFile) | ||
| if err != nil { | ||
| return errors.Wrap(err, "cannot retrieve recipient secret for Pushover notifier") | ||
| } else if recipient == "" { | ||
| return errors.New("Pushover recipient cannot be empty") | ||
| } | ||
|
|
||
| message, err := msg.New(msg.Options{ | ||
|
|
@@ -60,16 +72,77 @@ func (c *Client) Send(entry model.NotifEntry) error { | |
| return err | ||
| } | ||
|
|
||
| cancelCtx, cancel := context.WithCancelCause(context.Background()) | ||
| timeoutCtx, _ := context.WithTimeoutCause(cancelCtx, *c.cfg.Timeout, errors.WithStack(context.DeadlineExceeded)) //nolint:govet // no need to manually cancel this context as we already rely on parent | ||
| defer func() { cancel(errors.WithStack(context.Canceled)) }() | ||
|
|
||
| form := url.Values{} | ||
| form.Add("token", token) | ||
| form.Add("user", recipient) | ||
| form.Add("title", string(title)) | ||
| form.Add("message", string(body)) | ||
| form.Add("priority", strconv.Itoa(c.cfg.Priority)) | ||
| if c.cfg.Sound != "" { | ||
| form.Add("sound", c.cfg.Sound) | ||
| } | ||
| if c.meta.URL != "" { | ||
| form.Add("url", c.meta.URL) | ||
| } | ||
| if c.meta.Name != "" { | ||
| form.Add("url_title", c.meta.Name) | ||
| } | ||
| form.Add("timestamp", strconv.FormatInt(time.Now().Unix(), 10)) | ||
| form.Add("html", "1") | ||
|
|
||
| hc := http.Client{} | ||
| req, err := http.NewRequestWithContext(timeoutCtx, "POST", pushoverAPIURL, strings.NewReader(form.Encode())) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| req.Header.Set("Content-Type", "application/x-www-form-urlencoded") | ||
| req.Header.Set("User-Agent", c.meta.UserAgent) | ||
|
|
||
| resp, err := hc.Do(req) | ||
| if err != nil { | ||
|
There was a problem hiding this comment. @crazy-max why not check the status code of the response here? According to the docs, a non-200 response code will not generate an error. If the user entered incorrect credentials and you get a 401/403 here, for example, there's an opportunity to provide a much better error message. There was a problem hiding this comment. You only ever consume/log the StatusCode in case of a failure to parse JSON, which may not fail in the case of non-success responses because the error body might be JSON formatted. This may lead to very confusing behavior for the user to diagnose. |
||
| return err | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.Header != nil { | ||
| var appLimit, appRemaining int | ||
| var appReset time.Time | ||
| if limit := resp.Header.Get("X-Limit-App-Limit"); limit != "" { | ||
| if i, err := strconv.Atoi(limit); err == nil { | ||
| appLimit = i | ||
| } | ||
| } | ||
| if remaining := resp.Header.Get("X-Limit-App-Remaining"); remaining != "" { | ||
| if i, err := strconv.Atoi(remaining); err == nil { | ||
| appRemaining = i | ||
| } | ||
| } | ||
| if reset := resp.Header.Get("X-Limit-App-Reset"); reset != "" { | ||
| if i, err := strconv.Atoi(reset); err == nil { | ||
| appReset = time.Unix(int64(i), 0) | ||
| } | ||
| } | ||
| log.Debug().Msgf("Pushover app limit: %d, remaining: %d, reset: %s", appLimit, appRemaining, appReset) | ||
|
There was a problem hiding this comment. You don't want to return early with an error here? Don't we already know the POST failed due to rate limiting if we get here? Won't this run the risk of the app assuming the POST was successful and recording it as such in the DB, when in reality it never went through? I think ideally there'd be some mechanism to automatically retry the request later after Having that many images need an update at the same time, such that the rate limit is exhausted, might be somewhat of an edge case or only a first-run concern... But IMO it just speaks to the reliability & "completeness" of the Pushover integration! There was a problem hiding this comment. Ah forgive me, I misread this I guess the sort of thing I described above would need to live in an
Owner
Author
There was a problem hiding this comment. Yes didn't want yet to bail out if rate-limiting detected but we could as follow-up. Just wanted to log it first. |
||
| } | ||
|
|
||
| var respBody struct { | ||
| Status int `json:"status"` | ||
| Request string `json:"request"` | ||
| Errors []string `json:"errors"` | ||
| User string `json:"user"` | ||
| Token string `json:"token"` | ||
| } | ||
|
|
||
| if err = json.NewDecoder(resp.Body).Decode(&respBody); err != nil { | ||
|
There was a problem hiding this comment. Suggest logging the full unparsed response body (e.g. as a string) when debug logging turned on. As well as basic HTTP stuff like status code.
Owner
Author
There was a problem hiding this comment. It is next line if unmarshal fails: diun/internal/notif/pushover/client.go Line 141 in a70601f There was a problem hiding this comment. Oh yeah I meant unconditionally log it! Not just on failure to parse as JSON. I'm not sure if that's a convention you follow, but I find it useful to have those sorts of things in verbose logs for providing more context when diagnosing issues -- even if the call is successful. That's what the Debug log is for, IMO 🙂 There was a problem hiding this comment. And actually... On a second look, I'm not sure the existing log statement will work as intended. It's trying to serialize the struct, which won't be populated since JSON deserialization failed. I think you need to read the body as a string and log that instead. |
||
| return errors.Wrapf(err, "cannot decode JSON body response for HTTP %d %s status: %+v", resp.StatusCode, http.StatusText(resp.StatusCode), respBody) | ||
| } | ||
| if respBody.Status != 1 { | ||
| return errors.Errorf("Pushover API call failed with status %d: %v", respBody.Status, respBody.Errors) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might make sense to log the request body under Debug. (Maybe with token sanitized?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think we could