Merge pull request #554 from podlove/add-consistence-checker

Add consistence checker

lib/radiator/outline.ex

@@ -21,6 +21,7 @@ defmodule Radiator.Outline do
   alias Radiator.Outline.Node
   alias Radiator.Outline.NodeRepoResult
   alias Radiator.Outline.NodeRepository
+  alias Radiator.Outline.Validations, as: NodeValidator
   alias Radiator.Repo
 
   require Logger
@@ -138,7 +139,12 @@ defmodule Radiator.Outline do
       node ->
         parent_node = get_parent_node(node)
 
-        case validate_consistency_for_move(node, new_prev_id, new_parent_id, parent_node) do
+        case NodeValidator.validate_consistency_for_move(
+               node,
+               new_prev_id,
+               new_parent_id,
+               parent_node
+             ) do
           {:error, error} ->
             {:error, error}
 
@@ -149,39 +155,6 @@ defmodule Radiator.Outline do
     end
   end
 
-  defp validate_consistency_for_move(
-         %{prev_id: new_prev_id, parent_id: new_parent_id},
-         new_prev_id,
-         new_parent_id,
-         _parent_node
-       ) do
-    {:error, :noop}
-  end
-
-  # when prev is nil, every parent is allowed
-  defp validate_consistency_for_move(
-         node,
-         nil,
-         _new_parent_id,
-         _parent_node
-       ) do
-    {:ok, node}
-  end
-
-  # when prev is not nil, parent and prev must be consistent
-  defp validate_consistency_for_move(
-         node,
-         new_prev_id,
-         new_parent_id,
-         _parent_node
-       ) do
-    if NodeRepository.get_node(new_prev_id).parent_id == new_parent_id do
-      {:ok, node}
-    else
-      {:error, :parent_and_prev_not_consistent}
-    end
-  end
-
   # low level function to move a node
   defp do_move_node(node, new_prev_id, new_parent_id, prev_node, parent_node) do
     node_repo_result = %NodeRepoResult{node: node}

lib/radiator/outline/dispatch.ex

@@ -1,8 +1,7 @@
 defmodule Radiator.Outline.Dispatch do
   @moduledoc false
 
-  alias Radiator.Outline.Command
-  alias Radiator.Outline.EventProducer
+  alias Radiator.Outline.{Command, Event, EventProducer, Validations}
 
   def insert_node(attributes, user_id, event_id) do
     "insert_node"
@@ -33,6 +32,13 @@ defmodule Radiator.Outline.Dispatch do
   end
 
   def broadcast(event) do
+    if Mix.env() == :dev || Mix.env() == :test do
+      :ok =
+        event
+        |> Event.episode_id()
+        |> Validations.validate_tree_for_episode()
+    end
+
     Phoenix.PubSub.broadcast(Radiator.PubSub, "events", event)
   end
 

lib/radiator/outline/event.ex

@@ -10,6 +10,8 @@ defmodule Radiator.Outline.Event do
     NodeMovedEvent
   }
 
