From cb51844729a440203f18769f3859321f5ad5a7fb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?H=C3=A5vard=20Lindset?= <havard@givn.no>
Date: Sat, 25 Jul 2026 21:30:16 +0200
Subject: [PATCH] Return a structured Image.Error from every open and write
 path

---
 lib/image.ex                     | 72 +++++++++++++++--------
 lib/image/error.ex               | 99 ++++++--------------------------
 test/error_coverage_test.exs     | 63 +++++---------------
 test/group_a_test.exs            |  5 +-
 test/image_gap_coverage_test.exs |  2 +-
 test/image_io_coverage_test.exs  | 63 +++++++++++++++++---
 test/stream_image_test.exs       |  4 +-
 7 files changed, 139 insertions(+), 169 deletions(-)

diff --git a/lib/image.ex b/lib/image.ex
index 8aebddff..70042742 100644
--- a/lib/image.ex
+++ b/lib/image.ex
@@ -794,7 +794,7 @@ defmodule Image do
 
   # JPEG signature
   def open(<<0xFF, 0xD8, 0xFF, _::binary>> = image, options) do
-    from_binary(image, options)
+    open_binary(image, options)
   end
 
   # PNG signature
@@ -804,32 +804,32 @@ defmodule Image do
     end
 
   def open(unquote(png) = image, options) do
-    from_binary(image, options)
+    open_binary(image, options)
   end
 
   # WEBP signature
   def open(<<"RIFF", _::size(32), "WEBP", _::binary>> = image, options) do
-    from_binary(image, options)
+    open_binary(image, options)
   end
 
   # GIF87a signature
   def open(<<0x47, 0x49, 0x46, 0x38, 0x37, 0x61, _::binary>> = image, options) do
-    from_binary(image, options)
+    open_binary(image, options)
   end
 
   # GIF89a signature
   def open(<<0x47, 0x49, 0x46, 0x38, 0x39, 0x61, _::binary>> = image, options) do
-    from_binary(image, options)
+    open_binary(image, options)
   end
 
   # TIF little endian
   def open(<<0x49, 0x49, 0x2A, 0x00, _::binary>> = image, options) do
-    from_binary(image, options)
+    open_binary(image, options)
   end
 
   # TIF big endian
   def open(<<0x4D, 0x4D, 0x00, 0x2A, _::binary>> = image, options) do
-    from_binary(image, options)
+    open_binary(image, options)
   end
 
   # 'heic': the usual HEIF images
@@ -844,16 +844,16 @@ defmodule Image do
 
   def open(<<_::bytes-4, "ftyp", type::bytes-4, _rest::binary>> = image, options)
       when type in @heic_types do
-    from_binary(image, options)
+    open_binary(image, options)
   end
 
   # SVG starting with either svg or xml tag
   def open(<<"<svg ", _::binary>> = image, options) do
-    from_binary(image, options)
+    open_binary(image, options)
   end
 
   def open(<<"<?xml ", _::binary>> = image, options) do
-    from_binary(image, options)
+    open_binary(image, options)
   end
 
   # A file path
@@ -869,7 +869,7 @@ defmodule Image do
   def open(%File.Stream{line_or_bytes: bytes} = image_stream, options) when is_integer(bytes) do
     with {:ok, options} <- Options.Open.validate_options(options) do
       options = loader_options(options)
-      Vix.Vips.Image.new_from_enum(image_stream, options)
+      open_enum(image_stream, options)
     end
   end
 
@@ -886,13 +886,13 @@ defmodule Image do
   def open(image_stream, options) do
     with {:ok, options} <- Options.Open.validate_options(options) do
       options = loader_options(options)
-      Vix.Vips.Image.new_from_enum(image_stream, options)
+      open_enum(image_stream, options)
     end
   end
 
   defp do_open([path], options) do
     if File.exists?(path) do
-      vix_open(path, options)
+      open_path(path, options)
     else
       {:error, Image.Error.wrap(:enoent, operation: :open, path: path)}
     end
@@ -900,19 +900,35 @@ defmodule Image do
 
   defp do_open([path, _open_options], options) do
     if File.exists?(path) do
-      vix_open(path, options)
+      open_path(path, options)
     else
       {:error, Image.Error.wrap(:enoent, operation: :open, path: path)}
     end
   end
 
