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

migrate to zap logging #135

Merged
3 changes: 1 addition & 2 deletions api/clinic.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package api
import (
"context"
"encoding/json"
"fmt"
"net/http"

"go.uber.org/zap"
Expand Down Expand Up @@ -89,7 +88,7 @@ func (a *Api) InviteClinic(res http.ResponseWriter, req *http.Request, vars map[
existingInvite, err := a.checkForDuplicateClinicInvite(ctx, clinicId, inviterID)
if err != nil {
a.sendError(ctx, res, http.StatusInternalServerError, STATUS_ERR_FINDING_CONFIRMATION, err,
fmt.Sprintf("clinic %s user already has or had an invite from %v", clinicId, inviterID))
zap.String("inviterID", inviterID), "clinic already has or had an invite")
return
}
if existingInvite {
Expand Down
22 changes: 11 additions & 11 deletions api/clinicianInvites.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,9 @@ func (a *Api) SendClinicianInvite(res http.ResponseWriter, req *http.Request, va
return
}

code, msg, optionalErr := a.sendClinicianConfirmation(req, confirmation)
if code != 0 {
a.sendError(ctx, res, code, msg, optionalErr)
msg, err := a.sendClinicianConfirmation(req, confirmation)
if err != nil {
a.sendError(ctx, res, http.StatusInternalServerError, msg, err)
return
}

Expand Down Expand Up @@ -147,9 +147,9 @@ func (a *Api) ResendClinicianInvite(res http.ResponseWriter, req *http.Request,
confirmation.UserId = invitedUsr.UserID
}

code, msg, optionalErr := a.sendClinicianConfirmation(req, confirmation)
if code != 0 {
a.sendError(ctx, res, code, msg, optionalErr)
msg, err := a.sendClinicianConfirmation(req, confirmation)
if err != nil {
a.sendError(ctx, res, http.StatusInternalServerError, msg, err)
return
}

Expand Down Expand Up @@ -365,16 +365,16 @@ func (a *Api) CancelClinicianInvite(res http.ResponseWriter, req *http.Request,
}
}

func (a *Api) sendClinicianConfirmation(req *http.Request, confirmation *models.Confirmation) (code int, msg string, err error) {
func (a *Api) sendClinicianConfirmation(req *http.Request, confirmation *models.Confirmation) (msg string, err error) {
ctx := req.Context()
if err := a.addProfile(confirmation); err != nil {
a.logger(ctx).With(zap.Error(err)).Error(STATUS_ERR_ADDING_PROFILE)
return http.StatusInternalServerError, STATUS_ERR_SAVING_CONFIRMATION, err
return STATUS_ERR_SAVING_CONFIRMATION, err
}

confirmation.Modified = time.Now()
if err := a.Store.UpsertConfirmation(ctx, confirmation); err != nil {
return http.StatusInternalServerError, STATUS_ERR_SAVING_CONFIRMATION, err
return STATUS_ERR_SAVING_CONFIRMATION, err
}

a.logMetric("clinician_invite_created", req)
Expand All @@ -394,11 +394,11 @@ func (a *Api) sendClinicianConfirmation(req *http.Request, confirmation *models.
}

if !a.createAndSendNotification(req, confirmation, emailContent) {
return http.StatusInternalServerError, STATUS_ERR_SENDING_EMAIL, nil
return STATUS_ERR_SENDING_EMAIL, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should always return an error if you return a message otherwise in some places the code may not return a response, e.g.:

		msg, err := a.sendClinicianConfirmation(req, confirmation)
		if err != nil {
			a.sendError(ctx, res, http.StatusInternalServerError, msg, err)
			return
		}

}

a.logMetric("clinician_invite_sent", req)
return 0, "", nil
return "", nil
}

func (a *Api) cancelClinicianInviteWithStatus(res http.ResponseWriter, req *http.Request, filter, conf *models.Confirmation, statusUpdate models.Status) {
Expand Down
31 changes: 6 additions & 25 deletions api/forgot.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package api

import (
"context"
"encoding/json"
"net/http"

Expand Down Expand Up @@ -70,7 +69,6 @@ func (a *Api) passwordReset(res http.ResponseWriter, req *http.Request, vars map
resetCnf.Email = email
//there is nothing more to do other than notify the user
resetCnf.UpdateStatus(models.StatusCompleted)
a.logger(ctx).With(zap.String("email", email)).Info(STATUS_RESET_NO_ACCOUNT)
}

// addOrUpdateConfirmation logs and writes a response on errors
Expand All @@ -92,23 +90,6 @@ func (a *Api) passwordReset(res http.ResponseWriter, req *http.Request, vars map
res.WriteHeader(http.StatusOK)
}

// find the reset confirmation if it exists and hasn't expired
func (a *Api) findResetConfirmation(ctx context.Context, conf *models.Confirmation) (*models.Confirmation, bool, error) {
a.logger(ctx).With("conf", conf).Debug("finding reset confirmation")
found, err := a.Store.FindConfirmation(ctx, conf)
if err != nil {
return nil, false, err
}
if found == nil {
return nil, false, nil
}
if found.IsExpired() {
return nil, true, nil
}

return found, false, nil
}

// Accept the password change
//
// This call will be invoked by the lost password screen with the key that was included in the URL of the lost password screen.
Expand All @@ -135,19 +116,19 @@ func (a *Api) acceptPassword(res http.ResponseWriter, req *http.Request, vars ma

resetCnf := &models.Confirmation{Key: rb.Key, Email: rb.Email, Status: models.StatusPending, Type: models.TypePasswordReset}

conf, expired, err := a.findResetConfirmation(ctx, resetCnf)
conf, err := a.Store.FindConfirmation(ctx, resetCnf)
if err != nil {
a.sendError(ctx, res, http.StatusInternalServerError, STATUS_ERR_FINDING_CONFIRMATION, err)
return
}
if expired {
a.sendError(ctx, res, http.StatusNotFound, STATUS_RESET_EXPIRED)
return
}
if conf == nil {
a.sendError(ctx, res, http.StatusNotFound, STATUS_RESET_NOT_FOUND)
return
}
if conf.IsExpired() {
a.sendError(ctx, res, http.StatusNotFound, STATUS_RESET_EXPIRED)
return
}

if resetCnf.Key == "" || resetCnf.Email != conf.Email {
a.sendError(ctx, res, http.StatusBadRequest, STATUS_RESET_ERROR)
Expand All @@ -164,7 +145,7 @@ func (a *Api) acceptPassword(res http.ResponseWriter, req *http.Request, vars ma
}
conf.UpdateStatus(models.StatusCompleted)
// addOrUpdateConfirmation logs and writes a response on errors
if !a.addOrUpdateConfirmation(ctx, conf, res) {
if a.addOrUpdateConfirmation(ctx, conf, res) {
a.logMetricAsServer("password reset")
a.sendOK(ctx, res, STATUS_RESET_ACCEPTED)
return
Expand Down
46 changes: 27 additions & 19 deletions api/hydrophoneApi.go
Original file line number Diff line number Diff line change
Expand Up @@ -241,10 +241,10 @@ func (a *Api) SetHandlers(prefix string, rtr *mux.Router) {
// POST /confirm/resend/signup/:useremail
// POST /confirm/resend/invite/:inviteId
c.Handle("/resend/signup/{useremail}", vars(a.resendSignUp)).Methods("POST")
c.Handle("/resend/invite/{inviteId}", uid(vars(a.ResendInvite))).Methods("PATCH")
c.Handle("/resend/invite/{inviteId}", vars(a.ResendInvite)).Methods("PATCH")

rtr.Handle("/resend/signup/{useremail}", vars(a.resendSignUp)).Methods("POST")
rtr.Handle("/resend/invite/{inviteId}", uid(vars(a.ResendInvite))).Methods("PATCH")
rtr.Handle("/resend/invite/{inviteId}", vars(a.ResendInvite)).Methods("PATCH")

// PUT /confirm/accept/signup/:confirmationID
// PUT /confirm/accept/forgot/
Expand Down Expand Up @@ -579,17 +579,17 @@ func (a *Api) sendErrorWithCode(ctx context.Context, res http.ResponseWriter, st
}

func (a *Api) sendErrorLog(ctx context.Context, code int, reason string, extras ...interface{}) {
nonErrs, errs, fields := splitExtrasAndErrorsAndFields(extras)
details := splitExtrasAndErrorsAndFields(extras)
log := a.logger(ctx).WithOptions(zap.AddCallerSkip(2)).
Desugar().With(fields...).Sugar().
Desugar().With(details.Fields...).Sugar().
With(zap.Int("code", code)).
With(zap.Array("extras", zapArrayAny(nonErrs)))
if len(errs) == 1 {
log = log.With(zap.Error(errs[0]))
} else if len(errs) > 1 {
log = log.With(zap.Errors("errors", errs))
With(zap.Array("extras", zapArrayAny(details.NonErrors)))
if len(details.Errors) == 1 {
log = log.With(zap.Error(details.Errors[0]))
} else if len(details.Errors) > 1 {
log = log.With(zap.Errors("errors", details.Errors))
}
if code < http.StatusInternalServerError || len(errs) == 0 {
if code < http.StatusInternalServerError || len(details.Errors) == 0 {
// if there are no errors, use info to skip the stack trace, as it's
// probably not useful
log.Info(reason)
Expand All @@ -603,26 +603,34 @@ func (a *Api) sendOK(ctx context.Context, res http.ResponseWriter, reason string
a.sendModelAsResWithStatus(ctx, res, status.NewStatus(http.StatusOK, reason), http.StatusOK)
}

func splitExtrasAndErrorsAndFields(extras []interface{}) ([]interface{}, []error, []zapcore.Field) {
errs := []error{}
nonErrs := []interface{}{}
fields := []zap.Field{}
type extrasDetails struct {
Errors []error
NonErrors []interface{}
Fields []zap.Field
}

func splitExtrasAndErrorsAndFields(extras []interface{}) extrasDetails {
details := extrasDetails{
Errors: []error{},
NonErrors: []interface{}{},
Fields: []zap.Field{},
}
for _, extra := range extras {
if err, ok := extra.(error); ok {
if err != nil {
errs = append(errs, err)
details.Errors = append(details.Errors, err)
}
} else if field, ok := extra.(zap.Field); ok {
fields = append(fields, field)
details.Fields = append(details.Fields, field)
} else if extraErrs, ok := extra.([]error); ok {
if len(extraErrs) > 0 {
errs = append(errs, extraErrs...)
details.Errors = append(details.Errors, extraErrs...)
}
} else {
nonErrs = append(nonErrs, extra)
details.NonErrors = append(details.NonErrors, extra)
}
}
return nonErrs, errs, fields
return details
}

func (a *Api) tokenUserHasRequestedPermissions(tokenData *shoreline.TokenData, groupId string, requestedPermissions commonClients.Permissions) (commonClients.Permissions, error) {
Expand Down
1 change: 0 additions & 1 deletion api/patientInvites.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,6 @@ func (a *Api) AcceptPatientInvite(res http.ResponseWriter, req *http.Request, va
conf.UpdateStatus(models.StatusCompleted)
// addOrUpdateConfirmation logs and writes a response on errors
if !a.addOrUpdateConfirmation(ctx, conf, res) {
a.sendError(ctx, res, http.StatusInternalServerError, STATUS_ERR_SAVING_CONFIRMATION, err)
return
}

Expand Down
21 changes: 17 additions & 4 deletions events/events.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import (
"context"
"time"

"go.uber.org/zap"

"github.com/tidepool-org/go-common/events"
"github.com/tidepool-org/hydrophone/clients"
)
Expand All @@ -13,21 +15,32 @@ const deleteTimeout = 60 * time.Second
type handler struct {
events.NoopUserEventsHandler

store clients.StoreClient
store clients.StoreClient
logger *zap.SugaredLogger
}

var _ events.UserEventsHandler = &handler{}

func NewHandler(store clients.StoreClient) events.EventHandler {
func NewHandler(store clients.StoreClient, logger *zap.SugaredLogger) events.EventHandler {
return events.NewUserEventsHandler(&handler{
store: store,
store: store,
logger: logger,
})
}

func (h *handler) HandleDeleteUserEvent(payload events.DeleteUserEvent) error {
var err error
ctx, cancel := context.WithTimeout(context.Background(), deleteTimeout)
defer cancel()
if err := h.store.RemoveConfirmationsForUser(ctx, payload.UserID); err != nil {
defer func(err *error) {
log := h.logger.With(zap.String("userId", payload.UserID))
if err != nil {
log.With(zap.Error(*err)).Error("deleting confirmations")
} else {
log.With().Info("successfully deleted confirmations")
}
}(&err)
if err = h.store.RemoveConfirmationsForUser(ctx, payload.UserID); err != nil {
return err
}
return nil
Expand Down