From 65917f5bb98f1c0a0ce7285c284d25ea843c02c7 Mon Sep 17 00:00:00 2001 From: tobi <31960611+tsmethurst@users.noreply.github.com> Date: Wed, 27 Nov 2024 18:22:45 +0100 Subject: [PATCH 1/2] [bugfix] Log + ignore unknown notification types (#3577) * [bugfix] Log + ignore unknown notification types * pass context to ParseNotificationTypes --- .../client/notifications/notificationsget.go | 47 +++++++++++++------ .../notifications/notificationsget_test.go | 39 +++++++++++++++ internal/api/util/parsequery.go | 47 ------------------- internal/gtsmodel/notification.go | 36 +++++++++++++- 4 files changed, 106 insertions(+), 63 deletions(-) diff --git a/internal/api/client/notifications/notificationsget.go b/internal/api/client/notifications/notificationsget.go index 841768c63..7caadbe7d 100644 --- a/internal/api/client/notifications/notificationsget.go +++ b/internal/api/client/notifications/notificationsget.go @@ -18,11 +18,14 @@ package notifications import ( + "context" "net/http" "github.com/gin-gonic/gin" apiutil "github.com/superseriousbusiness/gotosocial/internal/api/util" "github.com/superseriousbusiness/gotosocial/internal/gtserror" + "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" + "github.com/superseriousbusiness/gotosocial/internal/log" "github.com/superseriousbusiness/gotosocial/internal/oauth" "github.com/superseriousbusiness/gotosocial/internal/paging" ) @@ -151,18 +154,6 @@ func (m *Module) NotificationsGETHandler(c *gin.Context) { return } - types, errWithCode := apiutil.ParseNotificationTypes(c.QueryArray(TypesKey)) - if errWithCode != nil { - apiutil.ErrorHandler(c, errWithCode, m.processor.InstanceGetV1) - return - } - - exclTypes, errWithCode := apiutil.ParseNotificationTypes(c.QueryArray(ExcludeTypesKey)) - if errWithCode != nil { - apiutil.ErrorHandler(c, errWithCode, m.processor.InstanceGetV1) - return - } - page, errWithCode := paging.ParseIDPage(c, 1, // min limit 80, // max limit @@ -173,12 +164,13 @@ func (m *Module) NotificationsGETHandler(c *gin.Context) { return } + ctx := c.Request.Context() resp, errWithCode := m.processor.Timeline().NotificationsGet( - c.Request.Context(), + ctx, authed, page, - types, - exclTypes, + ParseNotificationTypes(ctx, c.QueryArray(TypesKey)), // Include types. + ParseNotificationTypes(ctx, c.QueryArray(ExcludeTypesKey)), // Exclude types. ) if errWithCode != nil { apiutil.ErrorHandler(c, errWithCode, m.processor.InstanceGetV1) @@ -191,3 +183,28 @@ func (m *Module) NotificationsGETHandler(c *gin.Context) { apiutil.JSON(c, http.StatusOK, resp.Items) } + +// ParseNotificationTypes converts the given slice of string values +// to gtsmodel notification types, logging + skipping unknown types. +func ParseNotificationTypes( + ctx context.Context, + values []string, +) []gtsmodel.NotificationType { + if len(values) == 0 { + return nil + } + + ntypes := make([]gtsmodel.NotificationType, 0, len(values)) + for _, value := range values { + ntype := gtsmodel.NewNotificationType(value) + if ntype == gtsmodel.NotificationUnknown { + // Type we don't know about (yet), log and ignore it. + log.Debugf(ctx, "ignoring unknown type %s", value) + continue + } + + ntypes = append(ntypes, ntype) + } + + return ntypes +} diff --git a/internal/api/client/notifications/notificationsget_test.go b/internal/api/client/notifications/notificationsget_test.go index 97d0e854b..5a6f83959 100644 --- a/internal/api/client/notifications/notificationsget_test.go +++ b/internal/api/client/notifications/notificationsget_test.go @@ -248,6 +248,45 @@ func (suite *NotificationsTestSuite) TestGetNotificationsIncludeOneType() { } } +// Test including an unknown notification type, it should be ignored. +func (suite *NotificationsTestSuite) TestGetNotificationsIncludeUnknownType() { + testAccount := suite.testAccounts["local_account_1"] + testToken := suite.testTokens["local_account_1"] + testUser := suite.testUsers["local_account_1"] + + suite.addMoreNotifications(testAccount) + + maxID := "" + minID := "" + limit := 10 + types := []string{"favourite", "something.weird"} + excludeTypes := []string(nil) + expectedHTTPStatus := http.StatusOK + expectedBody := "" + + notifications, _, err := suite.getNotifications( + testAccount, + testToken, + testUser, + maxID, + minID, + limit, + types, + excludeTypes, + expectedHTTPStatus, + expectedBody, + ) + if err != nil { + suite.FailNow(err.Error()) + } + + // This should only include the fav notification. + suite.Len(notifications, 1) + for _, notification := range notifications { + suite.Equal("favourite", notification.Type) + } +} + func TestBookmarkTestSuite(t *testing.T) { suite.Run(t, new(NotificationsTestSuite)) } diff --git a/internal/api/util/parsequery.go b/internal/api/util/parsequery.go index 9929524c5..9f4c02aed 100644 --- a/internal/api/util/parsequery.go +++ b/internal/api/util/parsequery.go @@ -18,13 +18,11 @@ package util import ( - "errors" "fmt" "strconv" "strings" "github.com/superseriousbusiness/gotosocial/internal/gtserror" - "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" ) const ( @@ -218,51 +216,6 @@ func ParseInteractionReblogs(value string, defaultValue bool) (bool, gtserror.Wi return parseBool(value, defaultValue, InteractionReblogsKey) } -func ParseNotificationType(value string) (gtsmodel.NotificationType, gtserror.WithCode) { - switch strings.ToLower(value) { - case "follow": - return gtsmodel.NotificationFollow, nil - case "follow_request": - return gtsmodel.NotificationFollowRequest, nil - case "mention": - return gtsmodel.NotificationMention, nil - case "reblog": - return gtsmodel.NotificationReblog, nil - case "favourite": - return gtsmodel.NotificationFave, nil - case "poll": - return gtsmodel.NotificationPoll, nil - case "status": - return gtsmodel.NotificationStatus, nil - case "admin.sign_up": - return gtsmodel.NotificationSignup, nil - case "pending.favourite": - return gtsmodel.NotificationPendingFave, nil - case "pending.reply": - return gtsmodel.NotificationPendingReply, nil - case "pending.reblog": - return gtsmodel.NotificationPendingReblog, nil - default: - text := fmt.Sprintf("unrecognized notification type %s", value) - return 0, gtserror.NewErrorBadRequest(errors.New(text), text) - } -} - -func ParseNotificationTypes(values []string) ([]gtsmodel.NotificationType, gtserror.WithCode) { - if len(values) == 0 { - return nil, nil - } - ntypes := make([]gtsmodel.NotificationType, len(values)) - for i, value := range values { - ntype, errWithCode := ParseNotificationType(value) - if errWithCode != nil { - return nil, errWithCode - } - ntypes[i] = ntype - } - return ntypes, nil -} - /* Parse functions for *REQUIRED* parameters. */ diff --git a/internal/gtsmodel/notification.go b/internal/gtsmodel/notification.go index 49f1fe2bb..47bf7daa5 100644 --- a/internal/gtsmodel/notification.go +++ b/internal/gtsmodel/notification.go @@ -17,7 +17,10 @@ package gtsmodel -import "time" +import ( + "strings" + "time" +) // Notification models an alert/notification sent to an account about something like a reblog, like, new follow request, etc. type Notification struct { @@ -40,6 +43,7 @@ type Notification struct { const ( // Notification Types + NotificationUnknown NotificationType = 0 // NotificationUnknown -- unknown notification type, error if this occurs NotificationFollow NotificationType = 1 // NotificationFollow -- someone followed you NotificationFollowRequest NotificationType = 2 // NotificationFollowRequest -- someone requested to follow you NotificationMention NotificationType = 3 // NotificationMention -- someone mentioned you in their status @@ -82,3 +86,33 @@ func (t NotificationType) String() string { panic("invalid notification type") } } + +// NewNotificationType returns a notification type from the given value. +func NewNotificationType(in string) NotificationType { + switch strings.ToLower(in) { + case "follow": + return NotificationFollow + case "follow_request": + return NotificationFollowRequest + case "mention": + return NotificationMention + case "reblog": + return NotificationReblog + case "favourite": + return NotificationFave + case "poll": + return NotificationPoll + case "status": + return NotificationStatus + case "admin.sign_up": + return NotificationSignup + case "pending.favourite": + return NotificationPendingFave + case "pending.reply": + return NotificationPendingReply + case "pending.reblog": + return NotificationPendingReblog + default: + return NotificationUnknown + } +} From 312cb8b9c7e13802613fef33124a4570427e75a7 Mon Sep 17 00:00:00 2001 From: kim <89579420+NyaaaWhatsUpDoc@users.noreply.github.com> Date: Thu, 28 Nov 2024 11:54:22 +0000 Subject: [PATCH 2/2] [chore] rename New___(string) int signature functions to Parse___(string) int (#3580) * rename New___(string) int {} signature functions to Parse___(string) int {} * remove test output --- .../api/client/admin/domainpermissiondraftsget.go | 14 +++++--------- .../api/client/notifications/notificationsget.go | 12 ++++++------ .../20230828101322_admin_action_locking.go | 2 +- internal/gtsmodel/adminaction.go | 9 +++++---- internal/gtsmodel/domainpermission.go | 9 ++++++--- internal/gtsmodel/notification.go | 4 ++-- internal/processing/admin/accountaction.go | 2 +- 7 files changed, 26 insertions(+), 26 deletions(-) diff --git a/internal/api/client/admin/domainpermissiondraftsget.go b/internal/api/client/admin/domainpermissiondraftsget.go index dd3315857..d63179afc 100644 --- a/internal/api/client/admin/domainpermissiondraftsget.go +++ b/internal/api/client/admin/domainpermissiondraftsget.go @@ -147,16 +147,12 @@ func (m *Module) DomainPermissionDraftsGETHandler(c *gin.Context) { return } - permType := c.Query(apiutil.DomainPermissionPermTypeKey) - switch permType { - case "", "block", "allow": - // No problem. - - default: - // Invalid. + permTypeStr := c.Query(apiutil.DomainPermissionPermTypeKey) + permType := gtsmodel.ParseDomainPermissionType(permTypeStr) + if permType == gtsmodel.DomainPermissionUnknown { text := fmt.Sprintf( "permission_type %s not recognized, valid values are empty string, block, or allow", - permType, + permTypeStr, ) errWithCode := gtserror.NewErrorBadRequest(errors.New(text), text) apiutil.ErrorHandler(c, errWithCode, m.processor.InstanceGetV1) @@ -173,7 +169,7 @@ func (m *Module) DomainPermissionDraftsGETHandler(c *gin.Context) { c.Request.Context(), c.Query(apiutil.DomainPermissionSubscriptionIDKey), c.Query(apiutil.DomainPermissionDomainKey), - gtsmodel.NewDomainPermissionType(permType), + permType, page, ) if errWithCode != nil { diff --git a/internal/api/client/notifications/notificationsget.go b/internal/api/client/notifications/notificationsget.go index 7caadbe7d..b530c515d 100644 --- a/internal/api/client/notifications/notificationsget.go +++ b/internal/api/client/notifications/notificationsget.go @@ -169,8 +169,8 @@ func (m *Module) NotificationsGETHandler(c *gin.Context) { ctx, authed, page, - ParseNotificationTypes(ctx, c.QueryArray(TypesKey)), // Include types. - ParseNotificationTypes(ctx, c.QueryArray(ExcludeTypesKey)), // Exclude types. + parseNotificationTypes(ctx, c.QueryArray(TypesKey)), // Include types. + parseNotificationTypes(ctx, c.QueryArray(ExcludeTypesKey)), // Exclude types. ) if errWithCode != nil { apiutil.ErrorHandler(c, errWithCode, m.processor.InstanceGetV1) @@ -184,9 +184,9 @@ func (m *Module) NotificationsGETHandler(c *gin.Context) { apiutil.JSON(c, http.StatusOK, resp.Items) } -// ParseNotificationTypes converts the given slice of string values +// parseNotificationTypes converts the given slice of string values // to gtsmodel notification types, logging + skipping unknown types. -func ParseNotificationTypes( +func parseNotificationTypes( ctx context.Context, values []string, ) []gtsmodel.NotificationType { @@ -196,10 +196,10 @@ func ParseNotificationTypes( ntypes := make([]gtsmodel.NotificationType, 0, len(values)) for _, value := range values { - ntype := gtsmodel.NewNotificationType(value) + ntype := gtsmodel.ParseNotificationType(value) if ntype == gtsmodel.NotificationUnknown { // Type we don't know about (yet), log and ignore it. - log.Debugf(ctx, "ignoring unknown type %s", value) + log.Warnf(ctx, "ignoring unknown type %s", value) continue } diff --git a/internal/db/bundb/migrations/20230828101322_admin_action_locking.go b/internal/db/bundb/migrations/20230828101322_admin_action_locking.go index b72976cc9..29c29a747 100644 --- a/internal/db/bundb/migrations/20230828101322_admin_action_locking.go +++ b/internal/db/bundb/migrations/20230828101322_admin_action_locking.go @@ -77,7 +77,7 @@ func init() { UpdatedAt: oldAction.UpdatedAt, TargetCategory: gtsmodel.AdminActionCategoryAccount, TargetID: oldAction.TargetAccountID, - Type: gtsmodel.NewAdminActionType(string(oldAction.Type)), + Type: gtsmodel.ParseAdminActionType(string(oldAction.Type)), AccountID: oldAction.AccountID, Text: oldAction.Text, SendEmail: util.Ptr(oldAction.SendEmail), diff --git a/internal/gtsmodel/adminaction.go b/internal/gtsmodel/adminaction.go index e8b82e495..5ca8244a0 100644 --- a/internal/gtsmodel/adminaction.go +++ b/internal/gtsmodel/adminaction.go @@ -19,6 +19,7 @@ import ( "path" + "strings" "time" ) @@ -46,8 +47,8 @@ func (c AdminActionCategory) String() string { } } -func NewAdminActionCategory(in string) AdminActionCategory { - switch in { +func ParseAdminActionCategory(in string) AdminActionCategory { + switch strings.ToLower(in) { case "account": return AdminActionCategoryAccount case "domain": @@ -96,8 +97,8 @@ func (t AdminActionType) String() string { } } -func NewAdminActionType(in string) AdminActionType { - switch in { +func ParseAdminActionType(in string) AdminActionType { + switch strings.ToLower(in) { case "disable": return AdminActionDisable case "reenable": diff --git a/internal/gtsmodel/domainpermission.go b/internal/gtsmodel/domainpermission.go index 3d1ee873f..f1db4de59 100644 --- a/internal/gtsmodel/domainpermission.go +++ b/internal/gtsmodel/domainpermission.go @@ -17,7 +17,10 @@ package gtsmodel -import "time" +import ( + "strings" + "time" +) // DomainPermission models a domain permission // entry -- block / allow / draft / exclude. @@ -62,8 +65,8 @@ func (p DomainPermissionType) String() string { } } -func NewDomainPermissionType(in string) DomainPermissionType { - switch in { +func ParseDomainPermissionType(in string) DomainPermissionType { + switch strings.ToLower(in) { case "block": return DomainPermissionBlock case "allow": diff --git a/internal/gtsmodel/notification.go b/internal/gtsmodel/notification.go index 47bf7daa5..1ef805081 100644 --- a/internal/gtsmodel/notification.go +++ b/internal/gtsmodel/notification.go @@ -87,8 +87,8 @@ func (t NotificationType) String() string { } } -// NewNotificationType returns a notification type from the given value. -func NewNotificationType(in string) NotificationType { +// ParseNotificationType returns a notification type from the given value. +func ParseNotificationType(in string) NotificationType { switch strings.ToLower(in) { case "follow": return NotificationFollow diff --git a/internal/processing/admin/accountaction.go b/internal/processing/admin/accountaction.go index 7fd1047c4..59d4b420e 100644 --- a/internal/processing/admin/accountaction.go +++ b/internal/processing/admin/accountaction.go @@ -40,7 +40,7 @@ func (p *Processor) AccountAction( return "", gtserror.NewErrorInternalError(err) } - switch gtsmodel.NewAdminActionType(request.Type) { + switch gtsmodel.ParseAdminActionType(request.Type) { case gtsmodel.AdminActionSuspend: return p.accountActionSuspend(ctx, adminAcct, targetAcct, request.Text)