Phoenix Code Review

Phoenix Code Review

Quick Reference

Review Checklist

Controllers

• [ ] Business logic in contexts, not controllers
• [ ] Controllers return proper HTTP status codes
• [ ] Action clauses handle all expected patterns
• [ ] Fallback controllers handle errors consistently

Contexts

• [ ] Contexts are bounded by domain, not technical layer
• [ ] Public functions have clear, domain-focused names
• [ ] Changesets validate all user input
• [ ] No Ecto queries in controllers

Routing

• [ ] Verified routes (~p sigil) used, not string paths
• [ ] Pipelines group related plugs
• [ ] Resources use only needed actions
• [ ] Scopes group related routes

Plugs

• [ ] Authentication/authorization via plugs
• [ ] Plugs are composable and single-purpose
• [ ] Halt called after sending response in plugs

JSON APIs

• [ ] Proper content negotiation
• [ ] Consistent error response format
• [ ] Pagination for list endpoints

Valid Patterns (Do NOT Flag)

• Controller calling multiple contexts - Valid for orchestration
• Inline Ecto query in context - Context owns its data access
• Using `action_fallback` - Centralized error handling pattern
• Multiple pipelines per route - Composition is intentional
• `Plug.Conn.halt/1` without send - May be handled by fallback

Context-Sensitive Rules

Gates (run in order; each step has a pass condition)

1. Anchored evidence — For every planned finding, open the source and note file path + line number from that read (not from memory or diff snippets alone). Pass: each finding cites path:line that you opened. 2. “Handled elsewhere” sweep — Before reporting “missing validation,” “missing auth,” or “wrong status,” search the router (pipelines/scopes), controller (action_fallback, plug), and relevant context for existing checks. Pass: you recorded whether handling exists elsewhere (yes + where, or no after search). 3. Verification protocol — Load and apply review-verification-protocol for the issue type. Pass: that skill’s pre-report checks for that finding class are satisfied before you write the finding. 4. Finding shape — Emit each issue as [FILE:LINE] ISSUE_TITLE with a one-line rationale tied to the cited code. Pass: every line matches that pattern.

Before Submitting Findings

Do not report until Gates above pass. For full anti-false-positive steps, follow review-verification-protocol.

Phoenix Contexts

Bounded Contexts

Domain Boundaries

# GOOD - contexts bounded by domain
lib/my_app/
├── accounts/           # User identity & auth
│   ├── user.ex
│   └── accounts.ex
├── catalog/            # Product information
│   ├── product.ex
│   └── catalog.ex
└── orders/             # Purchase workflow
    ├── order.ex
    └── orders.ex

# BAD - contexts bounded by technical layer
lib/my_app/
├── models/
├── queries/
└── services/

Public API Design

# GOOD - domain-focused function names
defmodule MyApp.Accounts do
  def register_user(attrs)
  def authenticate_user(email, password)
  def reset_password(user, new_password)
end

# BAD - CRUD-focused names
defmodule MyApp.Accounts do
  def create_user(attrs)
  def get_user(id)
  def update_user(user, attrs)
end

Ecto Integration

Changesets in Contexts

defmodule MyApp.Accounts do
  alias MyApp.Accounts.User

  def create_user(attrs) do
    %User{}
    |> User.registration_changeset(attrs)
    |> Repo.insert()
  end

  def update_user(%User{} = user, attrs) do
    user
    |> User.update_changeset(attrs)
    |> Repo.update()
  end
end

Schema Definitions

defmodule MyApp.Accounts.User do
  use Ecto.Schema
  import Ecto.Changeset

  schema "users" do
    field :email, :string
    field :password_hash, :string
    field :password, :string, virtual: true

    timestamps()
  end

  def registration_changeset(user, attrs) do
    user
    |> cast(attrs, [:email, :password])
    |> validate_required([:email, :password])
    |> validate_format(:email, ~r/@/)
    |> validate_length(:password, min: 8)
    |> unique_constraint(:email)
    |> hash_password()
  end
end

Cross-Context Communication

# GOOD - contexts communicate through public APIs
defmodule MyApp.Orders do
  alias MyApp.Accounts

  def create_order(user_id, items) do
    with {:ok, user} <- Accounts.get_user(user_id),
         :ok <- Accounts.verify_can_purchase(user) do
      # Create order
    end
  end
end

