Fix strip_metadata/minimize_file_size silent no-op on file-path writes

lib/image.ex

@@ -886,9 +886,18 @@ defmodule Image do
   end
 
   defp loader_options(options) do
-    "[" <> Enum.map_join(options, ",", fn {k, v} -> "#{k}=#{v}" end) <> "]"
+    "[" <> Enum.map_join(options, ",", &loader_option/1) <> "]"
   end
 
+  # Flag-valued options are passed to libvips as a list of flag atoms.
+  # In a filename suffix string flags are written by name joined with `:`,
+  # e.g. `keep=VIPS_FOREIGN_KEEP_EXIF:VIPS_FOREIGN_KEEP_XMP`.
+  defp loader_option({key, value}) when is_list(value) do
+    "#{key}=" <> Enum.map_join(value, ":", &to_string/1)
+  end
+
+  defp loader_option({key, value}), do: "#{key}=#{value}"
+
   @doc """
   Opens an image file for image processing
   returning an image or raising an exception.

lib/image/options/write.ex

@@ -176,7 +176,7 @@ defmodule Image.Options.Write do
     options =
       options
       |> Keyword.delete(:strip_metadata)
-      |> Keyword.put(:strip, strip?)
+      |> put_strip_metadata(strip?)
 
     {:cont, options}
   end
@@ -220,7 +220,7 @@ defmodule Image.Options.Write do
     options =
       options
       |> Keyword.delete(:minimize_file_size)
-      |> Keyword.put(:strip, true)
+      |> put_strip_metadata(true)
       |> Keyword.put(:"optimize-coding", true)
       |> Keyword.put(:interlace, true)
       |> Keyword.put(:"optimize-scans", true)
@@ -235,7 +235,7 @@ defmodule Image.Options.Write do
     options =
       options
       |> Keyword.delete(:minimize_file_size)
-      |> Keyword.put(:strip, true)
+      |> put_strip_metadata(true)
       |> Keyword.put(:palette, true)
 
     {:cont, options}
@@ -247,7 +247,7 @@ defmodule Image.Options.Write do
       options
       |> Keyword.delete(:minimize_file_size)
       |> Keyword.put(:"min-size", true)
-      |> Keyword.put(:strip, true)
+      |> put_strip_metadata(true)
       |> Keyword.put(:mixed, true)
 
     {:cont, options}
@@ -259,7 +259,7 @@ defmodule Image.Options.Write do
     options =
       options
       |> Keyword.delete(:minimize_file_size)
-      |> Keyword.put(:strip, true)
+      |> put_strip_metadata(true)
 
     {:cont, options}
   end
@@ -425,6 +425,33 @@ defmodule Image.Options.Write do
   # Range 0..6
   defp conform_effort(effort, ".webp"), do: round(effort / 10 * 6)
 
+  # libvips 8.15 deprecated the boolean `strip` save option in favour of the
+  # `keep` flag. Vix removes deprecated properties from the operation it builds,
+  # so passing `strip: true` to `Vix.Vips.Image.write_to_file/3` is silently
+  # ignored on libvips >= 8.15 and no metadata is stripped. The `:memory`, stream
+  # and `Plug.Conn` targets escaped the bug because they serialise the options
+  # into a libvips filename suffix string (e.g. `".jpg[strip=true]"`) which is
+  # parsed by libvips' own option parser, and that parser still honours the
+  # deprecated `strip` alias.
+  #
+  # Emit the modern `keep` flag where it exists, so `strip: true` is converted to
+  # `keep: [:VIPS_FOREIGN_KEEP_NONE]`, and fall back to `strip` on libvips < 8.15
+  # where `keep` does not exist.
+  defp put_strip_metadata(options, false), do: options
+
+  defp put_strip_metadata(options, true) do
+    if keep_option_supported?() do
+      Keyword.put(options, :keep, [:VIPS_FOREIGN_KEEP_NONE])
+    else
+      Keyword.put(options, :strip, true)
+    end
+  end
+
+  defp keep_option_supported? do
+    {:ok, version} = Image.vips_version()
+    Version.compare(version, "8.15.0") != :lt
+  end
+
   defp image_type_from("", "") do
     {:error,
      %Image.Error{message: "Cannot determine image type", reason: "Cannot determine image type"}}

test/strip_metadata_test.exs

@@ -0,0 +1,83 @@
+defmodule Image.StripMetadataTest do
+  use ExUnit.Case, async: true
+  import Image.TestSupport
+
+  alias Vix.Vips.Image, as: Vimage
+
+  # Counts the number of EXIF/XMP metadata fields present on the image
+  # at `path` after reloading it from disk.
+  defp metadata_field_count(path) do
+    {:ok, image} = Image.open(path, access: :random)
+    {:ok, names} = Vimage.header_field_names(image)
+    Enum.count(names, &(&1 =~ ~r/exif|xmp/i))
+  end
+
+  defp out(name) do
+    unique = System.unique_integer([:positive])
+    path = Path.join(System.tmp_dir!(), "image_strip_test_#{unique}_#{name}")
+    on_exit(fn -> File.rm(path) end)
+    path
+  end
+
+  describe "strip_metadata: true when writing to a file path" do
+    test "removes EXIF/XMP metadata from a JPEG" do
+      {:ok, image} = Image.open(image_path("Kip_small.jpg"), access: :random)
+
+      kept = out("keep.jpg")
+      stripped = out("strip.jpg")
+
+      assert {:ok, _} = Image.write(image, kept, strip_metadata: false)
+      assert {:ok, _} = Image.write(image, stripped, strip_metadata: true)
+
+      # The source image genuinely has metadata, and not stripping retains it.
+      assert metadata_field_count(kept) > 0
+      # Stripping on a file path must actually remove it.
+      assert metadata_field_count(stripped) == 0
+      assert File.stat!(stripped).size < File.stat!(kept).size
+    end
+
+    test "removes EXIF/XMP metadata from a PNG" do
+      {:ok, image} = Image.open(image_path("jose.png"), access: :random)
+
+      kept = out("keep.png")
+      stripped = out("strip.png")
+
+      assert {:ok, _} = Image.write(image, kept, strip_metadata: false)
+      assert {:ok, _} = Image.write(image, stripped, strip_metadata: true)
+
+      assert metadata_field_count(kept) > 0
+      assert metadata_field_count(stripped) == 0
+    end
+  end
+
+  describe "minimize_file_size: true when writing to a file path" do
+    test "removes EXIF/XMP metadata from a JPEG" do
+      {:ok, image} = Image.open(image_path("Kip_small.jpg"), access: :random)
+
+      stripped = out("min.jpg")
+      assert {:ok, _} = Image.write(image, stripped, minimize_file_size: true)
+      assert metadata_field_count(stripped) == 0
+    end
+
+    test "removes EXIF/XMP metadata from a PNG" do
+      {:ok, image} = Image.open(image_path("jose.png"), access: :random)
+
+      stripped = out("min.png")
+      assert {:ok, _} = Image.write(image, stripped, minimize_file_size: true)
+      assert metadata_field_count(stripped) == 0
+    end
+  end
+
+  describe "strip_metadata when writing to :memory" do
+    # already worked, guards against regression
+
+    test "removes EXIF/XMP metadata from a JPEG buffer" do
+      {:ok, image} = Image.open(image_path("Kip_small.jpg"), access: :random)
+
+      {:ok, kept} = Image.write(image, :memory, suffix: ".jpg", strip_metadata: false)
+      {:ok, stripped} = Image.write(image, :memory, suffix: ".jpg", strip_metadata: true)
+
+      assert byte_size(stripped) < byte_size(kept)
+    end
+  end
+end