check if section has valid parent when importing notebook (#984)

* check if section has valid parent + tests

* formatting

* apply review feedback

* fix the forgotten test

* apply feedback

* apply feedback

* produce a warning if the section points to itself + tests

* Apply suggestions from code review

Co-authored-by: Jonatan Kłosko <jonatanklosko@gmail.com>

lib/livebook/live_markdown/import.ex

@@ -16,9 +16,9 @@ defmodule Livebook.LiveMarkdown.Import do
     {ast, rewrite_messages} = rewrite_ast(ast)
     elements = group_elements(ast)
     {notebook, build_messages} = build_notebook(elements)
-    notebook = postprocess_notebook(notebook)
+    {notebook, postprocess_messages} = postprocess_notebook(notebook)
 
-    {notebook, earmark_messages ++ rewrite_messages ++ build_messages}
+    {notebook, earmark_messages ++ rewrite_messages ++ build_messages ++ postprocess_messages}
   end
 
   defp earmark_message_to_string({_severity, line_number, message}) do
@@ -393,19 +393,47 @@ defmodule Livebook.LiveMarkdown.Import do
   end
 
   defp postprocess_notebook(notebook) do
-    sections =
+    {sections, {_branching_ids, warnings}} =
-      Enum.map(notebook.sections, fn section ->
+      notebook.sections
+      |> Enum.with_index()
+      |> Enum.map_reduce({MapSet.new(), []}, fn {section, section_idx},
+                                                {branching_ids, warnings} ->
         # Set parent_id based on the persisted branch_parent_index if present
         case section.parent_id do
           nil ->
-            section
+            {section, {branching_ids, warnings}}
 
           {:idx, parent_idx} ->
             parent = Enum.at(notebook.sections, parent_idx)
-            %{section | parent_id: parent.id}
+
+            cond do
+              section_idx <= parent_idx ->
+                {%{section | parent_id: nil},
+                 {
+                   branching_ids,
+                   [
+                     ~s{ignoring the parent section of "#{section.name}", because it comes later in the notebook}
+                     | warnings
+                   ]
+                 }}
+
+              !is_nil(parent) && MapSet.member?(branching_ids, parent.id) ->
+                {%{section | parent_id: nil},
+                 {
+                   branching_ids,
+                   [
+                     ~s{ignoring the parent section of "#{section.name}", because it is itself a branching section}
+                     | warnings
+                   ]
+                 }}
+
+              true ->
+                {%{section | parent_id: parent.id},
+                 {MapSet.put(branching_ids, section.id), warnings}}
+            end
         end
       end)
 
-    %{notebook | sections: sections}
+    {%{notebook | sections: sections}, Enum.reverse(warnings)}
   end
 end

test/livebook/live_markdown/import_test.exs

@@ -766,4 +766,120 @@ defmodule Livebook.LiveMarkdown.ImportTest do
              } = notebook
     end
   end
+
+  test "import notebook with invalid parent section produces a warning" do
+    markdown = """
+    # My Notebook
+
+    <!-- livebook:{"branch_parent_index":4} -->
+
+    ## Section 1
+
+    ```elixir
+    Process.info()
+    ```
+
+    ## Section 2
+
+    ```elixir
+    Process.info()
+    ```
+    """
+
+    assert {notebook,
+            [
+              "ignoring the parent section of \"Section 1\", because it comes later in the notebook"
+            ]} = Import.notebook_from_markdown(markdown)
+
+    assert %Notebook{
+             name: "My Notebook",
+             sections: [
+               %Notebook.Section{
+                 parent_id: nil
+               },
+               %Notebook.Section{
+                 parent_id: nil
+               }
+             ]
+           } = notebook
+  end
+
+  test "import notebook with parent section pointing to the section itself produces a warning" do
+    markdown = """
+    # My Notebook
+
+    <!-- livebook:{"branch_parent_index":0} -->
+
+    ## Section 1
+
+    ```elixir
+    Process.info()
+    ```
+    """
+
+    assert {notebook,
+            [
+              "ignoring the parent section of \"Section 1\", because it comes later in the notebook"
+            ]} = Import.notebook_from_markdown(markdown)
+
+    assert %Notebook{
+             name: "My Notebook",
+             sections: [
+               %Notebook.Section{
+                 parent_id: nil
+               }
+             ]
+           } = notebook
+  end
+
+  test "import notebook with parent section being a branching section  itself produces a warning" do
+    markdown = """
+    # My Notebook
+
+
+    ## Section 1
+
+    ```elixir
+    Process.info()
+    ```
+
+    <!-- livebook:{"branch_parent_index":0} -->
+
+    ## Section 2
+
+    ```elixir
+    Process.info()
+    ```
+    <!-- livebook:{"branch_parent_index":1} -->
+
+    ## Section 3
+
+    ```elixir
+    Process.info()
+    ```
+    """
+
+    assert {notebook,
+            [
+              "ignoring the parent section of \"Section 3\", because it is itself a branching section"
+            ]} = Import.notebook_from_markdown(markdown)
+
+    assert %Notebook{
+             name: "My Notebook",
+             sections: [
+               %Notebook.Section{
+                 name: "Section 1",
+                 parent_id: nil
+               },
+               %Notebook.Section{
+                 name: "Section 2",
+                 parent_id: _
+               },
+               %Notebook.Section{
+                 name: "Section 3",
+                 parent_id: nil
+               }
+             ]
+           } = notebook
+  end
 end