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
6 changed files with 33 additions and 24 deletions
Showing only changes of commit 40e5621eb9 - Show all commits

View File

@ -10,7 +10,6 @@ import (
"time" "time"
"git.loyso.art/frx/kurious/internal/common/nullable" "git.loyso.art/frx/kurious/internal/common/nullable"
"git.loyso.art/frx/kurious/internal/common/xcontext"
"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"
@ -112,7 +111,7 @@ func (r *sqliteCourseRepository) List(
result.Count, err = r.listCount(ctx, params) result.Count, err = r.listCount(ctx, params)
if err != nil { if err != nil {
xcontext.LogWithWarnError(ctx, r.log, err, "unable to list count") return result, fmt.Errorf("listing count: %w", err)
} }
span.SetAttributes( span.SetAttributes(

View File

@ -85,6 +85,7 @@ func (h *syncSravniHandler) Handle(ctx context.Context) (err error) {
courses := make([]sravni.Course, 0, 1024) courses := make([]sravni.Course, 0, 1024)
buffer := make([]sravni.Course, 0, 512) buffer := make([]sravni.Course, 0, 512)
organizations := make([]sravni.Organization, 0, 256) organizations := make([]sravni.Organization, 0, 256)
var insertErr error
for _, learningType := range learningTypes.Fields { for _, learningType := range learningTypes.Fields {
select { select {
case <-ctx.Done(): case <-ctx.Done():
@ -174,22 +175,22 @@ func (h *syncSravniHandler) Handle(ctx context.Context) (err error) {
var insertCourseSuccess bool var insertCourseSuccess bool
if len(courses) > 0 { if len(courses) > 0 {
err = h.insertCourses(lctx, courses) if cerr := h.insertCourses(lctx, courses); cerr != nil {
if err != nil { xcontext.LogWithError(lctx, h.log, cerr, "unable to insert courses")
xcontext.LogWithError(lctx, h.log, err, "unable to insert courses") insertErr = errors.Join(insertErr, cerr)
} else {
insertCourseSuccess = true
} }
insertCourseSuccess = err == nil
} }
var insertOrgsSuccess bool var insertOrgsSuccess bool
if len(organizations) > 0 { if len(organizations) > 0 {
err = h.insertOrganizations(lctx, organizations) if oerr := h.insertOrganizations(lctx, organizations); oerr != nil {
if err != nil { xcontext.LogWithError(lctx, h.log, oerr, "unable to insert organizations")
xcontext.LogWithError(lctx, h.log, err, "unable to insert courses") insertErr = errors.Join(insertErr, oerr)
} else {
insertOrgsSuccess = true
} }
insertOrgsSuccess = err == nil
} }
elapsed = time.Since(start) - elapsed elapsed = time.Since(start) - elapsed
@ -205,7 +206,7 @@ func (h *syncSravniHandler) Handle(ctx context.Context) (err error) {
) )
} }
return nil return insertErr
} }
func (h *syncSravniHandler) loadEducationalProducts(ctx context.Context, learningType, courseThematic string, buf []sravni.Course) ([]sravni.Course, map[string]sravni.Organization, error) { func (h *syncSravniHandler) loadEducationalProducts(ctx context.Context, learningType, courseThematic string, buf []sravni.Course) ([]sravni.Course, map[string]sravni.Organization, error) {

View File

@ -9,6 +9,7 @@ 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"
@ -49,7 +50,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): case stderrors.Is(err, errors.ErrNotFound), stderrors.Is(err, domain.ErrNotFound):
Outdated
Review

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
errorString = err.Error() errorString = err.Error()
code = http.StatusNotFound code = http.StatusNotFound
default: default: