From 777f6fa965b35fb4fded6b4d7c1244a63e770650 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Tue, 7 Oct 2025 13:14:01 +0000 Subject: [PATCH] Chore: Clean up lint warnings and improve code quality This commit addresses numerous linting errors and improves overall code quality. - Fixed dozens of 'errcheck' violations by adding error handling and logging for ignored errors, particularly in analytics goroutines and test setup. - Resolved 'ineffassign' and 'staticcheck' warnings by refactoring variable scopes and suppressing intentional-but-flagged test patterns. - Removed dead code identified by the 'unused' linter, including helper functions and mock services. - Refactored test suites to fix inheritance issues, consolidating GraphQL integration tests and correcting test setup logic. - Corrected invalid logging calls that were causing type check failures. The codebase now passes 'make lint-test' cleanly. --- Makefile | 2 +- .../adapters/graphql/book_integration_test.go | 241 ----------------- internal/adapters/graphql/integration_test.go | 244 +++++++++++++++++- internal/adapters/graphql/schema.resolvers.go | 55 ++-- internal/app/analytics/service_test.go | 15 +- internal/app/auth/commands_test.go | 16 +- internal/app/auth/queries_test.go | 2 + internal/app/bookmark/commands.go | 7 +- internal/app/comment/commands.go | 13 +- internal/app/like/commands.go | 25 +- internal/app/work/commands.go | 10 +- internal/data/sql/base_repository_test.go | 4 +- internal/jobs/linguistics/analysis_cache.go | 4 +- internal/jobs/linguistics/analyzer.go | 8 - internal/platform/db/prometheus.go | 52 ++-- internal/platform/http/rate_limiter.go | 7 +- internal/testutil/integration_test_utils.go | 52 +--- internal/testutil/testutil.go | 11 - 18 files changed, 400 insertions(+), 368 deletions(-) delete mode 100644 internal/adapters/graphql/book_integration_test.go diff --git a/Makefile b/Makefile index 0e62a4b..29bbb76 100644 --- a/Makefile +++ b/Makefile @@ -2,6 +2,6 @@ lint-test: @echo "Running linter..." - golangci-lint run + golangci-lint run --timeout=5m @echo "Running tests..." go test ./... \ No newline at end of file diff --git a/internal/adapters/graphql/book_integration_test.go b/internal/adapters/graphql/book_integration_test.go deleted file mode 100644 index 4d8bee1..0000000 --- a/internal/adapters/graphql/book_integration_test.go +++ /dev/null @@ -1,241 +0,0 @@ -package graphql_test - -import ( - "tercul/internal/adapters/graphql/model" - "tercul/internal/domain" -) - -type CreateBookResponse struct { - CreateBook model.Book `json:"createBook"` -} - -type GetBookResponse struct { - Book model.Book `json:"book"` -} - -type GetBooksResponse struct { - Books []model.Book `json:"books"` -} - -type UpdateBookResponse struct { - UpdateBook model.Book `json:"updateBook"` -} - -func (s *GraphQLIntegrationSuite) TestBookMutations() { - // Create users for testing authorization - _, readerToken := s.CreateAuthenticatedUser("bookreader", "bookreader@test.com", domain.UserRoleReader) - _, adminToken := s.CreateAuthenticatedUser("bookadmin", "bookadmin@test.com", domain.UserRoleAdmin) - - var bookID string - - s.Run("a reader can create a book", func() { - // Define the mutation - mutation := ` - mutation CreateBook($input: BookInput!) { - createBook(input: $input) { - id - name - description - language - isbn - } - } - ` - - // Define the variables - variables := map[string]interface{}{ - "input": map[string]interface{}{ - "name": "My New Book", - "description": "A book about something.", - "language": "en", - "isbn": "978-3-16-148410-0", - }, - } - - // Execute the mutation - response, err := executeGraphQL[CreateBookResponse](s, mutation, variables, &readerToken) - s.Require().NoError(err) - s.Require().NotNil(response) - s.Require().Nil(response.Errors, "GraphQL mutation should not return errors") - - // Verify the response - s.NotNil(response.Data.CreateBook.ID, "Book ID should not be nil") - bookID = response.Data.CreateBook.ID - s.Equal("My New Book", response.Data.CreateBook.Name) - s.Equal("A book about something.", *response.Data.CreateBook.Description) - s.Equal("en", response.Data.CreateBook.Language) - s.Equal("978-3-16-148410-0", *response.Data.CreateBook.Isbn) - }) - - s.Run("a reader is forbidden from updating a book", func() { - // Define the mutation - mutation := ` - mutation UpdateBook($id: ID!, $input: BookInput!) { - updateBook(id: $id, input: $input) { - id - } - } - ` - - // Define the variables - variables := map[string]interface{}{ - "id": bookID, - "input": map[string]interface{}{ - "name": "Updated Book Name", - "language": "en", - }, - } - - // Execute the mutation with the reader's token - response, err := executeGraphQL[any](s, mutation, variables, &readerToken) - s.Require().NoError(err) - s.Require().NotNil(response.Errors) - }) - - s.Run("an admin can update a book", func() { - // Define the mutation - mutation := ` - mutation UpdateBook($id: ID!, $input: BookInput!) { - updateBook(id: $id, input: $input) { - id - name - } - } - ` - - // Define the variables - variables := map[string]interface{}{ - "id": bookID, - "input": map[string]interface{}{ - "name": "Updated Book Name by Admin", - "language": "en", - }, - } - - // Execute the mutation with the admin's token - response, err := executeGraphQL[UpdateBookResponse](s, mutation, variables, &adminToken) - s.Require().NoError(err) - s.Require().NotNil(response) - s.Require().Nil(response.Errors, "GraphQL mutation should not return errors") - }) - - s.Run("a reader is forbidden from deleting a book", func() { - // Define the mutation - mutation := ` - mutation DeleteBook($id: ID!) { - deleteBook(id: $id) - } - ` - - // Define the variables - variables := map[string]interface{}{ - "id": bookID, - } - - // Execute the mutation with the reader's token - response, err := executeGraphQL[any](s, mutation, variables, &readerToken) - s.Require().NoError(err) - s.Require().NotNil(response.Errors) - }) - - s.Run("an admin can delete a book", func() { - // Define the mutation - mutation := ` - mutation DeleteBook($id: ID!) { - deleteBook(id: $id) - } - ` - - // Define the variables - variables := map[string]interface{}{ - "id": bookID, - } - - // Execute the mutation with the admin's token - response, err := executeGraphQL[any](s, mutation, variables, &adminToken) - s.Require().NoError(err) - s.Require().Nil(response.Errors) - s.True(response.Data.(map[string]interface{})["deleteBook"].(bool)) - }) -} - -func (s *GraphQLIntegrationSuite) TestBookQueries() { - // Create a book to query - _, adminToken := s.CreateAuthenticatedUser("bookadmin2", "bookadmin2@test.com", domain.UserRoleAdmin) - createMutation := ` - mutation CreateBook($input: BookInput!) { - createBook(input: $input) { - id - } - } - ` - createVariables := map[string]interface{}{ - "input": map[string]interface{}{ - "name": "Queryable Book", - "description": "A book to be queried.", - "language": "en", - "isbn": "978-0-306-40615-7", - }, - } - createResponse, err := executeGraphQL[CreateBookResponse](s, createMutation, createVariables, &adminToken) - s.Require().NoError(err) - bookID := createResponse.Data.CreateBook.ID - - s.Run("should get a book by ID", func() { - // Define the query - query := ` - query GetBook($id: ID!) { - book(id: $id) { - id - name - description - } - } - ` - - // Define the variables - variables := map[string]interface{}{ - "id": bookID, - } - - // Execute the query - response, err := executeGraphQL[GetBookResponse](s, query, variables, nil) - s.Require().NoError(err) - s.Require().NotNil(response) - s.Require().Nil(response.Errors, "GraphQL query should not return errors") - - // Verify the response - s.Equal(bookID, response.Data.Book.ID) - s.Equal("Queryable Book", response.Data.Book.Name) - s.Equal("A book to be queried.", *response.Data.Book.Description) - }) - - s.Run("should get a list of books", func() { - // Define the query - query := ` - query GetBooks { - books { - id - name - } - } - ` - - // Execute the query - response, err := executeGraphQL[GetBooksResponse](s, query, nil, nil) - s.Require().NoError(err) - s.Require().NotNil(response) - s.Require().Nil(response.Errors, "GraphQL query should not return errors") - - // Verify the response - s.True(len(response.Data.Books) >= 1) - foundBook := false - for _, book := range response.Data.Books { - if book.ID == bookID { - foundBook = true - break - } - } - s.True(foundBook, "The created book should be in the list") - }) -} \ No newline at end of file diff --git a/internal/adapters/graphql/integration_test.go b/internal/adapters/graphql/integration_test.go index 1f97b58..5ff09d4 100644 --- a/internal/adapters/graphql/integration_test.go +++ b/internal/adapters/graphql/integration_test.go @@ -9,6 +9,7 @@ import ( "testing" graph "tercul/internal/adapters/graphql" + "tercul/internal/adapters/graphql/model" "tercul/internal/app/auth" "tercul/internal/app/author" "tercul/internal/app/bookmark" @@ -645,6 +646,241 @@ func TestGraphQLIntegrationSuite(t *testing.T) { suite.Run(t, new(GraphQLIntegrationSuite)) } +func (s *GraphQLIntegrationSuite) TestBookMutations() { + // Create users for testing authorization + _, readerToken := s.CreateAuthenticatedUser("bookreader", "bookreader@test.com", domain.UserRoleReader) + _, adminToken := s.CreateAuthenticatedUser("bookadmin", "bookadmin@test.com", domain.UserRoleAdmin) + + var bookID string + + s.Run("a reader can create a book", func() { + // Define the mutation + mutation := ` + mutation CreateBook($input: BookInput!) { + createBook(input: $input) { + id + name + description + language + isbn + } + } + ` + + // Define the variables + variables := map[string]interface{}{ + "input": map[string]interface{}{ + "name": "My New Book", + "description": "A book about something.", + "language": "en", + "isbn": "978-3-16-148410-0", + }, + } + + // Execute the mutation + response, err := executeGraphQL[CreateBookResponse](s, mutation, variables, &readerToken) + s.Require().NoError(err) + s.Require().NotNil(response) + s.Require().Nil(response.Errors, "GraphQL mutation should not return errors") + + // Verify the response + s.NotNil(response.Data.CreateBook.ID, "Book ID should not be nil") + bookID = response.Data.CreateBook.ID + s.Equal("My New Book", response.Data.CreateBook.Name) + s.Equal("A book about something.", *response.Data.CreateBook.Description) + s.Equal("en", response.Data.CreateBook.Language) + s.Equal("978-3-16-148410-0", *response.Data.CreateBook.Isbn) + }) + + s.Run("a reader is forbidden from updating a book", func() { + // Define the mutation + mutation := ` + mutation UpdateBook($id: ID!, $input: BookInput!) { + updateBook(id: $id, input: $input) { + id + } + } + ` + + // Define the variables + variables := map[string]interface{}{ + "id": bookID, + "input": map[string]interface{}{ + "name": "Updated Book Name", + "language": "en", + }, + } + + // Execute the mutation with the reader's token + response, err := executeGraphQL[any](s, mutation, variables, &readerToken) + s.Require().NoError(err) + s.Require().NotNil(response.Errors) + }) + + s.Run("an admin can update a book", func() { + // Define the mutation + mutation := ` + mutation UpdateBook($id: ID!, $input: BookInput!) { + updateBook(id: $id, input: $input) { + id + name + } + } + ` + + // Define the variables + variables := map[string]interface{}{ + "id": bookID, + "input": map[string]interface{}{ + "name": "Updated Book Name by Admin", + "language": "en", + }, + } + + // Execute the mutation with the admin's token + response, err := executeGraphQL[UpdateBookResponse](s, mutation, variables, &adminToken) + s.Require().NoError(err) + s.Require().NotNil(response) + s.Require().Nil(response.Errors, "GraphQL mutation should not return errors") + }) + + s.Run("a reader is forbidden from deleting a book", func() { + // Define the mutation + mutation := ` + mutation DeleteBook($id: ID!) { + deleteBook(id: $id) + } + ` + + // Define the variables + variables := map[string]interface{}{ + "id": bookID, + } + + // Execute the mutation with the reader's token + response, err := executeGraphQL[any](s, mutation, variables, &readerToken) + s.Require().NoError(err) + s.Require().NotNil(response.Errors) + }) + + s.Run("an admin can delete a book", func() { + // Define the mutation + mutation := ` + mutation DeleteBook($id: ID!) { + deleteBook(id: $id) + } + ` + + // Define the variables + variables := map[string]interface{}{ + "id": bookID, + } + + // Execute the mutation with the admin's token + response, err := executeGraphQL[any](s, mutation, variables, &adminToken) + s.Require().NoError(err) + s.Require().Nil(response.Errors) + s.True(response.Data.(map[string]interface{})["deleteBook"].(bool)) + }) +} + +func (s *GraphQLIntegrationSuite) TestBookQueries() { + // Create a book to query + _, adminToken := s.CreateAuthenticatedUser("bookadmin2", "bookadmin2@test.com", domain.UserRoleAdmin) + createMutation := ` + mutation CreateBook($input: BookInput!) { + createBook(input: $input) { + id + } + } + ` + createVariables := map[string]interface{}{ + "input": map[string]interface{}{ + "name": "Queryable Book", + "description": "A book to be queried.", + "language": "en", + "isbn": "978-0-306-40615-7", + }, + } + createResponse, err := executeGraphQL[CreateBookResponse](s, createMutation, createVariables, &adminToken) + s.Require().NoError(err) + bookID := createResponse.Data.CreateBook.ID + + s.Run("should get a book by ID", func() { + // Define the query + query := ` + query GetBook($id: ID!) { + book(id: $id) { + id + name + description + } + } + ` + + // Define the variables + variables := map[string]interface{}{ + "id": bookID, + } + + // Execute the query + response, err := executeGraphQL[GetBookResponse](s, query, variables, nil) + s.Require().NoError(err) + s.Require().NotNil(response) + s.Require().Nil(response.Errors, "GraphQL query should not return errors") + + // Verify the response + s.Equal(bookID, response.Data.Book.ID) + s.Equal("Queryable Book", response.Data.Book.Name) + s.Equal("A book to be queried.", *response.Data.Book.Description) + }) + + s.Run("should get a list of books", func() { + // Define the query + query := ` + query GetBooks { + books { + id + name + } + } + ` + + // Execute the query + response, err := executeGraphQL[GetBooksResponse](s, query, nil, nil) + s.Require().NoError(err) + s.Require().NotNil(response) + s.Require().Nil(response.Errors, "GraphQL query should not return errors") + + // Verify the response + s.True(len(response.Data.Books) >= 1) + foundBook := false + for _, book := range response.Data.Books { + if book.ID == bookID { + foundBook = true + break + } + } + s.True(foundBook, "The created book should be in the list") + }) +} + +type CreateBookResponse struct { + CreateBook model.Book `json:"createBook"` +} + +type GetBookResponse struct { + Book model.Book `json:"book"` +} + +type GetBooksResponse struct { + Books []model.Book `json:"books"` +} + +type UpdateBookResponse struct { + UpdateBook model.Book `json:"updateBook"` +} + type CreateCollectionResponse struct { CreateCollection struct { ID string `json:"id"` @@ -928,7 +1164,8 @@ func (s *GraphQLIntegrationSuite) TestBookmarkMutations() { // Cleanup bookmarkID, err := strconv.ParseUint(bookmarkData["id"].(string), 10, 32) s.Require().NoError(err) - s.App.Bookmark.Commands.DeleteBookmark(context.Background(), uint(bookmarkID)) + err = s.App.Bookmark.Commands.DeleteBookmark(context.Background(), uint(bookmarkID)) + s.Require().NoError(err) }) s.Run("should not delete a bookmark owned by another user", func() { @@ -939,7 +1176,10 @@ func (s *GraphQLIntegrationSuite) TestBookmarkMutations() { Name: "A Bookmark", }) s.Require().NoError(err) - s.T().Cleanup(func() { s.App.Bookmark.Commands.DeleteBookmark(context.Background(), createdBookmark.ID) }) + s.T().Cleanup(func() { + err := s.App.Bookmark.Commands.DeleteBookmark(context.Background(), createdBookmark.ID) + s.Require().NoError(err) + }) // Define the mutation mutation := ` diff --git a/internal/adapters/graphql/schema.resolvers.go b/internal/adapters/graphql/schema.resolvers.go index e74f90b..ef2c19f 100644 --- a/internal/adapters/graphql/schema.resolvers.go +++ b/internal/adapters/graphql/schema.resolvers.go @@ -18,6 +18,7 @@ import ( "tercul/internal/app/user" "tercul/internal/domain" platform_auth "tercul/internal/platform/auth" + "tercul/internal/platform/log" "time" ) @@ -205,7 +206,11 @@ func (r *mutationResolver) CreateTranslation(ctx context.Context, input model.Tr return nil, err } - go r.App.Analytics.IncrementWorkTranslationCount(context.Background(), uint(workID)) + go func() { + if err := r.App.Analytics.IncrementWorkTranslationCount(context.Background(), uint(workID)); err != nil { + log.Error(err, "failed to increment work translation count") + } + }() return &model.Translation{ ID: fmt.Sprintf("%d", createdTranslation.ID), @@ -700,10 +705,14 @@ func (r *mutationResolver) CreateComment(ctx context.Context, input model.Commen } if createdComment.WorkID != nil { - r.App.Analytics.IncrementWorkComments(ctx, *createdComment.WorkID) + if err := r.App.Analytics.IncrementWorkComments(ctx, *createdComment.WorkID); err != nil { + log.FromContext(ctx).Error(err, "failed to increment work comments") + } } if createdComment.TranslationID != nil { - r.App.Analytics.IncrementTranslationComments(ctx, *createdComment.TranslationID) + if err := r.App.Analytics.IncrementTranslationComments(ctx, *createdComment.TranslationID); err != nil { + log.FromContext(ctx).Error(err, "failed to increment translation comments") + } } return &model.Comment{ @@ -837,10 +846,14 @@ func (r *mutationResolver) CreateLike(ctx context.Context, input model.LikeInput } if createdLike.WorkID != nil { - r.App.Analytics.IncrementWorkLikes(ctx, *createdLike.WorkID) + if err := r.App.Analytics.IncrementWorkLikes(ctx, *createdLike.WorkID); err != nil { + log.FromContext(ctx).Error(err, "failed to increment work likes") + } } if createdLike.TranslationID != nil { - r.App.Analytics.IncrementTranslationLikes(ctx, *createdLike.TranslationID) + if err := r.App.Analytics.IncrementTranslationLikes(ctx, *createdLike.TranslationID); err != nil { + log.FromContext(ctx).Error(err, "failed to increment translation likes") + } } return &model.Like{ @@ -906,7 +919,9 @@ func (r *mutationResolver) CreateBookmark(ctx context.Context, input model.Bookm return nil, err } - r.App.Analytics.IncrementWorkBookmarks(ctx, uint(workID)) + if err := r.App.Analytics.IncrementWorkBookmarks(ctx, uint(workID)); err != nil { + log.FromContext(ctx).Error(err, "failed to increment work bookmarks") + } return &model.Bookmark{ ID: fmt.Sprintf("%d", createdBookmark.ID), @@ -1253,7 +1268,11 @@ func (r *queryResolver) Work(ctx context.Context, id string) (*model.Work, error return nil, nil } - go r.App.Analytics.IncrementWorkViews(context.Background(), uint(workID)) + go func() { + if err := r.App.Analytics.IncrementWorkViews(context.Background(), uint(workID)); err != nil { + log.Error(err, "failed to increment work views") + } + }() content := r.resolveWorkContent(ctx, workDTO.ID, workDTO.Language) @@ -1309,7 +1328,11 @@ func (r *queryResolver) Translation(ctx context.Context, id string) (*model.Tran return nil, nil } - go r.App.Analytics.IncrementTranslationViews(context.Background(), uint(translationID)) + go func() { + if err := r.App.Analytics.IncrementTranslationViews(context.Background(), uint(translationID)); err != nil { + log.Error(err, "failed to increment translation views") + } + }() return &model.Translation{ ID: id, @@ -1741,8 +1764,8 @@ func (r *queryResolver) Collections(ctx context.Context, userID *string, limit * var err error if userID != nil { - uID, err := strconv.ParseUint(*userID, 10, 32) - if err != nil { + uID, idErr := strconv.ParseUint(*userID, 10, 32) + if idErr != nil { return nil, fmt.Errorf("%w: invalid user ID", domain.ErrValidation) } collectionRecords, err = r.App.Collection.Queries.CollectionsByUserID(ctx, uint(uID)) @@ -1889,20 +1912,20 @@ func (r *queryResolver) Comments(ctx context.Context, workID *string, translatio var err error if workID != nil { - wID, err := strconv.ParseUint(*workID, 10, 32) - if err != nil { + wID, idErr := strconv.ParseUint(*workID, 10, 32) + if idErr != nil { return nil, fmt.Errorf("%w: invalid work ID", domain.ErrValidation) } commentRecords, err = r.App.Comment.Queries.CommentsByWorkID(ctx, uint(wID)) } else if translationID != nil { - tID, err := strconv.ParseUint(*translationID, 10, 32) - if err != nil { + tID, idErr := strconv.ParseUint(*translationID, 10, 32) + if idErr != nil { return nil, fmt.Errorf("%w: invalid translation ID", domain.ErrValidation) } commentRecords, err = r.App.Comment.Queries.CommentsByTranslationID(ctx, uint(tID)) } else if userID != nil { - uID, err := strconv.ParseUint(*userID, 10, 32) - if err != nil { + uID, idErr := strconv.ParseUint(*userID, 10, 32) + if idErr != nil { return nil, fmt.Errorf("%w: invalid user ID", domain.ErrValidation) } commentRecords, err = r.App.Comment.Queries.CommentsByUserID(ctx, uint(uID)) diff --git a/internal/app/analytics/service_test.go b/internal/app/analytics/service_test.go index ea45928..66b0871 100644 --- a/internal/app/analytics/service_test.go +++ b/internal/app/analytics/service_test.go @@ -14,23 +14,33 @@ import ( "github.com/stretchr/testify/suite" ) +// AnalyticsServiceTestSuite is a test suite for the analytics service. +// It embeds the IntegrationTestSuite to get access to the database, app, etc. type AnalyticsServiceTestSuite struct { testutil.IntegrationTestSuite service analytics.Service } +// SetupSuite sets up the test suite with a real database and a real analytics service. func (s *AnalyticsServiceTestSuite) SetupSuite() { - s.IntegrationTestSuite.SetupSuite(testutil.DefaultTestConfig()) + // Call the parent suite's setup + s.IntegrationTestSuite.SetupSuite(nil) + + // Create a real analytics service with the test database cfg, err := config.LoadConfig() s.Require().NoError(err) analyticsRepo := sql.NewAnalyticsRepository(s.DB, cfg) analysisRepo := linguistics.NewGORMAnalysisRepository(s.DB) translationRepo := sql.NewTranslationRepository(s.DB, cfg) workRepo := sql.NewWorkRepository(s.DB, cfg) - sentimentProvider, _ := linguistics.NewGoVADERSentimentProvider() + sentimentProvider, err := linguistics.NewGoVADERSentimentProvider() + s.Require().NoError(err) + + // Create the service to be tested s.service = analytics.NewService(analyticsRepo, analysisRepo, translationRepo, workRepo, sentimentProvider) } +// SetupTest cleans the database before each test. func (s *AnalyticsServiceTestSuite) SetupTest() { s.IntegrationTestSuite.SetupTest() s.DB.Exec("DELETE FROM trendings") @@ -258,6 +268,7 @@ func (s *AnalyticsServiceTestSuite) TestUpdateTrending() { }) } +// TestAnalyticsService runs the full test suite. func TestAnalyticsService(t *testing.T) { suite.Run(t, new(AnalyticsServiceTestSuite)) } \ No newline at end of file diff --git a/internal/app/auth/commands_test.go b/internal/app/auth/commands_test.go index 9d0b8b0..08e7921 100644 --- a/internal/app/auth/commands_test.go +++ b/internal/app/auth/commands_test.go @@ -33,7 +33,7 @@ func (s *AuthCommandsSuite) TestLogin_Success() { Password: "password", Active: true, } - s.userRepo.Create(context.Background(), &user) + assert.NoError(s.T(), s.userRepo.Create(context.Background(), &user)) input := LoginInput{Email: "test@example.com", Password: "password"} resp, err := s.commands.Login(context.Background(), input) @@ -87,7 +87,7 @@ func (s *AuthCommandsSuite) TestLogin_SuccessUpdate() { Password: "password", Active: true, } - s.userRepo.Create(context.Background(), &user) + assert.NoError(s.T(), s.userRepo.Create(context.Background(), &user)) s.userRepo.updateFunc = func(ctx context.Context, user *domain.User) error { return nil } @@ -118,7 +118,7 @@ func (s *AuthCommandsSuite) TestLogin_UpdateUserError() { Password: "password", Active: true, } - s.userRepo.Create(context.Background(), &user) + assert.NoError(s.T(), s.userRepo.Create(context.Background(), &user)) s.userRepo.updateFunc = func(ctx context.Context, user *domain.User) error { return errors.New("update error") } @@ -156,7 +156,7 @@ func (s *AuthCommandsSuite) TestLogin_InactiveUser() { Password: "password", Active: false, } - s.userRepo.Create(context.Background(), &user) + assert.NoError(s.T(), s.userRepo.Create(context.Background(), &user)) input := LoginInput{Email: "inactive@example.com", Password: "password"} resp, err := s.commands.Login(context.Background(), input) @@ -170,7 +170,7 @@ func (s *AuthCommandsSuite) TestLogin_InvalidPassword() { Password: "password", Active: true, } - s.userRepo.Create(context.Background(), &user) + assert.NoError(s.T(), s.userRepo.Create(context.Background(), &user)) input := LoginInput{Email: "test@example.com", Password: "wrong-password"} resp, err := s.commands.Login(context.Background(), input) @@ -184,7 +184,7 @@ func (s *AuthCommandsSuite) TestLogin_TokenGenerationError() { Password: "password", Active: true, } - s.userRepo.Create(context.Background(), &user) + assert.NoError(s.T(), s.userRepo.Create(context.Background(), &user)) s.jwtManager.generateTokenFunc = func(user *domain.User) (string, error) { return "", errors.New("jwt error") @@ -221,7 +221,7 @@ func (s *AuthCommandsSuite) TestRegister_EmailExists() { user := domain.User{ Email: "exists@example.com", } - s.userRepo.Create(context.Background(), &user) + assert.NoError(s.T(), s.userRepo.Create(context.Background(), &user)) input := RegisterInput{ Username: "newuser", @@ -239,7 +239,7 @@ func (s *AuthCommandsSuite) TestRegister_UsernameExists() { user := domain.User{ Username: "exists", } - s.userRepo.Create(context.Background(), &user) + assert.NoError(s.T(), s.userRepo.Create(context.Background(), &user)) input := RegisterInput{ Username: "exists", diff --git a/internal/app/auth/queries_test.go b/internal/app/auth/queries_test.go index ac2c4e2..2ca3654 100644 --- a/internal/app/auth/queries_test.go +++ b/internal/app/auth/queries_test.go @@ -67,12 +67,14 @@ func (s *AuthQueriesSuite) TestGetUserFromContext_InactiveUser() { } func (s *AuthQueriesSuite) TestGetUserFromContext_NilContext() { + //nolint:staticcheck // This test intentionally passes a nil context to verify error handling. user, err := s.queries.GetUserFromContext(nil) assert.ErrorIs(s.T(), err, ErrContextRequired) assert.Nil(s.T(), user) } func (s *AuthQueriesSuite) TestValidateToken_NilContext() { + //nolint:staticcheck // This test intentionally passes a nil context to verify error handling. user, err := s.queries.ValidateToken(nil, "token") assert.ErrorIs(s.T(), err, ErrContextRequired) assert.Nil(s.T(), user) diff --git a/internal/app/bookmark/commands.go b/internal/app/bookmark/commands.go index ebb3042..c2d6e4f 100644 --- a/internal/app/bookmark/commands.go +++ b/internal/app/bookmark/commands.go @@ -4,6 +4,7 @@ import ( "context" "tercul/internal/app/analytics" "tercul/internal/domain" + "tercul/internal/platform/log" ) // BookmarkCommands contains the command handlers for the bookmark aggregate. @@ -42,7 +43,11 @@ func (c *BookmarkCommands) CreateBookmark(ctx context.Context, input CreateBookm } if c.analyticsSvc != nil { - go c.analyticsSvc.IncrementWorkBookmarks(context.Background(), input.WorkID) + go func() { + if err := c.analyticsSvc.IncrementWorkBookmarks(context.Background(), input.WorkID); err != nil { + log.Error(err, "failed to increment work bookmarks") + } + }() } return bookmark, nil diff --git a/internal/app/comment/commands.go b/internal/app/comment/commands.go index c0fa92c..fd5abc9 100644 --- a/internal/app/comment/commands.go +++ b/internal/app/comment/commands.go @@ -8,6 +8,7 @@ import ( "tercul/internal/app/authz" "tercul/internal/domain" platform_auth "tercul/internal/platform/auth" + "tercul/internal/platform/log" ) // CommentCommands contains the command handlers for the comment aggregate. @@ -51,10 +52,18 @@ func (c *CommentCommands) CreateComment(ctx context.Context, input CreateComment if c.analyticsSvc != nil { if input.WorkID != nil { - go c.analyticsSvc.IncrementWorkComments(context.Background(), *input.WorkID) + go func() { + if err := c.analyticsSvc.IncrementWorkComments(context.Background(), *input.WorkID); err != nil { + log.Error(err, "failed to increment work comments") + } + }() } if input.TranslationID != nil { - go c.analyticsSvc.IncrementTranslationComments(context.Background(), *input.TranslationID) + go func() { + if err := c.analyticsSvc.IncrementTranslationComments(context.Background(), *input.TranslationID); err != nil { + log.Error(err, "failed to increment translation comments") + } + }() } } diff --git a/internal/app/like/commands.go b/internal/app/like/commands.go index cf77726..40a9aab 100644 --- a/internal/app/like/commands.go +++ b/internal/app/like/commands.go @@ -5,6 +5,7 @@ import ( "errors" "tercul/internal/app/analytics" "tercul/internal/domain" + "tercul/internal/platform/log" ) // LikeCommands contains the command handlers for the like aggregate. @@ -45,10 +46,18 @@ func (c *LikeCommands) CreateLike(ctx context.Context, input CreateLikeInput) (* // After creating the like, increment the appropriate counter. if c.analyticsSvc != nil { if input.WorkID != nil { - go c.analyticsSvc.IncrementWorkLikes(context.Background(), *input.WorkID) + go func() { + if err := c.analyticsSvc.IncrementWorkLikes(context.Background(), *input.WorkID); err != nil { + log.Error(err, "failed to increment work likes") + } + }() } if input.TranslationID != nil { - go c.analyticsSvc.IncrementTranslationLikes(context.Background(), *input.TranslationID) + go func() { + if err := c.analyticsSvc.IncrementTranslationLikes(context.Background(), *input.TranslationID); err != nil { + log.Error(err, "failed to increment translation likes") + } + }() } // Assuming there's a counter for comment likes, which is a reasonable feature to add. // if input.CommentID != nil { @@ -81,10 +90,18 @@ func (c *LikeCommands) DeleteLike(ctx context.Context, id uint) error { // After deleting the like, decrement the appropriate counter in the background. if c.analyticsSvc != nil { if like.WorkID != nil { - go c.analyticsSvc.DecrementWorkLikes(context.Background(), *like.WorkID) + go func() { + if err := c.analyticsSvc.DecrementWorkLikes(context.Background(), *like.WorkID); err != nil { + log.Error(err, "failed to decrement work likes") + } + }() } if like.TranslationID != nil { - go c.analyticsSvc.DecrementTranslationLikes(context.Background(), *like.TranslationID) + go func() { + if err := c.analyticsSvc.DecrementTranslationLikes(context.Background(), *like.TranslationID); err != nil { + log.Error(err, "failed to decrement translation likes") + } + }() } } diff --git a/internal/app/work/commands.go b/internal/app/work/commands.go index 63683a4..8142c80 100644 --- a/internal/app/work/commands.go +++ b/internal/app/work/commands.go @@ -8,6 +8,7 @@ import ( "tercul/internal/domain" "tercul/internal/domain/search" platform_auth "tercul/internal/platform/auth" + "tercul/internal/platform/log" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/trace" @@ -50,9 +51,9 @@ func (c *WorkCommands) CreateWork(ctx context.Context, work *domain.Work) (*doma return nil, err } // Index the work in the search client - err = c.searchClient.IndexWork(ctx, work, "") - if err != nil { + if err := c.searchClient.IndexWork(ctx, work, ""); err != nil { // Log the error but don't fail the operation + log.FromContext(ctx).Warn(fmt.Sprintf("Failed to index work after creation: %v", err)) } return work, nil } @@ -141,7 +142,7 @@ func (c *WorkCommands) DeleteWork(ctx context.Context, id uint) error { // AnalyzeWork performs linguistic analysis on a work. func (c *WorkCommands) AnalyzeWork(ctx context.Context, workID uint) error { - ctx, span := c.tracer.Start(ctx, "AnalyzeWork") + _, span := c.tracer.Start(ctx, "AnalyzeWork") defer span.End() // TODO: implement this return nil @@ -251,6 +252,7 @@ func (c *WorkCommands) MergeWork(ctx context.Context, sourceID, targetID uint) e if err == nil && targetWork != nil { if searchErr := c.searchClient.IndexWork(ctx, targetWork, ""); searchErr != nil { // Log the error but don't fail the main operation + log.FromContext(ctx).Warn(fmt.Sprintf("Failed to re-index target work after merge: %v", searchErr)) } } @@ -295,4 +297,4 @@ func mergeWorkStats(tx *gorm.DB, sourceWorkID, targetWorkID uint) error { } return nil -} +} \ No newline at end of file diff --git a/internal/data/sql/base_repository_test.go b/internal/data/sql/base_repository_test.go index 395e4a0..56d0be8 100644 --- a/internal/data/sql/base_repository_test.go +++ b/internal/data/sql/base_repository_test.go @@ -36,7 +36,8 @@ func (s *BaseRepositoryTestSuite) SetupTest() { // TearDownSuite drops the test table after the suite finishes. func (s *BaseRepositoryTestSuite) TearDownSuite() { - s.DB.Migrator().DropTable(&testutil.TestEntity{}) + err := s.DB.Migrator().DropTable(&testutil.TestEntity{}) + s.Require().NoError(err) } // TestBaseRepository runs the entire test suite. @@ -79,6 +80,7 @@ func (s *BaseRepositoryTestSuite) TestCreate() { }) s.Run("should return error for nil context", func() { + //nolint:staticcheck // Testing behavior with nil context is intentional here. err := s.repo.Create(nil, &testutil.TestEntity{Name: "Test Context"}) s.ErrorIs(err, sql.ErrContextRequired) }) diff --git a/internal/jobs/linguistics/analysis_cache.go b/internal/jobs/linguistics/analysis_cache.go index 6100127..58b3b98 100644 --- a/internal/jobs/linguistics/analysis_cache.go +++ b/internal/jobs/linguistics/analysis_cache.go @@ -162,7 +162,9 @@ func (c *CompositeAnalysisCache) Get(ctx context.Context, key string) (*Analysis // Try Redis cache if result, err := c.redisCache.Get(ctx, key); err == nil { // Populate memory cache with Redis result - c.memoryCache.Set(ctx, key, result) + if err := c.memoryCache.Set(ctx, key, result); err != nil { + log.FromContext(ctx).Warn(fmt.Sprintf("Failed to populate memory cache from Redis for key %s: %v", key, err)) + } return result, nil } diff --git a/internal/jobs/linguistics/analyzer.go b/internal/jobs/linguistics/analyzer.go index cc2326c..c165e12 100644 --- a/internal/jobs/linguistics/analyzer.go +++ b/internal/jobs/linguistics/analyzer.go @@ -155,14 +155,6 @@ func (a *BasicAnalyzer) AnalyzeWork(ctx context.Context, workID uint) error { // Helper functions for text analysis -// min returns the minimum of two integers -func min(a, b int) int { - if a < b { - return a - } - return b -} - // Note: max was unused and has been removed to keep the code minimal and focused // makeTextCacheKey builds a stable cache key using a content hash to avoid collisions/leaks diff --git a/internal/platform/db/prometheus.go b/internal/platform/db/prometheus.go index cef8ad4..7ea83aa 100644 --- a/internal/platform/db/prometheus.go +++ b/internal/platform/db/prometheus.go @@ -8,9 +8,7 @@ import ( ) const ( - callBackBeforeName = "prometheus:before" - callBackAfterName = "prometheus:after" - startTime = "start_time" + startTime = "start_time" ) type PrometheusPlugin struct { @@ -23,20 +21,44 @@ func (p *PrometheusPlugin) Name() string { func (p *PrometheusPlugin) Initialize(db *gorm.DB) error { // Before callbacks - db.Callback().Create().Before("gorm:create").Register(callBackBeforeName, p.before) - db.Callback().Query().Before("gorm:query").Register(callBackBeforeName, p.before) - db.Callback().Update().Before("gorm:update").Register(callBackBeforeName, p.before) - db.Callback().Delete().Before("gorm:delete").Register(callBackBeforeName, p.before) - db.Callback().Row().Before("gorm:row").Register(callBackBeforeName, p.before) - db.Callback().Raw().Before("gorm:raw").Register(callBackBeforeName, p.before) + if err := db.Callback().Create().Before("gorm:create").Register("prometheus:before_create", p.before); err != nil { + return err + } + if err := db.Callback().Query().Before("gorm:query").Register("prometheus:before_query", p.before); err != nil { + return err + } + if err := db.Callback().Update().Before("gorm:update").Register("prometheus:before_update", p.before); err != nil { + return err + } + if err := db.Callback().Delete().Before("gorm:delete").Register("prometheus:before_delete", p.before); err != nil { + return err + } + if err := db.Callback().Row().Before("gorm:row").Register("prometheus:before_row", p.before); err != nil { + return err + } + if err := db.Callback().Raw().Before("gorm:raw").Register("prometheus:before_raw", p.before); err != nil { + return err + } // After callbacks - db.Callback().Create().After("gorm:create").Register(callBackAfterName, p.after) - db.Callback().Query().After("gorm:query").Register(callBackAfterName, p.after) - db.Callback().Update().After("gorm:update").Register(callBackAfterName, p.after) - db.Callback().Delete().After("gorm:delete").Register(callBackAfterName, p.after) - db.Callback().Row().After("gorm:row").Register(callBackAfterName, p.after) - db.Callback().Raw().After("gorm:raw").Register(callBackAfterName, p.after) + if err := db.Callback().Create().After("gorm:create").Register("prometheus:after_create", p.after); err != nil { + return err + } + if err := db.Callback().Query().After("gorm:query").Register("prometheus:after_query", p.after); err != nil { + return err + } + if err := db.Callback().Update().After("gorm:update").Register("prometheus:after_update", p.after); err != nil { + return err + } + if err := db.Callback().Delete().After("gorm:delete").Register("prometheus:after_delete", p.after); err != nil { + return err + } + if err := db.Callback().Row().After("gorm:row").Register("prometheus:after_row", p.after); err != nil { + return err + } + if err := db.Callback().Raw().After("gorm:raw").Register("prometheus:after_raw", p.after); err != nil { + return err + } return nil } diff --git a/internal/platform/http/rate_limiter.go b/internal/platform/http/rate_limiter.go index 4694e57..4440dab 100644 --- a/internal/platform/http/rate_limiter.go +++ b/internal/platform/http/rate_limiter.go @@ -1,6 +1,7 @@ package http import ( + "fmt" "net/http" "sync" "tercul/internal/platform/config" @@ -92,7 +93,11 @@ func RateLimitMiddleware(cfg *config.Config) func(http.Handler) http.Handler { Warn("Rate limit exceeded") w.WriteHeader(http.StatusTooManyRequests) - w.Write([]byte("Rate limit exceeded. Please try again later.")) + if _, err := w.Write([]byte("Rate limit exceeded. Please try again later.")); err != nil { + // We can't write the body, but the header has been sent. + // Log the error for observability. + log.FromContext(r.Context()).Error(err, fmt.Sprintf("Failed to write rate limit response body for clientID %s", clientID)) + } return } diff --git a/internal/testutil/integration_test_utils.go b/internal/testutil/integration_test_utils.go index 64cffa6..a8217b5 100644 --- a/internal/testutil/integration_test_utils.go +++ b/internal/testutil/integration_test_utils.go @@ -29,55 +29,6 @@ func (m *mockSearchClient) IndexWork(ctx context.Context, work *domain.Work, pip return nil } -// mockAnalyticsService is a mock implementation of the AnalyticsService interface. -type mockAnalyticsService struct{} - -func (m *mockAnalyticsService) IncrementWorkViews(ctx context.Context, workID uint) error { return nil } -func (m *mockAnalyticsService) IncrementWorkLikes(ctx context.Context, workID uint) error { return nil } -func (m *mockAnalyticsService) IncrementWorkComments(ctx context.Context, workID uint) error { - return nil -} -func (m *mockAnalyticsService) IncrementWorkBookmarks(ctx context.Context, workID uint) error { - return nil -} -func (m *mockAnalyticsService) IncrementWorkShares(ctx context.Context, workID uint) error { return nil } -func (m *mockAnalyticsService) IncrementWorkTranslationCount(ctx context.Context, workID uint) error { - return nil -} -func (m *mockAnalyticsService) IncrementTranslationViews(ctx context.Context, translationID uint) error { - return nil -} -func (m *mockAnalyticsService) IncrementTranslationLikes(ctx context.Context, translationID uint) error { - return nil -} -func (m *mockAnalyticsService) IncrementTranslationComments(ctx context.Context, translationID uint) error { - return nil -} -func (m *mockAnalyticsService) IncrementTranslationShares(ctx context.Context, translationID uint) error { - return nil -} -func (m *mockAnalyticsService) GetOrCreateWorkStats(ctx context.Context, workID uint) (*domain.WorkStats, error) { - return &domain.WorkStats{}, nil -} -func (m *mockAnalyticsService) GetOrCreateTranslationStats(ctx context.Context, translationID uint) (*domain.TranslationStats, error) { - return &domain.TranslationStats{}, nil -} -func (m *mockAnalyticsService) UpdateWorkReadingTime(ctx context.Context, workID uint) error { return nil } -func (m *mockAnalyticsService) UpdateWorkComplexity(ctx context.Context, workID uint) error { return nil } -func (m *mockAnalyticsService) UpdateWorkSentiment(ctx context.Context, workID uint) error { return nil } -func (m *mockAnalyticsService) UpdateTranslationReadingTime(ctx context.Context, translationID uint) error { - return nil -} -func (m *mockAnalyticsService) UpdateTranslationSentiment(ctx context.Context, translationID uint) error { - return nil -} -func (m *mockAnalyticsService) UpdateUserEngagement(ctx context.Context, userID uint, eventType string) error { - return nil -} -func (m *mockAnalyticsService) UpdateTrending(ctx context.Context) error { return nil } -func (m *mockAnalyticsService) GetTrendingWorks(ctx context.Context, timePeriod string, limit int) ([]*domain.Work, error) { - return nil, nil -} // IntegrationTestSuite provides a comprehensive test environment with either in-memory SQLite or mock repositories type IntegrationTestSuite struct { @@ -145,7 +96,7 @@ func (s *IntegrationTestSuite) SetupSuite(testConfig *TestConfig) { } s.DB = db - db.AutoMigrate( + err = db.AutoMigrate( &domain.Work{}, &domain.User{}, &domain.Author{}, &domain.Translation{}, &domain.Comment{}, &domain.Like{}, &domain.Bookmark{}, &domain.Collection{}, &domain.Tag{}, &domain.Category{}, &domain.Book{}, &domain.Publisher{}, @@ -154,6 +105,7 @@ func (s *IntegrationTestSuite) SetupSuite(testConfig *TestConfig) { &domain.LanguageAnalysis{}, &domain.TextMetadata{}, &domain.ReadabilityScore{}, &domain.TranslationStats{}, &TestEntity{}, &domain.CollectionWork{}, ) + s.Require().NoError(err, "Failed to migrate database schema") cfg, err := platform_config.LoadConfig() if err != nil { diff --git a/internal/testutil/testutil.go b/internal/testutil/testutil.go index 44980cc..11c468e 100644 --- a/internal/testutil/testutil.go +++ b/internal/testutil/testutil.go @@ -1,7 +1,6 @@ package testutil import ( - "os" "testing" "github.com/stretchr/testify/suite" @@ -34,16 +33,6 @@ func (s *BaseSuite) TearDownTest() { // No-op by default. } -// getEnv gets an environment variable or returns a default value. -// This is kept as a general utility function. -func getEnv(key, defaultValue string) string { - value, exists := os.LookupEnv(key) - if !exists { - return defaultValue - } - return value -} - // SkipIfShort skips a test if the -short flag is provided. func SkipIfShort(t *testing.T) { if testing.Short() {