Add Expression.parse/1 and use it for validation

Multiple locations used `parse!` and converted a raised exception into
an error tuple. That's inneficient, repetitive, and violates the common
practice of avoiding exceptions for flow control.

lib/oban/cron/expression.ex

@@ -62,29 +62,40 @@ defmodule Oban.Cron.Expression do
     |> Integer.mod(7)
   end
 
-  @spec parse!(input :: binary()) :: t()
-  def parse!("@annually"), do: parse!("0 0 1 1 *")
-  def parse!("@yearly"), do: parse!("0 0 1 1 *")
-  def parse!("@monthly"), do: parse!("0 0 1 * *")
-  def parse!("@weekly"), do: parse!("0 0 * * 0")
-  def parse!("@midnight"), do: parse!("0 0 * * *")
-  def parse!("@daily"), do: parse!("0 0 * * *")
-  def parse!("@hourly"), do: parse!("0 * * * *")
-  def parse!("@reboot"), do: %__MODULE__{reboot?: true}
-
-  def parse!(input) when is_binary(input) do
+  @spec parse(input :: binary()) :: {:ok, t()} | {:error, Exception.t()}
+  def parse("@annually"), do: parse("0 0 1 1 *")
+  def parse("@yearly"), do: parse("0 0 1 1 *")
+  def parse("@monthly"), do: parse("0 0 1 * *")
+  def parse("@weekly"), do: parse("0 0 * * 0")
+  def parse("@midnight"), do: parse("0 0 * * *")
+  def parse("@daily"), do: parse("0 0 * * *")
+  def parse("@hourly"), do: parse("0 * * * *")
+  def parse("@reboot"), do: {:ok, %__MODULE__{reboot?: true}}
+
+  def parse(input) when is_binary(input) do
     case String.split(input, ~r/\s+/, trim: true, parts: 5) do
       [mip, hrp, dap, mop, wdp] ->
-        %__MODULE__{
-          minutes: parse_field(mip, 0..59),
-          hours: parse_field(hrp, 0..23),
-          days: parse_field(dap, 1..31),
-          months: mop |> trans_field(@mon_map) |> parse_field(1..12),
-          weekdays: wdp |> trans_field(@dow_map) |> parse_field(0..6)
-        }
+        {:ok,
+         %__MODULE__{
+           minutes: parse_field(mip, 0..59),
+           hours: parse_field(hrp, 0..23),
+           days: parse_field(dap, 1..31),
+           months: mop |> trans_field(@mon_map) |> parse_field(1..12),
+           weekdays: wdp |> trans_field(@dow_map) |> parse_field(0..6)
+         }}
 
       _parts ->
-        raise ArgumentError, "incorrect number of fields in expression: #{input}"
+        throw({:error, "incorrect number of fields in expression: #{input}"})
+    end
+  catch
+    {:error, message} -> {:error, %ArgumentError{message: message}}
+  end
+
+  @spec parse!(input :: binary()) :: t()
+  def parse!(input) when is_binary(input) do
+    case parse(input) do
+      {:ok, cron} -> cron
+      {:error, exception} -> raise(exception)
     end
   end
 
@@ -98,7 +109,7 @@ defmodule Oban.Cron.Expression do
       |> MapSet.new()
 
     unless MapSet.subset?(parsed, range_set) do
-      raise ArgumentError, "expression field #{field} is out of range #{inspect(range)}"
+      throw({:error, "expression field #{field} is out of range: #{inspect(range)}"})
     end
 
     parsed
@@ -126,7 +137,7 @@ defmodule Oban.Cron.Expression do
         parse_range(part, range)
 
       true ->
-        raise ArgumentError, "unrecognized cron expression: #{part}"
+        throw({:error, "unrecognized cron expression: #{part}"})
     end
   end
 
@@ -163,8 +174,10 @@ defmodule Oban.Cron.Expression do
         if rmin <= rmax do
           rmin..rmax
         else
-          raise ArgumentError,
-                "left side (#{rmin}) of a range must be less than or equal to the right side (#{rmax})"
+          throw(
+            {:error,
+             "left side (#{rmin}) of a range must be less than or equal to the right side (#{rmax})"}
+          )
         end
     end
   end

lib/oban/plugins/cron.ex