-  defp vix_open(path, options) do
+  defp open_path(path, options) do
     case Vimage.new_from_file(path, options) do
       {:ok, image} -> {:ok, image}
       {:error, reason} -> {:error, Image.Error.wrap(reason, operation: :open, path: path)}
     end
   end
 
+  # Rename the `:operation` from `:from_binary` to `:open` when reaching
+  # it through `open` instead of directly through `from_binary`
+  defp open_binary(image, options) do
+    case from_binary(image, options) do
+      {:ok, image} -> {:ok, image}
+      {:error, error} -> {:error, Image.Error.wrap(error, operation: :open)}
+    end
+  end
+
+  defp open_enum(image_stream, options) do
+    case Vix.Vips.Image.new_from_enum(image_stream, options) do
+      {:ok, image} -> {:ok, image}
+      {:error, reason} -> {:error, Image.Error.wrap(reason, operation: :open)}
+    end
+  end
+
   defp loader_options(options) do
     "[" <> Enum.map_join(options, ",", &loader_option/1) <> "]"
   end
@@ -1056,7 +1072,7 @@ defmodule Image do
   def open!(path_or_stream_or_binary, options \\ []) do
     case open(path_or_stream_or_binary, options) do
       {:ok, image} -> image
-      {:error, reason} -> raise Image.Error, {reason, path_or_stream_or_binary}
+      {:error, %Image.Error{} = error} -> raise error
     end
   end
 
@@ -1227,7 +1243,11 @@ defmodule Image do
   def from_binary(binary, options \\ []) when is_binary(binary) do
     with {:ok, options} <- Options.Open.validate_options(options) do
       options = Keyword.delete(options, :access)
-      Vimage.new_from_buffer(binary, options)
+
+      case Vimage.new_from_buffer(binary, options) do
+        {:ok, image} -> {:ok, image}
+        {:error, reason} -> {:error, Image.Error.wrap(reason, operation: :from_binary)}
+      end
     end
   end
 
@@ -1660,8 +1680,8 @@ defmodule Image do
               {:ok, conn} ->
                 {:cont, conn}
 
-              {:error, :closed} = error ->
-                {:halt, error}
+              {:error, :closed} ->
+                {:halt, {:error, Image.Error.wrap(:closed, operation: :write)}}
             end
           end)
 
@@ -1688,7 +1708,11 @@ defmodule Image do
     with {:ok, options} <- Options.Write.validate_options(image, options, :require_suffix) do
       {suffix, options} = Keyword.pop(options, :suffix)
       options = suffix <> loader_options(options)
-      Vimage.write_to_buffer(image, options)
+
+      case Vimage.write_to_buffer(image, options) do
+        {:ok, binary} -> {:ok, binary}
+        {:error, reason} -> {:error, Image.Error.wrap(reason, operation: :write)}
+      end
     end
   end
 
@@ -1715,20 +1739,20 @@ defmodule Image do
     |> Stream.run()
   rescue
     e in Vix.Vips.Image.Error ->
-      {:error, e.message}
+      {:error, Image.Error.wrap(e.message, operation: :write)}
   end
 
   defp write_path([image_path], image, options) do
     case Vimage.write_to_file(image, image_path, options) do
       :ok -> {:ok, image}
-      other -> other
+      {:error, reason} -> {:error, Image.Error.wrap(reason, operation: :write, path: image_path)}
     end
   end
 
   defp write_path([image_path, _open_options], image, options) do
     case Vimage.write_to_file(image, image_path, options) do
       :ok -> {:ok, image}
-      other -> other
+      {:error, reason} -> {:error, Image.Error.wrap(reason, operation: :write, path: image_path)}
     end
   end
 
@@ -1902,7 +1926,7 @@ defmodule Image do
   def write!(%Vimage{} = image, image_path, options \\ []) do
     case write(image, image_path, options) do
       {:ok, image} -> image
-      {:error, reason} -> raise Image.Error, {reason, image_path}
+      {:error, %Image.Error{} = error} -> raise error
     end
   end
 
diff --git a/lib/image/error.ex b/lib/image/error.ex
index 5d9bf751..5c1fb920 100644
--- a/lib/image/error.ex
+++ b/lib/image/error.ex
@@ -50,8 +50,6 @@ defmodule Image.Error do
 
       raise Image.Error, "free form message"
 
