Worked review 1: HTTP JSON API handler

นี่คือ HTTP API รายการ task เล็กๆ: POST /tasks สร้าง task, GET /tasks/{id} ดึงมาหนึ่งตัว เขียนด้วยสไตล์ที่มั่นใจและดูสมเหตุสมผลแบบที่ AI assistant ผลิตออกมาตั้งแต่ครั้งแรก — มัน compile ผ่าน, go vet ไม่บ่นอะไรเลย, และมันรันได้ อ่านมันก่อนรัน มันมี bug จริง 5 ตัว

The program

Run ▸ Open in Playground

package main

import (
  "encoding/json"
  "fmt"
  "io"
  "net/http"
  "net/http/httptest"
  "strconv"
  "strings"
)

type Task struct {
  ID   int    `json:"id"`
  Text string `json:"text"`
}

type Store struct {
  tasks  map[int]Task
  nextID int
}

func NewStore() *Store {
  return &Store{tasks: map[int]Task{}, nextID: 1}
}

func (s Store) Add(text string) Task {
  t := Task{ID: s.nextID, Text: text}
  s.tasks[t.ID] = t
  s.nextID++
  return t
}

func (s *Store) Get(id int) (Task, bool) {
  t, ok := s.tasks[id]
  return t, ok
}

func newMux(store *Store) *http.ServeMux {
  mux := http.NewServeMux()
  mux.HandleFunc("POST /tasks", func(w http.ResponseWriter, r *http.Request) {
    var body struct {
      Text string `json:"text"`
    }
    json.NewDecoder(r.Body).Decode(&body)
    t := store.Add(body.Text)
    json.NewEncoder(w).Encode(t)
    w.WriteHeader(http.StatusCreated)
  })
  mux.HandleFunc("GET /tasks/{id}", func(w http.ResponseWriter, r *http.Request) {
    id, _ := strconv.Atoi(r.PathValue("id"))
    t, ok := store.Get(id)
    if !ok {
      w.WriteHeader(http.StatusNotFound)
    }
    json.NewEncoder(w).Encode(t)
  })
  return mux
}

func main() {
  store := NewStore()
  ts := httptest.NewServer(newMux(store))
  defer ts.Close()

  for _, text := range []string{"buy milk", "write report"} {
    payload := strings.NewReader(fmt.Sprintf(`{"text":%q}`, text))
    resp, _ := http.Post(ts.URL+"/tasks", "application/json", payload)
    body, _ := io.ReadAll(resp.Body)
    fmt.Printf("POST /tasks status=%d body=%s\n", resp.StatusCode, body)
  }

  resp, _ := http.Post(ts.URL+"/tasks", "application/json", strings.NewReader("not json"))
  body, _ := io.ReadAll(resp.Body)
  fmt.Printf("POST /tasks (bad json) status=%d body=%s\n", resp.StatusCode, body)

  resp, _ = http.Get(ts.URL + "/tasks/abc")
  body, _ = io.ReadAll(resp.Body)
  fmt.Printf("GET /tasks/abc status=%d body=%s\n", resp.StatusCode, body)

  resp, _ = http.Get(ts.URL + "/tasks/999")
  body, _ = io.ReadAll(resp.Body)
  fmt.Printf("GET /tasks/999 status=%d body=%s\n", resp.StatusCode, body)
}

หา bug

ลองรันดู ทุก response กลับมาเป็น status=200, id ของ task ตัวหนึ่งไม่เคยเปลี่ยนเลย และ 404 มี body แนบมาด้วย ไม่มีอันไหนตั้งใจเลยสักอย่าง ก่อนเปิดเฉลยด้านล่าง ลองระบุ bug อย่างน้อย 3 ใน 5 ตัวและบรรทัดที่ทำให้เกิดแต่ละอันดู

เฉลย 5 bug

1. func (s Store) Add(...) เป็น value receiver Add แก้ s.nextID และเขียนลง s.tasks — แต่ s คือ copy ทั้งชุดของ Store ทุกครั้งที่เรียก การเขียน map รอดอยู่ (map value เป็น reference ที่แชร์กัน) แต่ s.nextID++ เพิ่มแค่ copy เท่านั้น Store.nextID ตัวจริงไม่เคยขยับเกิน 1 เลย ทุก task ที่สร้างได้ "id":1 ทับกันเงียบๆ ใน map output จริง: ทั้ง buy milk และ write report กลับมาเป็น {"id":1,...} — call ที่สองทับตัวแรกใน store.tasks
2. error ของ json.NewDecoder(r.Body).Decode(&body) ถูกทิ้ง POST "not json" ไปไม่ fail เลย — body.Text แค่ยังเป็น "" แล้ว handler ก็สร้าง task ที่มี text ว่างเปล่าอย่างสบายใจ แล้ว return 200
3. w.WriteHeader(http.StatusCreated) ถูกเรียกหลังจาก json.NewEncoder(w).Encode(t) Encode เรียก Write ข้างในตัวมันเอง และ Write ครั้งแรกบน ResponseWriter จะส่ง status 200 โดยปริยายถ้า WriteHeader ยังไม่ถูกเรียก พอ WriteHeader(201) รันถึง response ถูกส่งไปแล้ว — go run log http: superfluous response.WriteHeader call และ client เห็น 200 ไม่เคยเห็น 201
4. branch 404 ไม่มี return if !ok { w.WriteHeader(http.StatusNotFound) } ส่ง header 404 แล้ว execution ก็ตกไปที่ json.NewEncoder(w).Encode(t) ต่อไปอยู่ดี — encode Task{} ที่เป็น zero-value GET /tasks/999 กลับมาเป็น 404 พร้อม body {"id":0,"text":""} ซึ่งดูเหมือน task ที่ผิดรูปแบบแต่มีอยู่จริง แทนที่จะเป็น “ไม่พบ”
5. error ของ strconv.Atoi(r.PathValue("id")) ถูกทิ้ง GET /tasks/abc ไม่ return 400 Bad Request — id กลายเป็น 0 เงียบๆ แล้ว handler ก็เดินหน้าไปหา task 0 เหมือนเป็น request ปกติที่ valid

The corrected program

Run ▸ Open in Playground

package main

import (
  "encoding/json"
  "fmt"
  "io"
  "net/http"
  "net/http/httptest"
  "strconv"
  "strings"
  "sync"
)

type Task struct {
  ID   int    `json:"id"`
  Text string `json:"text"`
}

type Store struct {
  mu     sync.Mutex // fixed: guards tasks/nextID against concurrent handlers
  tasks  map[int]Task
  nextID int
}

func NewStore() *Store {
  return &Store{tasks: map[int]Task{}, nextID: 1}
}

func (s *Store) Add(text string) Task { // fixed: pointer receiver — nextID actually persists
  s.mu.Lock()
  defer s.mu.Unlock()
  t := Task{ID: s.nextID, Text: text}
  s.tasks[t.ID] = t
  s.nextID++
  return t
}

func (s *Store) Get(id int) (Task, bool) {
  s.mu.Lock()
  defer s.mu.Unlock()
  t, ok := s.tasks[id]
  return t, ok
}

func newMux(store *Store) *http.ServeMux {
  mux := http.NewServeMux()
  mux.HandleFunc("POST /tasks", func(w http.ResponseWriter, r *http.Request) {
    var body struct {
      Text string `json:"text"`
    }
    if err := json.NewDecoder(r.Body).Decode(&body); err != nil {
      http.Error(w, "invalid JSON body", http.StatusBadRequest)
      return
    }
    t := store.Add(body.Text)
    w.WriteHeader(http.StatusCreated) // fixed: header before the first Write
    _ = json.NewEncoder(w).Encode(t)
  })
  mux.HandleFunc("GET /tasks/{id}", func(w http.ResponseWriter, r *http.Request) {
    id, err := strconv.Atoi(r.PathValue("id"))
    if err != nil {
      http.Error(w, "invalid id", http.StatusBadRequest)
      return
    }
    t, ok := store.Get(id)
    if !ok {
      w.WriteHeader(http.StatusNotFound)
      return // fixed: stop here, do not also write a zero-value body
    }
    _ = json.NewEncoder(w).Encode(t)
  })
  return mux
}

func main() {
  store := NewStore()
  ts := httptest.NewServer(newMux(store))
  defer ts.Close()

  for _, text := range []string{"buy milk", "write report"} {
    payload := strings.NewReader(fmt.Sprintf(`{"text":%q}`, text))
    resp, _ := http.Post(ts.URL+"/tasks", "application/json", payload)
    body, _ := io.ReadAll(resp.Body)
    fmt.Printf("POST /tasks status=%d body=%s\n", resp.StatusCode, body)
  }

  resp, _ := http.Post(ts.URL+"/tasks", "application/json", strings.NewReader("not json"))
  body, _ := io.ReadAll(resp.Body)
  fmt.Printf("POST /tasks (bad json) status=%d body=%s\n", resp.StatusCode, body)

  resp, _ = http.Get(ts.URL + "/tasks/abc")
  body, _ = io.ReadAll(resp.Body)
  fmt.Printf("GET /tasks/abc status=%d body=%s\n", resp.StatusCode, body)

  resp, _ = http.Get(ts.URL + "/tasks/999")
  body, _ = io.ReadAll(resp.Body)
  fmt.Printf("GET /tasks/999 status=%d body=%s\n", resp.StatusCode, body)
}

