frx/kurious
1
0
Fork 0
Code Issues 2 Pull Requests Actions Packages Projects 1 Releases Wiki Activity

fix: critical bugs from code review (data corruption, error contract, HTTP hardening) #5

Merged
frx merged 7 commits from refactor/code-review-fixes into master 2026-06-28 14:04:25 +00:00
Conversation 12 Commits 7 Files Changed 20 +92 -40
11 changed files with 92 additions and 40 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 9088caf600 - Show all commits

3
internal/kurious/adapters/sqlite_learning_category_repository.go
View File

@ -8,6 +8,7 @@ import (
"log/slog" "log/slog"
"strings" "strings"
cerrors "git.loyso.art/frx/kurious/internal/common/errors"
"git.loyso.art/frx/kurious/internal/common/xslices" "git.loyso.art/frx/kurious/internal/common/xslices"
"git.loyso.art/frx/kurious/internal/kurious/domain" "git.loyso.art/frx/kurious/internal/kurious/domain"
@ -156,7 +157,7 @@ func (r *sqliteLearingCategoryRepository) Get(ctx context.Context, id string) (c
err = r.db.GetContext(ctx, &cdb, query, id) err = r.db.GetContext(ctx, &cdb, query, id)
if err != nil { if err != nil {
if errors.Is(err, sql.ErrNoRows) { if errors.Is(err, sql.ErrNoRows) {
return domain.LearningCategory{}, domain.ErrNotFound return domain.LearningCategory{}, cerrors.ErrNotFound
} }
return domain.LearningCategory{}, fmt.Errorf("executing query: %w", err) return domain.LearningCategory{}, fmt.Errorf("executing query: %w", err)
} }

3
internal/kurious/adapters/sqlite_learning_category_repository_test.go
View File

@ -3,6 +3,7 @@ package adapters
import ( import (
"testing" "testing"
"git.loyso.art/frx/kurious/internal/common/errors"
"git.loyso.art/frx/kurious/internal/common/nullable" "git.loyso.art/frx/kurious/internal/common/nullable"
"git.loyso.art/frx/kurious/internal/kurious/domain" "git.loyso.art/frx/kurious/internal/kurious/domain"
@ -75,7 +76,7 @@ func (s *sqliteLearningCategoriesRepositorySuite) TestUpsert() {
const categoryID = "test-id-1" const categoryID = "test-id-1"
repo := s.connection.LearningCategory() repo := s.connection.LearningCategory()
gotCategory, err := repo.Get(s.ctx, categoryID) gotCategory, err := repo.Get(s.ctx, categoryID)
s.ErrorIs(err, domain.ErrNotFound) s.ErrorIs(err, errors.ErrNotFound)
s.Empty(gotCategory) s.Empty(gotCategory)
createdCategory := domain.LearningCategory{ createdCategory := domain.LearningCategory{

7
internal/kurious/adapters/sqlite_organization_repository.go
View File

@ -9,6 +9,7 @@ import (
"strings" "strings"
"time" "time"
cerrors "git.loyso.art/frx/kurious/internal/common/errors"
"git.loyso.art/frx/kurious/internal/common/xslices" "git.loyso.art/frx/kurious/internal/common/xslices"
"git.loyso.art/frx/kurious/internal/kurious/domain" "git.loyso.art/frx/kurious/internal/kurious/domain"
"go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/attribute"
@ -228,7 +229,7 @@ func (r *sqliteOrganizationRepository) Get(ctx context.Context, params domain.Ge
err = r.db.GetContext(ctx, &orgdb, query, args...) err = r.db.GetContext(ctx, &orgdb, query, args...)
if err != nil { if err != nil {
if errors.Is(err, sql.ErrNoRows) { if errors.Is(err, sql.ErrNoRows) {
return out, domain.ErrNotFound return out, cerrors.ErrNotFound
} }
return out, fmt.Errorf("executing query: %w", err) return out, fmt.Errorf("executing query: %w", err)
} }
@ -306,14 +307,14 @@ func (r *sqliteOrganizationRepository) Delete(ctx context.Context, id string) (e
result, err := r.db.ExecContext(ctx, query, id) result, err := r.db.ExecContext(ctx, query, id)
if err != nil { if err != nil {
if errors.Is(err, sql.ErrNoRows) { if errors.Is(err, sql.ErrNoRows) {
return domain.ErrNotFound return cerrors.ErrNotFound
} }
return fmt.Errorf("executing query: %w", err) return fmt.Errorf("executing query: %w", err)
} }
affected, _ := result.RowsAffected() affected, _ := result.RowsAffected()
if affected == 0 { if affected == 0 {
return domain.ErrNotFound return cerrors.ErrNotFound
} }
return nil return nil

12
internal/kurious/domain/error.go
View File

@ -1,12 +1,8 @@
package domain package domain
const ( import (
ErrNotFound PlainError = "not found" cerrors "git.loyso.art/frx/kurious/internal/common/errors"
ErrNotImplemented PlainError = "not implemented"
) )
type PlainError string // ErrNotImplemented is delegated to common/errors.ErrNotImplemented for consistency.
var ErrNotImplemented = cerrors.ErrNotImplemented
frx commented 2026-06-28 09:59:17 +00:00
Outdated
Review
Copy Link

Domain package shouldn't depend on any other package

Domain package shouldn't depend on any other package
func (err PlainError) Error() string {
return string(err)
}

3
internal/kurious/ports/http/server.go
View File

@ -9,7 +9,6 @@ import (
"git.loyso.art/frx/kurious/internal/common/errors" "git.loyso.art/frx/kurious/internal/common/errors"
"git.loyso.art/frx/kurious/internal/common/xcontext" "git.loyso.art/frx/kurious/internal/common/xcontext"
"git.loyso.art/frx/kurious/internal/kurious/domain"
"git.loyso.art/frx/kurious/internal/kurious/service" "git.loyso.art/frx/kurious/internal/kurious/service"
"git.loyso.art/frx/kurious/pkg/xdefault" "git.loyso.art/frx/kurious/pkg/xdefault"
@ -50,7 +49,7 @@ func handleError(ctx context.Context, err error, w http.ResponseWriter, log *slo
case stderrors.As(err, &valErr): case stderrors.As(err, &valErr):
errorString = valErr.Error() errorString = valErr.Error()
code = http.StatusBadRequest code = http.StatusBadRequest
case stderrors.Is(err, errors.ErrNotFound), stderrors.Is(err, domain.ErrNotFound): case stderrors.Is(err, errors.ErrNotFound):
errorString = err.Error() errorString = err.Error()
frx commented 2026-06-28 07:24:43 +00:00
Outdated
Review
Copy Link

Let's unify ErrNotFound, why is there both, let's remove domain.ErrNotFound and move completely to errors.ErrNotFound

Let's unify ErrNotFound, why is there both, let's remove domain.ErrNotFound and move completely to errors.ErrNotFound
code = http.StatusNotFound code = http.StatusNotFound
default: default: