From e9227fca96cc59c40d005b4b16ffdf811cc24d26 Mon Sep 17 00:00:00 2001 From: openresearch Date: Thu, 25 Jun 2026 22:51:12 +0000 Subject: [PATCH 1/2] fix: code review improvements across delivery, usecase and consumer layers - bots search: ILIKE was matching exactly because the term was passed without wildcards. Wrap the (LIKE-escaped) term in %...% and add ESCAPE '\' so search performs a real substring match and user-supplied %/_ are matched literally. - error mapping: replace fragile strings.Contains(err.Error(), ...) checks with errors.Is against sentinel errors in posts_query handlers; also fix a fall-through that returned success on unmapped errors and a 500 that was wrongly encoded as a NotFound response. - stop leaking internal error text to clients: all 5xx (InternalServerError / ServiceUnavailable) responses now return a generic message and log the real error; 4xx validation messages are preserved. - kafka consumer: poison (undecodable) messages and unknown commands now send an error reply to the request-reply caller and commit instead of being silently dropped or blocking the partition forever. - bots create: drop the redundant re-read of the just-created bot by adding Service.CreateWithProfiles, which resolves profiles without a second Get. - add unit tests for the LIKE escaper and the bots create/CreateWithProfiles usecase logic. --- internal/delivery_http/bots/create.go | 7 +- internal/delivery_http/bots/get.go | 4 +- internal/delivery_http/bots/init.go | 2 +- internal/delivery_http/bots/profiles.go | 6 +- internal/delivery_http/bots/search.go | 2 +- internal/delivery_http/bots/summary.go | 2 +- internal/delivery_http/bots/update.go | 2 +- .../posts/posts_command_consumer/handler.go | 13 +- .../posts_command_producer/create_post.go | 15 +- .../posts_command_producer/delete_post.go | 6 +- .../posts_command_producer/publish_post.go | 9 +- .../posts_command_producer/seen_posts.go | 6 +- .../posts_command_producer/update_post.go | 8 +- .../posts/posts_query/check_group_id.go | 18 +- .../posts/posts_query/check_group_ids.go | 19 +- .../posts/posts_query/get_by_id.go | 7 +- .../posts/posts_query/list_posts.go | 3 +- internal/delivery_http/profiles/create.go | 2 +- internal/delivery_http/profiles/delete.go | 2 +- internal/delivery_http/profiles/get.go | 2 +- internal/delivery_http/profiles/list.go | 2 +- internal/delivery_http/profiles/update.go | 2 +- internal/repo/bots/search.go | 14 +- internal/repo/bots/search_test.go | 31 ++++ internal/usecase/bots/create.go | 21 +++ internal/usecase/bots/create_test.go | 166 ++++++++++++++++++ 26 files changed, 300 insertions(+), 71 deletions(-) create mode 100644 internal/repo/bots/search_test.go create mode 100644 internal/usecase/bots/create_test.go diff --git a/internal/delivery_http/bots/create.go b/internal/delivery_http/bots/create.go index c18b284..d571332 100644 --- a/internal/delivery_http/bots/create.go +++ b/internal/delivery_http/bots/create.go @@ -24,7 +24,7 @@ func (h *Handler) CreateBot(ctx context.Context, req *gen.BotInput) (gen.CreateB profiles = append(profiles, p.ID) } - created, err := h.u.Create(ctx, model.Bot{ + bot, profs, err := h.u.CreateWithProfiles(ctx, model.Bot{ Name: req.Name, SystemPrompt: desc, ModerationRequired: moderation, @@ -35,10 +35,5 @@ func (h *Handler) CreateBot(ctx context.Context, req *gen.BotInput) (gen.CreateB return nil, err } - bot, profs, err := h.u.GetWithProfiles(ctx, created.ID) - if err != nil { - h.log.Error(err, true, "CreateBot: get with profiles") - return nil, err - } return modelToDTO(&bot, profs), nil } diff --git a/internal/delivery_http/bots/get.go b/internal/delivery_http/bots/get.go index fef41a3..5f6ea0b 100644 --- a/internal/delivery_http/bots/get.go +++ b/internal/delivery_http/bots/get.go @@ -14,14 +14,14 @@ func (h *Handler) GetBotById(ctx context.Context, params gen.GetBotByIdParams) ( if errors.Is(err, constants.ErrNotFound) { return &gen.GetBotByIdNotFound{ ErrorCode: constants.NotFoundMsg, - Message: err.Error(), + Message: "bot not found", }, nil } if errors.Is(err, constants.ErrServiceUnavailable) { h.log.Error(err, true, "GetBotById: service unavailable") return &gen.GetBotByIdInternalServerError{ ErrorCode: constants.ServiceUnavailableMsg, - Message: err.Error(), + Message: constants.ServiceUnavailableMsg, }, nil } h.log.Error(err, true, "GetBotById: get with profiles") diff --git a/internal/delivery_http/bots/init.go b/internal/delivery_http/bots/init.go index 54823c7..6628e0e 100644 --- a/internal/delivery_http/bots/init.go +++ b/internal/delivery_http/bots/init.go @@ -9,7 +9,7 @@ import ( ) type usecase interface { - Create(ctx context.Context, bot model.Bot) (model.Bot, error) + CreateWithProfiles(ctx context.Context, bot model.Bot) (model.Bot, []model.Profile, error) Delete(ctx context.Context, id uuid.UUID) error GetWithProfiles(ctx context.Context, id uuid.UUID) (model.Bot, []model.Profile, error) List(ctx context.Context) ([]model.FullBot, error) diff --git a/internal/delivery_http/bots/profiles.go b/internal/delivery_http/bots/profiles.go index baa3f51..3743016 100644 --- a/internal/delivery_http/bots/profiles.go +++ b/internal/delivery_http/bots/profiles.go @@ -19,7 +19,7 @@ func (h Handler) AddProfileToBot(ctx context.Context, params gen.AddProfileToBot h.log.Error(err, true, "AddProfileToBot: add profile") return &gen.AddProfileToBotInternalServerError{ ErrorCode: constants.InternalMsg, - Message: err.Error(), + Message: constants.InternalMsg, }, nil } return &gen.NoContent{}, nil @@ -36,7 +36,7 @@ func (h Handler) RemoveProfileFromBot(ctx context.Context, params gen.RemoveProf h.log.Error(err, true, "RemoveProfileFromBot: remove profile") return &gen.RemoveProfileFromBotInternalServerError{ ErrorCode: constants.InternalMsg, - Message: err.Error(), + Message: constants.InternalMsg, }, nil } return &gen.NoContent{}, nil @@ -54,7 +54,7 @@ func (h Handler) GetBotProfiles(ctx context.Context, params gen.GetBotProfilesPa h.log.Error(err, true, "GetBotProfiles: get with profiles") return &gen.GetBotProfilesInternalServerError{ ErrorCode: constants.InternalMsg, - Message: err.Error(), + Message: constants.InternalMsg, }, nil } diff --git a/internal/delivery_http/bots/search.go b/internal/delivery_http/bots/search.go index f7c63ca..0011a37 100644 --- a/internal/delivery_http/bots/search.go +++ b/internal/delivery_http/bots/search.go @@ -13,7 +13,7 @@ func (h *Handler) SearchBots(ctx context.Context, params bots.SearchBotsParams) h.log.Error(err, true, "SearchBots: search") return &bots.SearchBotsInternalServerError{ ErrorCode: constants.InternalMsg, - Message: err.Error(), + Message: constants.InternalMsg, }, nil } diff --git a/internal/delivery_http/bots/summary.go b/internal/delivery_http/bots/summary.go index 9edab95..33c3ee8 100644 --- a/internal/delivery_http/bots/summary.go +++ b/internal/delivery_http/bots/summary.go @@ -13,7 +13,7 @@ func (h *Handler) SummaryBots(ctx context.Context) (bots.SummaryBotsRes, error) h.log.Error(err, true, "SummaryBots: get summary") return &bots.Error{ ErrorCode: constants.InternalMsg, - Message: err.Error(), + Message: constants.InternalMsg, }, nil } diff --git a/internal/delivery_http/bots/update.go b/internal/delivery_http/bots/update.go index 80b9c69..0062ccf 100644 --- a/internal/delivery_http/bots/update.go +++ b/internal/delivery_http/bots/update.go @@ -20,7 +20,7 @@ func (h *Handler) UpdateBotById(ctx context.Context, req *gen.BotInput, params g if errors.Is(err, constants.ErrNotFound) { return &gen.UpdateBotByIdNotFound{ ErrorCode: constants.NotFoundMsg, - Message: err.Error(), + Message: "bot not found", }, nil } h.log.Error(err, true, "UpdateBotById: get with profiles") diff --git a/internal/delivery_http/posts/posts_command_consumer/handler.go b/internal/delivery_http/posts/posts_command_consumer/handler.go index 7899fb1..1d7e36a 100644 --- a/internal/delivery_http/posts/posts_command_consumer/handler.go +++ b/internal/delivery_http/posts/posts_command_consumer/handler.go @@ -18,8 +18,14 @@ func (p *PostsCommandConsumer) HandleCommands() error { for message := range p.consumer.Start(ctx) { var env kafkaConfig.Envelope if err := jsoniter.Unmarshal(message.Msg.Value, &env); err != nil { + // A message we cannot even decode is a poison message: we can never + // process it, so we must not block the partition on it. Reply to the + // waiting caller with an error (so request-reply doesn't hang waiting + // for a response) and commit the offset to move past it. p.log.Error(err, false, constants.ErrUnmarshal.Error()) - _ = message.Ack(ctx) + if replyErr := sendErrReply(ctx, message, errors.Wrap(err, constants.UnmarshalMsg)); replyErr != nil { + p.log.Error(replyErr, false, "HandleCommands: failed to reply to poison message") + } continue } @@ -36,7 +42,10 @@ func (p *PostsCommandConsumer) HandleCommands() error { case posts.CmdPublish: err = p.handlePublishCommand(ctx, message, env.Payload) default: - err = errors.Errorf("unknown command received: %s", env.Command) + // Unknown command: reply with an error and commit so the consumer + // loop receives a decision and does not block on this message + // forever (Ack/Nack/Reply must be called exactly once per message). + err = sendErrReply(ctx, message, errors.Errorf("unknown command received: %s", env.Command)) } if err != nil { diff --git a/internal/delivery_http/posts/posts_command_producer/create_post.go b/internal/delivery_http/posts/posts_command_producer/create_post.go index 330ad32..9e511c6 100644 --- a/internal/delivery_http/posts/posts_command_producer/create_post.go +++ b/internal/delivery_http/posts/posts_command_producer/create_post.go @@ -2,7 +2,6 @@ package posts_command_producer import ( "context" - "fmt" "net/http" "time" @@ -11,7 +10,7 @@ import ( "github.com/goriiin/kotyari-bots_backend/internal/delivery_http/posts" gen "github.com/goriiin/kotyari-bots_backend/internal/gen/posts/posts_command" "github.com/goriiin/kotyari-bots_backend/internal/model" - "github.com/goriiin/kotyari-bots_backend/pkg/ierrors" + "github.com/goriiin/kotyari-bots_backend/pkg/constants" "github.com/goriiin/kotyari-bots_backend/pkg/user" jsoniter "github.com/json-iterator/go" ) @@ -25,7 +24,7 @@ func (p *PostsCommandHandler) CreatePost(ctx context.Context, req *gen.PostInput bot, err := p.fetcher.GetBot(ctx, req.BotId.String()) if err != nil { p.log.Error(err, true, "CreatePost: get bot") - return &gen.CreatePostInternalServerError{ErrorCode: http.StatusInternalServerError, Message: ierrors.GRPCToDomainError(err).Error()}, nil + return &gen.CreatePostInternalServerError{ErrorCode: http.StatusInternalServerError, Message: constants.InternalMsg}, nil } idsString := make([]string, 0, len(req.ProfileIds)) @@ -36,7 +35,7 @@ func (p *PostsCommandHandler) CreatePost(ctx context.Context, req *gen.PostInput profilesBatch, err := p.fetcher.GetProfiles(ctx, idsString) if err != nil { p.log.Error(err, true, "CreatePost: get profiles") - return &gen.CreatePostInternalServerError{ErrorCode: http.StatusInternalServerError, Message: ierrors.GRPCToDomainError(err).Error()}, nil + return &gen.CreatePostInternalServerError{ErrorCode: http.StatusInternalServerError, Message: constants.InternalMsg}, nil } postProfiles := make([]posts.CreatePostProfiles, 0, len(idsString)) @@ -70,7 +69,7 @@ func (p *PostsCommandHandler) CreatePost(ctx context.Context, req *gen.PostInput p.log.Error(err, true, "CreatePost: marshal") return &gen.CreatePostInternalServerError{ ErrorCode: http.StatusInternalServerError, - Message: err.Error(), + Message: constants.InternalMsg, }, nil } @@ -79,7 +78,7 @@ func (p *PostsCommandHandler) CreatePost(ctx context.Context, req *gen.PostInput p.log.Error(err, true, "CreatePost: request") return &gen.CreatePostInternalServerError{ ErrorCode: http.StatusInternalServerError, - Message: err.Error(), + Message: constants.InternalMsg, }, nil } @@ -89,7 +88,7 @@ func (p *PostsCommandHandler) CreatePost(ctx context.Context, req *gen.PostInput p.log.Error(err, true, "CreatePost: unmarshal response") return &gen.CreatePostInternalServerError{ ErrorCode: http.StatusInternalServerError, - Message: err.Error(), + Message: constants.InternalMsg, }, nil } @@ -97,7 +96,7 @@ func (p *PostsCommandHandler) CreatePost(ctx context.Context, req *gen.PostInput p.log.Warn("CreatePost: response error", errors.New(resp.Error)) return &gen.CreatePostInternalServerError{ ErrorCode: http.StatusInternalServerError, - Message: fmt.Sprintf("Failed to create post, %s", resp.Error), + Message: constants.InternalMsg, }, nil } diff --git a/internal/delivery_http/posts/posts_command_producer/delete_post.go b/internal/delivery_http/posts/posts_command_producer/delete_post.go index 487b7b7..71b2907 100644 --- a/internal/delivery_http/posts/posts_command_producer/delete_post.go +++ b/internal/delivery_http/posts/posts_command_producer/delete_post.go @@ -20,7 +20,7 @@ func (p *PostsCommandHandler) DeletePostById(ctx context.Context, params gen.Del p.log.Error(err, true, "DeletePostById: marshal") return &gen.DeletePostByIdInternalServerError{ ErrorCode: http.StatusInternalServerError, - Message: err.Error(), + Message: constants.InternalMsg, }, nil } @@ -30,7 +30,7 @@ func (p *PostsCommandHandler) DeletePostById(ctx context.Context, params gen.Del p.log.Error(err, true, "DeletePostById: request") return &gen.DeletePostByIdInternalServerError{ ErrorCode: http.StatusInternalServerError, - Message: err.Error(), + Message: constants.InternalMsg, }, nil } @@ -40,7 +40,7 @@ func (p *PostsCommandHandler) DeletePostById(ctx context.Context, params gen.Del p.log.Error(err, true, "DeletePostById: unmarshal response") return &gen.DeletePostByIdInternalServerError{ ErrorCode: http.StatusInternalServerError, - Message: err.Error(), + Message: constants.InternalMsg, }, nil } diff --git a/internal/delivery_http/posts/posts_command_producer/publish_post.go b/internal/delivery_http/posts/posts_command_producer/publish_post.go index 4fffb95..78e1162 100644 --- a/internal/delivery_http/posts/posts_command_producer/publish_post.go +++ b/internal/delivery_http/posts/posts_command_producer/publish_post.go @@ -8,6 +8,7 @@ import ( "github.com/go-faster/errors" "github.com/goriiin/kotyari-bots_backend/internal/delivery_http/posts" gen "github.com/goriiin/kotyari-bots_backend/internal/gen/posts/posts_command" + "github.com/goriiin/kotyari-bots_backend/pkg/constants" jsoniter "github.com/json-iterator/go" ) @@ -29,7 +30,7 @@ func (p *PostsCommandHandler) PublishPost(ctx context.Context, req *gen.PublishP p.log.Error(err, true, "PublishPost: marshal") return &gen.PublishPostInternalServerError{ ErrorCode: http.StatusInternalServerError, - Message: err.Error(), + Message: constants.InternalMsg, }, nil } @@ -38,7 +39,7 @@ func (p *PostsCommandHandler) PublishPost(ctx context.Context, req *gen.PublishP p.log.Error(err, true, "PublishPost: request") return &gen.PublishPostInternalServerError{ ErrorCode: http.StatusInternalServerError, - Message: err.Error(), + Message: constants.InternalMsg, }, nil } @@ -48,7 +49,7 @@ func (p *PostsCommandHandler) PublishPost(ctx context.Context, req *gen.PublishP p.log.Error(err, true, "PublishPost: unmarshal response") return &gen.PublishPostInternalServerError{ ErrorCode: http.StatusInternalServerError, - Message: err.Error(), + Message: constants.InternalMsg, }, nil } @@ -56,7 +57,7 @@ func (p *PostsCommandHandler) PublishPost(ctx context.Context, req *gen.PublishP p.log.Warn("PublishPost: response error", errors.New(resp.Error)) return &gen.PublishPostInternalServerError{ ErrorCode: http.StatusInternalServerError, - Message: resp.Error, + Message: constants.InternalMsg, }, nil } diff --git a/internal/delivery_http/posts/posts_command_producer/seen_posts.go b/internal/delivery_http/posts/posts_command_producer/seen_posts.go index 5722749..86f0d77 100644 --- a/internal/delivery_http/posts/posts_command_producer/seen_posts.go +++ b/internal/delivery_http/posts/posts_command_producer/seen_posts.go @@ -23,7 +23,7 @@ func (p *PostsCommandHandler) SeenPosts(ctx context.Context, req *gen.PostsSeenR p.log.Error(err, true, "SeenPosts: marshal") return &gen.SeenPostsInternalServerError{ ErrorCode: http.StatusInternalServerError, - Message: err.Error(), + Message: constants.InternalMsg, }, nil } @@ -32,7 +32,7 @@ func (p *PostsCommandHandler) SeenPosts(ctx context.Context, req *gen.PostsSeenR p.log.Error(err, true, "SeenPosts: request") return &gen.SeenPostsInternalServerError{ ErrorCode: http.StatusInternalServerError, - Message: err.Error(), + Message: constants.InternalMsg, }, nil } @@ -42,7 +42,7 @@ func (p *PostsCommandHandler) SeenPosts(ctx context.Context, req *gen.PostsSeenR p.log.Error(err, true, "SeenPosts: unmarshal response") return &gen.SeenPostsInternalServerError{ ErrorCode: http.StatusInternalServerError, - Message: err.Error(), + Message: constants.InternalMsg, }, nil } diff --git a/internal/delivery_http/posts/posts_command_producer/update_post.go b/internal/delivery_http/posts/posts_command_producer/update_post.go index bcf8b38..39928ea 100644 --- a/internal/delivery_http/posts/posts_command_producer/update_post.go +++ b/internal/delivery_http/posts/posts_command_producer/update_post.go @@ -24,7 +24,7 @@ func (p *PostsCommandHandler) UpdatePostById(ctx context.Context, req *gen.PostU p.log.Error(err, true, "UpdatePostById: marshal") return &gen.UpdatePostByIdInternalServerError{ ErrorCode: http.StatusInternalServerError, - Message: err.Error(), + Message: constants.InternalMsg, }, nil } @@ -33,7 +33,7 @@ func (p *PostsCommandHandler) UpdatePostById(ctx context.Context, req *gen.PostU p.log.Error(err, true, "UpdatePostById: request") return &gen.UpdatePostByIdInternalServerError{ ErrorCode: http.StatusInternalServerError, - Message: err.Error(), + Message: constants.InternalMsg, }, nil } @@ -43,7 +43,7 @@ func (p *PostsCommandHandler) UpdatePostById(ctx context.Context, req *gen.PostU p.log.Error(err, true, "UpdatePostById: unmarshal response") return &gen.UpdatePostByIdInternalServerError{ ErrorCode: http.StatusInternalServerError, - Message: err.Error(), + Message: constants.InternalMsg, }, nil } @@ -51,7 +51,7 @@ func (p *PostsCommandHandler) UpdatePostById(ctx context.Context, req *gen.PostU case strings.Contains(resp.Error, constants.InternalMsg): return &gen.UpdatePostByIdInternalServerError{ ErrorCode: http.StatusInternalServerError, - Message: resp.Error, + Message: constants.InternalMsg, }, nil case strings.Contains(resp.Error, constants.NotFoundMsg): diff --git a/internal/delivery_http/posts/posts_query/check_group_id.go b/internal/delivery_http/posts/posts_query/check_group_id.go index f4c8f87..332878c 100644 --- a/internal/delivery_http/posts/posts_query/check_group_id.go +++ b/internal/delivery_http/posts/posts_query/check_group_id.go @@ -3,8 +3,8 @@ package posts_query import ( "context" "net/http" - "strings" + "github.com/go-faster/errors" "github.com/goriiin/kotyari-bots_backend/internal/delivery_http/posts" gen "github.com/goriiin/kotyari-bots_backend/internal/gen/posts/posts_query" "github.com/goriiin/kotyari-bots_backend/pkg/constants" @@ -13,20 +13,18 @@ import ( func (p *PostsQueryHandler) CheckGroupId(ctx context.Context, params gen.CheckGroupIdParams) (gen.CheckGroupIdRes, error) { groupPosts, err := p.repo.GetByGroupId(ctx, params.GroupId) if err != nil { - switch { - case strings.Contains(err.Error(), constants.NotFoundMsg): + if errors.Is(err, constants.ErrNotFound) { return &gen.CheckGroupIdNotFound{ ErrorCode: http.StatusNotFound, Message: "Посты с этим groupID еще не готовы", }, nil - - case strings.Contains(err.Error(), constants.InternalMsg): - p.log.Error(err, true, "CheckGroupId: get by group id") - return &gen.CheckGroupIdInternalServerError{ - ErrorCode: http.StatusInternalServerError, - Message: err.Error(), - }, nil } + + p.log.Error(err, true, "CheckGroupId: get by group id") + return &gen.CheckGroupIdInternalServerError{ + ErrorCode: http.StatusInternalServerError, + Message: constants.InternalMsg, + }, nil } return posts.QueryPostsToHttp(groupPosts), nil diff --git a/internal/delivery_http/posts/posts_query/check_group_ids.go b/internal/delivery_http/posts/posts_query/check_group_ids.go index 1ba7a7c..afc5adc 100644 --- a/internal/delivery_http/posts/posts_query/check_group_ids.go +++ b/internal/delivery_http/posts/posts_query/check_group_ids.go @@ -3,8 +3,8 @@ package posts_query import ( "context" "net/http" - "strings" + "github.com/go-faster/errors" "github.com/goriiin/kotyari-bots_backend/internal/delivery_http/posts" gen "github.com/goriiin/kotyari-bots_backend/internal/gen/posts/posts_query" "github.com/goriiin/kotyari-bots_backend/pkg/constants" @@ -12,22 +12,19 @@ import ( func (p *PostsQueryHandler) CheckGroupIds(ctx context.Context) (gen.CheckGroupIdsRes, error) { postsStatuses, err := p.repo.CheckGroupIds(ctx) - if err != nil { - switch { - case strings.Contains(err.Error(), constants.NotFoundMsg): + if errors.Is(err, constants.ErrNotFound) { return &gen.CheckGroupIdsNotFound{ ErrorCode: http.StatusNotFound, Message: "Постов нет", }, nil - - case strings.Contains(err.Error(), constants.InternalMsg): - p.log.Error(err, true, "CheckGroupIds: check group ids") - return &gen.CheckGroupIdsNotFound{ - ErrorCode: http.StatusInternalServerError, - Message: err.Error(), - }, nil } + + p.log.Error(err, true, "CheckGroupIds: check group ids") + return &gen.CheckGroupIdsInternalServerError{ + ErrorCode: http.StatusInternalServerError, + Message: constants.InternalMsg, + }, nil } return posts.PostsCheckModelsToHttpSlice(postsStatuses), nil diff --git a/internal/delivery_http/posts/posts_query/get_by_id.go b/internal/delivery_http/posts/posts_query/get_by_id.go index 059475d..468f956 100644 --- a/internal/delivery_http/posts/posts_query/get_by_id.go +++ b/internal/delivery_http/posts/posts_query/get_by_id.go @@ -3,8 +3,8 @@ package posts_query import ( "context" "net/http" - "strings" + "github.com/go-faster/errors" "github.com/goriiin/kotyari-bots_backend/internal/delivery_http/posts" gen "github.com/goriiin/kotyari-bots_backend/internal/gen/posts/posts_query" "github.com/goriiin/kotyari-bots_backend/pkg/constants" @@ -15,11 +15,12 @@ func (p *PostsQueryHandler) GetPostById(ctx context.Context, params gen.GetPostB // Пока возвращается пост без категорий post, err := p.repo.GetByID(ctx, params.PostId) if err != nil { - if strings.Contains(err.Error(), constants.NotFoundMsg) { + if errors.Is(err, constants.ErrNotFound) { return &gen.GetPostByIdNotFound{ErrorCode: http.StatusNotFound, Message: "post not found"}, nil } - return &gen.GetPostByIdInternalServerError{ErrorCode: http.StatusInternalServerError, Message: err.Error()}, nil + p.log.Error(err, true, "GetPostById: get by id") + return &gen.GetPostByIdInternalServerError{ErrorCode: http.StatusInternalServerError, Message: constants.InternalMsg}, nil } return posts.QueryModelToHttp(post), nil diff --git a/internal/delivery_http/posts/posts_query/list_posts.go b/internal/delivery_http/posts/posts_query/list_posts.go index 5786b9b..850601f 100644 --- a/internal/delivery_http/posts/posts_query/list_posts.go +++ b/internal/delivery_http/posts/posts_query/list_posts.go @@ -6,13 +6,14 @@ import ( "github.com/goriiin/kotyari-bots_backend/internal/delivery_http/posts" gen "github.com/goriiin/kotyari-bots_backend/internal/gen/posts/posts_query" + "github.com/goriiin/kotyari-bots_backend/pkg/constants" ) func (p *PostsQueryHandler) ListPosts(ctx context.Context) (gen.ListPostsRes, error) { postsModels, err := p.repo.ListPosts(ctx) if err != nil { p.log.Error(err, true, "ListPosts: list posts") - return &gen.ListPostsInternalServerError{ErrorCode: http.StatusInternalServerError, Message: err.Error()}, nil + return &gen.ListPostsInternalServerError{ErrorCode: http.StatusInternalServerError, Message: constants.InternalMsg}, nil } return posts.QueryPostsToHttp(postsModels), nil diff --git a/internal/delivery_http/profiles/create.go b/internal/delivery_http/profiles/create.go index 1c7a8df..4d4a4dd 100644 --- a/internal/delivery_http/profiles/create.go +++ b/internal/delivery_http/profiles/create.go @@ -16,7 +16,7 @@ func (h *HTTPHandler) CreateMyProfile(ctx context.Context, req *gen.ProfileInput return &gen.CreateMyProfileBadRequest{ErrorCode: constants.ErrValidationMsg, Message: err.Error()}, nil } h.log.Error(err, true, "CreateMyProfile: create") - return &gen.CreateMyProfileInternalServerError{ErrorCode: constants.ErrInternalMsg, Message: err.Error()}, nil + return &gen.CreateMyProfileInternalServerError{ErrorCode: constants.ErrInternalMsg, Message: constants.ErrInternalMsg}, nil } return modelToHttpDTO(&created), nil } diff --git a/internal/delivery_http/profiles/delete.go b/internal/delivery_http/profiles/delete.go index 5a048c6..c4c7b1b 100644 --- a/internal/delivery_http/profiles/delete.go +++ b/internal/delivery_http/profiles/delete.go @@ -15,7 +15,7 @@ func (h *HTTPHandler) DeleteProfileById(ctx context.Context, params gen.DeletePr return &gen.DeleteProfileByIdNotFound{ErrorCode: constants.ErrNotFoundMsg, Message: "profile not found"}, nil } h.log.Error(err, true, "DeleteProfileById: delete") - return &gen.DeleteProfileByIdInternalServerError{ErrorCode: constants.ErrInternalMsg, Message: err.Error()}, nil + return &gen.DeleteProfileByIdInternalServerError{ErrorCode: constants.ErrInternalMsg, Message: constants.ErrInternalMsg}, nil } return &gen.NoContent{}, nil } diff --git a/internal/delivery_http/profiles/get.go b/internal/delivery_http/profiles/get.go index e8b81b3..e7a7855 100644 --- a/internal/delivery_http/profiles/get.go +++ b/internal/delivery_http/profiles/get.go @@ -15,7 +15,7 @@ func (h *HTTPHandler) GetProfileById(ctx context.Context, params gen.GetProfileB return &gen.GetProfileByIdNotFound{ErrorCode: constants.ErrNotFoundMsg, Message: "profile not found"}, nil } h.log.Error(err, true, "GetProfileById: get") - return &gen.GetProfileByIdInternalServerError{ErrorCode: constants.ErrInternalMsg, Message: err.Error()}, nil + return &gen.GetProfileByIdInternalServerError{ErrorCode: constants.ErrInternalMsg, Message: constants.ErrInternalMsg}, nil } return modelToHttpDTO(&p), nil } diff --git a/internal/delivery_http/profiles/list.go b/internal/delivery_http/profiles/list.go index 2a1f578..f6f2e66 100644 --- a/internal/delivery_http/profiles/list.go +++ b/internal/delivery_http/profiles/list.go @@ -11,7 +11,7 @@ func (h *HTTPHandler) ListMyProfiles(ctx context.Context) (gen.ListMyProfilesRes profiles, err := h.u.List(ctx) if err != nil { h.log.Error(err, true, "ListMyProfiles: list") - return &gen.ListMyProfilesInternalServerError{ErrorCode: constants.ErrInternalMsg, Message: err.Error()}, nil + return &gen.ListMyProfilesInternalServerError{ErrorCode: constants.ErrInternalMsg, Message: constants.ErrInternalMsg}, nil } dtoProfiles := make([]gen.Profile, len(profiles)) diff --git a/internal/delivery_http/profiles/update.go b/internal/delivery_http/profiles/update.go index 8ef3a13..18f7309 100644 --- a/internal/delivery_http/profiles/update.go +++ b/internal/delivery_http/profiles/update.go @@ -18,7 +18,7 @@ func (h *HTTPHandler) UpdateProfileById(ctx context.Context, req *gen.ProfileInp return &gen.UpdateProfileByIdBadRequest{ErrorCode: constants.ErrValidationMsg, Message: err.Error()}, nil } h.log.Error(err, true, "UpdateProfileById: update") - return &gen.UpdateProfileByIdInternalServerError{ErrorCode: constants.ErrInternalMsg, Message: err.Error()}, nil + return &gen.UpdateProfileByIdInternalServerError{ErrorCode: constants.ErrInternalMsg, Message: constants.ErrInternalMsg}, nil } return modelToHttpDTO(&updated), nil } diff --git a/internal/repo/bots/search.go b/internal/repo/bots/search.go index 7759c5f..850c2e6 100644 --- a/internal/repo/bots/search.go +++ b/internal/repo/bots/search.go @@ -2,18 +2,28 @@ package bots import ( "context" + "strings" "github.com/goriiin/kotyari-bots_backend/internal/model" "github.com/goriiin/kotyari-bots_backend/pkg/user" "github.com/jackc/pgx/v5" ) +// likeEscaper escapes the LIKE/ILIKE wildcard characters so that a user-supplied +// search term is matched literally as a substring instead of being interpreted +// as a pattern. The backslash is used as the ESCAPE character in the query. +var likeEscaper = strings.NewReplacer(`\`, `\\`, `%`, `\%`, `_`, `\_`) + func (r BotsRepository) Search(ctx context.Context, query string) ([]model.Bot, error) { userID, err := user.GetID(ctx) if err != nil { return nil, err } + // Wrap the (escaped) term in wildcards so ILIKE performs a substring match + // rather than an exact, case-insensitive comparison. + pattern := "%" + likeEscaper.Replace(query) + "%" + rows, err := r.db.Query(ctx, ` SELECT id, @@ -27,9 +37,9 @@ func (r BotsRepository) Search(ctx context.Context, query string) ([]model.Bot, FROM bots WHERE is_deleted = false AND user_id = $2 - AND (bot_name ILIKE $1 OR system_prompt ILIKE $1) + AND (bot_name ILIKE $1 ESCAPE '\' OR system_prompt ILIKE $1 ESCAPE '\') ORDER BY created_at DESC - `, query, userID) + `, pattern, userID) if err != nil { return nil, err } diff --git a/internal/repo/bots/search_test.go b/internal/repo/bots/search_test.go new file mode 100644 index 0000000..5ec1365 --- /dev/null +++ b/internal/repo/bots/search_test.go @@ -0,0 +1,31 @@ +package bots + +import "testing" + +// TestLikeEscaper verifies that LIKE/ILIKE wildcard characters supplied by the +// user are escaped so they are matched literally instead of being interpreted +// as pattern metacharacters. The repository wraps the escaped term in `%...%` +// itself, so the escaper must only neutralise `\`, `%` and `_`. +func TestLikeEscaper(t *testing.T) { + cases := []struct { + name string + input string + want string + }{ + {name: "plain text is unchanged", input: "hello", want: "hello"}, + {name: "percent is escaped", input: "50%", want: `50\%`}, + {name: "underscore is escaped", input: "a_b", want: `a\_b`}, + {name: "backslash is escaped first", input: `a\b`, want: `a\\b`}, + {name: "mixed metacharacters", input: `%_\`, want: `\%\_\\`}, + {name: "empty string", input: "", want: ""}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := likeEscaper.Replace(tc.input) + if got != tc.want { + t.Fatalf("likeEscaper.Replace(%q) = %q, want %q", tc.input, got, tc.want) + } + }) + } +} diff --git a/internal/usecase/bots/create.go b/internal/usecase/bots/create.go index 303db8b..b5ca714 100644 --- a/internal/usecase/bots/create.go +++ b/internal/usecase/bots/create.go @@ -39,3 +39,24 @@ func (s *Service) Create(ctx context.Context, bot model.Bot) (model.Bot, error) } return b, nil } + +// CreateWithProfiles creates a bot and returns it together with its resolved +// profiles, without re-reading the freshly created bot from the database. +// The profiles still have to be fetched because the bot only stores their IDs. +func (s *Service) CreateWithProfiles(ctx context.Context, bot model.Bot) (model.Bot, []model.Profile, error) { + created, err := s.Create(ctx, bot) + if err != nil { + return model.Bot{}, nil, err + } + + if len(created.ProfileIDs) == 0 { + return created, []model.Profile{}, nil + } + + profiles, err := s.pg.GetProfilesByIDs(ctx, created.ProfileIDs) + if err != nil { + return model.Bot{}, nil, err + } + + return created, profiles, nil +} diff --git a/internal/usecase/bots/create_test.go b/internal/usecase/bots/create_test.go new file mode 100644 index 0000000..83a01b7 --- /dev/null +++ b/internal/usecase/bots/create_test.go @@ -0,0 +1,166 @@ +package bots + +import ( + "context" + "testing" + + "github.com/go-faster/errors" + "github.com/google/uuid" + "github.com/goriiin/kotyari-bots_backend/internal/model" + "github.com/goriiin/kotyari-bots_backend/pkg/constants" +) + +// --- test doubles for the usecase collaborators --- + +type fakeRepo struct { + createCalls int + createErr error + lastCreated model.Bot +} + +func (f *fakeRepo) Create(_ context.Context, b model.Bot) error { + f.createCalls++ + f.lastCreated = b + return f.createErr +} +func (f *fakeRepo) Get(context.Context, uuid.UUID) (model.Bot, error) { + return model.Bot{}, nil +} +func (f *fakeRepo) List(context.Context) ([]model.Bot, error) { return nil, nil } +func (f *fakeRepo) Update(context.Context, model.Bot) error { return nil } +func (f *fakeRepo) Delete(context.Context, uuid.UUID) error { return nil } +func (f *fakeRepo) AddProfileID(context.Context, uuid.UUID, uuid.UUID) error { return nil } +func (f *fakeRepo) RemoveProfileID(context.Context, uuid.UUID, uuid.UUID) error { return nil } +func (f *fakeRepo) Search(context.Context, string) ([]model.Bot, error) { + return nil, nil +} +func (f *fakeRepo) GetSummary(context.Context) (model.BotsSummary, error) { + return model.BotsSummary{}, nil +} + +type fakeValidator struct { + err error + gotIDs []uuid.UUID + callCount int +} + +func (f *fakeValidator) ValidateProfilesExist(_ context.Context, ids []uuid.UUID) error { + f.callCount++ + f.gotIDs = ids + return f.err +} + +type fakeGateway struct { + profiles []model.Profile + err error + callCount int +} + +func (f *fakeGateway) GetProfilesByIDs(_ context.Context, _ []uuid.UUID) ([]model.Profile, error) { + f.callCount++ + return f.profiles, f.err +} + +func newService(r *fakeRepo, v *fakeValidator, g *fakeGateway) *Service { + return NewService(r, v, g) +} + +func TestCreate_TrimsNameAndRejectsEmpty(t *testing.T) { + repo := &fakeRepo{} + s := newService(repo, &fakeValidator{}, &fakeGateway{}) + + _, err := s.Create(context.Background(), model.Bot{Name: " "}) + if err == nil { + t.Fatal("expected validation error for blank name, got nil") + } + if !errors.Is(err, constants.ErrValidation) { + t.Fatalf("expected ErrValidation, got %v", err) + } + if repo.createCalls != 0 { + t.Fatalf("repo.Create must not be called on validation failure, called %d times", repo.createCalls) + } +} + +func TestCreate_Success(t *testing.T) { + repo := &fakeRepo{} + validator := &fakeValidator{} + s := newService(repo, validator, &fakeGateway{}) + + ids := []uuid.UUID{uuid.New(), uuid.New()} + got, err := s.Create(context.Background(), model.Bot{Name: " My Bot ", ProfileIDs: ids}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got.Name != "My Bot" { + t.Fatalf("expected trimmed name %q, got %q", "My Bot", got.Name) + } + if got.ProfilesCount != len(ids) { + t.Fatalf("expected ProfilesCount %d, got %d", len(ids), got.ProfilesCount) + } + if got.ID == uuid.Nil { + t.Fatal("expected a generated ID, got uuid.Nil") + } + if repo.createCalls != 1 { + t.Fatalf("expected repo.Create to be called once, got %d", repo.createCalls) + } + if validator.callCount != 1 { + t.Fatalf("expected profile validation to run once, got %d", validator.callCount) + } +} + +func TestCreate_PropagatesValidatorError(t *testing.T) { + repo := &fakeRepo{} + validator := &fakeValidator{err: constants.ErrNotFound} + s := newService(repo, validator, &fakeGateway{}) + + _, err := s.Create(context.Background(), model.Bot{Name: "Bot", ProfileIDs: []uuid.UUID{uuid.New()}}) + if !errors.Is(err, constants.ErrNotFound) { + t.Fatalf("expected ErrNotFound from validator, got %v", err) + } + if repo.createCalls != 0 { + t.Fatalf("repo.Create must not be called when validation fails, called %d", repo.createCalls) + } +} + +func TestCreateWithProfiles_SkipsGatewayWhenNoProfiles(t *testing.T) { + repo := &fakeRepo{} + gw := &fakeGateway{} + s := newService(repo, &fakeValidator{}, gw) + + bot, profiles, err := s.CreateWithProfiles(context.Background(), model.Bot{Name: "Bot"}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if bot.Name != "Bot" { + t.Fatalf("expected name %q, got %q", "Bot", bot.Name) + } + if len(profiles) != 0 { + t.Fatalf("expected no profiles, got %d", len(profiles)) + } + if gw.callCount != 0 { + t.Fatalf("gateway must not be queried when bot has no profiles, called %d", gw.callCount) + } +} + +func TestCreateWithProfiles_ResolvesProfiles(t *testing.T) { + repo := &fakeRepo{} + gw := &fakeGateway{profiles: []model.Profile{{ID: uuid.New()}}} + s := newService(repo, &fakeValidator{}, gw) + + _, profiles, err := s.CreateWithProfiles(context.Background(), model.Bot{ + Name: "Bot", + ProfileIDs: []uuid.UUID{uuid.New()}, + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(profiles) != 1 { + t.Fatalf("expected 1 resolved profile, got %d", len(profiles)) + } + if gw.callCount != 1 { + t.Fatalf("expected gateway to be queried once, got %d", gw.callCount) + } + if repo.createCalls != 1 { + t.Fatalf("expected exactly one repo.Create (no redundant re-read), got %d", repo.createCalls) + } +} From bf49d65b7c69842bd6a3e6f97fdd94c7672cee9e Mon Sep 17 00:00:00 2001 From: openresearch Date: Thu, 25 Jun 2026 23:26:26 +0000 Subject: [PATCH 2/2] =?UTF-8?q?fix:=20second=20review=20pass=20=E2=80=94?= =?UTF-8?q?=20IDOR,=20broken=20create/auth,=20lifecycle=20&=20error=20hand?= =?UTF-8?q?ling?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security / correctness (high): - IDOR: post update/delete/seen/publish never set UserID, and the command repos filtered only by id — any user could mutate any post. Thread UserID from the producer handlers through the Kafka DTOs into the repos, which now scope by `AND user_id = $N`. - profiles.GetByID was not tenant-scoped (cross-tenant read by ID); now filters by user_id. The gRPC-only GetByIDs/Exist stay by-ID (no user_id in contract) and are documented as such. - repo CreatePost: INSERT omitted NOT-NULL bot_name/profile_name and user_id (every insert failed); the categories branch returned before tx.Commit (silently dropping the write); and the deferred rollback overwrote the real error. All three fixed. - auth.Config expected `addr` but every config supplies auth_grpc.host/port, so the dial address was always empty and auth always failed. Config now reads host/port (addr still honored as an override) and errors if unset. - posts_query Run() built a second auth client with a nil logger (panicked on the first auth failure); it now reuses the client from the app struct. - bots handlers returned (nil, err), yielding a generic 500 for validation (should be 400) and not-found (should be 404); they now return typed ogen responses. SeenPosts not-found returned a 500-typed body; now 404. Lifecycle / robustness (medium): - kafka request-reply: a double Ack/Nack/Reply could block the handler on a full channel; the decision signal is now non-blocking and documented. - kafka basic consumer: `defer cancel()` inside the read loop accumulated one live timer per iteration; now cancels per iteration. - graceful shutdown: consumer/producer mains handle SIGINT/SIGTERM and call Close() instead of relying on a defer that never runs; Close errors are logged rather than log.Fatal'd inside a defer (which skipped remaining cleanup). - aggregator/reddit mains: handle the previously-ignored config load error and drop the no-op WatchConfig reassignment (constructor had already captured the pointer); reddit no longer log.Fatal's from the watch callback. - aggregator AddTopics batch was never Closed (leak); reddit performRequests leaked the response body on 403, could block goroutines on unbuffered sends, and cancelled its context on return while feeding goroutines still ran. Low: - profiles.GetByID/bots.Get use errors.Is / RowToStructByName; bots Update and Add/RemoveProfileID now filter is_deleted = false. - logger Warn/Info/Debug joined all supplied errors instead of dropping them when more than one was passed. Tests: add unit tests for auth.dialAddr, logger.joinErrs, and the KafkaUpdatePostRequest UserID mapping (the IDOR guard). --- cmd/aggregator/main.go | 16 ++----- cmd/posts_command_consumer/main.go | 28 +++++++++--- cmd/posts_command_producer/main.go | 26 ++++++++--- cmd/reddit/main.go | 19 ++------ internal/adapters/auth/client.go | 22 +++++++++- internal/adapters/auth/config_test.go | 35 +++++++++++++++ internal/apps/posts_query/run.go | 26 +++-------- .../reddit/perform_request.go | 38 +++++++++------- internal/delivery_http/bots/create.go | 13 +++++- internal/delivery_http/bots/delete.go | 8 ++++ internal/delivery_http/bots/list.go | 6 ++- internal/delivery_http/bots/update.go | 22 +++++++++- internal/delivery_http/posts/kafka_dto.go | 7 +-- .../delivery_http/posts/kafka_dto_test.go | 33 ++++++++++++++ .../posts_command_consumer/delete_post.go | 2 +- .../posts/posts_command_consumer/init.go | 4 +- .../posts_command_consumer/seen_posts.go | 2 +- .../posts_command_producer/create_post.go | 12 +++++- .../posts_command_producer/delete_post.go | 8 +++- .../posts_command_producer/publish_post.go | 7 +++ .../posts_command_producer/seen_posts.go | 9 +++- .../posts_command_producer/update_post.go | 7 +++ internal/kafka/consumer/init_basic.go | 12 ++++-- internal/kafka/consumer/init_reqest_reply.go | 25 +++++++++-- internal/logger/logger.go | 21 ++++++--- internal/logger/logger_test.go | 43 +++++++++++++++++++ internal/repo/aggregator/add_topics.go | 5 +++ internal/repo/bots/get.go | 4 +- internal/repo/bots/profiles.go | 4 +- internal/repo/bots/update.go | 2 +- .../repo/posts/posts_command/create_post.go | 25 +++++++---- .../repo/posts/posts_command/delete_post.go | 6 +-- .../repo/posts/posts_command/seen_posts.go | 5 ++- .../repo/posts/posts_command/update_post.go | 4 +- internal/repo/profiles/get.go | 18 ++++++-- 35 files changed, 396 insertions(+), 128 deletions(-) create mode 100644 internal/adapters/auth/config_test.go create mode 100644 internal/delivery_http/posts/kafka_dto_test.go create mode 100644 internal/logger/logger_test.go diff --git a/cmd/aggregator/main.go b/cmd/aggregator/main.go index 5347c31..9a86a94 100644 --- a/cmd/aggregator/main.go +++ b/cmd/aggregator/main.go @@ -7,19 +7,11 @@ import ( "github.com/goriiin/kotyari-bots_backend/pkg/config" ) -// const local = "local-config" -const docker = "docker-config" - func main() { - cfg, _ := config.New[aggregator.AggregatorAppConfig]() - config.WatchConfig(func() { - newCfg, err := config.NewWithConfig[aggregator.AggregatorAppConfig](docker) - if err != nil { - return - } - - cfg = newCfg - }) + cfg, err := config.New[aggregator.AggregatorAppConfig]() + if err != nil { + log.Fatalf("config load: %v", err) + } app, err := aggregator.NewAggregatorApp(cfg) if err != nil { diff --git a/cmd/posts_command_consumer/main.go b/cmd/posts_command_consumer/main.go index 68ba498..837a20a 100644 --- a/cmd/posts_command_consumer/main.go +++ b/cmd/posts_command_consumer/main.go @@ -2,6 +2,9 @@ package main import ( "log" + "os" + "os/signal" + "syscall" "github.com/goriiin/kotyari-bots_backend/internal/apps/posts_command_consumer" "github.com/goriiin/kotyari-bots_backend/pkg/config" @@ -23,14 +26,27 @@ func main() { log.Fatal(err) } - defer func(app *posts_command_consumer.PostsCommandConsumer) { - err := app.Close() + // Run in the background so we can react to OS signals and close resources + // gracefully instead of being killed with the DB pool / Kafka reader open. + runErr := make(chan error, 1) + go func() { + runErr <- app.Run() + }() + + sig := make(chan os.Signal, 1) + signal.Notify(sig, syscall.SIGINT, syscall.SIGTERM) + + select { + case err := <-runErr: if err != nil { - log.Fatal(err) + log.Println("run error:", err) } - }(app) + case s := <-sig: + log.Printf("received signal %s, shutting down", s) + } - if err = app.Run(); err != nil { - log.Println(err) + if err := app.Close(); err != nil { + // Log, don't Fatal: os.Exit here would skip any remaining cleanup. + log.Println("close error:", err) } } diff --git a/cmd/posts_command_producer/main.go b/cmd/posts_command_producer/main.go index 249daca..d4c6a90 100644 --- a/cmd/posts_command_producer/main.go +++ b/cmd/posts_command_producer/main.go @@ -2,6 +2,9 @@ package main import ( "log" + "os" + "os/signal" + "syscall" "github.com/goriiin/kotyari-bots_backend/internal/apps/posts_command_producer" "github.com/goriiin/kotyari-bots_backend/pkg/config" @@ -18,14 +21,25 @@ func main() { log.Fatal(err) } - defer func(app *posts_command_producer.PostsCommandProducerApp) { - err := app.Close() + runErr := make(chan error, 1) + go func() { + runErr <- app.Run() + }() + + sig := make(chan os.Signal, 1) + signal.Notify(sig, syscall.SIGINT, syscall.SIGTERM) + + select { + case err := <-runErr: if err != nil { - log.Fatal(err) + log.Println("run error:", err) } - }(app) + case s := <-sig: + log.Printf("received signal %s, shutting down", s) + } - if err = app.Run(); err != nil { - log.Println(err) + if err := app.Close(); err != nil { + // Log, don't Fatal: os.Exit here would skip any remaining cleanup. + log.Println("close error:", err) } } diff --git a/cmd/reddit/main.go b/cmd/reddit/main.go index dcf7b07..bd557d4 100644 --- a/cmd/reddit/main.go +++ b/cmd/reddit/main.go @@ -7,22 +7,11 @@ import ( "github.com/goriiin/kotyari-bots_backend/pkg/config" ) -// TODO: change behaviour -// const local = "local-config" -const docker = "docker-config" - func main() { - cfg, _ := config.New[redditapp.RedditAppConfig]() - - config.WatchConfig(func() { - newCfg, err := config.NewWithConfig[redditapp.RedditAppConfig](docker) - if err != nil { - log.Fatalf("error parsing config in runtime: %s", err.Error()) - return - } - - cfg = newCfg - }) + cfg, err := config.New[redditapp.RedditAppConfig]() + if err != nil { + log.Fatalf("config load: %v", err) + } app, err := redditapp.NewRedditAPIApp(cfg) if err != nil { diff --git a/internal/adapters/auth/client.go b/internal/adapters/auth/client.go index 9d64c4e..7ab9a8e 100644 --- a/internal/adapters/auth/client.go +++ b/internal/adapters/auth/client.go @@ -3,6 +3,7 @@ package auth import ( "context" "errors" + "fmt" "time" "github.com/google/uuid" @@ -13,10 +14,24 @@ import ( ) type Config struct { + // Host/Port are how the gRPC endpoint is configured in the YAML + // (auth_grpc.host / auth_grpc.port). Addr, when set, takes precedence and + // is used verbatim as the dial target. + Host string `mapstructure:"host"` + Port int `mapstructure:"port"` Addr string `mapstructure:"addr"` Timeout time.Duration `mapstructure:"timeout"` } +// dialAddr returns the gRPC dial target, preferring an explicit Addr and +// falling back to host:port. +func (c Config) dialAddr() string { + if c.Addr != "" { + return c.Addr + } + return fmt.Sprintf("%s:%d", c.Host, c.Port) +} + type Client struct { grpcClient authgen.UsersProviderClient log *logger.Logger @@ -24,7 +39,12 @@ type Client struct { } func NewClient(cfg Config, log *logger.Logger) (*Client, error) { - conn, err := grpc.NewClient(cfg.Addr, + addr := cfg.dialAddr() + if addr == ":0" || addr == "" { + return nil, errors.New("auth: gRPC address is not configured (set auth_grpc.host/port or auth_grpc.addr)") + } + + conn, err := grpc.NewClient(addr, grpc.WithTransportCredentials(insecure.NewCredentials()), ) if err != nil { diff --git a/internal/adapters/auth/config_test.go b/internal/adapters/auth/config_test.go new file mode 100644 index 0000000..c9674c6 --- /dev/null +++ b/internal/adapters/auth/config_test.go @@ -0,0 +1,35 @@ +package auth + +import "testing" + +func TestConfigDialAddr(t *testing.T) { + cases := []struct { + name string + cfg Config + want string + }{ + { + name: "host and port", + cfg: Config{Host: "auth_rs", Port: 3001}, + want: "auth_rs:3001", + }, + { + name: "explicit addr wins over host/port", + cfg: Config{Host: "auth_rs", Port: 3001, Addr: "override:9999"}, + want: "override:9999", + }, + { + name: "addr only", + cfg: Config{Addr: "localhost:50051"}, + want: "localhost:50051", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := tc.cfg.dialAddr(); got != tc.want { + t.Fatalf("dialAddr() = %q, want %q", got, tc.want) + } + }) + } +} diff --git a/internal/apps/posts_query/run.go b/internal/apps/posts_query/run.go index 29caa29..bf689b5 100644 --- a/internal/apps/posts_query/run.go +++ b/internal/apps/posts_query/run.go @@ -13,27 +13,11 @@ import ( ) func (p *PostsQueryApp) Run() error { - // Re-initializing auth client here or extracting from App struct would be cleaner, - // assuming we can modify NewPostsQueryApp to store authClient or create it here. - // For simplicity based on previous pattern, creating a new one or assuming it's available. - - // Better approach: Pass it via struct (requires modifying init.go return struct) - // Since I can modify files fully: - - l := log.Default() // using std log for run errors as per original - - // Create auth client again or pass it. Let's create it to stick to the pattern of separation - // but normally it should be in struct. - // NOTE: Please update struct in init.go to store authClient if you want reuse. - // Implementing creation here for strict compliance with "Run" interface. - - authClient, err := auth.NewClient(p.config.Auth, nil) // logger nil might be issue, better pass it - if err != nil { - return err - } - - if err := p.startHTTPServer(p.handler, authClient); err != nil { - l.Printf("Error happened starting server %v", err) + // Reuse the auth client built (with a real logger) in NewPostsQueryApp instead + // of constructing a second one with a nil logger, which panicked on the first + // auth failure inside VerifySession. + if err := p.startHTTPServer(p.handler, p.authClient); err != nil { + log.Printf("Error happened starting server %v", err) return err } return nil diff --git a/internal/delivery_http/api_integrations/reddit/perform_request.go b/internal/delivery_http/api_integrations/reddit/perform_request.go index 2e49f2f..77fa330 100644 --- a/internal/delivery_http/api_integrations/reddit/perform_request.go +++ b/internal/delivery_http/api_integrations/reddit/perform_request.go @@ -5,7 +5,6 @@ import ( "encoding/json" "io" "net/http" - "sync" "time" "github.com/go-faster/errors" @@ -19,11 +18,14 @@ const ( ) func (r *RedditAPIDelivery) performRequests() (chan PostData, error) { + // NOTE: ownership of cancel is handed to the draining goroutine below — we + // must NOT defer-cancel here, or the context would be cancelled the moment + // this function returns, killing the goroutines that still feed `posts`. ctx, cancel := context.WithTimeout(context.Background(), defaultErrGroupWaitTime) - defer cancel() integrations, err := r.integration.GetIntegrations(ctx, redditAPIString) if err != nil { + cancel() return nil, err } @@ -32,7 +34,7 @@ func (r *RedditAPIDelivery) performRequests() (chan PostData, error) { for _, integration := range integrations { g.Go(func() error { - req, err := http.NewRequest(http.MethodGet, integration.Url, http.NoBody) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, integration.Url, http.NoBody) if err != nil { return errors.Wrap(err, "failed to create request") } @@ -41,6 +43,9 @@ func (r *RedditAPIDelivery) performRequests() (chan PostData, error) { if err != nil { return errors.Wrap(err, "failed to perform request") } + // Always close the body, including on the Forbidden early-return + // below (previously leaked the connection on every blocked request). + defer func() { _ = resp.Body.Close() }() if resp.StatusCode == http.StatusForbidden { return errors.New("request was blocked") @@ -51,16 +56,18 @@ func (r *RedditAPIDelivery) performRequests() (chan PostData, error) { return errors.Wrapf(err, "bad response body: %s", string(body)) } - err = resp.Body.Close() - if err != nil { - return errors.Wrap(err, "failed to close body") - } - var redditAPIResponse RedditAPIResponse if err := json.Unmarshal(body, &redditAPIResponse); err != nil { return errors.Wrapf(err, "failed to unmarhsal: %s", integration.Url) } - redditAPIResponses <- redditAPIResponse + + // Respect cancellation so a stalled consumer can't wedge this + // goroutine forever on an unbuffered send. + select { + case redditAPIResponses <- redditAPIResponse: + case <-ctx.Done(): + return ctx.Err() + } return nil }) @@ -76,17 +83,18 @@ func (r *RedditAPIDelivery) performRequests() (chan PostData, error) { posts := make(chan PostData) - var wg sync.WaitGroup go func() { + defer cancel() + defer close(posts) for redditNews := range redditAPIResponses { - wg.Add(1) for _, post := range redditNews.Data.Posts { - posts <- post.PostData + select { + case posts <- post.PostData: + case <-ctx.Done(): + return + } } - wg.Done() } - wg.Wait() - close(posts) }() return posts, nil diff --git a/internal/delivery_http/bots/create.go b/internal/delivery_http/bots/create.go index d571332..2eea8c6 100644 --- a/internal/delivery_http/bots/create.go +++ b/internal/delivery_http/bots/create.go @@ -3,9 +3,11 @@ package bots import ( "context" + "github.com/go-faster/errors" "github.com/google/uuid" gen "github.com/goriiin/kotyari-bots_backend/internal/gen/bots" "github.com/goriiin/kotyari-bots_backend/internal/model" + "github.com/goriiin/kotyari-bots_backend/pkg/constants" ) func (h *Handler) CreateBot(ctx context.Context, req *gen.BotInput) (gen.CreateBotRes, error) { @@ -31,8 +33,17 @@ func (h *Handler) CreateBot(ctx context.Context, req *gen.BotInput) (gen.CreateB ProfileIDs: profiles, }) if err != nil { + if errors.Is(err, constants.ErrValidation) { + return &gen.CreateBotBadRequest{ + ErrorCode: constants.ValidationMsg, + Message: err.Error(), + }, nil + } h.log.Error(err, true, "CreateBot: create bot") - return nil, err + return &gen.CreateBotInternalServerError{ + ErrorCode: constants.InternalMsg, + Message: constants.InternalMsg, + }, nil } return modelToDTO(&bot, profs), nil diff --git a/internal/delivery_http/bots/delete.go b/internal/delivery_http/bots/delete.go index 4566357..7a83f26 100644 --- a/internal/delivery_http/bots/delete.go +++ b/internal/delivery_http/bots/delete.go @@ -3,12 +3,20 @@ package bots import ( "context" + "github.com/go-faster/errors" gen "github.com/goriiin/kotyari-bots_backend/internal/gen/bots" + "github.com/goriiin/kotyari-bots_backend/pkg/constants" ) func (h *Handler) DeleteBotById(ctx context.Context, params gen.DeleteBotByIdParams) (gen.DeleteBotByIdRes, error) { err := h.u.Delete(ctx, params.BotId) if err != nil { + if errors.Is(err, constants.ErrNotFound) { + return &gen.DeleteBotByIdNotFound{ + ErrorCode: constants.NotFoundMsg, + Message: "bot not found", + }, nil + } h.log.Error(err, true, "DeleteBotById: delete") return nil, err } diff --git a/internal/delivery_http/bots/list.go b/internal/delivery_http/bots/list.go index 3ac7ccc..7cfc553 100644 --- a/internal/delivery_http/bots/list.go +++ b/internal/delivery_http/bots/list.go @@ -4,13 +4,17 @@ import ( "context" gen "github.com/goriiin/kotyari-bots_backend/internal/gen/bots" + "github.com/goriiin/kotyari-bots_backend/pkg/constants" ) func (h *Handler) ListBots(ctx context.Context) (gen.ListBotsRes, error) { bots, err := h.u.List(ctx) if err != nil { h.log.Error(err, true, "ListBots: list") - return nil, err + return &gen.ListBotsInternalServerError{ + ErrorCode: constants.InternalMsg, + Message: constants.InternalMsg, + }, nil } genBots := make([]gen.Bot, len(bots)) diff --git a/internal/delivery_http/bots/update.go b/internal/delivery_http/bots/update.go index 0062ccf..99dc855 100644 --- a/internal/delivery_http/bots/update.go +++ b/internal/delivery_http/bots/update.go @@ -11,8 +11,23 @@ import ( func (h *Handler) UpdateBotById(ctx context.Context, req *gen.BotInput, params gen.UpdateBotByIdParams) (gen.UpdateBotByIdRes, error) { _, err := h.u.Update(ctx, dtoToModel(req, params.BotId)) if err != nil { + switch { + case errors.Is(err, constants.ErrValidation): + return &gen.UpdateBotByIdBadRequest{ + ErrorCode: constants.ValidationMsg, + Message: err.Error(), + }, nil + case errors.Is(err, constants.ErrNotFound): + return &gen.UpdateBotByIdNotFound{ + ErrorCode: constants.NotFoundMsg, + Message: "bot not found", + }, nil + } h.log.Error(err, true, "UpdateBotById: update") - return nil, err + return &gen.UpdateBotByIdInternalServerError{ + ErrorCode: constants.InternalMsg, + Message: constants.InternalMsg, + }, nil } bot, profiles, err := h.u.GetWithProfiles(ctx, params.BotId) @@ -24,7 +39,10 @@ func (h *Handler) UpdateBotById(ctx context.Context, req *gen.BotInput, params g }, nil } h.log.Error(err, true, "UpdateBotById: get with profiles") - return nil, err + return &gen.UpdateBotByIdInternalServerError{ + ErrorCode: constants.InternalMsg, + Message: constants.InternalMsg, + }, nil } return modelToDTO(&bot, profiles), nil diff --git a/internal/delivery_http/posts/kafka_dto.go b/internal/delivery_http/posts/kafka_dto.go index 9553e7e..4780f8a 100644 --- a/internal/delivery_http/posts/kafka_dto.go +++ b/internal/delivery_http/posts/kafka_dto.go @@ -98,8 +98,9 @@ func (r KafkaResponse) PostCommandToGen() *gen.Post { func (u KafkaUpdatePostRequest) ToModel() model.Post { return model.Post{ - ID: u.PostID, - Title: u.Title, - Text: u.Text, + ID: u.PostID, + UserID: u.UserID, + Title: u.Title, + Text: u.Text, } } diff --git a/internal/delivery_http/posts/kafka_dto_test.go b/internal/delivery_http/posts/kafka_dto_test.go new file mode 100644 index 0000000..ea79f75 --- /dev/null +++ b/internal/delivery_http/posts/kafka_dto_test.go @@ -0,0 +1,33 @@ +package posts + +import ( + "testing" + + "github.com/google/uuid" +) + +// TestKafkaUpdatePostRequestToModel guards the IDOR fix: the UserID must be +// carried from the Kafka command into the model so the repository can scope the +// UPDATE by user_id (otherwise any user could edit any post). +func TestKafkaUpdatePostRequestToModel(t *testing.T) { + postID := uuid.New() + userID := uuid.New() + + req := KafkaUpdatePostRequest{ + PostID: postID, + UserID: userID, + Title: "title", + Text: "text", + } + + m := req.ToModel() + if m.ID != postID { + t.Fatalf("ID = %v, want %v", m.ID, postID) + } + if m.UserID != userID { + t.Fatalf("UserID = %v, want %v (IDOR scoping would be lost)", m.UserID, userID) + } + if m.Title != "title" || m.Text != "text" { + t.Fatalf("title/text not carried: %+v", m) + } +} diff --git a/internal/delivery_http/posts/posts_command_consumer/delete_post.go b/internal/delivery_http/posts/posts_command_consumer/delete_post.go index 8f599d5..74c3b36 100644 --- a/internal/delivery_http/posts/posts_command_consumer/delete_post.go +++ b/internal/delivery_http/posts/posts_command_consumer/delete_post.go @@ -15,7 +15,7 @@ func (p *PostsCommandConsumer) DeletePost(ctx context.Context, payload []byte) e return errors.Wrap(err, "failed to unmarshal") } - err = p.repo.DeletePost(ctx, req.PostID) + err = p.repo.DeletePost(ctx, req.PostID, req.UserID) if err != nil { return errors.Wrap(err, "failed to delete post") } diff --git a/internal/delivery_http/posts/posts_command_consumer/init.go b/internal/delivery_http/posts/posts_command_consumer/init.go index df762dd..be7905d 100644 --- a/internal/delivery_http/posts/posts_command_consumer/init.go +++ b/internal/delivery_http/posts/posts_command_consumer/init.go @@ -20,10 +20,10 @@ type postsGetter interface { type repo interface { CreatePost(ctx context.Context, post model.Post, categoryIDs []uuid.UUID) (model.Post, error) UpdatePost(ctx context.Context, post model.Post) (model.Post, error) - DeletePost(ctx context.Context, id uuid.UUID) error + DeletePost(ctx context.Context, id, userID uuid.UUID) error CreatePostsBatch(ctx context.Context, posts []model.Post) (err error) UpdatePostsBatch(ctx context.Context, posts []model.Post) (err error) - SeenPostsBatch(ctx context.Context, postsIds []uuid.UUID) (err error) + SeenPostsBatch(ctx context.Context, postsIds []uuid.UUID, userID uuid.UUID) (err error) } type consumer interface { diff --git a/internal/delivery_http/posts/posts_command_consumer/seen_posts.go b/internal/delivery_http/posts/posts_command_consumer/seen_posts.go index 8a3ec79..6d747ce 100644 --- a/internal/delivery_http/posts/posts_command_consumer/seen_posts.go +++ b/internal/delivery_http/posts/posts_command_consumer/seen_posts.go @@ -16,7 +16,7 @@ func (p *PostsCommandConsumer) SeenPosts(ctx context.Context, payload []byte) er return errors.Wrap(err, "failed to unwrap") } - err = p.repo.SeenPostsBatch(ctx, seenPosts.PostIDs) + err = p.repo.SeenPostsBatch(ctx, seenPosts.PostIDs, seenPosts.UserID) if err != nil { return errors.Wrap(err, "failed to change posts status") } diff --git a/internal/delivery_http/posts/posts_command_producer/create_post.go b/internal/delivery_http/posts/posts_command_producer/create_post.go index 9e511c6..a88d982 100644 --- a/internal/delivery_http/posts/posts_command_producer/create_post.go +++ b/internal/delivery_http/posts/posts_command_producer/create_post.go @@ -40,7 +40,11 @@ func (p *PostsCommandHandler) CreatePost(ctx context.Context, req *gen.PostInput postProfiles := make([]posts.CreatePostProfiles, 0, len(idsString)) for _, profile := range profilesBatch.Profiles { - profileID, _ := uuid.Parse(profile.Id) + profileID, parseErr := uuid.Parse(profile.Id) + if parseErr != nil { + p.log.Error(parseErr, true, "CreatePost: parse profile id") + return &gen.CreatePostInternalServerError{ErrorCode: http.StatusInternalServerError, Message: constants.InternalMsg}, nil + } postProfiles = append(postProfiles, posts.CreatePostProfiles{ ProfileID: profileID, ProfilePrompt: profile.Prompt, @@ -49,7 +53,11 @@ func (p *PostsCommandHandler) CreatePost(ctx context.Context, req *gen.PostInput } groupID := uuid.New() - botID, _ := uuid.Parse(bot.Id) + botID, parseErr := uuid.Parse(bot.Id) + if parseErr != nil { + p.log.Error(parseErr, true, "CreatePost: parse bot id") + return &gen.CreatePostInternalServerError{ErrorCode: http.StatusInternalServerError, Message: constants.InternalMsg}, nil + } createPostRequest := posts.KafkaCreatePostRequest{ PostID: uuid.New(), UserID: userID, diff --git a/internal/delivery_http/posts/posts_command_producer/delete_post.go b/internal/delivery_http/posts/posts_command_producer/delete_post.go index 71b2907..0814d12 100644 --- a/internal/delivery_http/posts/posts_command_producer/delete_post.go +++ b/internal/delivery_http/posts/posts_command_producer/delete_post.go @@ -9,11 +9,17 @@ import ( "github.com/goriiin/kotyari-bots_backend/internal/delivery_http/posts" gen "github.com/goriiin/kotyari-bots_backend/internal/gen/posts/posts_command" "github.com/goriiin/kotyari-bots_backend/pkg/constants" + "github.com/goriiin/kotyari-bots_backend/pkg/user" "github.com/json-iterator/go" ) func (p *PostsCommandHandler) DeletePostById(ctx context.Context, params gen.DeletePostByIdParams) (gen.DeletePostByIdRes, error) { - req := posts.KafkaDeletePostRequest{PostID: params.PostId} + userID, err := user.GetID(ctx) + if err != nil { + return nil, err + } + + req := posts.KafkaDeletePostRequest{PostID: params.PostId, UserID: userID} rawReq, err := jsoniter.Marshal(req) if err != nil { diff --git a/internal/delivery_http/posts/posts_command_producer/publish_post.go b/internal/delivery_http/posts/posts_command_producer/publish_post.go index 78e1162..e5feb1a 100644 --- a/internal/delivery_http/posts/posts_command_producer/publish_post.go +++ b/internal/delivery_http/posts/posts_command_producer/publish_post.go @@ -9,6 +9,7 @@ import ( "github.com/goriiin/kotyari-bots_backend/internal/delivery_http/posts" gen "github.com/goriiin/kotyari-bots_backend/internal/gen/posts/posts_command" "github.com/goriiin/kotyari-bots_backend/pkg/constants" + "github.com/goriiin/kotyari-bots_backend/pkg/user" jsoniter "github.com/json-iterator/go" ) @@ -20,8 +21,14 @@ func (p *PostsCommandHandler) PublishPost(ctx context.Context, req *gen.PublishP }, nil } + userID, err := user.GetID(ctx) + if err != nil { + return nil, err + } + publishRequest := posts.KafkaPublishPostRequest{ PostID: params.PostId, + UserID: userID, Approved: req.Approved, } diff --git a/internal/delivery_http/posts/posts_command_producer/seen_posts.go b/internal/delivery_http/posts/posts_command_producer/seen_posts.go index 86f0d77..f6e34b9 100644 --- a/internal/delivery_http/posts/posts_command_producer/seen_posts.go +++ b/internal/delivery_http/posts/posts_command_producer/seen_posts.go @@ -10,12 +10,19 @@ import ( "github.com/goriiin/kotyari-bots_backend/internal/delivery_http/posts" gen "github.com/goriiin/kotyari-bots_backend/internal/gen/posts/posts_command" "github.com/goriiin/kotyari-bots_backend/pkg/constants" + "github.com/goriiin/kotyari-bots_backend/pkg/user" jsoniter "github.com/json-iterator/go" ) func (p *PostsCommandHandler) SeenPosts(ctx context.Context, req *gen.PostsSeenRequest) (gen.SeenPostsRes, error) { + userID, err := user.GetID(ctx) + if err != nil { + return nil, err + } + seenPostsRequest := posts.KafkaSeenPostsRequest{ PostIDs: req.Seen, + UserID: userID, } rawReq, err := jsoniter.Marshal(seenPostsRequest) @@ -54,7 +61,7 @@ func (p *PostsCommandHandler) SeenPosts(ctx context.Context, req *gen.PostsSeenR }, nil case strings.Contains(resp.Error, constants.NotFoundMsg): - return &gen.SeenPostsInternalServerError{ + return &gen.SeenPostsNotFound{ ErrorCode: http.StatusNotFound, Message: "post not found", }, nil diff --git a/internal/delivery_http/posts/posts_command_producer/update_post.go b/internal/delivery_http/posts/posts_command_producer/update_post.go index 39928ea..9a1279e 100644 --- a/internal/delivery_http/posts/posts_command_producer/update_post.go +++ b/internal/delivery_http/posts/posts_command_producer/update_post.go @@ -9,12 +9,19 @@ import ( "github.com/goriiin/kotyari-bots_backend/internal/delivery_http/posts" gen "github.com/goriiin/kotyari-bots_backend/internal/gen/posts/posts_command" "github.com/goriiin/kotyari-bots_backend/pkg/constants" + "github.com/goriiin/kotyari-bots_backend/pkg/user" "github.com/json-iterator/go" ) func (p *PostsCommandHandler) UpdatePostById(ctx context.Context, req *gen.PostUpdate, params gen.UpdatePostByIdParams) (gen.UpdatePostByIdRes, error) { + userID, err := user.GetID(ctx) + if err != nil { + return nil, err + } + updatePostRequest := posts.KafkaUpdatePostRequest{ PostID: params.PostId, + UserID: userID, Title: req.Title, Text: req.Text, } diff --git a/internal/kafka/consumer/init_basic.go b/internal/kafka/consumer/init_basic.go index 6b96c1d..24dd305 100644 --- a/internal/kafka/consumer/init_basic.go +++ b/internal/kafka/consumer/init_basic.go @@ -44,11 +44,13 @@ func (k *KafkaConsumer) ReadBatches(ctx context.Context) <-chan []kafka.Message for { var messages []kafka.Message - ctx, cancel := context.WithTimeout(ctx, batchTimeout) - defer cancel() + // Per-batch timeout context. Cancel it at the end of each iteration + // instead of deferring (a defer here would accumulate one live timer + // per loop iteration for the lifetime of this goroutine). + batchCtx, cancel := context.WithTimeout(ctx, batchTimeout) for len(messages) < batchSize { - message, err := k.reader.ReadMessage(ctx) + message, err := k.reader.ReadMessage(batchCtx) if err != nil { if errors.Is(err, context.DeadlineExceeded) { break @@ -56,16 +58,20 @@ func (k *KafkaConsumer) ReadBatches(ctx context.Context) <-chan []kafka.Message if errors.Is(err, context.Canceled) { k.log.Warn("kafka is shutting down", err) + cancel() return } k.log.Error(err, false, "unexpected error happened") + cancel() return } messages = append(messages, message) } + cancel() + if len(messages) > 0 { select { case batches <- messages: diff --git a/internal/kafka/consumer/init_reqest_reply.go b/internal/kafka/consumer/init_reqest_reply.go index a135108..9e95a11 100644 --- a/internal/kafka/consumer/init_reqest_reply.go +++ b/internal/kafka/consumer/init_reqest_reply.go @@ -78,19 +78,32 @@ func (c *KafkaRequestReplyConsumer) Start(ctx context.Context) <-chan kafkaConfi messageProcessed := false for !messageProcessed { + // Buffered so the first decision never blocks; subsequent decisions + // for the same message are dropped via the non-blocking signal() + // helper instead of blocking the handler goroutine forever. done := make(chan error, 1) corrID := kafkaConfig.GetHeader(m, "correlation_id") + // signal records the message's terminal decision exactly once. + // Extra calls (a buggy handler invoking Ack/Nack/Reply more than + // once) are ignored rather than deadlocking on a full channel. + signal := func(err error) { + select { + case done <- err: + default: + } + } + cm := kafkaConfig.CommittableMessage{ Msg: m, Ack: func(commitCtx context.Context) error { - done <- nil + signal(nil) return nil }, Nack: func(_ context.Context, err error) error { - done <- err + signal(err) return nil }, @@ -102,12 +115,16 @@ func (c *KafkaRequestReplyConsumer) Start(ctx context.Context) <-chan kafkaConfi Headers: headers, }) if err != nil { - done <- fmt.Errorf("failed to reply: %w", err) + signal(fmt.Errorf("failed to reply: %w", err)) return err } + // moveOffset == false means "I replied, but I'll Ack/Nack + // separately" (e.g. create-then-async). The caller MUST then + // call Ack/Nack, otherwise the message is retried after the + // reader's session times out rather than blocking forever. if moveOffset { - done <- nil + signal(nil) } return nil diff --git a/internal/logger/logger.go b/internal/logger/logger.go index 32c4f37..911c69b 100644 --- a/internal/logger/logger.go +++ b/internal/logger/logger.go @@ -1,6 +1,7 @@ package logger import ( + stderrors "errors" "os" "strings" @@ -9,6 +10,14 @@ import ( "github.com/rs/zerolog/pkgerrors" ) +// joinErrs combines any number of (possibly nil) errors into a single error, +// returning nil when none are non-nil. Used so the variadic logging helpers +// below attach every supplied error instead of silently dropping all of them +// whenever more than one is passed. +func joinErrs(errs ...error) error { + return stderrors.Join(errs...) +} + const service = "service_name" type Logger struct { @@ -59,8 +68,8 @@ func (l *Logger) Error(err error, withStack bool, msg ...string) { } func (l *Logger) Warn(msg string, err ...error) { - if len(err) == 1 { - l.log.Warn().Err(err[0]).Msg(msg) + if joined := joinErrs(err...); joined != nil { + l.log.Warn().Err(joined).Msg(msg) return } @@ -68,8 +77,8 @@ func (l *Logger) Warn(msg string, err ...error) { } func (l *Logger) Info(msg string, err ...error) { - if len(err) == 1 { - l.log.Info().Err(err[0]).Msg(msg) + if joined := joinErrs(err...); joined != nil { + l.log.Info().Err(joined).Msg(msg) return } @@ -77,8 +86,8 @@ func (l *Logger) Info(msg string, err ...error) { } func (l *Logger) Debug(msg string, err ...error) { - if len(err) == 1 { - l.log.Debug().Err(err[0]).Msg(msg) + if joined := joinErrs(err...); joined != nil { + l.log.Debug().Err(joined).Msg(msg) return } diff --git a/internal/logger/logger_test.go b/internal/logger/logger_test.go new file mode 100644 index 0000000..21f068e --- /dev/null +++ b/internal/logger/logger_test.go @@ -0,0 +1,43 @@ +package logger + +import ( + "errors" + "testing" +) + +// TestJoinErrs ensures the variadic logging helpers no longer silently drop +// errors when more than one is passed: joinErrs must surface every non-nil +// error and return nil only when there are none. +func TestJoinErrs(t *testing.T) { + err1 := errors.New("first") + err2 := errors.New("second") + + t.Run("nil when empty", func(t *testing.T) { + if joinErrs() != nil { + t.Fatal("expected nil for no errors") + } + }) + + t.Run("nil when all nil", func(t *testing.T) { + if joinErrs(nil, nil) != nil { + t.Fatal("expected nil when all errors are nil") + } + }) + + t.Run("single error passes through", func(t *testing.T) { + got := joinErrs(err1) + if !errors.Is(got, err1) { + t.Fatalf("expected joined error to wrap err1, got %v", got) + } + }) + + t.Run("multiple errors are all retained", func(t *testing.T) { + got := joinErrs(err1, err2) + if got == nil { + t.Fatal("expected non-nil joined error") + } + if !errors.Is(got, err1) || !errors.Is(got, err2) { + t.Fatalf("expected joined error to wrap both err1 and err2, got %v", got) + } + }) +} diff --git a/internal/repo/aggregator/add_topics.go b/internal/repo/aggregator/add_topics.go index 57c0c79..8f29d95 100644 --- a/internal/repo/aggregator/add_topics.go +++ b/internal/repo/aggregator/add_topics.go @@ -24,6 +24,11 @@ func (a *AggregatorRepo) AddTopics(ctx context.Context, topics []model.Topic) er } b := a.db.SendBatch(ctx, batch) + defer func() { + // Always release the batch results / connection, even on early return. + _ = b.Close() + }() + for range topics { _, err := b.Exec() if err != nil { diff --git a/internal/repo/bots/get.go b/internal/repo/bots/get.go index b0a3961..e187a7b 100644 --- a/internal/repo/bots/get.go +++ b/internal/repo/bots/get.go @@ -27,7 +27,7 @@ func (r BotsRepository) Get(ctx context.Context, id uuid.UUID) (model.Bot, error if err != nil { return model.Bot{}, err } - dto, err := pgx.CollectOneRow(rows, pgx.RowToStructByPos[botDTO]) + dto, err := pgx.CollectOneRow(rows, pgx.RowToStructByName[botDTO]) if err != nil { if errors.Is(err, pgx.ErrNoRows) { return model.Bot{}, constants.ErrNotFound @@ -47,7 +47,7 @@ func (r BotsRepository) Get(ctx context.Context, id uuid.UUID) (model.Bot, error if err != nil { return model.Bot{}, err } - dto, err := pgx.CollectOneRow(rows, pgx.RowToStructByPos[botDTO]) + dto, err := pgx.CollectOneRow(rows, pgx.RowToStructByName[botDTO]) if err != nil { if errors.Is(err, pgx.ErrNoRows) { return model.Bot{}, constants.ErrNotFound diff --git a/internal/repo/bots/profiles.go b/internal/repo/bots/profiles.go index b2a3a3c..4ffbf9f 100644 --- a/internal/repo/bots/profiles.go +++ b/internal/repo/bots/profiles.go @@ -20,7 +20,7 @@ func (r BotsRepository) AddProfileID(ctx context.Context, botID, profileID uuid. profile_ids = array_append(COALESCE(profile_ids, '{}'::uuid[]), $2), profiles_count = COALESCE(array_length(array_append(COALESCE(profile_ids, '{}'::uuid[]), $2), 1), 0), updated_at = now() - WHERE id = $1 AND user_id = $3 + WHERE id = $1 AND user_id = $3 AND is_deleted = false AND NOT $2 = ANY(COALESCE(profile_ids, '{}'::uuid[])) `, botID, profileID, userID) if err != nil { @@ -44,7 +44,7 @@ func (r BotsRepository) RemoveProfileID(ctx context.Context, botID, profileID uu profile_ids = array_remove(COALESCE(profile_ids, '{}'::uuid[]), $2), profiles_count = COALESCE(array_length(array_remove(COALESCE(profile_ids, '{}'::uuid[]), $2), 1), 0), updated_at = now() - WHERE id = $1 AND user_id = $3 + WHERE id = $1 AND user_id = $3 AND is_deleted = false `, botID, profileID, userID) if err != nil { return err diff --git a/internal/repo/bots/update.go b/internal/repo/bots/update.go index 5f5d0c8..72684b1 100644 --- a/internal/repo/bots/update.go +++ b/internal/repo/bots/update.go @@ -22,7 +22,7 @@ func (r *BotsRepository) Update(ctx context.Context, b model.Bot) error { profile_ids = $5, profiles_count = $6, updated_at = now() - WHERE id = $1 AND user_id = $7`, + WHERE id = $1 AND user_id = $7 AND is_deleted = false`, b.ID, b.Name, b.SystemPrompt, b.ModerationRequired, b.ProfileIDs, len(b.ProfileIDs), userID, ) if err != nil { diff --git a/internal/repo/posts/posts_command/create_post.go b/internal/repo/posts/posts_command/create_post.go index 2e2aea9..9acd5bd 100644 --- a/internal/repo/posts/posts_command/create_post.go +++ b/internal/repo/posts/posts_command/create_post.go @@ -21,10 +21,13 @@ func (p *PostsCommandRepo) CreatePost(ctx context.Context, post model.Post, cate return model.Post{}, errors.Wrapf(constants.ErrInternal, "failed to begin transaction: %s", err.Error()) } + // Roll back unless the function returns successfully. Rollback after a + // successful Commit is a no-op (pgx returns ErrTxClosed, which we ignore), + // and we must not overwrite the real error with the rollback's result. + committed := false defer func() { - if err != nil { - err = tx.Rollback(ctx) - // TODO: log err + if !committed { + _ = tx.Rollback(ctx) } }() @@ -34,16 +37,19 @@ func (p *PostsCommandRepo) CreatePost(ctx context.Context, post model.Post, cate } const query = ` - INSERT INTO posts (id, otveti_id, bot_id, profile_id, group_id, user_prompt, platform_type, post_type, post_title, post_text, is_seen) - VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$10, $11) + INSERT INTO posts (id, user_id, otveti_id, bot_id, bot_name, profile_id, profile_name, group_id, user_prompt, platform_type, post_type, post_title, post_text, is_seen) + VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$10,$11,$12,$13,$14) RETURNING created_at, updated_at ` row := tx.QueryRow(ctx, query, post.ID, + post.UserID, post.OtvetiID, post.BotID, + post.BotName, post.ProfileID, + post.ProfileName, post.GroupID, post.UserPrompt, post.Platform, @@ -57,14 +63,15 @@ func (p *PostsCommandRepo) CreatePost(ctx context.Context, post model.Post, cate } if len(categoryIDs) > 0 { - err = p.insertPostCategoriesBatch(ctx, tx, post.ID, categoryIDs) - return model.Post{}, err + if err = p.insertPostCategoriesBatch(ctx, tx, post.ID, categoryIDs); err != nil { + return model.Post{}, err + } } - err = tx.Commit(ctx) - if err != nil { + if err = tx.Commit(ctx); err != nil { return model.Post{}, errors.Wrap(constants.ErrInternal, "failed to commit transaction") } + committed = true return post, nil } diff --git a/internal/repo/posts/posts_command/delete_post.go b/internal/repo/posts/posts_command/delete_post.go index 9a5bf0b..ba86825 100644 --- a/internal/repo/posts/posts_command/delete_post.go +++ b/internal/repo/posts/posts_command/delete_post.go @@ -8,12 +8,12 @@ import ( "github.com/goriiin/kotyari-bots_backend/pkg/constants" ) -func (p *PostsCommandRepo) DeletePost(ctx context.Context, id uuid.UUID) error { +func (p *PostsCommandRepo) DeletePost(ctx context.Context, id, userID uuid.UUID) error { const query = ` - DELETE FROM posts WHERE id=$1 + DELETE FROM posts WHERE id=$1 AND user_id=$2 ` - ct, err := p.db.Exec(ctx, query, id) + ct, err := p.db.Exec(ctx, query, id, userID) if err != nil { return errors.Wrapf(constants.ErrInternal, "failed to delete post: %s", err.Error()) } diff --git a/internal/repo/posts/posts_command/seen_posts.go b/internal/repo/posts/posts_command/seen_posts.go index 4b57b90..6d6efdf 100644 --- a/internal/repo/posts/posts_command/seen_posts.go +++ b/internal/repo/posts/posts_command/seen_posts.go @@ -10,11 +10,11 @@ import ( "github.com/jackc/pgx/v5" ) -func (p *PostsCommandRepo) SeenPostsBatch(ctx context.Context, postsIds []uuid.UUID) (err error) { +func (p *PostsCommandRepo) SeenPostsBatch(ctx context.Context, postsIds []uuid.UUID, userID uuid.UUID) (err error) { const query = ` UPDATE posts SET is_seen = $1 - WHERE id = $2 + WHERE id = $2 AND user_id = $3 ` batch := &pgx.Batch{} @@ -23,6 +23,7 @@ func (p *PostsCommandRepo) SeenPostsBatch(ctx context.Context, postsIds []uuid.U batch.Queue(query, true, id, + userID, ) } diff --git a/internal/repo/posts/posts_command/update_post.go b/internal/repo/posts/posts_command/update_post.go index 0599e43..8574f04 100644 --- a/internal/repo/posts/posts_command/update_post.go +++ b/internal/repo/posts/posts_command/update_post.go @@ -14,11 +14,11 @@ func (p *PostsCommandRepo) UpdatePost(ctx context.Context, post model.Post) (mod const query = ` UPDATE posts SET post_title=$1, post_text=$2, updated_at=NOW() - WHERE id=$3 + WHERE id=$3 AND user_id=$4 RETURNING id, otveti_id, bot_id, bot_name, profile_id, profile_name, group_id, platform_type, user_prompt, post_type, post_title, post_text, created_at, updated_at ` - rows, err := p.db.Query(ctx, query, post.Title, post.Text, post.ID) + rows, err := p.db.Query(ctx, query, post.Title, post.Text, post.ID, post.UserID) if err != nil { return model.Post{}, errors.Wrapf(constants.ErrInternal, "failed to update post: %s", err.Error()) } diff --git a/internal/repo/profiles/get.go b/internal/repo/profiles/get.go index da2e996..75ba648 100644 --- a/internal/repo/profiles/get.go +++ b/internal/repo/profiles/get.go @@ -3,25 +3,32 @@ package profiles import ( "context" + "github.com/go-faster/errors" "github.com/google/uuid" "github.com/goriiin/kotyari-bots_backend/internal/constants" "github.com/goriiin/kotyari-bots_backend/internal/model" + "github.com/goriiin/kotyari-bots_backend/pkg/user" "github.com/jackc/pgx/v5" ) func (r *Repository) GetByID(ctx context.Context, id uuid.UUID) (model.Profile, error) { + userID, err := user.GetID(ctx) + if err != nil { + return model.Profile{}, err + } + rows, err := r.db.Query(ctx, `SELECT id, name, email, system_prompt, created_at, updated_at FROM profiles - WHERE id=$1`, - id) + WHERE id=$1 AND user_id=$2`, + id, userID) if err != nil { return model.Profile{}, err } dto, err := pgx.CollectOneRow(rows, pgx.RowToStructByName[profileDTO]) if err != nil { - if err == pgx.ErrNoRows { + if errors.Is(err, pgx.ErrNoRows) { return model.Profile{}, constants.ErrNotFound } return model.Profile{}, err @@ -29,6 +36,11 @@ func (r *Repository) GetByID(ctx context.Context, id uuid.UUID) (model.Profile, return dto.toModel(), nil } +// GetByIDs is an internal service-to-service lookup (called over gRPC by the +// bots service to resolve a bot's profiles). The gRPC contract carries no +// user_id, so this is intentionally not tenant-scoped; callers must only pass +// IDs they are already authorized to read. User-facing reads go through +// GetByID, which is scoped by user_id. func (r *Repository) GetByIDs(ctx context.Context, ids []uuid.UUID) ([]model.Profile, error) { rows, err := r.db.Query(ctx, `SELECT id, name, email, system_prompt, created_at, updated_at FROM profiles WHERE id = ANY($1)`, ids) if err != nil {