Improve input type parsing (#517)

* Improve input type parsing

* Move type parsing to Cell.Input

* Add info comment

lib/livebook/live_markdown/export.ex

@@ -106,7 +106,7 @@ defmodule Livebook.LiveMarkdown.Export do
     json =
       %{
         livebook_object: :cell_input,
-        type: cell.type,
+        type: Cell.Input.type_to_string(cell.type),
         name: cell.name,
         value: value
       }

lib/livebook/live_markdown/import.ex

@@ -14,14 +14,11 @@ defmodule Livebook.LiveMarkdown.Import do
     earmark_messages = Enum.map(earmark_messages, &earmark_message_to_string/1)
 
     {ast, rewrite_messages} = rewrite_ast(ast)
+    elements = group_elements(ast)
+    {notebook, build_messages} = build_notebook(elements)
+    notebook = postprocess_notebook(notebook)
 
-    notebook =
+    {notebook, earmark_messages ++ rewrite_messages ++ build_messages}
-      ast
-      |> group_elements()
-      |> build_notebook()
-      |> postprocess_notebook()
-
-    {notebook, earmark_messages ++ rewrite_messages}
   end
 
   defp earmark_message_to_string({_severity, line_number, message}) do
@@ -196,61 +193,68 @@ defmodule Livebook.LiveMarkdown.Import do
   # Note that the list of elements is reversed:
   # first we group elements by traversing Earmark AST top-down
   # and then aggregate elements into data strictures going bottom-up.
-  defp build_notebook(elems, cells \\ [], sections \\ [])
+  defp build_notebook(elems, cells \\ [], sections \\ [], messages \\ [])
 
-  defp build_notebook([{:cell, :elixir, source, outputs} | elems], cells, sections) do
+  defp build_notebook([{:cell, :elixir, source, outputs} | elems], cells, sections, messages) do
     {metadata, elems} = grab_metadata(elems)
     attrs = cell_metadata_to_attrs(:elixir, metadata)
     outputs = Enum.map(outputs, &{:text, &1})
     cell = %{Notebook.Cell.new(:elixir) | source: source, outputs: outputs} |> Map.merge(attrs)
-    build_notebook(elems, [cell | cells], sections)
+    build_notebook(elems, [cell | cells], sections, messages)
   end
 
-  defp build_notebook([{:cell, :markdown, md_ast} | elems], cells, sections) do
+  defp build_notebook([{:cell, :markdown, md_ast} | elems], cells, sections, messages) do
     {metadata, elems} = grab_metadata(elems)
     attrs = cell_metadata_to_attrs(:markdown, metadata)
     source = md_ast |> Enum.reverse() |> MarkdownHelpers.markdown_from_ast()
     cell = %{Notebook.Cell.new(:markdown) | source: source} |> Map.merge(attrs)
-    build_notebook(elems, [cell | cells], sections)
+    build_notebook(elems, [cell | cells], sections, messages)
   end
 
-  defp build_notebook([{:cell, :input, data} | elems], cells, sections) do
+  defp build_notebook([{:cell, :input, data} | elems], cells, sections, messages) do
-    attrs = parse_input_attrs(data)
+    case parse_input_attrs(data) do
-    cell = Notebook.Cell.new(:input) |> Map.merge(attrs)
+      {:ok, attrs} ->
-    build_notebook(elems, [cell | cells], sections)
+        cell = Notebook.Cell.new(:input) |> Map.merge(attrs)
+        build_notebook(elems, [cell | cells], sections, messages)
+
+      {:error, message} ->
+        build_notebook(elems, cells, sections, [message | messages])
+    end
   end
 
-  defp build_notebook([{:section_name, content} | elems], cells, sections) do
+  defp build_notebook([{:section_name, content} | elems], cells, sections, messages) do
     name = text_from_markdown(content)
     {metadata, elems} = grab_metadata(elems)
     attrs = section_metadata_to_attrs(metadata)
     section = %{Notebook.Section.new() | name: name, cells: cells} |> Map.merge(attrs)
-    build_notebook(elems, [], [section | sections])
+    build_notebook(elems, [], [section | sections], messages)
   end
 
   # If there are section-less cells, put them in a default one.
-  defp build_notebook([{:notebook_name, _content} | _] = elems, cells, sections)
+  defp build_notebook([{:notebook_name, _content} | _] = elems, cells, sections, messages)
        when cells != [] do
     section = %{Notebook.Section.new() | cells: cells}
-    build_notebook(elems, [], [section | sections])
+    build_notebook(elems, [], [section | sections], messages)
   end
 
   # If there are section-less cells, put them in a default one.
-  defp build_notebook([] = elems, cells, sections) when cells != [] do
+  defp build_notebook([] = elems, cells, sections, messages) when cells != [] do
     section = %{Notebook.Section.new() | cells: cells}
-    build_notebook(elems, [], [section | sections])
+    build_notebook(elems, [], [section | sections], messages)
   end
 
-  defp build_notebook([{:notebook_name, content} | elems], [], sections) do
+  defp build_notebook([{:notebook_name, content} | elems], [], sections, messages) do
     name = text_from_markdown(content)
     {metadata, []} = grab_metadata(elems)
     attrs = notebook_metadata_to_attrs(metadata)
-    %{Notebook.new() | name: name, sections: sections} |> Map.merge(attrs)
+    notebook = %{Notebook.new() | name: name, sections: sections} |> Map.merge(attrs)
+    {notebook, messages}
   end
 
   # If there's no explicit notebook heading, use the defaults.
-  defp build_notebook([], [], sections) do
+  defp build_notebook([], [], sections, messages) do
-    %{Notebook.new() | sections: sections}
+    notebook = %{Notebook.new() | sections: sections}
+    {notebook, messages}
   end
 
   defp text_from_markdown(markdown) do
@@ -268,16 +272,28 @@ defmodule Livebook.LiveMarkdown.Import do
   defp grab_metadata(elems), do: {%{}, elems}
 
   defp parse_input_attrs(data) do
-    type = data["type"] |> String.to_existing_atom()
+    with {:ok, type} <- parse_input_type(data["type"]) do
+      {:ok,
+       %{
+         type: type,
+         name: data["name"],
+         value: data["value"],
+         # Fields with implicit value
+         reactive: Map.get(data, "reactive", false),
+         props: data |> Map.get("props", %{}) |> parse_input_props(type)
+       }}
+    end
+  end
 
-    %{
+  defp parse_input_type(string) do
-      type: type,
+    case Notebook.Cell.Input.type_from_string(string) do
-      name: data["name"],
+      {:ok, type} ->
-      value: data["value"],
+        {:ok, type}
-      # Fields with implicit value
+
-      reactive: Map.get(data, "reactive", false),
+      :error ->
-      props: data |> Map.get("props", %{}) |> parse_input_props(type)
+        {:error,
-    }
+         "unrecognised input type #{inspect(string)}, if it's a valid type it means your Livebook version doesn't support it"}
+    end
   end
 
   defp parse_input_props(data, type) do

lib/livebook/notebook/cell/input.ex

@@ -20,6 +20,7 @@ defmodule Livebook.Notebook.Cell.Input do
           props: props()
         }
 
