Phase 1 complete: wire-type codecs and the response packets

ROADMAP.md

@@ -17,9 +17,11 @@ Decisions made along the way are recorded in
 ## Where things stand
 
 - Toolchain current: Erlang 27.3.4.15 / Elixir 1.20.4, `db_connection` 2.10, CI green
-- `Mysqlex.Protocol` connects over TCP and reads the server's `Protocol::HandshakeV10`
-- Everything else is a stub that returns `DBConnection.ConnectionError`
-- Test suite is empty — the first real test starts phase 1
+- Phase 1 is done: framing, the socket read loop, the wire-type codecs, and the
+  three generic response packets. 31 tests, 4 of them properties
+- `Mysqlex.Protocol` still holds the 2018 handshake parse and the stubbed
+  `DBConnection` callbacks; nothing routes through `Mysqlex.Client` yet
+- No dispatcher: nothing inspects a payload's first byte to choose a parser
 
 **Known broken, fix first:** `parse_payload_body/2` reads `server_version` as a
 fixed 6 bytes, but the protocol says NUL-terminated; `auth_plugin_data_part_2` is
@@ -38,7 +40,28 @@ easy to get right.
   decisions against a production driver. Reading first means implementing someone
   else's design without having had the thought.
 - Keep `mix format`, `mix credo --strict`, `mix dialyzer` and
-  `mix compile --warnings-as-errors` clean — CI enforces all four.
+  `mix compile --warnings-as-errors` clean — CI enforces all four. Gate on the
+  exit code, not on filtered output: a pipe into `grep` or `tail` exits 0 even
+  when the tests failed.
+- **Nothing lands on `master` directly.** Each slice gets a branch and a pull
+  request, reviewed before merging.
+
+### How the tests are organised
+
+The module boundaries follow what a test needs, so most of the suite runs
+without a database:
+
+| Tier | Needs | Example |
+| --- | --- | --- |
+| 1 | nothing — pure functions | codecs, framing, auth scrambles |
+| 2 | captured bytes in `test/fixtures/` | packet body parsers |
+| 3 | a scripted `Mysqlex.Transport` in `test/support/` | the read loop, the connection phase |
+| 4 | a real MySQL, tagged `:integration` and excluded by default | queries, prepared statements |
+
+Tier 1 and 2 are where property tests and spec examples go. Where the protocol
+documentation includes example bytes, use those rather than hand-built fixtures:
+a fixture you author tests the parser against your reading of the spec, which is
+how a misreading passes.
 
 ### Local server
 
@@ -59,14 +82,25 @@ GRANT ALL ON mysqlex_test.* TO 'mysqlex'@'%';
 
 Each phase should end somewhere runnable.
 
-### 1. Foundation
+### 1. Foundation — done
 
-- [ ] `recv_packet` / `send_packet`: 3-byte little-endian length + 1-byte sequence id
-- [ ] Sequence id tracking, and resetting it per command
-- [ ] Packets larger than 2^24-1: splitting on send, reassembly on receive
-- [ ] Buffering — a packet can span several TCP reads
-- [ ] Codecs: `int<lenenc>`, `string<lenenc>`, `string<NUL>`, `string<EOF>`, fixed ints
-- [ ] `OK_Packet`, `ERR_Packet`, `EOF_Packet`, including the `CLIENT_DEPRECATE_EOF` form
+- [x] Frame codec: 3-byte little-endian length + 1-byte sequence id
+- [x] Packets larger than 2^24-1: splitting on send, reassembly on receive
+- [x] Buffering — a packet can span several TCP reads, or several arrive in one
+- [x] `Mysqlex.Transport` behaviour, so reads can be driven without a server
+- [x] Codecs: `int<lenenc>`, `string<lenenc>`, `string<NUL>`; `string<EOF>` is
+      `rest` and fixed-width fields are inline patterns, so neither needs a function
+- [x] `OK_Packet`, `ERR_Packet`, `EOF_Packet`
+
+Deliberately not here:
+
+- Sequence id tracking across a command — it is a parameter to the codec, and
+  tracking belongs to whatever owns the connection, which does not exist yet
+- The dispatcher that decides whether a payload beginning `0xFE` is an EOF
+  packet, an OK standing in for one, or a length-encoded integer. It needs the
+  payload length and the command context
+- `CLIENT_SESSION_TRACK` in the OK packet — the capability will not be
+  advertised until phase 5
 
 ### 2. Connection phase
 