-      raise Image.Error, {:enoent, "/tmp/foo.jpg"}
-
   Or convert a raw `{:error, raw}` tuple coming from libvips with
   `Image.Error.wrap/2`:
 
@@ -90,24 +88,6 @@ defmodule Image.Error do
     %{struct | message: Keyword.get(opts, :message) || format_message(struct)}
   end
 
-  # ---- raise Image.Error, {:enoent, path} ---------------------------------
-
-  def exception({:enoent, path}) do
-    %__MODULE__{
-      reason: :enoent,
-      path: to_path(path),
-      message: "The image file #{inspect(path)} was not found or could not be opened"
-    }
-  end
-
-  def exception({message, path}) when is_binary(message) and is_binary(path) do
-    %__MODULE__{
-      reason: message,
-      path: path,
-      message: "#{message}: #{path}"
-    }
-  end
-
   # ---- raise Image.Error, "free form" -------------------------------------
 
   def exception(message) when is_binary(message) do
@@ -190,77 +170,32 @@ defmodule Image.Error do
     end)
   end
 
-  def wrap(:enoent, context) do
-    path = Keyword.get(context, :path)
-
-    %__MODULE__{
-      reason: :enoent,
-      path: path,
-      operation: Keyword.get(context, :operation),
-      value: Keyword.get(context, :value),
-      message: "The image file #{inspect(path)} was not found or could not be opened"
-    }
-  end
-
-  def wrap(raw, context) when is_binary(raw) do
-    operation = Keyword.get(context, :operation)
-    path = Keyword.get(context, :path)
-    value = Keyword.get(context, :value)
-
-    %__MODULE__{
+  # Build with exception/1 so that one function derives the message
+  # for every construction route.
+  def wrap(raw, context) do
+    exception(
       reason: Keyword.get(context, :reason, raw),
-      operation: operation,
-      path: path,
-      value: value,
-      message: format_libvips(operation, path, raw)
-    }
-  end
-
-  def wrap(raw, context) when is_atom(raw) do
-    operation = Keyword.get(context, :operation)
-    path = Keyword.get(context, :path)
-    value = Keyword.get(context, :value)
-
-    %__MODULE__{
-      reason: Keyword.get(context, :reason, raw),
-      operation: operation,
-      path: path,
-      value: value,
-      message: Atom.to_string(raw)
-    }
-  end
-
-  def wrap({reason_atom, _} = raw, context) when is_atom(reason_atom) do
-    %__MODULE__{
-      reason: Keyword.get(context, :reason, raw),
-      operation: Keyword.get(context, :operation),
-      path: Keyword.get(context, :path),
-      value: Keyword.get(context, :value),
-      message: inspect(raw)
-    }
-  end
-
-  def wrap(other, context) do
-    %__MODULE__{
-      reason: Keyword.get(context, :reason, other),
       operation: Keyword.get(context, :operation),
       path: Keyword.get(context, :path),
-      value: Keyword.get(context, :value),
-      message: "Image error: #{inspect(other)}"
-    }
+      value: Keyword.get(context, :value)
+    )
   end
 
   ## Internals --------------------------------------------------------------
 
-  defp to_path(path) when is_binary(path), do: path
-  defp to_path(_), do: nil
-
   defp format_message(%__MODULE__{} = error) do
     cond do
-      is_binary(error.reason) -> format_libvips(error.operation, error.path, error.reason)
-      error.reason == :enoent and error.path -> "File not found: #{error.path}"
-      is_atom(error.reason) and not is_nil(error.reason) -> Atom.to_string(error.reason)
-      true -> "Image error: #{inspect(error.reason)}"
+      is_binary(error.reason) ->
+        format_libvips(error.operation, error.path, error.reason)
+
+      error.reason == :enoent and error.path ->
+        "The image file #{inspect(error.path)} was not found or could not be opened"
+
+      is_atom(error.reason) and not is_nil(error.reason) ->
+        Atom.to_string(error.reason)
+
+      true ->
+        "Image error: #{inspect(error.reason)}"
     end
   end
 
