Improve filter validation and test coverage

lib/gc_index_relay/nostr/filter.ex

@@ -2,6 +2,7 @@ defmodule GcIndexRelay.Nostr.Filter do
   alias GcIndexRelay.Nostr.Event
   alias GcIndexRelay.Repo
   alias GcIndexRelay.Nostr.Tag
+  alias GcIndexRelay.Nostr.Validator
   import Ecto.Query
 
   defstruct [:ids, :authors, :kinds, :tags, :since, :until, :limit]
@@ -16,26 +17,210 @@ defmodule GcIndexRelay.Nostr.Filter do
           limit: pos_integer() | nil
         }
 
-  @spec from_map(map()) :: t()
+  @spec from_map(map()) :: {:ok, t()} | {:error, String.t()}
   def from_map(map) when is_map(map) do
+    # Extract tag filters (keys starting with "#")
     tags =
       for {"#" <> k, v} <- map,
           do: {k, v},
           into: %{}
 
-    %__MODULE__{
-      ids: map["ids"] || nil,
-      authors: map["authors"] || nil,
-      kinds: map["kinds"] || nil,
-      tags:
-        case tags do
-          tags when map_size(tags) == 0 -> nil
-          tags -> tags
-        end,
-      since: map["since"] || nil,
-      until: map["until"] || nil,
-      limit: map["limit"] || nil
-    }
+    # Validate only known keys are present
+    with :ok <- validate_not_empty(map),
+         :ok <- validate_known_keys(map),
+         {:ok, ids} <- validate_ids(map["ids"]),
+         {:ok, authors} <- validate_authors(map["authors"]),
+         {:ok, kinds} <- validate_kinds(map["kinds"]),
+         {:ok, validated_tags} <- validate_tags(tags),
+         {:ok, since} <- validate_timestamp(map["since"], :since),
+         {:ok, until} <- validate_timestamp(map["until"], :until),
+         {:ok, _} <- validate_timestamp_range(map["since"], map["until"]),
+         {:ok, limit} <- validate_limit(map["limit"]) do
+      {:ok,
+       %__MODULE__{
+         ids: ids,
+         authors: authors,
+         kinds: kinds,
+         tags: validated_tags,
+         since: since,
+         until: until,
+         limit: limit
+       }}
+    else
+      {:error, reason} -> {:error, reason}
+    end
+  end
+
+  # Validate that filter is not empty
+  @spec validate_not_empty(map()) :: :ok | {:error, String.t()}
+  defp validate_not_empty(map) when map_size(map) == 0 do
+    {:error, "Filter cannot be empty - at least one filter field must be specified"}
+  end
+
+  defp validate_not_empty(_map), do: :ok
+
+  # Validate that only known filter keys are present
+  @spec validate_known_keys(map()) :: :ok | {:error, String.t()}
+  defp validate_known_keys(map) do
+    known_keys = ["ids", "authors", "kinds", "since", "until", "limit"]
+
+    unknown_keys =
+      map
+      |> Map.keys()
+      |> Enum.reject(fn key ->
+        key in known_keys or String.starts_with?(key, "#")
+      end)
+
+    case unknown_keys do
+      [] -> :ok
+      [key | _] -> {:error, "Unknown filter key: '#{key}'"}
+    end
+  end
+
+  @spec validate_ids([String.t()] | nil) :: {:ok, [String.t()] | nil} | {:error, String.t()}
+  defp validate_ids(nil), do: {:ok, nil}
+  defp validate_ids([]), do: {:ok, nil}
+
+  defp validate_ids(ids) when is_list(ids) do
+    if Enum.all?(ids, &Validator.valid_hex_id?/1) do
+      {:ok, ids}
+    else
+      invalid = Enum.find(ids, &(!Validator.valid_hex_id?(&1)))
+      {:error, "Invalid id in filter: '#{invalid}' must be exactly 64 lowercase hex characters"}
+    end
+  end
+
+  defp validate_ids(_), do: {:error, "Filter 'ids' must be an array of strings"}
+
+  @spec validate_authors([String.t()] | nil) :: {:ok, [String.t()] | nil} | {:error, String.t()}
+  defp validate_authors(nil), do: {:ok, nil}
+  defp validate_authors([]), do: {:ok, nil}
+
+  defp validate_authors(authors) when is_list(authors) do
+    if Enum.all?(authors, &Validator.valid_hex_id?/1) do
+      {:ok, authors}
+    else
+      invalid = Enum.find(authors, &(!Validator.valid_hex_id?(&1)))
+
+      {:error,
+       "Invalid author in filter: '#{invalid}' must be exactly 64 lowercase hex characters"}
+    end
+  end
+
+  defp validate_authors(_), do: {:error, "Filter 'authors' must be an array of strings"}
+
+  @spec validate_kinds([integer()] | nil) :: {:ok, [integer()] | nil} | {:error, String.t()}
+  defp validate_kinds(nil), do: {:ok, nil}
+  defp validate_kinds([]), do: {:ok, nil}
+
+  defp validate_kinds(kinds) when is_list(kinds) do
+    cond do
+      !Enum.all?(kinds, &is_integer/1) ->
+        invalid = Enum.find(kinds, &(!is_integer(&1)))
+        {:error, "Invalid kind in filter: '#{inspect(invalid)}' must be an integer"}
+
+      !Enum.all?(kinds, &(&1 >= 0 and &1 < 40_000)) ->
+        invalid = Enum.find(kinds, &(&1 < 0 or &1 >= 40_000))
+
+        {:error,
+         "Invalid kind in filter: '#{invalid}' must be in the range [0, 40000), got #{invalid}"}
+
+      true ->
+        {:ok, kinds}
+    end
+  end
+
+  defp validate_kinds(_), do: {:error, "Filter 'kinds' must be an array of integers"}
+
+  @spec validate_tags(map()) :: {:ok, map() | nil} | {:error, String.t()}
+  defp validate_tags(tags) when map_size(tags) == 0, do: {:ok, nil}
+
+  defp validate_tags(tags) when is_map(tags) do
+    with :ok <- validate_tag_keys(tags),
+         :ok <- validate_tag_values(tags) do
+      {:ok, tags}
+    else
+      {:error, reason} -> {:error, reason}
+    end
+  end
+
+  defp validate_tags(_), do: {:error, "Filter tags must be an object"}
+
+  @spec validate_tag_keys(map()) :: :ok | {:error, String.t()}
+  defp validate_tag_keys(tags) do
+    # Inline validation logic for better locality of behavior
+    invalid_key =
+      tags
+      |> Map.keys()
+      |> Enum.find(fn k ->
+        !is_binary(k) or String.length(k) != 1 or !String.match?(k, ~r/^[a-zA-Z]$/)
+      end)
+
+    case invalid_key do
+      nil -> :ok
+      key -> {:error, "Invalid tag key '##{key}': must be a single letter (a-z, A-Z)"}
+    end
+  end
+
+  @spec validate_tag_values(map()) :: :ok | {:error, String.t()}
+  defp validate_tag_values(tags) do
+    invalid_entry =
+      Enum.find(tags, fn {_k, v} ->
+        !is_list(v) or !Enum.all?(v, &is_binary/1)
+      end)
+
+    case invalid_entry do
+      nil ->
+        :ok
+
+      {key, value} ->
+        {:error,
+         "Invalid tag value for '##{key}': '#{inspect(value)}' must be an array of strings"}
+    end
+  end
+
+  @spec validate_timestamp(integer() | nil, atom()) ::
+          {:ok, integer() | nil} | {:error, String.t()}
+  defp validate_timestamp(nil, _field), do: {:ok, nil}
+
+  defp validate_timestamp(ts, _field) when is_integer(ts) and ts >= 0 do
+    {:ok, ts}
+  end
+
+  defp validate_timestamp(ts, field) when is_integer(ts) do
+    {:error, "Filter '#{field}' must be a non-negative integer, got #{ts}"}
+  end
+
+  defp validate_timestamp(ts, field) do
+    {:error, "Filter '#{field}' must be an integer, got #{inspect(ts)}"}
+  end
+
+  @spec validate_timestamp_range(integer() | nil, integer() | nil) ::
+          {:ok, :valid} | {:error, String.t()}
+  defp validate_timestamp_range(nil, _until), do: {:ok, :valid}
+  defp validate_timestamp_range(_since, nil), do: {:ok, :valid}
+
+  defp validate_timestamp_range(since, until) when since <= until do
+    {:ok, :valid}
+  end
+
+  defp validate_timestamp_range(since, until) do
+    {:error, "Filter 'since' (#{since}) must be less than or equal to 'until' (#{until})"}
+  end
+
+  @spec validate_limit(pos_integer() | nil) :: {:ok, pos_integer() | nil} | {:error, String.t()}
+  defp validate_limit(nil), do: {:ok, nil}
+
+  defp validate_limit(limit) when is_integer(limit) and limit > 0 do
+    {:ok, limit}
+  end
+
+  defp validate_limit(limit) when is_integer(limit) do
+    {:error, "Filter 'limit' must be a positive integer, got #{limit}"}
+  end
+
+  defp validate_limit(limit) do
+    {:error, "Filter 'limit' must be an integer, got #{inspect(limit)}"}
   end
 
   @doc """

lib/gc_index_relay/nostr/validator.ex

@@ -10,19 +10,51 @@ defmodule GcIndexRelay.Nostr.Validator do
 
   alias GcIndexRelay.Nostr.PubEvent
 
+  @doc """
+  Validates that a string is exactly 64 lowercase hexadecimal characters.
+
+  This is used for Nostr event IDs and public keys (schnorr pubkeys).
+
+  ## Examples
+
+      iex> Validator.valid_hex_id?("a" |> String.duplicate(64))
+      true
+
+      iex> Validator.valid_hex_id?("A" |> String.duplicate(64))
+      false
+
+      iex> Validator.valid_hex_id?("abc")
+      false
+  """
+  @spec valid_hex_id?(any()) :: boolean()
+  def valid_hex_id?(id) when is_binary(id) do
+    String.length(id) == 64 and String.match?(id, ~r/^[0-9a-f]{64}$/)
+  end
+
+  def valid_hex_id?(_), do: false
+
   @doc """
   Validates a Nostr event ID per [NIP-01](https://github.com/nostr-protocol/nips/blob/master/01.md).
+
+  Performs two checks:
+  1. Format validation - ensures ID is 64 lowercase hex characters
+  2. Semantic validation - ensures ID matches the computed hash of event data
   """
   def validate_id(event) when is_struct(event, PubEvent) do
-    if valid_id?(event),
-      do: {:ok, event},
-      else: {:error, "ID #{event.id} is invalid for the given event"}
+    cond do
+      !valid_hex_id?(event.id) ->
+        {:error, "ID #{event.id} has invalid format (must be 64 lowercase hex characters)"}
+
+      !valid_id?(event) ->
+        {:error, "ID #{event.id} is invalid for the given event"}
+
+      true ->
+        {:ok, event}
+    end
   end
 
-  defp valid_id?(event)
-       when is_struct(event, PubEvent) do
+  defp valid_id?(event) when is_struct(event, PubEvent) do
     computed_id = compute_id!(event)
-
     event.id == computed_id
   end
 

test/gc_index_relay/nostr/filter_test.exs

@@ -0,0 +1,352 @@
+defmodule GcIndexRelay.Nostr.FilterTest do
+  use ExUnit.Case, async: true
+  alias GcIndexRelay.Nostr.Filter
+
+  @moduletag :unit
+
+  describe "from_map/1 with valid filters" do
+    test "returns {:ok, filter} with valid ids" do
+      valid_id = String.duplicate("a", 64)
+      assert {:ok, filter} = Filter.from_map(%{"ids" => [valid_id]})
+      assert filter.ids == [valid_id]
+    end
+
+    test "returns {:ok, filter} with valid authors" do
+      valid_author = String.duplicate("b", 64)
+      assert {:ok, filter} = Filter.from_map(%{"authors" => [valid_author]})
+      assert filter.authors == [valid_author]
+    end
+
+    test "returns {:ok, filter} with valid kinds" do
+      assert {:ok, filter} = Filter.from_map(%{"kinds" => [0, 1, 3]})
+      assert filter.kinds == [0, 1, 3]
+    end
+
+    test "returns {:ok, filter} with valid tags (lowercase)" do
+      assert {:ok, filter} = Filter.from_map(%{"#e" => ["value1", "value2"]})
+      assert filter.tags == %{"e" => ["value1", "value2"]}
+    end
+
+    test "returns {:ok, filter} with valid tags (uppercase)" do
+      assert {:ok, filter} = Filter.from_map(%{"#E" => ["value1"]})
+      assert filter.tags == %{"E" => ["value1"]}
+    end
+
+    test "returns {:ok, filter} with multiple valid tags" do
+      assert {:ok, filter} = Filter.from_map(%{"#e" => ["val1"], "#p" => ["val2"]})
+      assert filter.tags == %{"e" => ["val1"], "p" => ["val2"]}
+    end
+
+    test "returns {:ok, filter} with valid since" do
+      assert {:ok, filter} = Filter.from_map(%{"since" => 1_640_000_000})
+      assert filter.since == 1_640_000_000
+    end
+
+    test "returns {:ok, filter} with valid until" do
+      assert {:ok, filter} = Filter.from_map(%{"until" => 1_640_000_000})
+      assert filter.until == 1_640_000_000
+    end
+
+    test "returns {:ok, filter} with valid since and until" do
+      assert {:ok, filter} = Filter.from_map(%{"since" => 1000, "until" => 2000})
+      assert filter.since == 1000
+      assert filter.until == 2000
+    end
+
+    test "returns {:ok, filter} with valid limit" do
+      assert {:ok, filter} = Filter.from_map(%{"limit" => 10})
+      assert filter.limit == 10
+    end
+
+    test "returns {:ok, filter} with all valid fields" do
+      valid_id = String.duplicate("a", 64)
+      valid_author = String.duplicate("b", 64)
+
+      assert {:ok, filter} =
+               Filter.from_map(%{
+                 "ids" => [valid_id],
+                 "authors" => [valid_author],
+                 "kinds" => [0, 1],
+                 "#e" => ["event1"],
+                 "#p" => ["pubkey1"],
+                 "since" => 1000,
+                 "until" => 2000,
+                 "limit" => 10
+               })
+
+      assert filter.ids == [valid_id]
+      assert filter.authors == [valid_author]
+      assert filter.kinds == [0, 1]
+      assert filter.tags == %{"e" => ["event1"], "p" => ["pubkey1"]}
+      assert filter.since == 1000
+      assert filter.until == 2000
+      assert filter.limit == 10
+    end
+
+    test "normalizes empty ids array to nil" do
+      assert {:ok, filter} = Filter.from_map(%{"ids" => []})
+      assert filter.ids == nil
+    end
+
+    test "normalizes empty authors array to nil" do
+      assert {:ok, filter} = Filter.from_map(%{"authors" => []})
+      assert filter.authors == nil
+    end
+
+    test "normalizes empty kinds array to nil" do
+      assert {:ok, filter} = Filter.from_map(%{"kinds" => []})
+      assert filter.kinds == nil
+    end
+
+    test "returns {:ok, filter} with since = 0" do
+      assert {:ok, filter} = Filter.from_map(%{"since" => 0})
+      assert filter.since == 0
+    end
+
+    test "returns {:ok, filter} with until = 0" do
+      assert {:ok, filter} = Filter.from_map(%{"until" => 0})
+      assert filter.until == 0
+    end
+
+    test "returns {:ok, filter} when since equals until" do
+      assert {:ok, filter} = Filter.from_map(%{"since" => 1000, "until" => 1000})
+      assert filter.since == 1000
+      assert filter.until == 1000
+    end
+  end
+
+  describe "from_map/1 with empty filter" do
+    test "returns {:error, message} for completely empty map" do
+      assert {:error, message} = Filter.from_map(%{})
+      assert message =~ "Filter cannot be empty"
+      assert message =~ "at least one filter field must be specified"
+    end
+  end
+
+  describe "from_map/1 with invalid hex fields (ids/authors)" do
+    @hex_field_test_cases [
+      %{
+        field: "ids",
+        invalid_value_msg: "Invalid id in filter",
+        array_type_msg: "Filter 'ids' must be an array of strings"
+      },
+      %{
+        field: "authors",
+        invalid_value_msg: "Invalid author in filter",
+        array_type_msg: "Filter 'authors' must be an array of strings"
+      }
+    ]
+
+    for %{field: field, invalid_value_msg: invalid_msg, array_type_msg: array_msg} <-
+          @hex_field_test_cases do
+      test "returns {:error, message} for short #{field}" do
+        short_value = String.duplicate("a", 32)
+        assert {:error, message} = Filter.from_map(%{unquote(field) => [short_value]})
+        assert message =~ unquote(invalid_msg)
+        assert message =~ "must be exactly 64 lowercase hex characters"
+      end
+
+      test "returns {:error, message} for long #{field}" do
+        long_value = String.duplicate("a", 128)
+        assert {:error, message} = Filter.from_map(%{unquote(field) => [long_value]})
+        assert message =~ unquote(invalid_msg)
+        assert message =~ "must be exactly 64 lowercase hex characters"
+      end
+
+      test "returns {:error, message} for uppercase hex #{field}" do
+        uppercase_value = String.duplicate("A", 64)
+        assert {:error, message} = Filter.from_map(%{unquote(field) => [uppercase_value]})
+        assert message =~ unquote(invalid_msg)
+        assert message =~ "must be exactly 64 lowercase hex characters"
+      end
+
+      test "returns {:error, message} for non-hex characters in #{field}" do
+        invalid_value = String.duplicate("z", 64)
+        assert {:error, message} = Filter.from_map(%{unquote(field) => [invalid_value]})
+        assert message =~ unquote(invalid_msg)
+        assert message =~ "must be exactly 64 lowercase hex characters"
+      end
+
+      test "returns {:error, message} for mixed valid and invalid #{field}" do
+        valid_value = String.duplicate("a", 64)
+        invalid_value = "short"
+        assert {:error, message} = Filter.from_map(%{unquote(field) => [valid_value, invalid_value]})
+        assert message =~ unquote(invalid_msg)
+      end
+
+      test "returns {:error, message} for non-array #{field}" do
+        assert {:error, message} = Filter.from_map(%{unquote(field) => "not-an-array"})
+        assert message =~ unquote(array_msg)
+      end
+
+      test "returns {:error, message} for non-string in #{field} array" do
+        assert {:error, message} = Filter.from_map(%{unquote(field) => [123]})
+        assert message =~ unquote(invalid_msg)
+      end
+    end
+  end
+
+  describe "from_map/1 with invalid kinds" do
+    test "returns {:error, message} for non-array kinds" do
+      assert {:error, message} = Filter.from_map(%{"kinds" => "not-an-array"})
+      assert message =~ "Filter 'kinds' must be an array of integers"
+    end
+
+    @invalid_kind_types [
+      %{value: [1, "2", 3], desc: "non-integer string"},
+      %{value: [1.5], desc: "float"},
+      %{value: [nil], desc: "nil"}
+    ]
+
+    for %{value: value, desc: desc} <- @invalid_kind_types do
+      test "returns {:error, message} for #{desc} in kinds array" do
+        assert {:error, message} = Filter.from_map(%{"kinds" => unquote(Macro.escape(value))})
+        assert message =~ "Invalid kind in filter"
+        assert message =~ "must be an integer"
+      end
+    end
+
+    @invalid_kind_ranges [
+      %{value: -1, desc: "negative kind"},
+      %{value: 40_000, desc: "kind >= 40000"},
+      %{value: 50_000, desc: "kind > 40000"}
+    ]
+
+    for %{value: value, desc: desc} <- @invalid_kind_ranges do
+      test "returns {:error, message} for #{desc}" do
+        assert {:error, message} = Filter.from_map(%{"kinds" => [unquote(value)]})
+        assert message =~ "Invalid kind in filter"
+        assert message =~ "must be in the range [0, 40000)"
+        assert message =~ "got #{unquote(value)}"
+      end
+    end
+  end
+
+  describe "from_map/1 with invalid tags" do
+    @invalid_tag_keys [
+      %{key: "#ee", desc: "multi-character"},
+      %{key: "#1", desc: "numeric"},
+      %{key: "#@", desc: "special character"}
+    ]
+
+    for %{key: key, desc: desc} <- @invalid_tag_keys do
+      test "returns {:error, message} for #{desc} tag key" do
+        assert {:error, message} = Filter.from_map(%{unquote(key) => ["value"]})
+        assert message =~ "Invalid tag key '#{unquote(key)}'"
+        assert message =~ "must be a single letter"
+      end
+    end
+
+    test "returns {:error, message} for non-array tag value" do
+      assert {:error, message} = Filter.from_map(%{"#e" => "not-an-array"})
+      assert message =~ "Invalid tag value for '#e'"
+      assert message =~ "must be an array of strings"
+    end
+
+    @invalid_tag_values [
+      %{value: ["valid", 123], desc: "non-string"},
+      %{value: [nil], desc: "nil"}
+    ]
+
+    for %{value: value, desc: desc} <- @invalid_tag_values do
+      test "returns {:error, message} for #{desc} in tag value array" do
+        assert {:error, message} = Filter.from_map(%{"#e" => unquote(Macro.escape(value))})
+        assert message =~ "Invalid tag value for '#e'"
+        assert message =~ "must be an array of strings"
+      end
+    end
+  end
+
+  describe "from_map/1 with invalid timestamps" do
+    @timestamp_fields ["since", "until"]
+
+    for field <- @timestamp_fields do
+      test "returns {:error, message} for non-integer #{field}" do
+        assert {:error, message} = Filter.from_map(%{unquote(field) => "not-an-integer"})
+        assert message =~ "Filter '#{unquote(field)}' must be an integer"
+      end
+
+      test "returns {:error, message} for negative #{field}" do
+        assert {:error, message} = Filter.from_map(%{unquote(field) => -1})
+        assert message =~ "Filter '#{unquote(field)}' must be a non-negative integer"
+        assert message =~ "got -1"
+      end
+
+      test "returns {:error, message} for float #{field}" do
+        assert {:error, message} = Filter.from_map(%{unquote(field) => 1.5})
+        assert message =~ "Filter '#{unquote(field)}' must be an integer"
+      end
+    end
+  end
+
+  describe "from_map/1 with invalid timestamp range" do
+    test "returns {:error, message} when since > until" do
+      assert {:error, message} = Filter.from_map(%{"since" => 2000, "until" => 1000})
+      assert message =~ "Filter 'since' (2000) must be less than or equal to 'until' (1000)"
+    end
+
+    test "returns {:error, message} when since > until (large values)" do
+      assert {:error, message} =
+               Filter.from_map(%{"since" => 1_640_000_000, "until" => 1_630_000_000})
+
+      assert message =~ "Filter 'since'"
+      assert message =~ "must be less than or equal to"
+      assert message =~ "until"
+    end
+  end
+
+  describe "from_map/1 with invalid limit" do
+    test "returns {:error, message} for non-integer limit" do
+      assert {:error, message} = Filter.from_map(%{"limit" => "not-an-integer"})
+      assert message =~ "Filter 'limit' must be an integer"
+    end
+
+    test "returns {:error, message} for float limit" do
+      assert {:error, message} = Filter.from_map(%{"limit" => 10.5})
+      assert message =~ "Filter 'limit' must be an integer"
+    end
+
+    @invalid_limit_values [
+      %{value: 0, desc: "zero"},
+      %{value: -5, desc: "negative"}
+    ]
+
+    for %{value: value, desc: desc} <- @invalid_limit_values do
+      test "returns {:error, message} for #{desc} limit" do
+        assert {:error, message} = Filter.from_map(%{"limit" => unquote(value)})
+        assert message =~ "Filter 'limit' must be a positive integer"
+        assert message =~ "got #{unquote(value)}"
+      end
+    end
+  end
+
+  describe "from_map/1 with unknown keys" do
+    test "returns {:error, message} for literal 'tags' key" do
+      assert {:error, message} = Filter.from_map(%{"tags" => ["value"]})
+      assert message =~ "Unknown filter key"
+      assert message =~ "tags"
+    end
+
+    test "returns {:error, message} for arbitrary unknown key" do
+      assert {:error, message} = Filter.from_map(%{"random_key" => "value"})
+      assert message =~ "Unknown filter key"
+      assert message =~ "random_key"
+    end
+
+    test "returns {:error, message} for 'search' key" do
+      assert {:error, message} = Filter.from_map(%{"search" => "text"})
+      assert message =~ "Unknown filter key"
+      assert message =~ "search"
+    end
+
+    test "returns {:error, message} even with valid fields present" do
+      valid_id = String.duplicate("a", 64)
+
+      assert {:error, message} =
+               Filter.from_map(%{"ids" => [valid_id], "unknown" => "value"})
+
+      assert message =~ "Unknown filter key"
+      assert message =~ "unknown"
+    end
+  end
+end

test/gc_index_relay/nostr/validator_test.exs

@@ -2,12 +2,52 @@ defmodule GcIndexRelay.Nostr.ValidatorTest do
   use ExUnit.Case, async: true
 
   alias GcIndexRelay.Nostr.Validator
-  alias GcIndexRelay.Nostr.PubEvent
 
   import GcIndexRelay.NostrFixtures
 
   @moduletag :unit
 
+  describe "valid_hex_id?/1" do
+    @valid_hex_ids [
+      %{id: String.duplicate("a", 64), desc: "64-character lowercase hex"},
+      %{
+        id: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef",
+        desc: "mixed lowercase hex characters"
+      }
+    ]
+
+    for %{id: id, desc: desc} <- @valid_hex_ids do
+      test "returns true for valid #{desc}" do
+        assert Validator.valid_hex_id?(unquote(id))
+      end
+    end
+
+    @invalid_hex_ids [
+      %{id: String.duplicate("A", 64), desc: "uppercase hex"},
+      %{id: String.duplicate("a", 32), desc: "short hex string"},
+      %{id: String.duplicate("a", 128), desc: "long hex string"},
+      %{id: String.duplicate("z", 64), desc: "non-hex characters"},
+      %{id: nil, desc: "nil"}
+    ]
+
+    for %{id: id, desc: desc} <- @invalid_hex_ids do
+      test "returns false for #{desc}" do
+        refute Validator.valid_hex_id?(unquote(Macro.escape(id)))
+      end
+    end
+
+    @invalid_types [
+      %{value: 123, desc: "integer"},
+      %{value: [], desc: "empty list"}
+    ]
+
+    for %{value: value, desc: desc} <- @invalid_types do
+      test "returns false for non-string type: #{desc}" do
+        refute Validator.valid_hex_id?(unquote(Macro.escape(value)))
+      end
+    end
+  end
+
   describe "validate_id/1" do
     test "returns {:ok, event} for valid event ID" do
       event = valid_pub_event_fixture()
@@ -15,48 +55,42 @@ defmodule GcIndexRelay.Nostr.ValidatorTest do
       assert {:ok, ^event} = Validator.validate_id(event)
     end
 
-    test "returns {:error, message} for invalid event ID" do
+    test "returns {:error, message} for semantically invalid event ID" do
       event = invalid_id_pub_event_fixture()
 
       assert {:error, message} = Validator.validate_id(event)
       assert message =~ "ID"
-      assert message =~ "is invalid"
+      assert message =~ "is invalid for the given event"
     end
 
-    test "handles nil id gracefully" do
-      event = %PubEvent{
+    @invalid_id_formats [
+      %{
         id: nil,
-        pubkey: "0db15182c4ad3418b4fbab75304be7ade9cfa430a21c1c5320c9298f54ea5406",
-        created_at: 1_640_000_000,
-        kind: 1,
-        tags: [],
-        content: "Test content",
-        sig:
-          "304402201a9c5a7c3f5b2e4d6e8a9f0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f6a7b8c9d0e02204f5e6d7c8b9a0f1e2d3c4b5a6978695a4b3c2d1e0f9a8b7c6d5e4f3a2b1c0d9"
-      }
-
-      # Should return error for nil id
-      assert {:error, _message} = Validator.validate_id(event)
-    end
-
-    test "handles non-hex characters in id" do
-      event = valid_pub_event_fixture()
-
-      invalid_event = %{
-        event
-        | id: "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz"
+        desc: "nil id"
+      },
+      %{
+        id: String.duplicate("z", 64),
+        desc: "non-hex characters in id"
+      },
+      %{
+        id: String.duplicate("a", 32),
+        desc: "incorrect length id"
+      },
+      %{
+        id: String.duplicate("A", 64),
+        desc: "uppercase hex id"
       }
+    ]
 
-      assert {:error, _message} = Validator.validate_id(invalid_event)
-    end
-
-    test "handles incorrect length id" do
-      event = valid_pub_event_fixture()
-      # ID should be 64 hex chars (32 bytes), use 32 chars instead
-      short_id = String.duplicate("a", 32)
-      invalid_event = %{event | id: short_id}
+    for %{id: id, desc: desc} <- @invalid_id_formats do
+      test "returns {:error, message} for #{desc}" do
+        event = valid_pub_event_fixture()
+        invalid_event = %{event | id: unquote(Macro.escape(id))}
 
-      assert {:error, _message} = Validator.validate_id(invalid_event)
+        assert {:error, message} = Validator.validate_id(invalid_event)
+        assert message =~ "invalid format"
+        assert message =~ "64 lowercase hex characters"
+      end
     end
   end
 
@@ -89,39 +123,19 @@ defmodule GcIndexRelay.Nostr.ValidatorTest do
       assert message =~ "is invalid"
     end
 
-    test "handles nil signature gracefully" do
-      event = %PubEvent{
-        id: "4376c65d2f232afbe9b882a35baa4f6fe8667c4e684749af565f981833ed6a65",
-        pubkey: "0db15182c4ad3418b4fbab75304be7ade9cfa430a21c1c5320c9298f54ea5406",
-        created_at: 1_640_000_000,
-        kind: 1,
-        tags: [],
-        content: "Test content",
-        sig: nil
-      }
-
-      # Should return error
-      assert {:error, _message} = Validator.validate_signature(event)
-    end
+    @invalid_signature_formats [
+      %{sig: nil, desc: "nil signature"},
+      %{sig: String.duplicate("z", 128), desc: "non-hex characters in signature"},
+      %{sig: String.duplicate("a", 32), desc: "incorrect length signature"}
+    ]
 
-    test "handles non-hex characters in signature" do
-      event = valid_pub_event_fixture()
-
-      invalid_event = %{
-        event
-        | sig: "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz"
-      }
-
-      assert {:error, _message} = Validator.validate_signature(invalid_event)
-    end
-
-    test "handles incorrect length signature" do
-      event = valid_pub_event_fixture()
-      # Nostr event signatures must be 64 bytes
-      short_sig = String.duplicate("a", 32)
-      invalid_event = %{event | sig: short_sig}
+    for %{sig: sig, desc: desc} <- @invalid_signature_formats do
+      test "handles #{desc} gracefully" do
+        event = valid_pub_event_fixture()
+        invalid_event = %{event | sig: unquote(Macro.escape(sig))}
 
-      assert {:error, _message} = Validator.validate_signature(invalid_event)
+        assert {:error, _message} = Validator.validate_signature(invalid_event)
+      end
     end
   end