@@ -119,6 +153,9 @@ Each stub in `lib/mysqlex/protocol.ex` maps onto protocol work above:
 - [ ] Transactions and savepoints, `handle_status`
 - [ ] Replace the remaining stubs
 
+Replacing `db_connection` with a hand-written connection and pool is an option
+from here, not a prerequisite — see decision 15.
+
 ### 7. Replication
 
 - [ ] `COM_BINLOG_DUMP`

lib/mysqlex/error.ex

@@ -0,0 +1,11 @@
+defmodule Mysqlex.Error do
+  @moduledoc """
+  An error reported by the server.
+
+  `code` is MySQL's own error number and is precise; `sql_state` is the ANSI SQL
+  five-character code and is portable across databases. `sql_state` is `nil` when
+  the connection did not negotiate `CLIENT_PROTOCOL_41`.
+  """
+
+  defstruct [:code, :sql_state, :message]
+end

lib/mysqlex/protocol/codec.ex

@@ -0,0 +1,64 @@
+defmodule Mysqlex.Protocol.Codec do
+  @moduledoc """
+  The protocol's own wire types — the grammar every packet body is built from.
+
+  See `docs/mysql-protocol/page_protocol_basic_dt_integers.md` and
+  `docs/mysql-protocol/page_protocol_basic_dt_strings.md`.
+  """
+
+  @doc """
+  Encodes a length-encoded integer in the shortest form that holds it.
+
+  The one-byte form stops at 250, not 251. `0xFB` marks a NULL column in a text
+  resultset row and `0xFC` to `0xFE` are the prefixes for the longer forms, so
+  those byte values cannot stand for themselves.
+  """
+  def encode_int_lenenc(value) when value < 251, do: <<value>>
+  def encode_int_lenenc(value) when value < 0x10000, do: <<0xFC, value::little-16>>
+  def encode_int_lenenc(value) when value < 0x1000000, do: <<0xFD, value::little-24>>
+  def encode_int_lenenc(value), do: <<0xFE, value::little-64>>
+
+  @doc """
+  Decodes a length-encoded integer, returning it with the bytes that follow it.
+
+  `0xFB` matches no clause: in the one place it appears it means NULL, and the
+  row parser is responsible for recognising that before asking for an integer.
+  """
+  def decode_int_lenenc(<<0xFC, value::little-16, rest::binary>>), do: {value, rest}
+  def decode_int_lenenc(<<0xFD, value::little-24, rest::binary>>), do: {value, rest}
+  def decode_int_lenenc(<<0xFE, value::little-64, rest::binary>>), do: {value, rest}
+  def decode_int_lenenc(<<value, rest::binary>>) when value < 251, do: {value, rest}
+
+  @doc """
+  Encodes a string as its length followed by its bytes, as iodata.
+  """
+  def encode_string_lenenc(string), do: [encode_int_lenenc(byte_size(string)), string]
+
+  @doc """
+  Decodes a length-prefixed string, returning it with the bytes that follow it.
+  """
+  def decode_string_lenenc(data) do
+    {length, rest} = decode_int_lenenc(data)
+    <<string::binary-size(^length), rest::binary>> = rest
+
+    {string, rest}
+  end
+
+  @doc """
+  Encodes a string with a terminating NUL byte, as iodata.
+
+  The string must not itself contain a NUL, since the terminator is the only
+  thing marking where it ends.
+  """
+  def encode_string_nul(string), do: [string, 0]
+
+  @doc """
+  Decodes a NUL-terminated string, returning it with the bytes that follow the
+  terminator.
+  """
+  def decode_string_nul(data) do
+    [string, rest] = :binary.split(data, <<0>>)
+
+    {string, rest}
+  end
+end

lib/mysqlex/protocol/eof.ex