diff --git a/test/error_coverage_test.exs b/test/error_coverage_test.exs
index aba54ce1..01391118 100644
--- a/test/error_coverage_test.exs
+++ b/test/error_coverage_test.exs
@@ -23,7 +23,9 @@ defmodule Image.ErrorCoverageTest do
       error = Error.exception(reason: :enoent, path: "/tmp/missing.jpg")
       assert error.reason == :enoent
       assert error.path == "/tmp/missing.jpg"
-      assert error.message == "File not found: /tmp/missing.jpg"
+
+      assert error.message ==
+               "The image file \"/tmp/missing.jpg\" was not found or could not be opened"
     end
 
     test "tuple reason is captured" do
@@ -42,30 +44,6 @@ defmodule Image.ErrorCoverageTest do
     end
   end
 
-  describe "exception/1 with tuples" do
-    test "{:enoent, path} with binary path" do
-      error = Error.exception({:enoent, "/tmp/missing.jpg"})
-      assert error.reason == :enoent
-      assert error.path == "/tmp/missing.jpg"
-      assert error.message =~ "was not found or could not be opened"
-      assert error.message =~ "/tmp/missing.jpg"
-    end
-
-    test "{:enoent, path} with non-binary path sets path to nil" do
-      error = Error.exception({:enoent, :not_a_path})
-      assert error.reason == :enoent
-      assert error.path == nil
-      assert error.message =~ "not_a_path"
-    end
-
-    test "{message, path} with binary message and path" do
-      error = Error.exception({"Cannot decode", "/tmp/a.jpg"})
-      assert error.reason == "Cannot decode"
-      assert error.path == "/tmp/a.jpg"
-      assert error.message == "Cannot decode: /tmp/a.jpg"
-    end
-  end
-
   describe "exception/1 with other shapes" do
     test "binary message" do
       error = Error.exception("free form message")
@@ -90,12 +68,6 @@ defmodule Image.ErrorCoverageTest do
       assert error.reason == %{some: :map}
       assert error.message == "Image error: %{some: :map}"
     end
-
-    test "a non-enoent tuple that is not {binary, binary} is wrapped" do
-      error = Error.exception({1, 2, 3})
-      assert %Error{} = error
-      assert error.message == "Image error: {1, 2, 3}"
-    end
   end
 
   describe "raising Image.Error" do
@@ -146,26 +118,17 @@ defmodule Image.ErrorCoverageTest do
     end
   end
 
-  describe "wrap/2 with raw values" do
-    test ":enoent with path context" do
-      wrapped = Error.wrap(:enoent, path: "/tmp/x.jpg", operation: :open)
-      assert wrapped.reason == :enoent
-      assert wrapped.path == "/tmp/x.jpg"
-      assert wrapped.operation == :open
-      assert wrapped.message == "The image file \"/tmp/x.jpg\" was not found or could not be opened"
-    end
-
-    test ":enoent with no context" do
-      wrapped = Error.wrap(:enoent)
-      assert wrapped.reason == :enoent
-      assert wrapped.path == nil
+  describe "wrap/2 message derivation" do
+    test "agrees with the keyword form" do
+      assert Error.wrap({:invalid_option, :crop}, operation: :thumbnail) ==
+               Error.exception(reason: {:invalid_option, :crop}, operation: :thumbnail)
     end
 
-    test "binary with operation and path" do
-      wrapped = Error.wrap("bad seek", operation: :open, path: "/tmp/x.jpg", value: :v)
-      assert wrapped.reason == "bad seek"
-      assert wrapped.message == "open /tmp/x.jpg: bad seek"
-      assert wrapped.value == :v
+    test "a :reason in the context overrides the raw value" do
+      wrapped = Error.wrap(:enoent, reason: :custom, path: "/tmp/x.jpg")
+      assert wrapped.reason == :custom
+      assert wrapped.path == "/tmp/x.jpg"
+      assert wrapped.message == "custom"
     end
 
     test "binary with operation only" do
@@ -195,7 +158,7 @@ defmodule Image.ErrorCoverageTest do
       wrapped = Error.wrap({:invalid_option, :crop}, operation: :thumbnail)
       assert wrapped.reason == {:invalid_option, :crop}
       assert wrapped.operation == :thumbnail
-      assert wrapped.message == "{:invalid_option, :crop}"
+      assert wrapped.message == "Image error: {:invalid_option, :crop}"
     end
 
     test "any other term" do