# BAD - reaching into another context's internals
defmodule MyApp.Orders do
  alias MyApp.Accounts.User
  alias MyApp.Repo

  def create_order(user_id, items) do
    user = Repo.get!(User, user_id)  # Bypasses Accounts context!
    # ...
  end
end

Review Questions

1. Are contexts bounded by business domain, not technical layer? 2. Do public functions have domain-focused names? 3. Are changesets used for all data validation? 4. Do contexts communicate through public APIs only?

Phoenix Controllers

Action Structure

Keep Controllers Thin

# GOOD - delegates to context
defmodule MyAppWeb.UserController do
  use MyAppWeb, :controller

  alias MyApp.Accounts

  def create(conn, %{"user" => user_params}) do
    case Accounts.register_user(user_params) do
      {:ok, user} ->
        conn
        |> put_status(:created)
        |> render(:show, user: user)

      {:error, changeset} ->
        conn
        |> put_status(:unprocessable_entity)
        |> render(:error, changeset: changeset)
    end
  end
end

# BAD - business logic in controller
defmodule MyAppWeb.UserController do
  def create(conn, %{"user" => params}) do
    changeset = User.changeset(%User{}, params)

    if changeset.valid? do
      # Validation logic here...
      # Email verification logic here...
      # Password hashing here...
    end
  end
end

Action Fallback

defmodule MyAppWeb.UserController do
  use MyAppWeb, :controller

  action_fallback MyAppWeb.FallbackController

  def show(conn, %{"id" => id}) do
    with {:ok, user} <- Accounts.get_user(id) do
      render(conn, :show, user: user)
    end
  end
end

defmodule MyAppWeb.FallbackController do
  use MyAppWeb, :controller

  def call(conn, {:error, :not_found}) do
    conn
    |> put_status(:not_found)
    |> put_view(MyAppWeb.ErrorJSON)
    |> render(:"404")
  end

  def call(conn, {:error, %Ecto.Changeset{} = changeset}) do
    conn
    |> put_status(:unprocessable_entity)
    |> put_view(MyAppWeb.ChangesetJSON)
    |> render(:error, changeset: changeset)
  end
end

Parameter Handling

Pattern Match in Function Head

# GOOD - pattern match expected params
def update(conn, %{"id" => id, "user" => user_params}) do
  # ...
end

# GOOD - handle missing params explicitly
def update(conn, %{"id" => id}) do
  conn
  |> put_status(:bad_request)
  |> json(%{error: "Missing user params"})
end

Strong Parameters via Changesets

# Changeset controls which fields are accepted
def registration_changeset(user, attrs) do
  user
  |> cast(attrs, [:email, :password, :name])  # Only these fields
  |> validate_required([:email, :password])
end

HTTP Status Codes

Review Questions

1. Is business logic delegated to contexts? 2. Do actions use appropriate HTTP status codes? 3. Is action_fallback used for consistent error handling? 4. Are parameters validated via changesets?

Phoenix Plugs

Custom Plugs

Module Plug Structure

defmodule MyAppWeb.Plugs.RequireAuth do
  import Plug.Conn
  import Phoenix.Controller

  def init(opts), do: opts

  def call(conn, _opts) do
    if conn.assigns[:current_user] do
      conn
    else
      conn
      |> put_status(:unauthorized)
      |> put_view(MyAppWeb.ErrorJSON)
      |> render(:"401")
      |> halt()  # IMPORTANT: halt after sending response
    end
  end
end

Function Plug

defmodule MyAppWeb.UserController do
  plug :load_user when action in [:show, :edit, :update]

  defp load_user(conn, _opts) do
    case Accounts.get_user(conn.params["id"]) do
      {:ok, user} -> assign(conn, :user, user)
      {:error, :not_found} ->
        conn
        |> put_status(:not_found)
        |> render(:not_found)
        |> halt()
    end
  end
end

Authentication Pattern

defmodule MyAppWeb.Plugs.LoadCurrentUser do
  import Plug.Conn

  def init(opts), do: opts

  def call(conn, _opts) do
    user_id = get_session(conn, :user_id)

    cond do
      conn.assigns[:current_user] ->
        conn  # Already loaded

      user_id && user = Accounts.get_user!(user_id) ->
        assign(conn, :current_user, user)

      true ->
        assign(conn, :current_user, nil)
    end
  end
end