@@ -0,0 +1,36 @@
+defmodule Mysqlex.Protocol.Eof do
+  @moduledoc """
+  The EOF_Packet, which marks the end of a resultset.
+
+  Deprecated since MySQL 5.7.5 in favour of an OK packet standing in for it, so
+  a client advertising `CLIENT_DEPRECATE_EOF` will not receive one. It is still
+  needed for servers and connections that do not use that capability.
+
+  Note the field order: `warnings` comes before `status_flags`, the reverse of
+  the OK packet, which carries the same two values the other way round.
+
+  The spec also warns that this packet can appear where a length-encoded integer
+  can appear — both start `0xFE` — and that a payload shorter than 9 bytes is
+  what distinguishes it. That check needs the payload length, so it belongs to
+  whatever dispatches on the payload rather than here.
+
+  See `docs/mysql-protocol/page_protocol_basic_eof_packet.md`.
+  """
+
+  import Bitwise
+
+  # CLIENT_PROTOCOL_41
+  @protocol_41 0x0200
+
+  defstruct warnings: 0, status_flags: 0
+
+  @doc """
+  Decodes an EOF_Packet payload.
+  """
+  def decode(<<0xFE, warnings::little-16, status_flags::little-16>>, capabilities)
+      when (capabilities &&& @protocol_41) != 0 do
+    {:ok, %__MODULE__{warnings: warnings, status_flags: status_flags}}
+  end
+
+  def decode(<<0xFE>>, _capabilities), do: {:ok, %__MODULE__{}}
+end

lib/mysqlex/protocol/err.ex

