Техпрогон
ВойтиНачать бесплатно
Код-ревью Senior Eloquent, N+1 и миграции обновлено

Код-ревью: контроллер заказов на Laravel

Вопрос

Контроллер выводит заказы пользователя и создаёт новые. Найдите проблемы с производительностью, корректностью и безопасностью.

class OrderController extends Controller
{
    public function index(Request $request)
    {
        $orders = Order::all()->where('user_id', $request->user_id);
        foreach ($orders as $order) {
            $order->items_count = $order->items->count();
            $order->customer_name = $order->user->name;
        }
        return view('orders.index', ['orders' => $orders]);
    }

    public function store(Request $request)
    {
        $order = Order::create($request->all());
        foreach ($request->items as $item) {
            OrderItem::create(['order_id' => $order->id] + $item);
        }
        Mail::to($order->user)->send(new OrderCreated($order));
        return $order;
    }
}

Коротко

Две главные проблемы: работа с данными в памяти вместо базы (all() и N+1) и отсутствие проверок (чужие заказы, массовое присваивание без валидации). Улучшенная версия:

Что должен найти ревьюер

  1. Производительностьстрока 5
    Order::all() грузит все заказы всех пользователей, а where фильтрует уже коллекцию в памяти. Фильтр нужен в запросе: Order::where(...).
  2. Безопасностьстрока 5
    user_id берётся из запроса: любой увидит чужие заказы (IDOR). Нужно $request->user()->orders().
  3. Производительностьстрока 7
    N+1: items грузятся отдельным запросом на каждый заказ ради подсчёта. Нужен withCount("items").
  4. Производительностьстрока 8
    N+1 по пользователю: with("user") или вообще не грузить, ведь это один и тот же текущий пользователь.
  5. Производительностьстрока 5
    Нет пагинации: у активного клиента тысячи заказов. Нужен paginate().
  6. Безопасностьстрока 15
    create($request->all()) без валидации: mass assignment (например, status или total от клиента). Нужны FormRequest и validated().
  7. Ошибкастрока 16
    Заказ и позиции создаются без транзакции: при ошибке на третьей позиции в базе останется неполный заказ. Нужен DB::transaction.
  8. Производительностьстрока 17
    Позиции вставляются по одной: N запросов. Лучше $order->items()->createMany($items) или пакетная вставка.
  9. Производительностьстрока 19
    Письмо отправляется синхронно в запросе: медленный SMTP задерживает ответ, а сбой почты роняет создание заказа. Нужен queue() или ShouldQueue, причём после коммита (afterCommit).
  10. Стильстрока 20
    Модель возвращается напрямую: наружу уходят все поля. Лучше API Resource и статус 201.

Разбор

Две главные проблемы: работа с данными в памяти вместо базы (all() и N+1) и отсутствие проверок (чужие заказы, массовое присваивание без валидации). Улучшенная версия:

public function index(Request $request)
{
    $orders = $request->user()->orders()->withCount('items')->latest()->paginate(20);
    return view('orders.index', compact('orders'));
}

public function store(StoreOrderRequest $request)
{
    $order = DB::transaction(function () use ($request) {
        $order = $request->user()->orders()->create($request->safe()->except('items'));
        $order->items()->createMany($request->validated('items'));
        return $order;
    });
    Mail::to($request->user())->queue(new OrderCreated($order));
    return new OrderResource($order);
}

Ответьте сами, а не прочитайте

В тренажёре вопрос попадёт в повторения и вернётся, когда начнёт забываться. Плюс билеты с зачётом: 20 вопросов, не больше 2 ошибок.

Потренироваться бесплатно

Предыдущий вопрос

Связи Eloquent

Следующий вопрос

Локальные скоупы

Урок по теме

Eloquent, N+1 и миграции

Ещё по теме «Eloquent, N+1 и миграции»

Все вопросы модуля