Worked review 3: inventory service

inventory service เล็ก ๆ ที่สร้างจาก dataclass และ cache decorator — ทั้งสองอย่างดูเหมือนใช้ถูกต้องแบบตำราเมื่อดูเดี่ยว ๆ แต่ผิดในแบบที่ AI assistant มักเอามารวมกัน

Run ▸ Open in an online REPL

from dataclasses import dataclass
from functools import lru_cache
from typing import Any


@dataclass
class Product:
    sku: str
    price: float

    def __eq__(self, other: object) -> bool:
        if not isinstance(other, Product):
            return NotImplemented
        return self.sku == other.sku


class InventoryService:
    _catalog: dict[str, Product] = {}  # class attribute -- shared by every instance

    def __init__(self, name: str) -> None:
        self.name = name

    def stock(self, product: Product) -> None:
        self._catalog[product.sku] = product

    @lru_cache
    def get_price(self, sku: str) -> float:
        return self._catalog[sku].price

    def update_price(self, sku: str, new_price: float) -> None:
        self._catalog[sku].price = new_price

    def apply_bulk_update(self, updates: Any) -> None:
        for sku, new_price in updates.items():
            self.update_price(sku, new_price)


warehouse_a = InventoryService("warehouse-a")
warehouse_b = InventoryService("warehouse-b")

warehouse_a.stock(Product(sku="WIDGET", price=9.99))
print("warehouse_b sees WIDGET too:", "WIDGET" in warehouse_b._catalog)

print("price before update:", warehouse_a.get_price("WIDGET"))
warehouse_a.update_price("WIDGET", 12.99)
print("price after update (still cached):", warehouse_a.get_price("WIDGET"))

updates_from_csv = [("WIDGET", 15.99)]  # a list of tuples, not the dict the code assumes
try:
    warehouse_a.apply_bulk_update(updates_from_csv)
except AttributeError as e:
    print(f"apply_bulk_update crashed: {e}")

seen: set[Product] = set()
try:
    seen.add(Product(sku="WIDGET", price=12.99))
except TypeError as e:
    print(f"seen.add crashed: {e}")

output จริง (การ crash ทั้งสองอันด้านล่างถูกจับและพิมพ์ออกมาโดยตั้งใจ เพื่อให้ demo ทั้งหมดรันจนจบ — แต่ละอันคือหนึ่งในสี่ finding):

warehouse_b sees WIDGET too: True
price before update: 9.99
price after update (still cached): 9.99
apply_bulk_update crashed: 'list' object has no attribute 'items'
seen.add crashed: cannot use 'Product' as a set element (unhashable type: 'Product')

สิ่งที่พบ