@@ -0,0 +1,35 @@
+defmodule Mysqlex.Protocol.Err do
+  @moduledoc """
+  The ERR_Packet, which the server sends whenever something goes wrong.
+
+  See `docs/mysql-protocol/page_protocol_basic_err_packet.md`.
+  """
+
+  import Bitwise
+
+  alias Mysqlex.Error
+
+  # CLIENT_PROTOCOL_41
+  @protocol_41 0x0200
+
+  @doc """
+  Decodes an ERR_Packet payload.
+
+  The SQL state is only sent when `CLIENT_PROTOCOL_41` was negotiated, so the
+  capability flags decide how much of the packet there is to read. The message
+  runs to the end of the payload and carries no length of its own.
+  """
+  def decode(<<0xFF, code::little-16, rest::binary>>, capabilities) do
+    {sql_state, message} = split_sql_state(rest, (capabilities &&& @protocol_41) != 0)
+
+    {:ok, %Error{code: code, sql_state: sql_state, message: message}}
+  end
+
+  defp split_sql_state(<<?#, sql_state::binary-size(5), message::binary>>, true),
+    do: {sql_state, message}
+
+  # The marker carries no information — it exists so that clients older than 4.1
+  # could tell whether a SQL state was present. Treat its absence as the server
+  # having sent none rather than as a malformed packet.
+  defp split_sql_state(message, _protocol_41), do: {nil, message}
+end

lib/mysqlex/protocol/ok.ex

@@ -0,0 +1,56 @@
+defmodule Mysqlex.Protocol.Ok do
+  @moduledoc """
+  The OK_Packet, which the server sends when a command completes successfully.
+
+  Since 5.7.5 it also stands in for the EOF packet, which is why the header is
+  `0x00` or `0xFE`. The two are told apart by length rather than by header — OK
+  is `0x00` with a payload of 7 bytes or more, EOF is `0xFE` with fewer than 8 —
+  and that decision belongs to whatever dispatches on the payload, since only it
+  knows the length.
+
+  See `docs/mysql-protocol/page_protocol_basic_ok_packet.md`.
+  """
+
+  import Bitwise
+
+  alias Mysqlex.Protocol.Codec
+
+  # CLIENT_PROTOCOL_41, CLIENT_TRANSACTIONS
+  @protocol_41 0x0200
+  @transactions 0x2000
+
+  defstruct affected_rows: 0, last_insert_id: 0, status_flags: 0, warnings: 0, info: ""
+
+  @doc """
+  Decodes an OK_Packet payload.
+
+  `CLIENT_SESSION_TRACK` is not handled: with that capability `info` becomes a
+  length-encoded string followed by an optional session-state block, a different
+  shape entirely. The driver does not advertise the capability, so the server
+  does not send it.
+  """
+  def decode(<<header, rest::binary>>, capabilities) when header in [0x00, 0xFE] do
+    {affected_rows, rest} = Codec.decode_int_lenenc(rest)
+    {last_insert_id, rest} = Codec.decode_int_lenenc(rest)
+    {status_flags, warnings, info} = decode_status(rest, capabilities)
+
+    {:ok,
+     %__MODULE__{
+       affected_rows: affected_rows,
+       last_insert_id: last_insert_id,
+       status_flags: status_flags,
+       warnings: warnings,
+       info: info
+     }}
+  end
+
+  defp decode_status(<<status_flags::little-16, warnings::little-16, info::binary>>, capabilities)
+       when (capabilities &&& @protocol_41) != 0,
+       do: {status_flags, warnings, info}
+
+  defp decode_status(<<status_flags::little-16, info::binary>>, capabilities)
+       when (capabilities &&& @transactions) != 0,
+       do: {status_flags, 0, info}
+
+  defp decode_status(info, _capabilities), do: {0, 0, info}
+end

test/mysqlex/protocol/codec_test.exs

@@ -0,0 +1,91 @@
+defmodule Mysqlex.Protocol.CodecTest do
+  use ExUnit.Case, async: true
+  use ExUnitProperties
+
+  alias Mysqlex.Protocol.Codec
+
+  # One value either side of every size boundary in the spec's table.
+  @int_lenenc [
+    {0, <<0>>},
+    {250, <<250>>},
+    {251, <<0xFC, 251::little-16>>},
+    {0xFFFF, <<0xFC, 0xFFFF::little-16>>},
+    {0x10000, <<0xFD, 0x10000::little-24>>},
+    {0xFFFFFF, <<0xFD, 0xFFFFFF::little-24>>},
+    {0x1000000, <<0xFE, 0x1000000::little-64>>},
+    {0xFFFFFFFFFFFFFFFF, <<0xFE, 0xFFFFFFFFFFFFFFFF::little-64>>}
+  ]
+
+  describe "int<lenenc>" do
+    test "encodes each size boundary" do
+      for {value, encoded} <- @int_lenenc do
+        assert Codec.encode_int_lenenc(value) == encoded,
+               "#{value} should encode to #{inspect(encoded, base: :hex)}"
+      end
+    end
+
+    test "decodes each size boundary" do
+      for {value, encoded} <- @int_lenenc do
+        assert {^value, ""} = Codec.decode_int_lenenc(encoded)
+      end
+    end
+
+    test "leaves the bytes after the integer" do
+      assert {250, "rest"} = Codec.decode_int_lenenc(<<250, "rest">>)
+      assert {0x10000, "rest"} = Codec.decode_int_lenenc(<<0xFD, 0x10000::little-24, "rest">>)
+    end
+
+    property "decoding inverts encoding" do
+      check all(value <- integer(0..(2 ** 64 - 1))) do
+        assert {^value, ""} = Codec.decode_int_lenenc(Codec.encode_int_lenenc(value))
+      end
+    end
+  end
+
+  describe "string<lenenc>" do
+    test "prefixes the string with its length" do
+      assert IO.iodata_to_binary(Codec.encode_string_lenenc("hello")) == <<5, "hello">>
+      assert IO.iodata_to_binary(Codec.encode_string_lenenc("")) == <<0>>
+    end
+
+    test "uses a multi-byte length prefix once the string is long enough" do
+      long = :binary.copy("x", 300)
+
+      assert IO.iodata_to_binary(Codec.encode_string_lenenc(long)) ==
+               <<0xFC, 300::little-16>> <> long
+    end
+
+    test "decodes the string and leaves what follows" do
+      assert {"hello", "rest"} = Codec.decode_string_lenenc(<<5, "hello", "rest">>)
+      assert {"", "rest"} = Codec.decode_string_lenenc(<<0, "rest">>)
+    end
+
+    property "decoding inverts encoding" do
+      check all(string <- binary()) do
+        encoded = IO.iodata_to_binary(Codec.encode_string_lenenc(string))
+
+        assert {^string, ""} = Codec.decode_string_lenenc(encoded)
+      end
+    end
+  end
+
+  describe "string<NUL>" do
+    test "reads up to the terminator and leaves what follows" do
+      assert {"hello", "rest"} = Codec.decode_string_nul(<<"hello", 0, "rest">>)
+      assert {"", "rest"} = Codec.decode_string_nul(<<0, "rest">>)
+    end
+
+    test "stops at the first terminator" do
+      assert {"a", <<"b", 0>>} = Codec.decode_string_nul(<<"a", 0, "b", 0>>)
+    end
+
+    property "decoding inverts encoding for strings holding no NUL" do
+      check all(bytes <- list_of(integer(1..255))) do
+        string = :erlang.list_to_binary(bytes)
+        encoded = IO.iodata_to_binary(Codec.encode_string_nul(string))
+
+        assert {^string, ""} = Codec.decode_string_nul(encoded)
+      end
+    end
+  end
+end