actual fix 👀 + tests

lib/sweet_xml.ex

@@ -275,39 +275,12 @@ defmodule SweetXml do
   """
   @spec parse(doc, opts :: list) :: xmlElement
   def parse(doc, opts \\ []) do
-    dtd_arg = :proplists.get_value(:dtd, opts, :all)
-    opts = :proplists.delete(:dtd, opts)
-
-    {opts, do_after} = case :proplists.split(opts, [:rules]) do
-      {[[]], opts} ->
-        ets = :ets.new(nil, [])
-        opts = opts ++ SweetXml.Options.handle_dtd(dtd_arg).(ets)
-        {opts, {:cleanup, ets}}
-
-      {[[{:rules, ets}] = rules], opts} ->
-        opts = rules ++ opts ++ SweetXml.Options.handle_dtd(dtd_arg).(ets)
-        {opts, :not_ours}
-
-      {[[{:rules, _read_fun, _write_fun, _ets}]], opts} ->
-        ets = :ets.new(nil, [])
-        dtd_opts = SweetXml.Options.handle_dtd(dtd_arg).(ets)
-        _ = case :proplists.split(dtd_opts, [:rules]) do
-          {[], _opts} -> :ok
-          {[_], _opts} ->
-            require Logger
-            _ = Logger.warn("rules opt will be overriden because of the dtd option")
-            :warned
-        end
-        {opts ++ dtd_opts, {:cleanup, ets}}
-    end
+    {opts, do_after} = SweetXml.Options.set_up(opts, RuntimeError)
 
     try do
       do_parse(doc, opts)
     after
-      case do_after do
-        :not_ours -> :ok
-        {:cleanup, ets} -> :ets.delete(ets)
-      end
+      _ = SweetXml.Options.clean_up(do_after)
     end
   end
 
@@ -487,40 +460,11 @@ defmodule SweetXml do
     stream([IO.iodata_to_binary(doc)], options_callback)
   end
   def stream(doc, options_callback) do
-    do_after_go = fn
-      :not_ours -> :ok
-      {:cleanup, ets} -> :ets.delete(ets)
-    end
     Stream.resource fn ->
       {parent, ref} = waiter = {self(), make_ref()}
       opts = options_callback.(fn e -> send(parent, {:event, ref, e}) end)
 
-      dtd_arg = :proplists.get_value(:dtd, opts, :all)
-      opts = :proplists.delete(:dtd, opts)
-
-      {opts, do_after} = case :proplists.split(opts, [:rules]) do
-        {[[]], opts} ->
-          ets = :ets.new(nil, [:public])
-          opts = opts ++ SweetXml.Options.handle_dtd(dtd_arg).(ets)
-          {opts, {:cleanup, ets}}
-
-        {[[{:rules, ets}] = rules], opts} ->
-          opts = rules ++ opts ++ SweetXml.Options.handle_dtd(dtd_arg).(ets)
-          {opts, :not_ours}
-
-        {[[{:rules, _read_fun, _write_fun, _ets}] = rules], opts} ->
-          ets = :ets.new(nil, [:public])
-          dtd_opts = SweetXml.Options.handle_dtd(dtd_arg).(ets)
-          # If we don't use the `:rules` option for the DTDs, then we can keep the original one
-          extra_opts = case :proplists.split(dtd_opts, [:rules]) do
-            {[], _opts} -> rules
-            {[[{:rules, _, _, _}] = extra], _opts} ->
-              require Logger
-              _ = Logger.warn("rules opt will be overriden because of the dtd option")
-              extra
-          end
-          {opts ++ dtd_opts ++ extra_opts, {:cleanup, ets}}
-      end
+      {opts, do_after} = SweetXml.Options.set_up(opts, RuntimeError)
 
       pid = spawn_link fn -> :xmerl_scan.string('', opts ++ continuation_opts(doc, waiter)) end
       {ref, pid, Process.monitor(pid), do_after}
@@ -536,12 +480,12 @@ defmodule SweetXml do
       end
     end, fn
       {:parse_ended, do_after} ->
-        do_after_go.(do_after)
+      _ = SweetXml.Options.clean_up(do_after)
         :ok
 
       {ref, pid, monref, do_after} ->
         _ = Process.demonitor(monref)
-        do_after_go.(do_after)
+        _ = SweetXml.Options.clean_up(do_after)
         flush_halt(pid, ref)
     end
   end
@@ -559,39 +503,11 @@ defmodule SweetXml do
     stream([IO.iodata_to_binary(doc)], options_callback)
   end
   def stream!(doc, options_callback) do
-    do_after_go = fn
-      :not_ours -> :ok
-      {:cleanup, ets} -> :ets.delete(ets)
-    end
     Stream.resource fn ->
       {parent, ref} = waiter = {self(), make_ref()}
       opts = options_callback.(fn e -> send(parent, {:event, ref, e}) end)
 
-      dtd_arg = :proplists.get_value(:dtd, opts, :all)
-      opts = :proplists.delete(:dtd, opts)
-
-      {opts, do_after} = case :proplists.split(opts, [:rules]) do
-        {[[]], opts} ->
-          ets = :ets.new(nil, [:public])
-          opts = opts ++ SweetXml.Options.handle_dtd(dtd_arg, SweetXml.DTDError).(ets)
-          {opts, {:cleanup, ets}}
-
-        {[[{:rules, ets}] = rules], opts} ->
-          opts = rules ++ opts ++ SweetXml.Options.handle_dtd(dtd_arg, SweetXml.DTDError).(ets)
-          {opts, :not_ours}
-
-        {[[{:rules, _read_fun, _write_fun, _ets}]], opts} ->
-          ets = :ets.new(nil, [:public])
-          dtd_opts = SweetXml.Options.handle_dtd(dtd_arg, SweetXml.DTDError).(ets)
-          _ = case :proplists.split(dtd_opts, [:rules]) do
-            {[], _opts} -> :ok
-            {[_], _opts} ->
-              require Logger
-              _ = Logger.warn("rules opt will be overriden because of the dtd option")
-              :warned
-          end
-          {opts ++ dtd_opts, {:cleanup, ets}}
-      end
+      {opts, do_after} = SweetXml.Options.set_up(opts, SweetXml.DTDError)
 
       {pid, monref} = spawn_monitor(fn -> :xmerl_scan.string('', opts ++ continuation_opts(doc, waiter)) end)
       {ref, pid, monref, do_after}
@@ -611,20 +527,20 @@ defmodule SweetXml do
       end
     end, fn
       {:parse_ended, do_after} ->
-        do_after_go.(do_after)
+        _ = SweetXml.Options.clean_up(do_after)
         :ok
 
       {:fatal, error, do_after} ->
-        do_after_go.(do_after)
+        _ = SweetXml.Options.clean_up(do_after)
         raise SweetXml.XmerlFatal, error
 
       {:error, {exception, stacktrace}, do_after} ->
-        do_after_go.(do_after)
+        _ = SweetXml.Options.clean_up(do_after)
         reraise(exception, stacktrace)
 
       {ref, pid, monref, do_after} ->
         _ = Process.demonitor(monref)
-        do_after_go.(do_after)
+        _ = SweetXml.Options.clean_up(do_after)
         flush_halt(pid, ref)
     end
   end

lib/sweet_xml/options.ex

@@ -4,6 +4,8 @@ defmodule SweetXml.Options do
 
   @moduledoc false
 
+  # The dtd exception_module exists for backward compatibility.
+
   def handle_dtd(dtd_option, exception_module \\ RuntimeError)
 
   def handle_dtd(:all, _exception_module) do
@@ -66,4 +68,51 @@ defmodule SweetXml.Options do
       [{:rules, read, write, ets}]
     end
   end
+
+  # This is kind of hard to follow :/
+  # Basically, when we are not given any :rules option, then we do whatever.
+  # When we are given a simple :rules/1 option, we assume it is an ets table, and we reuse it for the DTDs handling.
+  # When we are given a :rules/3 option with function callbacks, we create our own table,
+  # we set up the DTD handling opts, and then we check what is our expected behavior.
+  # If we actually use the newly created ets (therefore creating a new :rules/3 option),
+  # then we simply discard the user's option.
+  # If we don't, we keep what the user gave us.
+  # In both case, we add a rules/1 option because if we don't xmerl will do it.
+  def set_up(opts, dtd_exception_module) do
+    dtd_arg = :proplists.get_value(:dtd, opts, :all)
+    opts = :proplists.delete(:dtd, opts)
+
+    ret = {_opts, _do_after} = case :proplists.split(opts, [:rules]) do
+      {[[]], opts} ->
+        ets = :ets.new(nil, [:public])
+        opts = opts ++ SweetXml.Options.handle_dtd(dtd_arg, dtd_exception_module).(ets) ++ [rules: ets]
+        {opts, {:cleanup, ets}}
+
+      # The only time we don't add a :rules/1
+      {[[{:rules, ets}] = rules], opts} ->
+        opts = rules ++ opts ++ SweetXml.Options.handle_dtd(dtd_arg, dtd_exception_module).(ets)
+        {opts, :not_ours}
+
+      {[[{:rules, _read_fun, _write_fun, _ets}] = rules], opts} ->
+        ets = :ets.new(nil, [:public])
+        dtd_opts = SweetXml.Options.handle_dtd(dtd_arg, dtd_exception_module).(ets)
+        # If we don't use the `:rules/3` option for the DTDs, then we can keep the original one.
+        case :proplists.split(dtd_opts, [:rules]) do
+          {[[]], _opts} ->
+            opts = opts ++ dtd_opts ++ rules ++ [rules: ets]
+            {opts, {:cleanup, ets}}
+
+          {[[_]], _opts} ->
+            require Logger
+            _ = Logger.warn("rules opt will be overriden because of the dtd option")
+            opts = opts ++ dtd_opts ++ [rules: ets]
+            {opts, {:cleanup, ets}}
+        end
+    end
+
+    ret
+  end
+
+  def clean_up(:not_ours), do: :ok
+  def clean_up({:cleanup, ets}), do: (:ets.delete(ets) ; :ok)
 end

test/issue_41_test.exs

@@ -0,0 +1,43 @@
+defmodule Issue41Test do
+  use ExUnit.Case, async: false
+
+  test "warns on rules override: `dtd: :none`" do
+    import ExUnit.CaptureLog
+    rules = {:rules, fn _cntxt, _nm, _stt -> :undefined end, fn _cntxt, _nm, _vl, stt -> stt end, nil}
+    # Warning ! Sensitive to async tests.
+    assert capture_log(fn ->
+      SweetXml.parse("<tag></tag>", [rules, dtd: :none])
+    end) =~ "rules opt will be overriden because of the dtd option"
+  end
+
+  test "keeps our rules/1" do
+    tid = :ets.new(nil, [])
+    _ = SweetXml.parse("<tag></tag>", [{:rules, tid}])
+    assert true = :ets.delete(tid)
+  end
+
+  test "keeps provided rules/3 with no strict dtd" do
+    tid = :ets.new(nil, [])
+    write_fun = fn cntxt, nm, vl, stt ->
+      _ = :ets.insert(tid, {{cntxt, nm}, vl})
+      stt
+    end
+
+    read_fun = fn cntxt, nm, _stt ->
+      case :ets.lookup(tid, {cntxt, nm}) do
+        [] -> :undefined
+        [{_, vl}] -> vl
+      end
+    end
+
+    rules = {:rules, read_fun, write_fun, tid}
+    xml = ~S"""
+    <!DOCTYPE foo [ <!ELEMENT foo ANY > ] >
+    <tag></tag>
+    """
+    _ = SweetXml.parse(xml, [rules, dtd: :internal_only])
+    residue = :ets.lookup(tid, {:elem_def, :foo})
+    true = :ets.delete(tid)
+    assert [_] = residue
+  end
+end

test/sweet_xml_test.exs

@@ -1,5 +1,5 @@
 defmodule SweetXmlTest do
-  use ExUnit.Case, async: true
+  use ExUnit.Case, async: false
   doctest SweetXml
 
   import SweetXml