RM API: provide useful feedback when trigger action is malformed

CHANGELOG.md

@@ -4,6 +4,11 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
 
+## [1.1.0-rc.0] - Unreleased
+### Fixed
+- [astarte_realm_management_api] Provide detailed feedback when a trigger action
+ is malformed. Fix [#748](https://github.com/astarte-platform/astarte/issues/748).
+
 ## [1.1.0-alpha.0] - 2022-11-24
 ### Added
 - [astarte_data_updater_plant] Add support for device introspection triggers.

apps/astarte_realm_management_api/lib/astarte_realm_management_api/triggers/action.ex

@@ -0,0 +1,93 @@
+#
+# This file is part of Astarte.
+#
+# Copyright 2023 SECO Mind Srl
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+defmodule Astarte.RealmManagement.API.Triggers.Action do
+ @moduledoc """
+ This module provides a wrapper for data held
+ either in an Astarte.RealmManagement.API.Triggers.AMQPAction
+ or in an Astarte.RealmManagement.API.Triggers.HttpAction struct.
+ Validation is performed in the related modules.
+ """
+ use Ecto.Schema
+ alias Astarte.RealmManagement.API.Triggers.Action
+
+ @primary_key false
+ embedded_schema do
+ field :http_url, :string
+ field :http_method, :string
+ field :http_static_headers, {:map, :string}
+ field :template, :string
+ field :template_type, :string
+ field :http_post_url, :string, virtual: true
+ field :amqp_exchange, :string
+ field :amqp_routing_key, :string
+ field :amqp_static_headers, {:map, :string}
+ field :amqp_message_expiration_ms, :integer
+ field :amqp_message_priority, :integer
+ field :amqp_message_persistent, :boolean
+ end
+
+ defimpl Jason.Encoder, for: Action do
+ # HTTP actions must have the 'http_url' field set
+ def encode(%Action{http_url: http_url} = action, opts) when not is_nil(http_url) do
+ %Action{
+ http_method: http_method,
+ http_static_headers: http_static_headers,
+ template: template,
+ template_type: template_type
+ } = action
+
+ %{
+ "http_url" => http_url,
+ "http_method" => http_method
+ }
+ |> maybe_put("http_static_headers", http_static_headers)
+ |> maybe_put("template", template)
+ |> maybe_put("template_type", template_type)
+ |> Jason.Encode.map(opts)
+ end
+
+ # AMQP actions must have the 'amqp_exchange' field set
+ def encode(%Action{amqp_exchange: amqp_exchange} = action, opts)
+ when not is_nil(amqp_exchange) do
+ %Action{
+ amqp_routing_key: amqp_routing_key,
+ amqp_static_headers: amqp_headers,
+ amqp_message_expiration_ms: amqp_message_expiration_ms,
+ amqp_message_persistent: amqp_message_persistent,
+ amqp_message_priority: amqp_message_priority
+ } = action
+
+ %{
+ "amqp_exchange" => amqp_exchange,
+ "amqp_routing_key" => amqp_routing_key,
+ "amqp_message_expiration_ms" => amqp_message_expiration_ms,
+ "amqp_message_persistent" => amqp_message_persistent
+ }
+ |> maybe_put("amqp_static_headers", amqp_headers)
+ |> maybe_put("amqp_message_priority", amqp_message_priority)
+ |> Jason.Encode.map(opts)
+ end
+
+ defp maybe_put(map, _key, nil),
+ do: map
+
+ defp maybe_put(map, key, value),
+ do: Map.put(map, key, value)
+ end
+end

apps/astarte_realm_management_api/lib/astarte_realm_management_api/triggers/amqp_action.ex

@@ -71,33 +71,4 @@ defmodule Astarte.RealmManagement.API.Triggers.AMQPAction do
  end
  end)
  end
-
- defimpl Jason.Encoder, for: AMQPAction do
- def encode(action, opts) do
- %AMQPAction{
- amqp_exchange: amqp_exchange,
- amqp_routing_key: amqp_routing_key,
- amqp_static_headers: amqp_headers,
- amqp_message_expiration_ms: amqp_message_expiration_ms,
- amqp_message_persistent: amqp_message_persistent,
- amqp_message_priority: amqp_message_priority
- } = action
-
- %{
- "amqp_exchange" => amqp_exchange,
- "amqp_routing_key" => amqp_routing_key,
- "amqp_message_expiration_ms" => amqp_message_expiration_ms,
- "amqp_message_persistent" => amqp_message_persistent
- }
- |> maybe_put("amqp_static_headers", amqp_headers)
- |> maybe_put("amqp_message_priority", amqp_message_priority)
- |> Jason.Encode.map(opts)
- end
-
- defp maybe_put(map, _key, nil),
- do: map
-
- defp maybe_put(map, key, value),
- do: Map.put(map, key, value)
- end
 end

apps/astarte_realm_management_api/lib/astarte_realm_management_api/triggers/http_action.ex

@@ -156,31 +156,4 @@ defmodule Astarte.RealmManagement.API.Triggers.HttpAction do
  end
  end)
  end
-
- defimpl Jason.Encoder, for: HttpAction do
- def encode(action, opts) do
- %HttpAction{
- http_url: http_url,
- http_method: http_method,
- http_static_headers: http_static_headers,
- template: template,
- template_type: template_type
- } = action
-
- %{
- "http_url" => http_url,
- "http_method" => http_method
- }
- |> maybe_put("http_static_headers", http_static_headers)
- |> maybe_put("template", template)
- |> maybe_put("template_type", template_type)
- |> Jason.Encode.map(opts)
- end
-
- defp maybe_put(map, _key, nil),
- do: map
-
- defp maybe_put(map, key, value),
- do: Map.put(map, key, value)
- end
 end

apps/astarte_realm_management_api/lib/astarte_realm_management_api/triggers/trigger.ex

@@ -19,14 +19,15 @@
 defmodule Astarte.RealmManagement.API.Triggers.Trigger do
  use Ecto.Schema
  import Ecto.Changeset
+ alias Ecto.Changeset
  alias Astarte.Core.Triggers.SimpleTriggerConfig
- alias Astarte.RealmManagement.API.Triggers.{AMQPAction, HttpAction, Trigger}
+ alias Astarte.RealmManagement.API.Triggers.{Action, AMQPAction, HttpAction, Trigger}
 
  @derive {Phoenix.Param, key: :name}
  @primary_key false
  embedded_schema do
  field :name, :string
- field :action, :any, virtual: true
+ embeds_one :action, Action
  field :policy, :string
  embeds_one :amqp_action, AMQPAction
  embeds_one :http_action, HttpAction
@@ -74,25 +75,39 @@ defmodule Astarte.RealmManagement.API.Triggers.Trigger do
  end
 
  def normalize(changeset) do
- amqp_action = get_field(changeset, :amqp_action)
- http_action = get_field(changeset, :http_action)
+ amqp_action = get_change(changeset, :amqp_action)
+ http_action = get_change(changeset, :http_action)
 
  case {amqp_action, http_action} do
  {nil, nil} ->
  changeset
 
  {_, nil} ->
- changeset
- |> delete_change(:amqp_action)
- |> put_change(:action, amqp_action)
+ normalize_action(changeset, amqp_action)
 
  {nil, _} ->
- changeset
- |> delete_change(:http_action)
- |> put_change(:action, http_action)
+ normalize_action(changeset, http_action)
  end
  end
 