+  # Make sure to keep this in sync with `type_from_string/1`
   @type type ::
           :text | :url | :number | :password | :textarea | :color | :range | :select | :checkbox
 
@@ -114,4 +115,31 @@ defmodule Livebook.Notebook.Cell.Input do
   def reactive_update?(cell, prev_cell) do
     cell.reactive and cell.value != prev_cell.value and validate(cell) == :ok
   end
+
+  @doc """
+  Parses input type from string.
+  """
+  @spec type_from_string(String.t()) :: {:ok, type()} | :error
+  def type_from_string(string) do
+    case string do
+      "text" -> {:ok, :text}
+      "url" -> {:ok, :url}
+      "number" -> {:ok, :number}
+      "password" -> {:ok, :password}
+      "textarea" -> {:ok, :textarea}
+      "color" -> {:ok, :color}
+      "range" -> {:ok, :range}
+      "select" -> {:ok, :select}
+      "checkbox" -> {:ok, :checkbox}
+      _other -> :error
+    end
+  end
+
+  @doc """
+  Converts inpu type to string.
+  """
+  @spec type_to_string(type()) :: String.t()
+  def type_to_string(type) do
+    Atom.to_string(type)
+  end
 end

test/livebook/live_markdown/import_test.exs

@@ -581,4 +581,20 @@ defmodule Livebook.LiveMarkdown.ImportTest do
 
     assert %Notebook{name: "My Notebook", autosave_interval_s: 10} = notebook
   end
+
+  test "skips invalid input type and returns a message" do
+    markdown = """
+    # My Notebook
+
+    ## Section 1
+
+    <!-- livebook:{"livebook_object":"cell_input","type":"input_from_the_future"} -->
+    """
+
+    {_notebook, messages} = Import.notebook_from_markdown(markdown)
+
+    assert [
+             ~s{unrecognised input type "input_from_the_future", if it's a valid type it means your Livebook version doesn't support it}
+           ] == messages
+  end
 end