Bug 1: _catalog เป็น class attribute ที่แชร์ทุก instance ของ InventoryService (catalog #15)

อาการ: warehouse_b sees WIDGET too: True — warehouse_a.stock(...) ถูกเรียก ไม่เคยเรียก warehouse_b.stock(...) เลย แต่ "WIDGET" in warehouse_b._catalog เป็น True อยู่ดี warehouse สองตัวที่สร้างแยกกันโดยอิสระแชร์ inventory เดียวกัน

หลักฐาน: _catalog: dict[str, Product] = {} ประกาศระดับ class นอก __init__ — dict เพียงตัวเดียวเป๊ะ ที่ class เป็นเจ้าของ ไม่ใช่ instance ไหนเลย

ทางแก้: initialize mutable state ต่อ instance ใน __init__:

def __init__(self, name: str) -> None:
    self.name = name
    self._catalog: dict[str, Product] = {}

Bug 2: @lru_cache บน get_price คืนค่าเก่าหลัง update_price (spec: lru_cache บน method ที่มี mutable state ข้างใต้)

อาการ: get_price("WIDGET") คืน 9.99 ทั้งก่อน และ หลัง update_price("WIDGET", 12.99) รัน — การอัปเดตราคาไม่เคยมองเห็นได้ผ่าน getter ที่ cache ไว้เลย

หลักฐาน: lru_cache ไม่รู้เลยว่า self._catalog[sku].price สามารถเปลี่ยนระหว่าง call ได้ — มันจำ return value แรก สำหรับ key (self, sku) ตัวหนึ่งไว้ตลอดไป และไม่มีอะไรใน update_price บอกให้มันลืม

ทางแก้: ทางแก้ที่ง่ายที่สุดคือไม่ cache ค่าที่เปลี่ยนได้จากใต้มือ — get_price ที่นี่เป็น dict lookup แบบ O(1) อยู่แล้ว ดังนั้นการ cache ไม่ได้อะไรเลยและมีแต่ความเสี่ยงเรื่องค่าเก่าค้าง:

def get_price(self, sku: str) -> float:
    return self._catalog[sku].price

Bug 3: updates ที่ type เป็น Any ซ่อนความไม่ตรงกันของ shape จริง (catalog #16)

อาการ: apply_bulk_update(updates_from_csv) crash ด้วย AttributeError: 'list' object has no attribute 'items' — ผู้เรียกส่ง list ของ tuple มา ไม่ใช่ dict ที่ function body สมมติไว้ และไม่มีอะไรจับสิ่งนี้ก่อนที่จะถึง runtime เลย

หลักฐาน: ทั้ง mypy --strict และ pyright ที่รันกับไฟล์นี้เป๊ะ รายงาน zero issue — Any ปิดการเช็คที่ควรจะจับความไม่ตรงกันนี้พอดี (ดู สิ่งที่เครื่องมือจับได้ ด้านล่าง)

ทางแก้: ให้ updates มี type จริง:

def apply_bulk_update(self, updates: dict[str, float]) -> None:
    for sku, new_price in updates.items():
        self.update_price(sku, new_price)

หลังจากเปลี่ยนแบบนี้ การส่ง updates_from_csv (list ไม่ใช่ dict) จะกลายเป็น error ของ mypy/pyright ที่จับได้ก่อนที่โค้ดจะรันเลยด้วยซ้ำ

Bug 4: eq ไม่มี hash ทำให้ Product unhashable (catalog #14)

อาการ: seen.add(Product(sku="WIDGET", price=12.99)) raise TypeError: cannot use 'Product' as a set element (unhashable type: 'Product') — ถูกจับและพิมพ์ไว้ที่นี่แค่เพื่อให้ demo จบได้ ในโค้ดจริงนี่จะเป็น crash ที่ไม่มีใครจับ

หลักฐาน: การนิยาม __eq__ บน class ทำให้ Python ตั้ง __hash__ เป็น None อัตโนมัติ เว้นแต่ __hash__ จะถูกนิยามด้วย — นี่เป็นพฤติกรรม Python มาตรฐานที่มีเอกสารรองรับ ไม่ได้เฉพาะเจาะจงกับ dataclass

ทางแก้: นิยาม __hash__ คู่กับ __eq__ ให้สอดคล้องกับสิ่งที่ __eq__ เทียบจริง:

def __hash__(self) -> int:
    return hash(self.sku)

เวอร์ชันที่แก้แล้ว

แก้ทั้งสี่ bug แล้ว: ย้าย _catalog เข้า __init__, เอา lru_cache ออกจาก get_price, ให้ updates มี type จริงเป็น dict[str, float] และให้ Product มี __hash__ ที่สอดคล้องกัน ทางแก้ของ bug 2 ยังใช้ dataclasses.replace เพื่อสร้าง Product ใหม่ทุกครั้งที่อัปเดตราคาแทนที่จะ mutate ตัวเดิม ทำให้ Product ปลอดภัยพอที่จะทำเป็น frozen=True และ immutable/hashable ได้จริง:

Run ▸ Open in an online REPL

from dataclasses import dataclass


@dataclass(frozen=True)
class Product:
    sku: str
    price: float


class InventoryService:
    def __init__(self, name: str) -> None:
        self.name = name
        self._catalog: dict[str, Product] = {}  # per-instance, not shared

    def stock(self, product: Product) -> None:
        self._catalog[product.sku] = product

    def get_price(self, sku: str) -> float:
        return self._catalog[sku].price  # no cache -- always reflects the current price

    def update_price(self, sku: str, new_price: float) -> None:
        existing = self._catalog[sku]
        self._catalog[sku] = Product(sku=existing.sku, price=new_price)

    def apply_bulk_update(self, updates: dict[str, float]) -> None:
        for sku, new_price in updates.items():
            self.update_price(sku, new_price)


warehouse_a = InventoryService("warehouse-a")
warehouse_b = InventoryService("warehouse-b")

warehouse_a.stock(Product(sku="WIDGET", price=9.99))
print("warehouse_b sees WIDGET too:", "WIDGET" in warehouse_b._catalog)

print("price before update:", warehouse_a.get_price("WIDGET"))
warehouse_a.update_price("WIDGET", 12.99)
print("price after update:", warehouse_a.get_price("WIDGET"))

updates_from_csv = {"WIDGET": 15.99}
warehouse_a.apply_bulk_update(updates_from_csv)
print("price after bulk update:", warehouse_a.get_price("WIDGET"))

seen: set[Product] = set()
seen.add(Product(sku="WIDGET", price=15.99))
print("Product is hashable now, set has", len(seen), "item")

output จริง:

warehouse_b sees WIDGET too: False
price before update: 9.99
price after update: 12.99
price after bulk update: 15.99
Product is hashable now, set has 1 item

สิ่งที่เครื่องมือจับได้

ruff บนไฟล์ที่มี bug — สามในสี่ bug จับได้ตรง ๆ:

PLW1641 Object does not implement `__hash__` method
RUF012 Mutable default value for class attribute
ANN401 Dynamically typed expressions (typing.Any) are disallowed in `updates`

mypy --strict และ pyright บนไฟล์ที่มี bug — สะอาดหมดจดทั้งคู่:

mypy --strict:  Success: no issues found in 1 source file
pyright:        0 errors, 0 warnings, 0 informations

Bug 2 — ค่า lru_cache ที่ค้างเก่า — ไม่มีเครื่องมือไหนใน catalog นี้จับได้เลย มันไม่ใช่ type error ไม่ใช่ security issue และไม่ใช่ style nit มันเป็นผลลัพธ์เชิงตรรกะของการ cache ค่าที่มาจาก mutable state และไม่มีเครื่องมือไหนที่นี่คิดเรื่อง cache invalidation เลย

ruff, mypy --strict และ pyright บนไฟล์ที่แก้แล้ว — สะอาดทั้งหมด:

ruff:           All checks passed!
mypy --strict:  Success: no issues found in 1 source file
pyright:        0 errors, 0 warnings, 0 informations

🐍 Any และ cache ที่ค้างเก่า มองไม่เห็นทั้งคู่จาก strict type checking

ไฟล์นี้เป็นเครื่องเตือนว่า mypy --strict และ pyright ที่รายงาน zero issue ไม่เหมือนกับโค้ดที่ถูกต้อง — bug 3 มองไม่เห็นทั้งคู่เพราะ Any เลือกที่จะไม่เช็ค และ bug 2 มองไม่เห็นทั้งคู่เพราะ type checker ไม่มี model ว่า เมื่อไหร่ ค่าที่ cache ไว้จะเก่า การอ่าน type ของ argument จริงที่ apply_bulk_update ถูกเรียก และการอ่านว่าข้อมูลข้างใต้ get_price เปลี่ยนได้หรือเปล่า คือ judgment call ที่ Review Checklist มีไว้กระตุ้นให้ทำพอดี

warehouse_a.stock(...) ถูกเรียก แต่ warehouse_b._catalog แสดง product เดียวกัน อะไรเป็นสาเหตุ?

InventoryService has a bug in its __init__ method _catalog is declared at class level (outside __init__), making it one dict shared by every instance of InventoryService, not a per-instance dict Python automatically synchronizes dict attributes between instances of the same class This is expected -- all InventoryService instances are supposed to share inventory

get_price("WIDGET") คืน 9.99 แม้หลังจาก update_price("WIDGET", 12.99) ถูกเรียกแล้ว ทำไม @lru_cache ถึงทำให้เกิดสิ่งนี้?

lru_cache has a bug that fails to update cached values lru_cache remembers the return value for a given set of arguments and has no way to know the underlying data (self._catalog[sku].price) changed -- it will keep returning the first result until the cache is explicitly cleared lru_cache only works on functions, not methods, so this code doesn't actually cache anything update_price silently fails to actually update the price

apply_bulk_update(updates: Any) crash ตอน runtime เมื่อส่ง list แทน dict แต่ mypy --strict และ pyright ทั้งคู่รายงาน zero error บนไฟล์ ทำไมทั้งคู่ถึงจับไม่ได้?

Both tools have an unrelated bug Any tells both type checkers to skip validating anything about that parameter's usage -- it is a deliberate, total opt-out from type checking for that value, not a gap in either tool's implementation The tools were run with the wrong configuration Type checkers cannot analyze functions that use .items()

ทำไมการนิยาม __eq__ บน Product โดยไม่นิยาม __hash__ ด้วย ทำให้ instance ใช้ใน set ไม่ได้ ทั้งที่ __eq__'s เองไม่ได้พูดถึง set membership เลย?

Sets require every element to define a custom __str__ method Defining __eq__ makes Python automatically set __hash__ to None on that class (unless __hash__ is also explicitly defined), because two objects that compare equal must hash equal -- a guarantee Python cannot verify without your help This only happens for dataclasses, not regular classes __eq__ and __hash__ are unrelated; this must be a different bug

Submit

Mark as complete