Authorization Pattern

defmodule MyAppWeb.Plugs.RequireAdmin do
  import Plug.Conn
  import Phoenix.Controller

  def init(opts), do: opts

  def call(conn, _opts) do
    user = conn.assigns[:current_user]

    if user && user.admin do
      conn
    else
      conn
      |> put_status(:forbidden)
      |> put_view(MyAppWeb.ErrorJSON)
      |> render(:"403")
      |> halt()
    end
  end
end

Plug Composition

# In router
pipeline :authenticated do
  plug MyAppWeb.Plugs.LoadCurrentUser
  plug MyAppWeb.Plugs.RequireAuth
end

pipeline :admin do
  plug MyAppWeb.Plugs.RequireAdmin
end

scope "/admin", MyAppWeb.Admin do
  pipe_through [:browser, :authenticated, :admin]
  # ...
end

Common Mistakes

Forgetting to Halt

# BAD - continues to controller after sending response
def call(conn, _opts) do
  if unauthorized?(conn) do
    conn
    |> send_resp(401, "Unauthorized")
    # Missing halt()! Controller still runs
  else
    conn
  end
end

# GOOD
def call(conn, _opts) do
  if unauthorized?(conn) do
    conn
    |> send_resp(401, "Unauthorized")
    |> halt()
  else
    conn
  end
end

Modifying Halted Conn

# BAD - checking after halt
def call(conn, _opts) do
  conn = maybe_halt(conn)
  assign(conn, :data, load_data())  # Runs even if halted!
end

# GOOD - check halted status
def call(conn, _opts) do
  conn = maybe_halt(conn)

  if conn.halted do
    conn
  else
    assign(conn, :data, load_data())
  end
end

Review Questions

1. Do plugs call halt() after sending a response? 2. Is authentication handled via plugs, not controller logic? 3. Are plugs composable and single-purpose? 4. Is halted status checked before further processing?

Phoenix Routing

Verified Routes

Use ~p Sigil

# GOOD - verified at compile time
~p"/users/#{user.id}"
~p"/users/#{user}/edit"

# BAD - string interpolation (no compile-time check)
"/users/#{user.id}"
"/users/#{user.id}/edit"

In Templates

<%# GOOD %>
<.link navigate={~p"/users/#{@user}"}>Profile</.link>

<%# BAD %>
<.link navigate={"/users/#{@user.id}"}>Profile</.link>

Pipelines

Group Related Plugs

pipeline :browser do
  plug :accepts, ["html"]
  plug :fetch_session
  plug :fetch_live_flash
  plug :put_root_layout, html: {MyAppWeb.Layouts, :root}
  plug :protect_from_forgery
  plug :put_secure_browser_headers
end

pipeline :api do
  plug :accepts, ["json"]
end

pipeline :authenticated do
  plug MyAppWeb.Plugs.RequireAuth
  plug MyAppWeb.Plugs.LoadCurrentUser
end

Compose Pipelines

scope "/", MyAppWeb do
  pipe_through [:browser, :authenticated]

  resources "/settings", SettingsController, only: [:edit, :update]
end

scope "/admin", MyAppWeb.Admin do
  pipe_through [:browser, :authenticated, :require_admin]

  resources "/users", UserController
end

Resources

Limit Actions

# GOOD - only needed actions
resources "/users", UserController, only: [:index, :show, :create]
resources "/sessions", SessionController, only: [:new, :create, :delete]

# BAD - all actions when not needed
resources "/users", UserController  # Generates 7 routes

Nested Resources

# GOOD - shallow nesting
resources "/posts", PostController do
  resources "/comments", CommentController, only: [:create]
end

resources "/comments", CommentController, only: [:show, :update, :delete]

# BAD - deep nesting
resources "/users", UserController do
  resources "/posts", PostController do
    resources "/comments", CommentController  # Too deep!
  end
end

Scopes

# API versioning
scope "/api", MyAppWeb.API do
  scope "/v1", V1 do
    pipe_through :api
    resources "/users", UserController
  end

  scope "/v2", V2 do
    pipe_through [:api, :v2_transforms]
    resources "/users", UserController
  end
end

Review Questions

1. Are verified routes (~p) used instead of string paths? 2. Are pipelines composed for authentication/authorization? 3. Do resources specify only needed actions? 4. Is nesting kept shallow (max 1 level)?