Make scripts endpoint consistent with api by taking name instead of id (#2398)

Fixes #2396

Author: kybishop

lib/nerves_hub/scripts.ex

@@ -46,6 +46,31 @@ defmodule NervesHub.Scripts do
     end
   end
 
+  def get_by_product_and_name(product, name) do
+    case Repo.get_by(Script, name: name, product_id: product.id) do
+      nil ->
+        {:error, :not_found}
+
+      script ->
+        {:ok, script}
+    end
+  end
+
+  def get_by_product_and_name_with_id_fallback(product, name_or_id) do
+    # Try to find by name first
+    case get_by_product_and_name(product, name_or_id) do
+      {:ok, script} ->
+        {:ok, script}
+
+      {:error, :not_found} ->
+        # If not found by name, try by ID
+        case Integer.parse(name_or_id) do
+          {id, ""} -> get(product, id)
+          _ -> {:error, :not_found}
+        end
+    end
+  end
+
   @spec create(Product.t(), User.t(), map()) :: {:ok, Script.t()} | {:error, Changeset.t()}
   def create(product, user, params) do
     Script.create_changeset(product, user, params)

lib/nerves_hub_web/controllers/api/openapi/device_controller_specs.ex

@@ -293,16 +293,21 @@ defmodule NervesHubWeb.API.OpenAPI.DeviceControllerSpecs do
 
     additional_parameters = [
       %OpenApiSpex.Parameter{
-        name: :script_id,
+        name: :name_or_id,
         in: :path,
-        description: "Support Script ID",
+        description: "Support Script Name or ID. Name takes priority.",
         required: true,
-        schema: %OpenApiSpex.Schema{type: :integer},
-        example: "123"
+        schema: %OpenApiSpex.Schema{
+          oneOf: [
+            %OpenApiSpex.Schema{type: :string},
+            %OpenApiSpex.Schema{type: :integer}
+          ]
+        },
+        example: "my-script"
       },
       %OpenApiSpex.Parameter{
         name: :timeout,
-        in: :path,
+        in: :query,
         description: "How long to wait for a device response in milliseconds",
         required: false,
         schema: %OpenApiSpex.Schema{type: :integer},
@@ -325,7 +330,7 @@ defmodule NervesHubWeb.API.OpenAPI.DeviceControllerSpecs do
 
     add_to_paths(
       openapi,
-      "#{opts.path_prefix}/scripts/{script_id}",
+      "#{opts.path_prefix}/scripts/{name_or_id}",
       %OpenApiSpex.PathItem{post: send_script_operation}
     )
   end

lib/nerves_hub_web/controllers/api/script_controller.ex

@@ -24,8 +24,8 @@ defmodule NervesHubWeb.API.ScriptController do
   # This operation is defined in `NervesHubWeb.API.OpenAPI.DeviceControllerSpecs`
   operation(:send, false)
 
-  def send(%{assigns: %{device: device}} = conn, %{"id" => id} = params) do
-    with {:ok, command} <- Scripts.get(device.product, id),
+  def send(%{assigns: %{device: device}} = conn, %{"name_or_id" => name_or_id} = params) do
+    with {:ok, command} <- Scripts.get_by_product_and_name_with_id_fallback(device.product, name_or_id),
          {:ok, timeout} <- get_timeout_param(params),
          {:ok, io} <- Scripts.Runner.send(device, command, timeout) do
       text(conn, io)

lib/nerves_hub_web/router.ex

@@ -89,7 +89,7 @@ defmodule NervesHubWeb.Router do
       delete("/penalty", DeviceController, :penalty)
 
       get("/scripts", ScriptController, :index)
-      post("/scripts/:id", ScriptController, :send)
+      post("/scripts/:name_or_id", ScriptController, :send)
     end
 
     scope "/" do
@@ -154,7 +154,7 @@ defmodule NervesHubWeb.Router do
                   delete("/penalty", DeviceController, :penalty)
 
                   scope "/scripts", as: :device do
-                    post("/:id", ScriptController, :send)
+                    post("/:name_or_id", ScriptController, :send)
                   end
 
                   scope "/certificates" do

test/nerves_hub_web/controllers/api/script_controller_test.exs

@@ -1,5 +1,6 @@
 defmodule NervesHubWeb.API.ScriptControllerTest do
   use NervesHubWeb.APIConnCase, async: true
+  use Mimic
 
   alias NervesHub.Fixtures
 
@@ -44,4 +45,54 @@ defmodule NervesHubWeb.API.ScriptControllerTest do
       assert script_response["id"] == script_with_tags.id
     end
   end
+
+  describe "send" do
+    test "sends script to device by name", %{conn: conn, device: device, product: product, user: user} do
+      script = Fixtures.support_script_fixture(product, user, %{name: "test-script"})
+
+      path = Routes.api_script_path(conn, :send, device, script.name)
+
+      NervesHub.Scripts.Runner
+      |> expect(:send, fn _, _, _ -> {:ok, "hello"} end)
+
+      conn
+      |> post(path)
+      |> response(200)
+    end
+
+    test "sends script to device by id", %{conn: conn, device: device, product: product, user: user} do
+      script = Fixtures.support_script_fixture(product, user)
+
+      path = Routes.api_script_path(conn, :send, device, script.id)
+
+      NervesHub.Scripts.Runner
+      |> expect(:send, fn _, _, _ -> {:ok, "hello"} end)
+
+      conn
+      |> post(path)
+      |> response(200)
+    end
+
+    test "returns error when script not found by name", %{conn: conn, device: device} do
+      path = Routes.api_script_path(conn, :send, device, "nonexistent-script")
+
+      resp =
+        conn
+        |> post(path)
+        |> json_response(503)
+
+      assert resp == %{"errors" => %{"detail" => "not_found"}}
+    end
+
+    test "returns error when script not found by id", %{conn: conn, device: device} do
+      path = Routes.api_script_path(conn, :send, device, 99_999)
+
+      resp =
+        conn
+        |> post(path)
+        |> json_response(503)
+
+      assert resp == %{"errors" => %{"detail" => "not_found"}}
+    end
+  end
 end