+ defp normalize_action(parent_changeset, action_changeset) do
+ %Changeset{changes: changes, errors: errors} = action_changeset
+
+ parent_changeset
+ |> delete_change(:amqp_action)
+ |> delete_change(:http_action)
+ |> put_change(:action, changes)
+ |> put_errors(:action, errors)
+ end
+
+ defp put_errors(changeset, field, errors) do
+ Enum.reduce(errors, changeset, fn {subfield, {message, keys}}, acc ->
+ Changeset.update_change(acc, field, fn sub_changeset ->
+ Changeset.add_error(sub_changeset, subfield, message, keys)
+ end)
+ end)
+ end
+
  def policy_name_regex do
  ~r/^(?!@).+$/
  end

apps/astarte_realm_management_api/test/astarte_realm_management_api/triggers/action_test.exs

@@ -0,0 +1,121 @@
+#
+# This file is part of Astarte.
+#
+# Copyright 2023 SECO Mind Srl
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+defmodule Astarte.RealmManagement.API.Triggers.ActionTest do
+ use ExUnit.Case
+ alias Astarte.RealmManagement.API.Triggers.Action
+
+ describe "well-formed HTTP action is correctly encoded" do
+ test "when headers are set" do
+ action = %Action{
+ http_url: "https://example.com/",
+ http_method: "put",
+ http_static_headers: %{
+ "Foo" => "Bar",
+ "Key" => "Value"
+ }
+ }
+
+ jason_out_map =
+ action
+ |> Jason.encode!()
+ |> Jason.decode!()
+
+ assert %{
+ "http_url" => "https://example.com/",
+ "http_method" => "put",
+ "http_static_headers" => %{"Foo" => "Bar", "Key" => "Value"}
+ } = jason_out_map
+ end
+ end
+
+ describe "well-formed AMQPAction is correctly encoded" do
+ test "when all is set" do
+ action = %Action{
+ amqp_exchange: "astarte_events_test_custom_exchange",
+ amqp_routing_key: "test_routing_key",
+ amqp_message_persistent: true,
+ amqp_message_expiration_ms: 5000,
+ amqp_message_priority: 3
+ }
+
+ jason_out_map =
+ action
+ |> Jason.encode!()
+ |> Jason.decode!()
+
+ assert jason_out_map == %{
+ "amqp_exchange" => "astarte_events_test_custom_exchange",
+ "amqp_routing_key" => "test_routing_key",
+ "amqp_message_persistent" => true,
+ "amqp_message_expiration_ms" => 5000,
+ "amqp_message_priority" => 3
+ }
+ end
+
+ test "when message priority is not set (it is omitted)" do
+ action = %Action{
+ amqp_exchange: "astarte_events_test_custom_exchange",
+ amqp_routing_key: "test_routing_key",
+ amqp_message_persistent: false,
+ amqp_message_expiration_ms: 5000
+ }
+
+ jason_out_map =
+ action
+ |> Jason.encode!()
+ |> Jason.decode!()
+
+ assert jason_out_map == %{
+ "amqp_exchange" => "astarte_events_test_custom_exchange",
+ "amqp_routing_key" => "test_routing_key",
+ "amqp_message_persistent" => false,
+ "amqp_message_expiration_ms" => 5000
+ }
+ end
+
+ test "when headers are set" do
+ action = %Action{
+ amqp_exchange: "astarte_events_test_custom_exchange",
+ amqp_routing_key: "test",
+ amqp_message_persistent: true,
+ amqp_message_expiration_ms: 100,
+ amqp_static_headers: %{
+ "Foo" => "Bar",
+ "X-Test" => "Test"
+ }
+ }
+
+ jason_out_map =
+ action
+ |> Jason.encode!()
+ |> Jason.decode!()
+
+ assert jason_out_map == %{
+ "amqp_exchange" => "astarte_events_test_custom_exchange",
+ "amqp_routing_key" => "test",
+ "amqp_message_persistent" => true,
+ "amqp_message_expiration_ms" => 100,
+ "amqp_static_headers" => %{
+ "Foo" => "Bar",
+ "X-Test" => "Test"
+ }
+ }
+ end
+ end
+end