+  alias Radiator.Outline.NodeRepository
+
   def payload(%NodeInsertedEvent{} = event) do
     %{
       node_id: event.node.uuid,
@@ -46,4 +48,20 @@ defmodule Radiator.Outline.Event do
   def event_type(%NodeDeletedEvent{} = _event), do: "NodeDeletedEvent"
 
   def event_type(%NodeMovedEvent{} = _event), do: "NodeMovedEvent"
+
+  def episode_id(%NodeInsertedEvent{} = event) do
+    event.node.episode_id
+  end
+
+  def episode_id(%NodeContentChangedEvent{} = event) do
+    NodeRepository.get_node(event.node_id).episode_id
+  end
+
+  def episode_id(%NodeDeletedEvent{} = event) do
+    NodeRepository.get_node(event.next_id).episode_id
+  end
+
+  def episode_id(%NodeMovedEvent{} = event) do
+    NodeRepository.get_node(event.node_id).episode_id
+  end
 end

lib/radiator/outline/node.ex

@@ -15,7 +15,6 @@ defmodule Radiator.Outline.Node do
     field :parent_id, Ecto.UUID
     field :prev_id, Ecto.UUID
     field :level, :integer, virtual: true
-    field :position, :integer, virtual: true
 
     belongs_to :episode, Episode
 
@@ -24,11 +23,8 @@ defmodule Radiator.Outline.Node do
 
   @doc """
   A changeset for inserting a new node
-  Work in progress. Since we currently ignore the tree structure, there is
-  no concept for a root node.
-  Also questionable wether a node really needs a content from beginning. So probably a root
-  doesnt have a content
-  Another issue might be we need to create the uuid upfront and pass it here
+  A content is not mandatory,
+  The uuid might be generated upfront
   """
   def insert_changeset(node, attributes) do
     node

lib/radiator/outline/node_repository.ex

@@ -86,9 +86,9 @@ defmodule Radiator.Outline.NodeRepository do
 
   """
   def count_nodes_by_episode(episode_id) do
-    episode_id
-    |> list_nodes_by_episode()
-    |> Enum.count()
+    Node
+    |> where([p], p.episode_id == ^episode_id)
+    |> Repo.aggregate(:count)
   end
 
   @doc """

lib/radiator/outline/validations.ex

@@ -0,0 +1,104 @@
+defmodule Radiator.Outline.Validations do
+  @moduledoc """
+    Collection of consistency validations for the outline tree.
+  """
+  alias Radiator.Outline
+  alias Radiator.Outline.Node
+  alias Radiator.Outline.NodeRepository
+
+  def validate_consistency_for_move(
+        %{prev_id: new_prev_id, parent_id: new_parent_id},
+        new_prev_id,
+        new_parent_id,
+        _parent_node
+      ) do
+    {:error, :noop}
+  end
+
+  # when prev is nil, every parent is allowed
+  def validate_consistency_for_move(
+        node,
+        nil,
+        _new_parent_id,
+        _parent_node
+      ) do
+    {:ok, node}
+  end
+
+  # when prev is not nil, parent and prev must be consistent
+  def validate_consistency_for_move(
+        node,
+        new_prev_id,
+        new_parent_id,
+        _parent_node
+      ) do
+    if NodeRepository.get_node(new_prev_id).parent_id == new_parent_id do
+      {:ok, node}
+    else
+      {:error, :parent_and_prev_not_consistent}
+    end
+  end
+
+  @doc """
+  Validates a tree for an episode.
+  Returns :ok if the tree is valid
+  """
+  def validate_tree_for_episode(episode_id) do
+    {:ok, tree_nodes} = Outline.get_node_tree(episode_id)
+
+    if Enum.count(tree_nodes) == NodeRepository.count_nodes_by_episode(episode_id) do
+      validate_tree_nodes(tree_nodes)
+    else
+      {:error, :node_count_not_consistent}
+    end
+  end
+
+  # iterate through the levels of the tree
+  # every level has 1 node with prev_id nil
+  # all other nodes in level have prev_id set and are connected to the previous node
+  # should be used in dev and test only
+  # might crash if the tree is not consistent
+  defp validate_tree_nodes(tree_nodes) do
+    tree_nodes
+    |> Enum.group_by(& &1.parent_id)
+    |> Enum.map(fn {_level, nodes} ->
+      validate_sub_tree(nodes)
+    end)
+    |> Enum.reject(&(&1 == :ok))
+    |> first_error()
+  end
+
+  defp first_error([]), do: :ok
+  defp first_error([err | _]), do: err
+
+  defp validate_sub_tree(nodes) do
+    # get the node with prev_id nil
+    first_node = Enum.find(nodes, &(&1.prev_id == nil))
+    # get the rest of the nodes
+    rest_nodes = Enum.reject(nodes, &(&1.prev_id == nil))
+
+    if Enum.count(rest_nodes) + 1 != Enum.count(nodes) do
+      {:error, :prev_id_not_consistent}
+    else
+      validate_prev_node(first_node, rest_nodes)
+    end
+  end
+
+  def validate_prev_node(node, rest_nodes, searched_nodes \\ [])
+
+  def validate_prev_node(
+        %Node{uuid: id},
+        [%Node{prev_id: id} = node | rest_nodes],
+        searched_nodes
+      ) do
+    validate_prev_node(node, rest_nodes ++ searched_nodes, [])
+  end
+
+  def validate_prev_node(%Node{}, [], []), do: :ok
+
+  def validate_prev_node(%Node{} = prev_node, [node | rest_nodes], search_nodes) do
+    validate_prev_node(prev_node, rest_nodes, search_nodes ++ [node])
+  end
+
+  def validate_prev_node(%Node{}, [], _search_nodes), do: {:error, :prev_id_not_consistent}
+end

lib/radiator_web/live/episode_live/index.ex

@@ -11,10 +11,8 @@ defmodule RadiatorWeb.EpisodeLive.Index do
   }
 
   alias Radiator.Outline.NodeRepository