@@ -126,14 +126,7 @@ defmodule Oban.Plugins.Cron do
       {:error, %ArgumentError{message: "expression field 60 is out of range 0..59"}}
   """
   @spec parse(input :: binary()) :: {:ok, expression()} | {:error, Exception.t()}
-  def parse(input) when is_binary(input) do
-    expression = Expression.parse!(input)
-
-    {:ok, expression}
-  rescue
-    error in [ArgumentError] ->
-      {:error, error}
-  end
+  defdelegate parse(input), to: Expression
 
   @doc false
   @spec interval_to_next_minute(Time.t()) :: pos_integer()

lib/oban/validation.ex

@@ -1,6 +1,8 @@
 defmodule Oban.Validation do
   @moduledoc false
 
+  alias Oban.Cron.Expression
+
   @type validator ::
           ({atom(), term()} ->
              :ok
@@ -111,12 +113,13 @@ defmodule Oban.Validation do
   end
 
   defp validate_type(:schedule, key, val) do
-    Oban.Cron.Expression.parse!(val)
+    case Expression.parse(val) do
+      {:ok, _cron} ->
+        :ok
 
-    :ok
-  rescue
-    error in [ArgumentError] ->
-      {:error, "expected #{inspect(key)} to be a valid cron schedule, got: #{error.message}"}
+      {:error, error} ->
+        {:error, "expected #{inspect(key)} to be a valid cron schedule, got: #{error.message}"}
+    end
   end
 
   defp validate_type(:string, key, val) when not is_binary(val) do

test/oban/cron/expression_test.exs

@@ -3,6 +3,13 @@ defmodule Oban.Cron.ExpressionTest do
 
   alias Oban.Cron.Expression, as: Expr
 
+  describe "parse/1" do
+    test "parsing expressions without raising an exception" do
+      assert {:ok, _} = Expr.parse("* * * * *")
+      assert {:error, %ArgumentError{}} = Expr.parse("* * * *")
+    end
+  end
+
   describe "parse!/1" do
     property "expressions with literals, wildcards, ranges, steps and lists are parsed" do
       check all minutes <- minutes(),
@@ -43,36 +50,6 @@ defmodule Oban.Cron.ExpressionTest do
         assert_raise ArgumentError, fn -> Expr.parse!(expression) end
       end
     end
-
-    test "parsing range expressions where left side is greater than the right side fails" do
-      expressions = [
-        "9-5 * * * *",
-        "* 4-3 * * *",
-        "* * 27-2 * *",
-        "* * * 11-1 *",
-        "* * * * 6-0",
-        "* * * * SAT-SUN"
-      ]
-
-      for expression <- expressions do
-        assert_raise ArgumentError, fn -> Expr.parse!(expression) end
-      end
-    end
-
-    test "parsing non-standard expressions" do
-      assert Expr.parse!("0 0 1 1 *") == Expr.parse!("@annually")
-      assert Expr.parse!("0 0 1 1 *") == Expr.parse!("@yearly")
-      assert Expr.parse!("0 0 1 * *") == Expr.parse!("@monthly")
-      assert Expr.parse!("0 0 * * 0") == Expr.parse!("@weekly")
-      assert Expr.parse!("0 0 * * *") == Expr.parse!("@midnight")
-      assert Expr.parse!("0 0 * * *") == Expr.parse!("@daily")
-      assert Expr.parse!("0 * * * *") == Expr.parse!("@hourly")
-    end
-
-    test "parsing non-standard weekday ranges" do
-      assert MapSet.new([1, 2]) == Expr.parse!("* * * * MON-TUE").weekdays
-      assert MapSet.new([1, 2, 3, 4, 5]) == Expr.parse!("* * * * MON-FRI").weekdays
-    end
   end
 
   describe "now?/2" do
@@ -97,9 +74,7 @@ defmodule Oban.Cron.ExpressionTest do
     end
 
     test "the @reboot special expression evaluates to now" do
-      assert "@reboot"
-             |> Expr.parse!()
-             |> Expr.now?()
+      assert "@reboot" |> Expr.parse!() |> Expr.now?()
     end
 
     test "literal days of the week match the current datetime" do

test/oban/plugins/cron_test.exs

@@ -57,13 +57,11 @@ defmodule Oban.Plugins.CronTest do
 
   describe "parse/1" do
     test "returning valid expressions in a success tuple" do
-      {:ok, %Expression{}} = Cron.parse("0 0 1 * *")
+      assert {:ok, %Expression{}} = Cron.parse("0 0 1 * *")
     end
 
     test "wrapping invalid expressions in an error tuple" do
-      {:error, %ArgumentError{message: message}} = Cron.parse("60 24 13 * *")
-
-      assert message =~ "expression field 60 is out of range 0..59"
+      assert {:error, %ArgumentError{}} = Cron.parse("60 24 13 * *")
     end
   end