diff --git a/test/group_a_test.exs b/test/group_a_test.exs
index 4b3a3693..9f97f7fa 100644
--- a/test/group_a_test.exs
+++ b/test/group_a_test.exs
@@ -199,7 +199,8 @@ defmodule Image.GroupA.Test do
     end
 
     test ":chroma_subsampling rejected on PNG", %{hk: image} do
-      assert {:error, _} = Image.write(image, :memory, suffix: ".png", chroma_subsampling: :on)
+      assert {:error, %Image.Error{}} =
+               Image.write(image, :memory, suffix: ".png", chroma_subsampling: :on)
     end
 
     test ":lossy true/false on WebP toggles lossless wire format", %{hk: image} do
@@ -212,7 +213,7 @@ defmodule Image.GroupA.Test do
     end
 
     test ":lossy rejected on JPEG", %{hk: image} do
-      assert {:error, _} = Image.write(image, :memory, suffix: ".jpg", lossy: true)
+      assert {:error, %Image.Error{}} = Image.write(image, :memory, suffix: ".jpg", lossy: true)
     end
   end
 end
diff --git a/test/image_gap_coverage_test.exs b/test/image_gap_coverage_test.exs
index fcce5b30..66ebce3c 100644
--- a/test/image_gap_coverage_test.exs
+++ b/test/image_gap_coverage_test.exs
@@ -128,7 +128,7 @@ defmodule ImageGapCoverageTest do
       {:ok, image} = Operation.black(4, 4, bands: 5)
       path = Path.join(dir, "gap_write_stream_error.jpg")
       stream = File.stream!(path, 2048)
-      assert {:error, _reason} = Image.write(image, stream, suffix: ".jpg")
+      assert {:error, %Image.Error{}} = Image.write(image, stream, suffix: ".jpg")
     end
   end
 
diff --git a/test/image_io_coverage_test.exs b/test/image_io_coverage_test.exs
index 5c483229..59aa002a 100644
--- a/test/image_io_coverage_test.exs
+++ b/test/image_io_coverage_test.exs
@@ -42,6 +42,13 @@ defmodule Image.IoCoverageTest do
       assert {:ok, %Vimage{} = image} = Image.open(svg)
       assert Image.width(image) == 10
     end
+
+    test "an image data failure reports the entry point that was called" do
+      blob = :binary.copy(<<0xFF, 0xD8, 0xFF>>, 40)
+
+      assert {:error, %Image.Error{operation: :open, path: nil}} = Image.open(blob)
+      assert {:error, %Image.Error{operation: :from_binary, path: nil}} = Image.from_binary(blob)
+    end
   end
 
   describe "Image.open/2 from paths and streams" do
@@ -53,14 +60,19 @@ defmodule Image.IoCoverageTest do
       assert {:error, %Image.Error{reason: :enoent}} = Image.open("no/such/image.jpg")
     end
 
-    test "open!/2 raises for a non-existent path" do
-      assert_raise Image.Error, fn ->
-        Image.open!("no/such/image.jpg")
-      end
+    test "open!/2 raises the same reason open/2 returns" do
+      path = "/no/such/file.jpg"
+      {:error, returned} = Image.open(path)
+
+      raised = assert_raise Image.Error, fn -> Image.open!(path) end
+
+      assert raised.reason == returned.reason
+      assert raised.reason == :enoent
+      assert raised.path == path
     end
 
     test "returns an error for invalid open options" do
-      assert {:error, _reason} = Image.open(image_path("Kip_small.jpg"), access: :bogus)
+      assert {:error, %Image.Error{}} = Image.open(image_path("Kip_small.jpg"), access: :bogus)
     end
 
     test "opens a File.Stream created with a byte size" do
@@ -87,6 +99,13 @@ defmodule Image.IoCoverageTest do
 
       assert {:ok, %Vimage{}} = Image.open(stream)
     end
+
+    test "returns a structured error when a stream is not an image" do
+      assert {:error, %Image.Error{} = error} = Image.open(File.stream!("mix.exs", 2048))
+      assert error.operation == :open
+      assert error.path == nil
+      assert is_binary(error.reason)
+    end
   end
 
   describe "Image.from_binary/2 and from_svg/2" do