-  # alias Radiator.EventStore
   alias Radiator.Podcast
   alias Radiator.Podcast.Episode
-
   alias RadiatorWeb.OutlineComponents
 
   @impl true

test/radiator/outline/validations_test.exs

@@ -0,0 +1,97 @@
+defmodule Radiator.Outline.ValidationsTest do
+  @moduledoc false
+  use Radiator.DataCase
+
+  alias Radiator.Outline.Node
+  alias Radiator.Outline.NodeRepository
+  alias Radiator.Outline.Validations
+
+  import Ecto.Query, warn: false
+
+  describe "validate_tree_for_episode/1" do
+    setup :complex_node_fixture
+
+    test "validates a tree", %{
+      node_1: %Node{episode_id: episode_id}
+    } do
+      assert :ok = Validations.validate_tree_for_episode(episode_id)
+    end
+
+    test "a level might have different subtrees", %{
+      node_1: %Node{episode_id: episode_id} = node_1
+    } do
+      {:ok, %Node{} = _nested_node} =
+        %{
+          episode_id: episode_id,
+          parent_id: node_1.uuid,
+          prev_id: nil,
+          content: "child of node 1"
+        }
+        |> NodeRepository.create_node()
+
+      assert :ok = Validations.validate_tree_for_episode(episode_id)
+    end
+
+    test "when two nodes share the same prev_id the tree is invalid", %{
+      node_2: %Node{episode_id: episode_id} = node_2
+    } do
+      {:ok, %Node{} = _node_invalid} =
+        %{
+          episode_id: episode_id,
+          parent_id: node_2.parent_id,
+          prev_id: node_2.prev_id
+        }
+        |> NodeRepository.create_node()
+
+      assert {:error, :prev_id_not_consistent} =
+               Validations.validate_tree_for_episode(episode_id)
+    end
+
+    test "when a nodes has a non connected prev_id the tree is invalid", %{
+      node_2: %Node{episode_id: episode_id} = node_2
+    } do
+      {:ok, %Node{} = _node_invalid} =
+        %{
+          episode_id: episode_id,
+          parent_id: node_2.parent_id,
+          prev_id: node_2.prev_id
+        }
+        |> NodeRepository.create_node()
+
+      assert {:error, :prev_id_not_consistent} =
+               Validations.validate_tree_for_episode(episode_id)
+    end
+
+    test "when a parent has two childs with prev_id nil the tree is invalid", %{
+      nested_node_1: %Node{episode_id: episode_id, parent_id: parent_id}
+    } do
+      {:ok, %Node{} = _node_invalid} =
+        %{
+          episode_id: episode_id,
+          parent_id: parent_id,
+          prev_id: nil,
+          content: "invalid node"
+        }
+        |> NodeRepository.create_node()
+
+      assert {:error, :prev_id_not_consistent} =
+               Validations.validate_tree_for_episode(episode_id)
+    end
+
+    test "a tree with a node where parent and prev are not consistent is invalid", %{
+      parent_node: %Node{episode_id: episode_id} = parent_node,
+      nested_node_2: nested_node_2
+    } do
+      {:ok, %Node{} = _node_invalid} =
+        %{
+          episode_id: episode_id,
+          parent_id: parent_node.uuid,
+          prev_id: nested_node_2.uuid
+        }
+        |> NodeRepository.create_node()
+
+      result = Validations.validate_tree_for_episode(episode_id)
+      assert {:error, :prev_id_not_consistent} = result
+    end
+  end
+end