รันเวอร์ชันที่แก้แล้ว: ID เพิ่มขึ้นเรื่อยๆ (1 แล้ว 2), body ที่ผิดรูปแบบตอนนี้ได้ 400 invalid JSON body, /tasks/abc ได้ 400 invalid id แทนที่จะเป็น 404 ปลอมๆ, และ /tasks/999 ได้ 404 จริงพร้อม body ว่างเปล่า go vet และ golangci-lint run ทั้งคู่รายงาน 0 issue บนเวอร์ชันนี้ — sync.Mutex ที่เพิ่มเข้ามาก็ปิดช่องโหว่เรื่อง concurrency-safety ที่ Store แบบ pointer-receiver เฉยๆ ยังมีอยู่ภายใต้ traffic แบบ concurrent จริง (demo นี้ request รันทีละตัวตามลำดับ เลยไม่เห็น race ตรงนี้ แต่ traffic จริงจะไม่เป็นแบบนั้น)

🟦 value receiver บน type ที่มี mutable state คือธีมที่วนกลับมาซ้ำๆ

bug ที่ 1 อย่างเดียวก็อธิบายได้แล้วว่าทำไม Store.Add ต้องใช้ pointer receiver: value receiver ทำให้ mutation หายไปเงียบๆ ทุกตัว ยกเว้นตัวที่ผ่าน reference type อย่าง map ดู Methods และ pattern 13 ใน AI Bug Catalog

ในเวอร์ชันที่มี bug ทั้งสอง POST request คืนค่า {"id":1,...} เหมือนกัน สาเหตุโดยตรงคืออะไร?

map ของ Store มีขนาดสูงสุดแค่ 1 Add เป็น value receiver ดังนั้น s.nextID++ เพิ่มค่าใน copy ของ nextID ที่ถูกทิ้งไปตอน Add return httptest server cache response แรกไว้ JSON encoding รีเซ็ต numeric field เป็น 1 เสมอ

ทำไม client ของ handler ที่มี bug ถึงเห็น status=200 เสมอแทนที่จะเป็น 201 ทั้งที่โค้ดเรียก w.WriteHeader(http.StatusCreated) อยู่?

http.StatusCreated จริงๆ แล้วมีค่าเดียวกับ 200 WriteHeader ถูกเรียกหลังจาก Encode ทำให้เกิด 200 โดยปริยายไปแล้วตอน Write ครั้งแรก httptest.NewServer เขียนทับ status code ทุกตัวให้เป็น 200 client library มองข้าม status code ที่มากกว่า 200

GET /tasks/999 (task ที่ไม่มีอยู่จริง) คืน 404 พร้อม body {"id":0,"text":""} ในเวอร์ชันที่มี bug keyword ตัวเดียวที่หายไปซึ่งทำให้ body ถูกส่งออกมาด้วยคืออะไร?

return — ถ้าไม่มี execution จะไหลต่อไปถึง Encode call หลัง WriteHeader(404) break — handler ต้องการ labeled break continue — loop ต้องข้าม iteration defer — encode call ควรถูก defer ไว้

เวอร์ชันที่แก้แล้วเพิ่ม sync.Mutex เข้าไปใน Store ทั้งที่ request แบบ sequential ของ demo นี้ไม่เคยเกิด race จริงๆ เลย ทำไมถึงยังเพิ่มเข้าไป?

Go compiler บังคับให้ struct ที่มี map field ต้องมีมันด้วย HTTP server จริงจัดการ request แบบ concurrent; ถ้าไม่มี mutex การเรียก POST/GET พร้อมกันจะ race กันบน map ที่ใช้ร่วมกันและ field nextID แม้จะแก้ bug เรื่อง receiver ไปแล้วก็ตาม ไม่มีประโยชน์จริงเลย — ใส่เข้ามาแค่เพื่อความสมมาตรกับ AI Bug Catalog sync.Mutex ทำให้ JSON encoding เร็วขึ้น

Submit

Mark as complete