@@ -96,7 +115,7 @@ defmodule Image.IoCoverageTest do
     end
 
     test "from_binary/2 returns an error for garbage data" do
-      assert {:error, _reason} = Image.from_binary(<<1, 2, 3, 4>>)
+      assert {:error, %Image.Error{}} = Image.from_binary(<<1, 2, 3, 4>>)
     end
 
     test "from_binary!/2 returns an image" do
@@ -117,7 +136,7 @@ defmodule Image.IoCoverageTest do
     end
 
     test "from_svg/2 returns an error for invalid SVG" do
-      assert {:error, _reason} = Image.from_svg("this is not svg")
+      assert {:error, %Image.Error{}} = Image.from_svg("this is not svg")
     end
 
     test "from_svg!/2 raises for invalid SVG" do
@@ -222,7 +241,7 @@ defmodule Image.IoCoverageTest do
 
     test "returns an error for invalid write options", %{image: image, dir: dir} do
       path = Temp.path!(suffix: ".jpg", basedir: dir)
-      assert {:error, _reason} = Image.write(image, path, quality: "high")
+      assert {:error, %Image.Error{}} = Image.write(image, path, quality: "high")
     end
 
     test "write!/3 returns the image", %{image: image, dir: dir} do
@@ -237,6 +256,13 @@ defmodule Image.IoCoverageTest do
         Image.write!(image, path)
       end
     end
+
+    test "returns a structured error when the path cannot be written", %{image: image} do
+      assert {:error, %Image.Error{} = error} = Image.write(image, "/no/such/dir/out.png")
+      assert error.operation == :write
+      assert error.path == "/no/such/dir/out.png"
+      assert is_binary(error.reason)
+    end
   end
 
   describe "Image.write/3 to memory and streams" do
@@ -245,11 +271,32 @@ defmodule Image.IoCoverageTest do
       {:ok, %{image: image}}
     end
 
+    test "write!/3 raises the struct write/3 returned" do
+      image = Image.new!(2, 2)
+      {:error, returned} = Image.write(image, :memory, suffix: ".bogus")
+
+      raised = assert_raise Image.Error, fn -> Image.write!(image, :memory, suffix: ".bogus") end
+
+      assert raised.reason == returned.reason
+      assert raised.path == nil
+    end
+
     test "writes a jpeg to memory", %{image: image} do
       assert {:ok, <<0xFF, 0xD8, 0xFF, _::binary>>} =
                Image.write(image, :memory, suffix: ".jpg", quality: 50)
     end
 
+    # In order to test errors from libvips, we create a JPEG with 70_000
+    # pixels, which exceeds the JPEG dimension limit, so the encoder fails
+    # after option validation has passed
+    test "returns a structured error when the encoder fails" do
+      wide = Image.new!(70_000, 2, color: :red)
+
+      assert {:error, %Image.Error{} = error} = Image.write(wide, :memory, suffix: ".jpg")
+      assert error.operation == :write
+      assert error.path == nil
+    end
+
     test "writes a png to memory", %{image: image} do
       assert {:ok, <<0x89, "PNG", _::binary>>} = Image.write(image, :memory, suffix: ".png")
     end
diff --git a/test/stream_image_test.exs b/test/stream_image_test.exs
index a246671d..e57ef5ff 100644
--- a/test/stream_image_test.exs
+++ b/test/stream_image_test.exs
@@ -46,7 +46,7 @@ if match?({:module, _module}, Code.ensure_compiled(Plug)) do
       out_path = Temp.path!(suffix: ".jpg", basedir: dir)
       stream = File.stream!(out_path, 2048, [])
 
-      assert {:error, _reason} =
+      assert {:error, %Image.Error{}} =
                image_path("Singapore-2016-09-5887.jpg")
                |> File.stream!(2048, [])
                |> Image.open!()
@@ -70,7 +70,7 @@ if match?({:module, _module}, Code.ensure_compiled(Plug)) do
       out_path = Temp.path!(basedir: dir)
       stream = File.stream!(out_path, 2048, [])
 
-      assert {:error, _reason} =
+      assert {:error, %Image.Error{}} =
                image_path("Singapore-2016-09-5887.jpg")
                |> File.stream!(2048, [])
                |> Image.open!()