From 24397bb8e5c07c1a2b71a1f209ad844e74472edf Mon Sep 17 00:00:00 2001 From: Matthew Peck Date: Fri, 28 Oct 2016 13:17:26 -0400 Subject: [PATCH] Improve error messaging for command execution --- lib/cog/audit_message.ex | 4 ++++ lib/cog/command/command_resolver.ex | 12 ++++++++---- lib/cog/command/pipeline/executor.ex | 22 +++++++++++++--------- lib/cog/error_response.ex | 12 ++++++++++-- lib/cog/relay/relays.ex | 12 ++++++++---- lib/cog/relay/tracker.ex | 26 +++++++++++++++++--------- lib/cog/repository/bundles.ex | 21 +++++++++++++++++++-- mix.lock | 2 +- test/cog/relay/tracker_test.exs | 15 ++++++++++----- 9 files changed, 90 insertions(+), 36 deletions(-) diff --git a/lib/cog/audit_message.ex b/lib/cog/audit_message.ex index bdbc96ab..3f8ac604 100644 --- a/lib/cog/audit_message.ex +++ b/lib/cog/audit_message.ex @@ -19,6 +19,10 @@ defmodule Cog.AuditMessage do do: "User #{request.sender.handle} denied access to '#{current_invocation}'" def render({:no_relays, bundle_name}, _request), do: ErrorResponse.render({:no_relays, bundle_name}) # Uses same user message for now + def render({:no_relay_group, bundle_name}, _request), + do: ErrorResponse.render({:no_relay_group, bundle_name}) + def render({:no_enabled_relays, bundle_name}, _request), + do: ErrorResponse.render({:no_relay_group, bundle_name}) def render({:disabled_bundle, %Bundle{name: name}}, _request), do: "The #{name} bundle is currently disabled" def render({:command_error, response}, _request), diff --git a/lib/cog/command/command_resolver.ex b/lib/cog/command/command_resolver.ex index 1d5ea3b4..a6eb64a2 100644 --- a/lib/cog/command/command_resolver.ex +++ b/lib/cog/command/command_resolver.ex @@ -23,9 +23,13 @@ defmodule Cog.Command.CommandResolver do %SiteCommandAlias{pipeline: pipeline} -> {:pipeline, pipeline} :not_found -> - :not_found + {:error, :not_found} + {:not_enabled, bundle_name} -> + {:error, {:not_enabled, bundle_name}} + {:not_in_bundle, bundle_name} -> + {:error, {:not_in_bundle, bundle_name}} {:ambiguous, names} -> - {:ambiguous, names} + {:error, {:ambiguous, names}} end end end @@ -51,13 +55,13 @@ defmodule Cog.Command.CommandResolver do case Map.get(enabled_bundles, bundle_name) do nil -> # No enabled version of the requested bundle was found - :not_found + {:not_enabled, bundle_name} version -> case Bundles.command_for_bundle_version(command_name, bundle_name, version) do nil -> # The enabled bundle version does not contain the # requested command - :not_found + {:not_in_bundle, bundle_name} result -> {:ok, rules} = Rules.rules_for_command(result.command) Cog.Command.Pipeline.ParserMeta.new(result.command.bundle.name, diff --git a/lib/cog/command/pipeline/executor.ex b/lib/cog/command/pipeline/executor.ex index 6c15083a..b696c88d 100644 --- a/lib/cog/command/pipeline/executor.ex +++ b/lib/cog/command/pipeline/executor.ex @@ -228,9 +228,9 @@ defmodule Cog.Command.Pipeline.Executor do # fail_pipeline_with_error({:disabled_bundle, bundle}, state) # case assign_relay(relays, bundle_name, version) do - {nil, _} -> - fail_pipeline_with_error({:no_relays, bundle_name}, state) - {relay, relays} -> + {:error, reason} -> + fail_pipeline_with_error({reason, bundle_name}, state) + {:ok, {relay, relays}} -> topic = "/bot/commands/#{relay}/#{bundle_name}/#{command_name}" reply_to_topic = "#{state.topic}/reply" req = request_for_plan(current_plan, request, user, reply_to_topic, state.service_token) @@ -671,19 +671,23 @@ defmodule Cog.Command.Pipeline.Executor do # No assignment so let's pick one nil -> case Relays.pick_one(bundle_name, bundle_version) do - # No relays available - nil -> - {nil, relays} # Store the selected relay in the relay cache - relay -> - {relay, Map.put(relays, bundle_name, relay)} + {:ok, relay} -> + {:ok, {relay, Map.put(relays, bundle_name, relay)}} + error -> + # Query DB to clarify error before reporting to the user + if Cog.Repository.Bundles.assigned_to_group?(bundle_name) do + error + else + {:error, :no_relay_group} + end end # Relay was previously assigned relay -> # Is the bundle still available on the relay? If not, remove the current assignment from the cache # and select a new relay if Relays.relay_available?(relay, bundle_name, bundle_version) do - {relay, relays} + {:ok, {relay, relays}} else relays = Map.delete(relays, bundle_name) assign_relay(relays, bundle_name, bundle_version) diff --git a/lib/cog/error_response.ex b/lib/cog/error_response.ex index 2912e8cd..be8b4ebd 100644 --- a/lib/cog/error_response.ex +++ b/lib/cog/error_response.ex @@ -16,8 +16,16 @@ defmodule Cog.ErrorResponse do perms = Enum.map(Ast.Rule.permissions_used(rule), &("'#{&1}'")) |> Enum.join(", ") "Sorry, you aren't allowed to execute '#{current_invocation}' :(\n You will need at least one of the following permissions to run this command: #{perms}." end - def render({:no_relays, bundle_name}), # TODO: Add version, too? - do: "No Cog Relays supporting the `#{bundle_name}` bundle are currently online" + def render({:no_relays, bundle_name}) do # TODO: Add version, too? + "No Cog Relays supporting the `#{bundle_name}` bundle are currently online. " <> + "If you just assigned the bundle to a relay group you will need to wait for the relay refresh interval to pick up the new bundle." + end + def render({:no_relay_group, bundle_name}), + do: "Bundle '#{bundle_name}' is not assigned to a relay group. Assign the bundle to a relay group and try again." + def render({:no_enabled_relays, bundle_name}) do # TODO: Should we print the list of disabled relays? + "No Cog Relays supporting the `#{bundle_name}` bundle are currently available. " <> + "There are one or more relays serving the bundle, but they are disabled. Enable an appropriate relay and try again." + end def render({:disabled_bundle, %Bundle{name: name}}), do: "The #{name} bundle is currently disabled" def render({:timeout, full_command_name}), diff --git a/lib/cog/relay/relays.ex b/lib/cog/relay/relays.ex index 62018317..42f04341 100644 --- a/lib/cog/relay/relays.ex +++ b/lib/cog/relay/relays.ex @@ -108,8 +108,12 @@ defmodule Cog.Relay.Relays do tracker = remove_relay(state.tracker, relay.id) {:reply, :ok, %{state | tracker: tracker}} end - def handle_call({:relays_running, bundle_name, version} , _from, state), - do: {:reply, Tracker.relays(state.tracker, bundle_name, version), state} + def handle_call({:relays_running, bundle_name, version} , _from, state) do + case Tracker.relays(state.tracker, bundle_name, version) do + {:ok, relays} -> {:reply, relays, state} + {:error, _} -> {:reply, [], state} + end + end def handle_info({:publish, @relays_discovery_topic, message}, state) do try do @@ -202,8 +206,8 @@ defmodule Cog.Relay.Relays do defp random_relay(tracker, bundle, version) do case Tracker.relays(tracker, bundle, version) do - [] -> nil - relays -> Enum.random(relays) + {:ok, relays} -> {:ok, Enum.random(relays)} + error -> error end end diff --git a/lib/cog/relay/tracker.ex b/lib/cog/relay/tracker.ex index b7257ce1..d212879e 100644 --- a/lib/cog/relay/tracker.ex +++ b/lib/cog/relay/tracker.ex @@ -120,15 +120,19 @@ defmodule Cog.Relay.Tracker do end @doc """ - Return a list of relays serving the specified bundle version. If the bundle is - disabled, return an empty list. + Return a tuple with the list of relays serving the bundle or an error and + a reason. """ - @spec relays(t, bundle_name, version) :: [String.t] + @spec relays(t, bundle_name, version) :: {:ok, [String.t]} | {:error, atom()} def relays(tracker, bundle_name, bundle_version) when is_binary(bundle_name) do - tracker.map - |> Map.get({bundle_name, bundle_version}, MapSet.new) - |> MapSet.difference(tracker.disabled) - |> MapSet.to_list + relays = Map.get(tracker.map, {bundle_name, bundle_version}, MapSet.new) + enabled_relays = MapSet.difference(relays, tracker.disabled) + + case {MapSet.to_list(relays), MapSet.to_list(enabled_relays)} do + {[], []} -> {:error, :no_relays} + {_, []} -> {:error, :no_enabled_relays} + {_, relays} -> {:ok, relays} + end end @doc """ @@ -137,8 +141,12 @@ defmodule Cog.Relay.Tracker do """ @spec is_bundle_available?(t, relay_id, bundle_name, version) :: boolean() def is_bundle_available?(tracker, relay, bundle_name, bundle_version) do - available_relays = relays(tracker, bundle_name, bundle_version) - Enum.member?(available_relays, relay) + case relays(tracker, bundle_name, bundle_version) do + {:ok, available_relays} -> + Enum.member?(available_relays, relay) + _ -> + false + end end defp in_tracker?(tracker, relay_id) do diff --git a/lib/cog/repository/bundles.ex b/lib/cog/repository/bundles.ex index b480e008..428e2395 100644 --- a/lib/cog/repository/bundles.ex +++ b/lib/cog/repository/bundles.ex @@ -6,7 +6,7 @@ defmodule Cog.Repository.Bundles do require Logger alias Cog.Repo - alias Cog.Models.{Bundle, BundleVersion, BundleDynamicConfig, CommandVersion, Rule} + alias Cog.Models.{Bundle, BundleVersion, BundleDynamicConfig, CommandVersion, Rule, RelayGroupAssignment} alias Cog.Repository.Rules alias Cog.Queries alias Cog.BundleRegistry @@ -26,8 +26,9 @@ defmodule Cog.Repository.Bundles do commands: [command: :bundle]]] @bundle_dynamic_config_preloads [:bundle] + @embedded_bundle Cog.Util.Misc.embedded_bundle @reserved_bundle_names [ - Cog.Util.Misc.embedded_bundle, + @embedded_bundle, Cog.Util.Misc.site_namespace, "user", # a bundle named "user" would break alias resolution "cog" # we're going to squat on this for now to prevent potential confusion @@ -149,6 +150,22 @@ defmodule Cog.Repository.Bundles do end end + def assigned_to_group?(name) when name == @embedded_bundle, + # If it's the internal bundle we'll return true here since it doesn't + # run on a relay. + do: true + def assigned_to_group?(name) do + # If there are relay groups associated with the bundle the query will return + # true, otherwise it will return nil + query = from(b in Bundle, + join: r in RelayGroupAssignment, on: b.id == r.bundle_id, + where: b.name == ^name, + select: true) + + # Make sure that we return a boolean + if Repo.one(query), do: true, else: false + end + @doc """ Delete a bundle version, or an entire bundle (i.e., all versions for that bundle). You can only delete versions that are not currently diff --git a/mix.lock b/mix.lock index 564fef68..dbd53508 100644 --- a/mix.lock +++ b/mix.lock @@ -54,7 +54,7 @@ "phoenix_html": {:hex, :phoenix_html, "2.7.0", "19e12e2044340c2e43df206a06d059677c59ea1868bd1c35165438d592cd420b", [:mix], [{:plug, "~> 1.0", [hex: :plug, optional: false]}]}, "phoenix_live_reload": {:hex, :phoenix_live_reload, "1.0.5", "829218c4152ba1e9848e2bf8e161fcde6b4ec679a516259442561d21fde68d0b", [:mix], [{:fs, "~> 0.9.1", [hex: :fs, optional: false]}, {:phoenix, "~> 1.0 or ~> 1.2-rc", [hex: :phoenix, optional: false]}]}, "phoenix_pubsub": {:hex, :phoenix_pubsub, "1.0.1", "c10ddf6237007c804bf2b8f3c4d5b99009b42eca3a0dfac04ea2d8001186056a", [:mix], []}, - "piper": {:git, "https://github.com/operable/piper.git", "93a0034334b237cafd96cebb51257a7c3f4c14b1", []}, + "piper": {:git, "https://github.com/operable/piper.git", "22ce7b6eecfdb51c7453b31a3c0f54d3484552c4", []}, "plug": {:hex, :plug, "1.2.2", "cfbda521b54c92ab8ddffb173fbaabed8d8fc94bec07cd9bb58a84c1c501b0bd", [:mix], [{:cowboy, "~> 1.0", [hex: :cowboy, optional: true]}, {:mime, "~> 1.0", [hex: :mime, optional: false]}]}, "poison": {:hex, :poison, "2.2.0", "4763b69a8a77bd77d26f477d196428b741261a761257ff1cf92753a0d4d24a63", [:mix], []}, "poolboy": {:hex, :poolboy, "1.5.1", "6b46163901cfd0a1b43d692657ed9d7e599853b3b21b95ae5ae0a777cf9b6ca8", [:rebar], []}, diff --git a/test/cog/relay/tracker_test.exs b/test/cog/relay/tracker_test.exs index b3866bae..cbea6a63 100644 --- a/test/cog/relay/tracker_test.exs +++ b/test/cog/relay/tracker_test.exs @@ -90,12 +90,17 @@ defmodule Cog.Relay.Tracker.Test do defp bundle_spec(name, version), do: {name, version} - defp assert_missing({name, version}, tracker), - do: assert [] = Tracker.relays(tracker, name, version) + defp assert_missing(bundle_spec, tracker), + do: assert [] = actual_relays(tracker, bundle_spec) - defp assert_relays(tracker, {name, version}, expected_relays) do - actual_relays = Tracker.relays(tracker, name, version) - assert Enum.sort(expected_relays) == Enum.sort(actual_relays) + defp assert_relays(tracker, bundle_spec, expected_relays), + do: assert Enum.sort(expected_relays) == Enum.sort(actual_relays(tracker, bundle_spec)) + + defp actual_relays(tracker, {name, version}) do + case Tracker.relays(tracker, name, version) do + {:ok, relays} -> relays + {:error, _} -> [] + end end end