From 8fd134dec2a42d693006662d6b3c11704ee678b9 Mon Sep 17 00:00:00 2001 From: Joe Clark Date: Fri, 10 Apr 2026 16:52:24 +0100 Subject: [PATCH 01/25] Scope collection name uniqueness to project Replaces the global unique index on collections.name with a per-project (project_id, name) index, allowing sandboxes to share collection names with their parent. Updates the schema constraint to match and adds conflict detection to get_collection/1, plus a new get_collection/2 that takes a project_id for unambiguous lookup. --- lib/lightning/collections.ex | 14 ++++++++++++-- lib/lightning/collections/collection.ex | 4 ++-- ...scope_collection_name_uniqueness_to_project.exs | 8 ++++++++ 3 files changed, 22 insertions(+), 4 deletions(-) create mode 100644 priv/repo/migrations/20260410000000_scope_collection_name_uniqueness_to_project.exs diff --git a/lib/lightning/collections.ex b/lib/lightning/collections.ex index d89671a8de5..682acec71a0 100644 --- a/lib/lightning/collections.ex +++ b/lib/lightning/collections.ex @@ -53,9 +53,19 @@ defmodule Lightning.Collections do end @spec get_collection(String.t()) :: - {:ok, Collection.t()} | {:error, :not_found} + {:ok, Collection.t()} | {:error, :not_found} | {:error, :conflict} def get_collection(name) do - case Repo.get_by(Collection, name: name) do + case Repo.all(from c in Collection, where: c.name == ^name) do + [] -> {:error, :not_found} + [collection] -> {:ok, collection} + [_ | _] -> {:error, :conflict} + end + end + + @spec get_collection(Ecto.UUID.t(), String.t()) :: + {:ok, Collection.t()} | {:error, :not_found} + def get_collection(project_id, name) do + case Repo.get_by(Collection, project_id: project_id, name: name) do nil -> {:error, :not_found} collection -> {:ok, collection} end diff --git a/lib/lightning/collections/collection.ex b/lib/lightning/collections/collection.ex index ecb4c1a0791..a400af9312f 100644 --- a/lib/lightning/collections/collection.ex +++ b/lib/lightning/collections/collection.ex @@ -40,7 +40,7 @@ defmodule Lightning.Collections.Collection do |> validate_format(:name, ~r/^[a-z0-9]+([\-_.][a-z0-9]+)*$/, message: "Collection name must be URL safe" ) - |> unique_constraint([:name], + |> unique_constraint([:project_id, :name], message: "A collection with this name already exists" ) end @@ -50,7 +50,7 @@ defmodule Lightning.Collections.Collection do |> validate_format(:name, ~r/^[a-z0-9]+([\-_.][a-z0-9]+)*$/, message: "Collection name must be URL safe" ) - |> unique_constraint([:name], + |> unique_constraint([:project_id, :name], message: "A collection with this name already exists" ) end diff --git a/priv/repo/migrations/20260410000000_scope_collection_name_uniqueness_to_project.exs b/priv/repo/migrations/20260410000000_scope_collection_name_uniqueness_to_project.exs new file mode 100644 index 00000000000..71eeb5e42a5 --- /dev/null +++ b/priv/repo/migrations/20260410000000_scope_collection_name_uniqueness_to_project.exs @@ -0,0 +1,8 @@ +defmodule Lightning.Repo.Migrations.ScopeCollectionNameUniquenessToProject do + use Ecto.Migration + + def change do + drop unique_index(:collections, [:name]) + create unique_index(:collections, [:project_id, :name]) + end +end From 4f5d3da6e51a4f3d25dd6a0836dbc494ebaa9915 Mon Sep 17 00:00:00 2001 From: Joe Clark Date: Fri, 10 Apr 2026 16:55:33 +0100 Subject: [PATCH 02/25] Return 409 when v1 collection lookup finds a name conflict Adds a :conflict clause to FallbackController so that when get_collection/1 finds the same collection name in multiple projects, all actions return a 409 with a message directing clients to use the v2 API. --- lib/lightning_web/controllers/fallback_controller.ex | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/lib/lightning_web/controllers/fallback_controller.ex b/lib/lightning_web/controllers/fallback_controller.ex index 6e3f3c590bf..fbf777d0f3a 100644 --- a/lib/lightning_web/controllers/fallback_controller.ex +++ b/lib/lightning_web/controllers/fallback_controller.ex @@ -28,6 +28,15 @@ defmodule LightningWeb.FallbackController do |> render(:"401") end + def call(conn, {:error, :conflict}) do + conn + |> put_status(:conflict) + |> json(%{ + error: + "Multiple collections found with this name. Use API v2 with a project_id." + }) + end + def call(conn, {:error, :forbidden}) do conn |> put_status(:forbidden) From 9eba1db50aad564e4e72cdb49ec70594f71d3c8a Mon Sep 17 00:00:00 2001 From: Joe Clark Date: Fri, 10 Apr 2026 16:59:55 +0100 Subject: [PATCH 03/25] Test v1 collection conflict detection Fixes the create_collection test to match the new constraint name (collections_project_id_name_index). Adds tests for get_collection/1 returning :conflict when the same name exists in multiple projects, and for get_collection/2 resolving unambiguously by project_id. Adds controller tests asserting all v1 endpoints return 409 when a name conflict exists. --- test/lightning/collections_test.exs | 35 ++++++++++++++- .../collections_controller_test.exs | 44 +++++++++++++++++++ 2 files changed, 77 insertions(+), 2 deletions(-) diff --git a/test/lightning/collections_test.exs b/test/lightning/collections_test.exs index 02b35505a0d..606e7c6c7ef 100644 --- a/test/lightning/collections_test.exs +++ b/test/lightning/collections_test.exs @@ -17,6 +17,37 @@ defmodule Lightning.CollectionsTest do assert {:error, :not_found} = Collections.get_collection("nonexistent") end + + test "returns a conflict error when the same name exists in multiple projects" do + name = "shared-name" + insert(:collection, name: name) + insert(:collection, name: name) + + assert {:error, :conflict} = Collections.get_collection(name) + end + end + + describe "get_collection/2" do + test "returns the collection for the given project" do + name = "shared-name" + %{id: project_id_1} = project_1 = insert(:project) + %{id: project_id_2} = project_2 = insert(:project) + %{id: id_1} = insert(:collection, name: name, project: project_1) + %{id: id_2} = insert(:collection, name: name, project: project_2) + + assert {:ok, %Collection{id: ^id_1}} = + Collections.get_collection(project_id_1, name) + + assert {:ok, %Collection{id: ^id_2}} = + Collections.get_collection(project_id_2, name) + end + + test "returns not_found when the collection does not exist in the project" do + %{id: project_id} = insert(:project) + + assert {:error, :not_found} = + Collections.get_collection(project_id, "nonexistent") + end end describe "create_collection/2" do @@ -44,11 +75,11 @@ defmodule Lightning.CollectionsTest do assert {:error, %{ errors: [ - name: + project_id: {"A collection with this name already exists", [ constraint: :unique, - constraint_name: "collections_name_index" + constraint_name: "collections_project_id_name_index" ]} ] }} = diff --git a/test/lightning_web/collections_controller_test.exs b/test/lightning_web/collections_controller_test.exs index 4451f58d2fe..4d8bc8bf62e 100644 --- a/test/lightning_web/collections_controller_test.exs +++ b/test/lightning_web/collections_controller_test.exs @@ -1071,6 +1071,50 @@ defmodule LightningWeb.API.CollectionsControllerTest do end end + describe "v1 conflict — same collection name in multiple projects" do + setup %{conn: conn} do + user = insert(:user) + project_1 = insert(:project, project_users: [%{user: user}]) + project_2 = insert(:project, project_users: [%{user: user}]) + name = "shared-collection" + insert(:collection, name: name, project: project_1) + insert(:collection, name: name, project: project_2) + token = Lightning.Accounts.generate_api_token(user) + conn = assign_bearer(conn, token) + {:ok, conn: conn, name: name} + end + + test "GET /:name returns 409", %{conn: conn, name: name} do + conn = get(conn, ~p"/collections/#{name}") + assert json_response(conn, 409)["error"] =~ "Use API v2" + end + + test "GET /:name/:key returns 409", %{conn: conn, name: name} do + conn = get(conn, ~p"/collections/#{name}/some-key") + assert json_response(conn, 409)["error"] =~ "Use API v2" + end + + test "PUT /:name/:key returns 409", %{conn: conn, name: name} do + conn = put(conn, ~p"/collections/#{name}/some-key", value: "val") + assert json_response(conn, 409)["error"] =~ "Use API v2" + end + + test "POST /:name returns 409", %{conn: conn, name: name} do + conn = post(conn, ~p"/collections/#{name}", items: []) + assert json_response(conn, 409)["error"] =~ "Use API v2" + end + + test "DELETE /:name/:key returns 409", %{conn: conn, name: name} do + conn = delete(conn, ~p"/collections/#{name}/some-key") + assert json_response(conn, 409)["error"] =~ "Use API v2" + end + + test "DELETE /:name returns 409", %{conn: conn, name: name} do + conn = delete(conn, ~p"/collections/#{name}") + assert json_response(conn, 409)["error"] =~ "Use API v2" + end + end + defp encode_decode(item) do item |> Jason.encode!() From 5271c41de6eeb7dc50ce73c3eb65b59c33924bf5 Mon Sep 17 00:00:00 2001 From: Joe Clark Date: Fri, 10 Apr 2026 17:05:53 +0100 Subject: [PATCH 04/25] Add v2 collections API with project-scoped routing Replaces the explicit collections routes with a single wildcard that dispatches to v1 or v2 logic based on the X-API-VERSION header. V2 routes include project_id in the path, resolving collections unambiguously without name-collision risk. Also migrates the download endpoint to v2 (/:project_id/:name) and updates the LiveView download link accordingly. --- .../controllers/collections_controller.ex | 231 +++++++++++++++--- .../project_live/collections_component.ex | 4 +- lib/lightning_web/router.ex | 14 +- .../collections_controller_test.exs | 21 +- 4 files changed, 216 insertions(+), 54 deletions(-) diff --git a/lib/lightning_web/controllers/collections_controller.ex b/lib/lightning_web/controllers/collections_controller.ex index a75398030fb..aad30b76909 100644 --- a/lib/lightning_web/controllers/collections_controller.ex +++ b/lib/lightning_web/controllers/collections_controller.ex @@ -25,6 +25,63 @@ defmodule LightningWeb.CollectionsController do @valid_params ["key", "cursor", "limit" | @timestamp_params] + defp api_version(conn) do + case get_req_header(conn, "x-api-version") do + ["2"] -> :v2 + _ -> :v1 + end + end + + def dispatch(conn, %{"path" => path}) do + case {api_version(conn), conn.method, path} do + # V1 + {:v1, "GET", [name]} -> + stream(conn, name) + + {:v1, "GET", [name, key]} -> + get(conn, name, key) + + {:v1, "PUT", [name, key]} -> + put(conn, name, key, conn.body_params) + + {:v1, "POST", [name]} -> + put_all(conn, name, conn.body_params) + + {:v1, "DELETE", [name, key]} -> + delete(conn, name, key) + + {:v1, "DELETE", [name]} -> + delete_all(conn, name, Map.merge(conn.body_params, conn.query_params)) + + # V2 + {:v2, "GET", [project_id, name]} -> + stream(conn, project_id, name) + + {:v2, "GET", [project_id, name, key]} -> + get(conn, project_id, name, key) + + {:v2, "PUT", [project_id, name, key]} -> + put(conn, project_id, name, key, conn.body_params) + + {:v2, "POST", [project_id, name]} -> + put_all(conn, project_id, name, conn.body_params) + + {:v2, "DELETE", [project_id, name, key]} -> + delete(conn, project_id, name, key) + + {:v2, "DELETE", [project_id, name]} -> + delete_all( + conn, + project_id, + name, + Map.merge(conn.body_params, conn.query_params) + ) + + _ -> + send_resp(conn, 404, "Not found") + end + end + defp authorize(conn, collection) do subject = conn.assigns[:subject] || conn.assigns[:current_user] @@ -36,8 +93,18 @@ defmodule LightningWeb.CollectionsController do ) end - def put(conn, %{"name" => col_name, "key" => key, "value" => value}) do - with {:ok, collection} <- Collections.get_collection(col_name), + defp resolve_collection(name) do + Collections.get_collection(name) + end + + defp resolve_collection(project_id, name) do + Collections.get_collection(project_id, name) + end + + # V1 actions + + def put(conn, name, key, %{"value" => value}) do + with {:ok, collection} <- resolve_collection(name), :ok <- authorize(conn, collection), :ok <- Collections.put(collection, key, value) do json(conn, %{upserted: 1, error: nil}) @@ -50,8 +117,8 @@ defmodule LightningWeb.CollectionsController do end end - def put_all(conn, %{"name" => col_name, "items" => items}) do - with {:ok, collection} <- Collections.get_collection(col_name), + def put_all(conn, name, %{"items" => items}) do + with {:ok, collection} <- resolve_collection(name), :ok <- authorize(conn, collection), {:ok, count} <- Collections.put_all(collection, items) do json(conn, %{upserted: count, error: nil}) @@ -66,21 +133,18 @@ defmodule LightningWeb.CollectionsController do end end - def get(conn, %{"name" => col_name, "key" => key}) do - with {:ok, collection} <- Collections.get_collection(col_name), + def get(conn, name, key) do + with {:ok, collection} <- resolve_collection(name), :ok <- authorize(conn, collection) do case Collections.get(collection, key) do - nil -> - resp(conn, :no_content, "") - - item -> - json(conn, item) + nil -> resp(conn, :no_content, "") + item -> json(conn, item) end end end - def delete(conn, %{"name" => col_name, "key" => key}) do - with {:ok, collection} <- Collections.get_collection(col_name), + def delete(conn, name, key) do + with {:ok, collection} <- resolve_collection(name), :ok <- authorize(conn, collection) do case Collections.delete(collection, key) do :ok -> @@ -92,19 +156,109 @@ defmodule LightningWeb.CollectionsController do end end - def delete_all(conn, %{"name" => col_name} = params) do - with {:ok, collection} <- Collections.get_collection(col_name), + def delete_all(conn, name, params) do + with {:ok, collection} <- resolve_collection(name), :ok <- authorize(conn, collection) do key_param = params["key"] - {:ok, n} = Collections.delete_all(collection, key_param) + json(conn, %{key: key_param, deleted: n, error: nil}) + end + end + def stream(conn, name) do + with {:ok, collection, filters} <- validate_query(conn, name) do + key_pattern = conn.query_params["key"] + items_stream = stream_all_in_chunks(collection, filters, key_pattern) + response_limit = Map.fetch!(filters, :limit) + + case stream_chunked(conn, items_stream, response_limit) do + {:error, conn} -> conn + {:ok, conn} -> conn + end + end + end + + # V2 actions + + def put(conn, project_id, name, key, %{"value" => value}) do + with {:ok, collection} <- resolve_collection(project_id, name), + :ok <- authorize(conn, collection), + :ok <- Collections.put(collection, key, value) do + json(conn, %{upserted: 1, error: nil}) + else + {:error, %Ecto.Changeset{}} -> + json(conn, %{upserted: 0, error: "Format error"}) + + error -> + maybe_handle_limit_error(conn, error) + end + end + + def put_all(conn, project_id, name, %{"items" => items}) do + with {:ok, collection} <- resolve_collection(project_id, name), + :ok <- authorize(conn, collection), + {:ok, count} <- Collections.put_all(collection, items) do + json(conn, %{upserted: count, error: nil}) + else + {:error, :duplicate_key} -> + conn + |> put_status(:unprocessable_entity) + |> json(%{upserted: 0, error: "Duplicate key found"}) + + error -> + maybe_handle_limit_error(conn, error) + end + end + + def get(conn, project_id, name, key) do + with {:ok, collection} <- resolve_collection(project_id, name), + :ok <- authorize(conn, collection) do + case Collections.get(collection, key) do + nil -> resp(conn, :no_content, "") + item -> json(conn, item) + end + end + end + + def delete(conn, project_id, name, key) do + with {:ok, collection} <- resolve_collection(project_id, name), + :ok <- authorize(conn, collection) do + case Collections.delete(collection, key) do + :ok -> + json(conn, %{key: key, deleted: 1, error: nil}) + + {:error, :not_found} -> + json(conn, %{key: key, deleted: 0, error: "Item Not Found"}) + end + end + end + + def delete_all(conn, project_id, name, params) do + with {:ok, collection} <- resolve_collection(project_id, name), + :ok <- authorize(conn, collection) do + key_param = params["key"] + {:ok, n} = Collections.delete_all(collection, key_param) json(conn, %{key: key_param, deleted: n, error: nil}) end end - def download(conn, %{"name" => col_name}) do - with {:ok, collection} <- Collections.get_collection(col_name), + def stream(conn, project_id, name) do + with {:ok, collection, filters} <- validate_query(conn, project_id, name) do + key_pattern = conn.query_params["key"] + items_stream = stream_all_in_chunks(collection, filters, key_pattern) + response_limit = Map.fetch!(filters, :limit) + + case stream_chunked(conn, items_stream, response_limit) do + {:error, conn} -> conn + {:ok, conn} -> conn + end + end + end + + # Download (browser pipeline, v2 only) + + def download(conn, %{"project_id" => project_id, "name" => col_name}) do + with {:ok, collection} <- resolve_collection(project_id, col_name), :ok <- authorize(conn, collection) do items_stream = stream_all_in_chunks( @@ -123,20 +277,6 @@ defmodule LightningWeb.CollectionsController do end end - def stream(conn, %{"name" => col_name} = params) do - with {:ok, collection, filters} <- validate_query(conn, col_name) do - key_pattern = Map.get(params, "key") - - items_stream = stream_all_in_chunks(collection, filters, key_pattern) - response_limit = Map.fetch!(filters, :limit) - - case stream_chunked(conn, items_stream, response_limit) do - {:error, conn} -> conn - {:ok, conn} -> conn - end - end - end - defp stream_as_json_array(conn, items_stream) do conn = send_chunked(conn, 200) {:ok, conn} = Plug.Conn.chunk(conn, "[") @@ -204,18 +344,31 @@ defmodule LightningWeb.CollectionsController do end defp validate_query(conn, col_name) do - with {:ok, collection} <- Collections.get_collection(col_name), + with {:ok, collection} <- resolve_collection(col_name), + :ok <- authorize(conn, collection), + {:ok, filters} <- parse_query_params(conn.query_params) do + {:ok, collection, filters} + end + end + + defp validate_query(conn, project_id, col_name) do + with {:ok, collection} <- resolve_collection(project_id, col_name), :ok <- authorize(conn, collection), - query_params <- - Enum.into(conn.query_params, %{ - "cursor" => nil, - "limit" => "#{@default_stream_limit}" - }), - {:ok, filters} <- validate_query_params(query_params) do + {:ok, filters} <- parse_query_params(conn.query_params) do {:ok, collection, filters} end end + defp parse_query_params(query_params) do + query_params = + Enum.into(query_params, %{ + "cursor" => nil, + "limit" => "#{@default_stream_limit}" + }) + + validate_query_params(query_params) + end + defp validate_query_params( %{"cursor" => cursor, "limit" => limit} = query_params ) do diff --git a/lib/lightning_web/live/project_live/collections_component.ex b/lib/lightning_web/live/project_live/collections_component.ex index 534dbca1b1f..e866b666c89 100644 --- a/lib/lightning_web/live/project_live/collections_component.ex +++ b/lib/lightning_web/live/project_live/collections_component.ex @@ -207,7 +207,9 @@ defmodule LightningWeb.ProjectLive.CollectionsComponent do
Download diff --git a/lib/lightning_web/router.ex b/lib/lightning_web/router.ex index ac2ce518878..d7963fb318c 100644 --- a/lib/lightning_web/router.ex +++ b/lib/lightning_web/router.ex @@ -114,13 +114,7 @@ defmodule LightningWeb.Router do ## Collections scope "/collections", LightningWeb do pipe_through [:authenticated_api] - - get "/:name", CollectionsController, :stream - get "/:name/:key", CollectionsController, :get - put "/:name/:key", CollectionsController, :put - post "/:name", CollectionsController, :put_all - delete "/:name/:key", CollectionsController, :delete - delete "/:name", CollectionsController, :delete_all + match :*, "/*path", CollectionsController, :dispatch end ## Authentication routes @@ -145,7 +139,11 @@ defmodule LightningWeb.Router do post "/users/two-factor", UserTOTPController, :create get "/setup_vcs", VersionControlController, :index get "/download/yaml", DownloadsController, :download_project_yaml - get "/download/collections/:name", CollectionsController, :download + + get "/download/collections/:project_id/:name", + CollectionsController, + :download + get "/dataclip/body/:id", DataclipController, :show get "/projects/:project_id/jobs/:job_id/dataclips", diff --git a/test/lightning_web/collections_controller_test.exs b/test/lightning_web/collections_controller_test.exs index 4d8bc8bf62e..6ff00b1516f 100644 --- a/test/lightning_web/collections_controller_test.exs +++ b/test/lightning_web/collections_controller_test.exs @@ -996,7 +996,7 @@ defmodule LightningWeb.API.CollectionsControllerTest do end end - describe "GET /download/collections/:name" do + describe "GET /download/collections/:project_id/:name" do setup :register_and_log_in_user setup :create_project_for_current_user @@ -1022,7 +1022,8 @@ defmodule LightningWeb.API.CollectionsControllerTest do value: ~s({"name": "Bob"}) ) - response = get(conn, ~p"/download/collections/#{collection.name}") + response = + get(conn, ~p"/download/collections/#{project.id}/#{collection.name}") assert response.status == 200 @@ -1049,7 +1050,8 @@ defmodule LightningWeb.API.CollectionsControllerTest do } do collection = insert(:collection, project: project) - response = get(conn, ~p"/download/collections/#{collection.name}") + response = + get(conn, ~p"/download/collections/#{project.id}/#{collection.name}") assert response.status == 200 assert Jason.decode!(response.resp_body) == [] @@ -1059,13 +1061,20 @@ defmodule LightningWeb.API.CollectionsControllerTest do other_project = insert(:project) collection = insert(:collection, project: other_project) - response = get(conn, ~p"/download/collections/#{collection.name}") + response = + get( + conn, + ~p"/download/collections/#{other_project.id}/#{collection.name}" + ) assert response.status == 401 end - test "returns 404 for non-existent collection", %{conn: conn} do - response = get(conn, ~p"/download/collections/non-existent") + test "returns 404 for non-existent collection", %{ + conn: conn, + project: project + } do + response = get(conn, ~p"/download/collections/#{project.id}/non-existent") assert response.status == 404 end From b01d53a4020d6c28b6c8e5a04d9481fc92eaacba Mon Sep 17 00:00:00 2001 From: Joe Clark Date: Fri, 10 Apr 2026 17:07:31 +0100 Subject: [PATCH 05/25] Add v2 collections API tests Covers all v2 endpoints (GET item, stream, PUT, POST, DELETE item, DELETE all), including access control with personal and run tokens, and the key scenario where v2 resolves correctly by project_id when the same collection name exists in multiple projects. --- .../collections_controller_test.exs | 197 ++++++++++++++++++ 1 file changed, 197 insertions(+) diff --git a/test/lightning_web/collections_controller_test.exs b/test/lightning_web/collections_controller_test.exs index 6ff00b1516f..50ea9f44670 100644 --- a/test/lightning_web/collections_controller_test.exs +++ b/test/lightning_web/collections_controller_test.exs @@ -1124,6 +1124,203 @@ defmodule LightningWeb.API.CollectionsControllerTest do end end + describe "v2 API" do + setup %{conn: conn} do + user = insert(:user) + project = insert(:project, project_users: [%{user: user}]) + + collection = + insert(:collection, + project: project, + items: [%{key: "foo", value: "bar"}] + ) + + token = Lightning.Accounts.generate_api_token(user) + conn = assign_bearer(conn, token) |> put_req_header("x-api-version", "2") + {:ok, conn: conn, project: project, collection: collection} + end + + test "GET /:project_id/:name/:key returns the item", %{ + conn: conn, + project: project, + collection: collection + } do + item = hd(collection.items) + + conn = get(conn, ~p"/collections/#{project.id}/#{collection.name}/foo") + + assert json_response(conn, 200) == %{ + "key" => item.key, + "value" => item.value, + "created" => DateTime.to_iso8601(item.inserted_at), + "updated" => DateTime.to_iso8601(item.updated_at) + } + end + + test "GET /:project_id/:name/:key returns 204 when item not found", %{ + conn: conn, + project: project, + collection: collection + } do + conn = + get(conn, ~p"/collections/#{project.id}/#{collection.name}/nonexistent") + + assert response(conn, 204) == "" + end + + test "GET /:project_id/:name/:key returns 404 when collection not found", %{ + conn: conn, + project: project + } do + conn = get(conn, ~p"/collections/#{project.id}/nonexistent-collection/foo") + assert json_response(conn, 404) + end + + test "GET /:project_id/:name streams items", %{ + conn: conn, + project: project, + collection: collection + } do + conn = get(conn, ~p"/collections/#{project.id}/#{collection.name}") + body = json_response(conn, 200) + assert length(body["items"]) == 1 + assert hd(body["items"])["key"] == "foo" + end + + test "PUT /:project_id/:name/:key upserts an item", %{ + conn: conn, + project: project, + collection: collection + } do + conn = + put(conn, ~p"/collections/#{project.id}/#{collection.name}/new-key", + value: "new-val" + ) + + assert json_response(conn, 200) == %{"upserted" => 1, "error" => nil} + end + + test "POST /:project_id/:name upserts multiple items", %{ + conn: conn, + project: project, + collection: collection + } do + conn = + post(conn, ~p"/collections/#{project.id}/#{collection.name}", + items: [%{key: "a", value: "1"}, %{key: "b", value: "2"}] + ) + + assert json_response(conn, 200) == %{"upserted" => 2, "error" => nil} + end + + test "DELETE /:project_id/:name/:key deletes an item", %{ + conn: conn, + project: project, + collection: collection + } do + conn = delete(conn, ~p"/collections/#{project.id}/#{collection.name}/foo") + + assert json_response(conn, 200) == %{ + "key" => "foo", + "deleted" => 1, + "error" => nil + } + end + + test "DELETE /:project_id/:name deletes all items", %{ + conn: conn, + project: project, + collection: collection + } do + conn = delete(conn, ~p"/collections/#{project.id}/#{collection.name}") + assert json_response(conn, 200)["deleted"] == 1 + end + + test "returns 401 when user does not have access to the project", %{ + conn: conn + } do + other_project = insert(:project) + other_collection = insert(:collection, project: other_project) + + conn = + get( + conn, + ~p"/collections/#{other_project.id}/#{other_collection.name}/foo" + ) + + assert json_response(conn, 401) + end + + test "resolves correctly by project_id when same name exists in multiple projects", + %{ + conn: conn, + project: project, + collection: collection + } do + other_project = insert(:project, project_users: []) + insert(:collection, name: collection.name, project: other_project) + + # v2 should resolve to the correct project's collection + conn = get(conn, ~p"/collections/#{project.id}/#{collection.name}/foo") + assert json_response(conn, 200)["key"] == "foo" + end + + test "with a valid run token, can access own project's collection", %{ + conn: conn, + project: project, + collection: collection + } do + workflow = insert(:simple_workflow, project: project) + + workorder = + insert(:workorder, workflow: workflow, dataclip: insert(:dataclip)) + + run = + insert(:run, + work_order: workorder, + dataclip: workorder.dataclip, + starting_trigger: hd(workflow.triggers) + ) + + token = Lightning.Workers.generate_run_token(run) + + conn = + conn + |> assign_bearer(token) + |> get(~p"/collections/#{project.id}/#{collection.name}/foo") + + assert json_response(conn, 200)["key"] == "foo" + end + + test "with a run token, cannot access a different project's collection", %{ + conn: conn + } do + other_project = insert(:project) + other_collection = insert(:collection, project: other_project) + + workflow = insert(:simple_workflow) + + workorder = + insert(:workorder, workflow: workflow, dataclip: insert(:dataclip)) + + run = + insert(:run, + work_order: workorder, + dataclip: workorder.dataclip, + starting_trigger: hd(workflow.triggers) + ) + + token = Lightning.Workers.generate_run_token(run) + + conn = + conn + |> assign_bearer(token) + |> get(~p"/collections/#{other_project.id}/#{other_collection.name}/foo") + + assert json_response(conn, 401) + end + end + defp encode_decode(item) do item |> Jason.encode!() From f769675d32708d188419b93bed44f91dd4c03e71 Mon Sep 17 00:00:00 2001 From: Joe Clark Date: Fri, 10 Apr 2026 17:10:04 +0100 Subject: [PATCH 06/25] Clone empty collections from parent when provisioning a sandbox Adds clone_collections_from_parent/2 to the sandbox provisioning pipeline. When a sandbox is forked, empty collection records matching each parent collection name are created in the new project. Items are not copied. Uses on_conflict: :nothing so re-provisioning is safe. --- lib/lightning/projects/sandboxes.ex | 24 +++++++++++ test/lightning/sandboxes_test.exs | 63 +++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+) diff --git a/lib/lightning/projects/sandboxes.ex b/lib/lightning/projects/sandboxes.ex index cebcd8edfcb..5a9ac9cd6fc 100644 --- a/lib/lightning/projects/sandboxes.ex +++ b/lib/lightning/projects/sandboxes.ex @@ -566,6 +566,30 @@ defmodule Lightning.Projects.Sandboxes do |> copy_workflow_version_history(sandbox.workflow_id_mapping) |> create_initial_workflow_snapshots() |> copy_selected_dataclips(parent.id, Map.get(original_attrs, :dataclip_ids)) + |> clone_collections_from_parent(parent) + end + + defp clone_collections_from_parent(sandbox, parent) do + parent_collections = Lightning.Collections.list_project_collections(parent) + current_time = DateTime.utc_now() |> DateTime.truncate(:second) + + rows = + Enum.map(parent_collections, fn c -> + %{ + id: Ecto.UUID.generate(), + name: c.name, + project_id: sandbox.id, + byte_size_sum: 0, + inserted_at: current_time, + updated_at: current_time + } + end) + + Repo.insert_all(Lightning.Collections.Collection, rows, + on_conflict: :nothing + ) + + sandbox end defp copy_workflow_version_history(sandbox, workflow_id_mapping) do diff --git a/test/lightning/sandboxes_test.exs b/test/lightning/sandboxes_test.exs index aaceac714af..f4513e9a4ae 100644 --- a/test/lightning/sandboxes_test.exs +++ b/test/lightning/sandboxes_test.exs @@ -547,6 +547,69 @@ defmodule Lightning.Projects.SandboxesTest do end end + describe "collections provisioning" do + test "clones empty collection records from parent into sandbox" do + actor = insert(:user) + parent = insert(:project) + ensure_member!(parent, actor, :owner) + + insert(:collection, project: parent, name: "col-a") + + insert(:collection, + project: parent, + name: "col-b", + items: [%{key: "k", value: "v"}] + ) + + {:ok, sandbox} = Sandboxes.provision(parent, actor, %{name: "sandbox-x"}) + + sandbox_collections = + Lightning.Collections.list_project_collections(sandbox) + + assert Enum.map(sandbox_collections, & &1.name) |> Enum.sort() == [ + "col-a", + "col-b" + ] + + # Items are not copied + Enum.each(sandbox_collections, fn col -> + assert Lightning.Collections.get_all( + col, + %{cursor: nil, limit: 100}, + nil + ) == + [] + end) + end + + test "provisioning a parent with no collections creates no sandbox collections" do + actor = insert(:user) + parent = insert(:project) + ensure_member!(parent, actor, :owner) + + {:ok, sandbox} = Sandboxes.provision(parent, actor, %{name: "sandbox-x"}) + + assert Lightning.Collections.list_project_collections(sandbox) == [] + end + + test "re-provisioning is idempotent (on_conflict: :nothing)" do + actor = insert(:user) + parent = insert(:project) + ensure_member!(parent, actor, :owner) + + insert(:collection, project: parent, name: "col-a") + + {:ok, sandbox_1} = Sandboxes.provision(parent, actor, %{name: "sandbox-1"}) + {:ok, sandbox_2} = Sandboxes.provision(parent, actor, %{name: "sandbox-2"}) + + assert length(Lightning.Collections.list_project_collections(sandbox_1)) == + 1 + + assert length(Lightning.Collections.list_project_collections(sandbox_2)) == + 1 + end + end + describe "keychains" do test "clones only used keychains and rewires jobs to cloned keychains" do %{ From cd54286ebcae29e9c8f615e5c041e8700cb23736 Mon Sep 17 00:00:00 2001 From: Joe Clark Date: Fri, 10 Apr 2026 17:22:16 +0100 Subject: [PATCH 07/25] Sync collection names when merging a sandbox into its parent Adds sync_collections/2, called after a successful merge. Collections present in the sandbox but not the parent are created (empty) in the parent. Collections present in the parent but not the sandbox are deleted from the parent along with all their items. Collections in both are left untouched. Data is never merged. --- lib/lightning_web/live/sandbox_live/index.ex | 39 ++++++ .../live/sandbox_live/index_test.exs | 121 ++++++++++++++++++ 2 files changed, 160 insertions(+) diff --git a/lib/lightning_web/live/sandbox_live/index.ex b/lib/lightning_web/live/sandbox_live/index.ex index 33110a574d3..1b5fc2d7b6d 100644 --- a/lib/lightning_web/live/sandbox_live/index.ex +++ b/lib/lightning_web/live/sandbox_live/index.ex @@ -806,6 +806,7 @@ defmodule LightningWeb.SandboxLive.Index do case result do {:ok, _updated_target} = success -> + sync_collections(source, target) maybe_commit_to_github(target, "Merged sandbox #{source.name}") success @@ -814,6 +815,44 @@ defmodule LightningWeb.SandboxLive.Index do end end + defp sync_collections(sandbox, parent) do + sandbox_collections = Lightning.Collections.list_project_collections(sandbox) + parent_collections = Lightning.Collections.list_project_collections(parent) + + sandbox_names = MapSet.new(sandbox_collections, & &1.name) + parent_names = MapSet.new(parent_collections, & &1.name) + + to_create = MapSet.difference(sandbox_names, parent_names) + + if not Enum.empty?(to_create) do + current_time = DateTime.utc_now() |> DateTime.truncate(:second) + + rows = + Enum.map(to_create, fn name -> + %{ + id: Ecto.UUID.generate(), + name: name, + project_id: parent.id, + byte_size_sum: 0, + inserted_at: current_time, + updated_at: current_time + } + end) + + Repo.insert_all(Lightning.Collections.Collection, rows, + on_conflict: :nothing + ) + end + + to_delete = + parent_collections + |> Enum.filter( + &(&1.name in MapSet.difference(parent_names, sandbox_names)) + ) + + Enum.each(to_delete, &Lightning.Collections.delete_collection(&1.id)) + end + defp maybe_commit_to_github(project, commit_message) do with %{} = repo_connection <- VersionControl.get_repo_connection_for_project(project.id) do diff --git a/test/lightning_web/live/sandbox_live/index_test.exs b/test/lightning_web/live/sandbox_live/index_test.exs index 75a1e038430..3c52b8a67e1 100644 --- a/test/lightning_web/live/sandbox_live/index_test.exs +++ b/test/lightning_web/live/sandbox_live/index_test.exs @@ -1482,6 +1482,127 @@ defmodule LightningWeb.SandboxLive.IndexTest do end end + describe "collection sync on merge" do + setup :register_and_log_in_user + + setup %{user: user} do + root = + insert(:project, + name: "root", + project_users: [%{user: user, role: :owner}] + ) + + sandbox = + insert(:project, + name: "sandbox", + parent: root, + project_users: [%{user: user, role: :owner}] + ) + + {:ok, root: root, sandbox: sandbox} + end + + defp mock_provisioner_ok(target) do + Mimic.expect(Lightning.Projects.MergeProjects, :merge_project, fn _src, + _tgt, + _opts -> + "merged_yaml" + end) + + Mimic.expect(Lightning.Projects.Provisioner, :import_document, fn _tgt, + _actor, + _yaml, + _opts -> + {:ok, target} + end) + + Mimic.expect(Lightning.Projects, :delete_sandbox, fn source, _actor -> + {:ok, source} + end) + end + + test "new collections in sandbox are created in parent on merge", %{ + conn: conn, + root: root, + sandbox: sandbox + } do + insert(:collection, project: sandbox, name: "new-col") + + {:ok, view, _} = live(conn, ~p"/projects/#{root.id}/sandboxes") + mock_provisioner_ok(root) + + Mimic.allow(Lightning.Projects.MergeProjects, self(), view.pid) + Mimic.allow(Lightning.Projects.Provisioner, self(), view.pid) + Mimic.allow(Lightning.Projects, self(), view.pid) + + view + |> element("#branch-rewire-sandbox-#{sandbox.id} button") + |> render_click() + + view |> form("#merge-sandbox-modal form") |> render_submit() + + parent_names = + Lightning.Collections.list_project_collections(root) + |> Enum.map(& &1.name) + + assert "new-col" in parent_names + end + + test "collections deleted from sandbox are removed from parent on merge", %{ + conn: conn, + root: root, + sandbox: sandbox + } do + # Parent has a collection, sandbox does not + insert(:collection, project: root, name: "to-delete") + + {:ok, view, _} = live(conn, ~p"/projects/#{root.id}/sandboxes") + mock_provisioner_ok(root) + + Mimic.allow(Lightning.Projects.MergeProjects, self(), view.pid) + Mimic.allow(Lightning.Projects.Provisioner, self(), view.pid) + Mimic.allow(Lightning.Projects, self(), view.pid) + + view + |> element("#branch-rewire-sandbox-#{sandbox.id} button") + |> render_click() + + view |> form("#merge-sandbox-modal form") |> render_submit() + + parent_names = + Lightning.Collections.list_project_collections(root) + |> Enum.map(& &1.name) + + refute "to-delete" in parent_names + end + + test "collections present in both are unchanged after merge", %{ + conn: conn, + root: root, + sandbox: sandbox + } do + insert(:collection, project: root, name: "shared") + insert(:collection, project: sandbox, name: "shared") + + {:ok, view, _} = live(conn, ~p"/projects/#{root.id}/sandboxes") + mock_provisioner_ok(root) + + Mimic.allow(Lightning.Projects.MergeProjects, self(), view.pid) + Mimic.allow(Lightning.Projects.Provisioner, self(), view.pid) + Mimic.allow(Lightning.Projects, self(), view.pid) + + view + |> element("#branch-rewire-sandbox-#{sandbox.id} button") + |> render_click() + + view |> form("#merge-sandbox-modal form") |> render_submit() + + parent_collections = Lightning.Collections.list_project_collections(root) + assert length(parent_collections) == 1 + assert hd(parent_collections).name == "shared" + end + end + describe "Merge modal authorization" do setup :register_and_log_in_user From c479e38021e8fd53b128789996704670b6e40339 Mon Sep 17 00:00:00 2001 From: Joe Clark Date: Fri, 10 Apr 2026 17:29:41 +0100 Subject: [PATCH 08/25] Warn about collection sync behavior in merge modal Extends the merge confirmation warning to explain that collection names will be synchronized: new sandbox collections are created empty in the target, and collections deleted in the sandbox are permanently removed from the target including all their data. --- lib/lightning_web/live/sandbox_live/components.ex | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/lightning_web/live/sandbox_live/components.ex b/lib/lightning_web/live/sandbox_live/components.ex index f6e20e2fe61..9c065250ff3 100644 --- a/lib/lightning_web/live/sandbox_live/components.ex +++ b/lib/lightning_web/live/sandbox_live/components.ex @@ -382,6 +382,8 @@ defmodule LightningWeb.SandboxLive.Components do > <:message> Sandbox merging is in beta. For production projects, use the CLI to merge locally and preview changes first. + Collection names will be synchronized: new collections in the sandbox will be created (empty) in the target, + and collections deleted in the sandbox will be permanently removed from the target along with all their data. From 067c749a2e3a3501ea714060bea3b18c4e427b84 Mon Sep 17 00:00:00 2001 From: Joe Clark Date: Fri, 10 Apr 2026 17:54:27 +0100 Subject: [PATCH 09/25] Split dispatch/2 into dispatch_v1 and dispatch_v2 to satisfy credo The single dispatch function had a cyclomatic complexity of 14 (max 9). Splitting by version brings each function to 7 branches. --- .../controllers/collections_controller.ex | 54 +++++++++++-------- 1 file changed, 33 insertions(+), 21 deletions(-) diff --git a/lib/lightning_web/controllers/collections_controller.ex b/lib/lightning_web/controllers/collections_controller.ex index aad30b76909..c074b383f84 100644 --- a/lib/lightning_web/controllers/collections_controller.ex +++ b/lib/lightning_web/controllers/collections_controller.ex @@ -33,46 +33,58 @@ defmodule LightningWeb.CollectionsController do end def dispatch(conn, %{"path" => path}) do - case {api_version(conn), conn.method, path} do - # V1 - {:v1, "GET", [name]} -> + case api_version(conn) do + :v1 -> dispatch_v1(conn, conn.method, path) + :v2 -> dispatch_v2(conn, conn.method, path) + end + end + + defp dispatch_v1(conn, method, path) do + case {method, path} do + {"GET", [name]} -> stream(conn, name) - {:v1, "GET", [name, key]} -> + {"GET", [name, key]} -> get(conn, name, key) - {:v1, "PUT", [name, key]} -> + {"PUT", [name, key]} -> put(conn, name, key, conn.body_params) - {:v1, "POST", [name]} -> + {"POST", [name]} -> put_all(conn, name, conn.body_params) - {:v1, "DELETE", [name, key]} -> + {"DELETE", [name, key]} -> delete(conn, name, key) - {:v1, "DELETE", [name]} -> + {"DELETE", [name]} -> delete_all(conn, name, Map.merge(conn.body_params, conn.query_params)) - # V2 - {:v2, "GET", [project_id, name]} -> - stream(conn, project_id, name) + _ -> + send_resp(conn, 404, "Not found") + end + end + + defp dispatch_v2(conn, method, path) do + case {method, path} do + {"GET", [pid, name]} -> + stream(conn, pid, name) - {:v2, "GET", [project_id, name, key]} -> - get(conn, project_id, name, key) + {"GET", [pid, name, key]} -> + get(conn, pid, name, key) - {:v2, "PUT", [project_id, name, key]} -> - put(conn, project_id, name, key, conn.body_params) + {"PUT", [pid, name, key]} -> + put(conn, pid, name, key, conn.body_params) - {:v2, "POST", [project_id, name]} -> - put_all(conn, project_id, name, conn.body_params) + {"POST", [pid, name]} -> + put_all(conn, pid, name, conn.body_params) - {:v2, "DELETE", [project_id, name, key]} -> - delete(conn, project_id, name, key) + {"DELETE", [pid, name, key]} -> + delete(conn, pid, name, key) - {:v2, "DELETE", [project_id, name]} -> + {"DELETE", [pid, name]} -> delete_all( conn, - project_id, + pid, name, Map.merge(conn.body_params, conn.query_params) ) From 5c7cdbeb44488687eea68e8e0d80ab9ebc6bfb09 Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Mon, 13 Apr 2026 16:19:36 +0000 Subject: [PATCH 10/25] Refactor collections API for sandboxes Address review feedback on the sandbox collections work: - Add a plug that validates the x-api-version header early and returns 400 for unsupported values instead of silently falling back to v1 - Consolidate v1/v2 controller actions through a shared resolve/1 helper, dropping ~100 lines of duplication - Return 422 with a clear error on missing value/items in request body (previously raised FunctionClauseError) - Move sync_collections into the Sandboxes context, wrap creates and deletes in a single transaction, and replace the N delete_collection calls with a single batch delete_all - Scope the collection unique constraint error to :name instead of :project_id so the user-facing field reports the conflict - Tone down the merge warning copy while keeping the essential info Adds tests for unsupported/garbage x-api-version values, malformed request bodies, and transaction rollback behavior for sync_collections. --- lib/lightning/collections/collection.ex | 6 +- lib/lightning/projects/sandboxes.ex | 89 ++++- .../controllers/collections_controller.ex | 319 ++++++++---------- .../live/sandbox_live/components.ex | 3 +- lib/lightning_web/live/sandbox_live/index.ex | 41 +-- lib/lightning_web/plugs/api_version.ex | 40 +++ lib/lightning_web/router.ex | 2 +- test/lightning/collections_test.exs | 2 +- test/lightning/sandboxes_test.exs | 111 ++++++ .../collections_controller_test.exs | 75 ++++ 10 files changed, 443 insertions(+), 245 deletions(-) create mode 100644 lib/lightning_web/plugs/api_version.ex diff --git a/lib/lightning/collections/collection.ex b/lib/lightning/collections/collection.ex index a400af9312f..fc6b947b2f2 100644 --- a/lib/lightning/collections/collection.ex +++ b/lib/lightning/collections/collection.ex @@ -40,7 +40,8 @@ defmodule Lightning.Collections.Collection do |> validate_format(:name, ~r/^[a-z0-9]+([\-_.][a-z0-9]+)*$/, message: "Collection name must be URL safe" ) - |> unique_constraint([:project_id, :name], + |> unique_constraint(:name, + name: :collections_project_id_name_index, message: "A collection with this name already exists" ) end @@ -50,7 +51,8 @@ defmodule Lightning.Collections.Collection do |> validate_format(:name, ~r/^[a-z0-9]+([\-_.][a-z0-9]+)*$/, message: "Collection name must be URL safe" ) - |> unique_constraint([:project_id, :name], + |> unique_constraint(:name, + name: :collections_project_id_name_index, message: "A collection with this name already exists" ) end diff --git a/lib/lightning/projects/sandboxes.ex b/lib/lightning/projects/sandboxes.ex index 5a9ac9cd6fc..a34390ad748 100644 --- a/lib/lightning/projects/sandboxes.ex +++ b/lib/lightning/projects/sandboxes.ex @@ -36,6 +36,8 @@ defmodule Lightning.Projects.Sandboxes do import Ecto.Query alias Lightning.Accounts.User + alias Lightning.Collections + alias Lightning.Collections.Collection alias Lightning.Credentials.KeychainCredential alias Lightning.Policies.Permissions alias Lightning.Projects.Project @@ -570,26 +572,79 @@ defmodule Lightning.Projects.Sandboxes do end defp clone_collections_from_parent(sandbox, parent) do - parent_collections = Lightning.Collections.list_project_collections(parent) - current_time = DateTime.utc_now() |> DateTime.truncate(:second) + parent_names = parent |> Collections.list_project_collections() |> names() + insert_empty_collections(sandbox.id, parent_names) + sandbox + end - rows = - Enum.map(parent_collections, fn c -> - %{ - id: Ecto.UUID.generate(), - name: c.name, - project_id: sandbox.id, - byte_size_sum: 0, - inserted_at: current_time, - updated_at: current_time - } - end) + @doc """ + Synchronises collection names from a sandbox to its merge target. - Repo.insert_all(Lightning.Collections.Collection, rows, - on_conflict: :nothing - ) + After a successful merge, this brings the target's set of collections in + line with the sandbox's: - sandbox + * Collections present in the sandbox but missing from the target are + created (empty) in the target. + * Collections present in the target but missing from the sandbox are + deleted from the target, along with all their items. + + **Collection data is never copied or merged.** Only the set of collection + names is synchronised, mirroring the sandbox-is-for-configuration model. + + The create and delete operations run in a single transaction; a failure + leaves the target's collections unchanged. + """ + @spec sync_collections(Project.t(), Project.t()) :: + {:ok, %{created: non_neg_integer(), deleted: non_neg_integer()}} + | {:error, term()} + def sync_collections(%Project{} = source, %Project{} = target) do + source_names = source |> Collections.list_project_collections() |> names() + + target_collections = Collections.list_project_collections(target) + target_names = names(target_collections) + + to_create = MapSet.difference(source_names, target_names) + + to_delete_ids = + for c <- target_collections, + c.name in MapSet.difference(target_names, source_names), + do: c.id + + Repo.transaction(fn -> + {created, _} = insert_empty_collections(target.id, to_create) + {deleted, _} = delete_collections(to_delete_ids) + %{created: created, deleted: deleted} + end) + end + + defp names(collections), do: MapSet.new(collections, & &1.name) + + defp insert_empty_collections(project_id, names) do + if Enum.empty?(names) do + {0, nil} + else + now = DateTime.utc_now() |> DateTime.truncate(:second) + + rows = + Enum.map(names, fn name -> + %{ + id: Ecto.UUID.generate(), + name: name, + project_id: project_id, + byte_size_sum: 0, + inserted_at: now, + updated_at: now + } + end) + + Repo.insert_all(Collection, rows, on_conflict: :nothing) + end + end + + defp delete_collections([]), do: {0, nil} + + defp delete_collections(ids) do + Repo.delete_all(from c in Collection, where: c.id in ^ids) end defp copy_workflow_version_history(sandbox, workflow_id_mapping) do diff --git a/lib/lightning_web/controllers/collections_controller.ex b/lib/lightning_web/controllers/collections_controller.ex index c074b383f84..1fe3caff4ea 100644 --- a/lib/lightning_web/controllers/collections_controller.ex +++ b/lib/lightning_web/controllers/collections_controller.ex @@ -2,6 +2,7 @@ defmodule LightningWeb.CollectionsController do use LightningWeb, :controller alias Lightning.Collections + alias Lightning.Collections.Collection alias Lightning.Extensions.Message alias Lightning.Policies.Permissions @@ -25,175 +26,102 @@ defmodule LightningWeb.CollectionsController do @valid_params ["key", "cursor", "limit" | @timestamp_params] - defp api_version(conn) do - case get_req_header(conn, "x-api-version") do - ["2"] -> :v2 - _ -> :v1 - end - end - - def dispatch(conn, %{"path" => path}) do - case api_version(conn) do - :v1 -> dispatch_v1(conn, conn.method, path) - :v2 -> dispatch_v2(conn, conn.method, path) - end - end - - defp dispatch_v1(conn, method, path) do - case {method, path} do - {"GET", [name]} -> - stream(conn, name) - - {"GET", [name, key]} -> - get(conn, name, key) - - {"PUT", [name, key]} -> - put(conn, name, key, conn.body_params) - - {"POST", [name]} -> - put_all(conn, name, conn.body_params) - - {"DELETE", [name, key]} -> - delete(conn, name, key) - - {"DELETE", [name]} -> - delete_all(conn, name, Map.merge(conn.body_params, conn.query_params)) - - _ -> - send_resp(conn, 404, "Not found") - end - end - - defp dispatch_v2(conn, method, path) do - case {method, path} do - {"GET", [pid, name]} -> - stream(conn, pid, name) + @doc """ + Entry point for all collection API routes. - {"GET", [pid, name, key]} -> - get(conn, pid, name, key) + API version is resolved by `LightningWeb.Plugs.ApiVersion` and stored in + `conn.assigns.api_version`. Path shape is then used to route to the + appropriate action: - {"PUT", [pid, name, key]} -> - put(conn, pid, name, key, conn.body_params) + * V1 is name-scoped: `/:name`, `/:name/:key` + * V2 is project-scoped: `/:project_id/:name`, `/:project_id/:name/:key` - {"POST", [pid, name]} -> - put_all(conn, pid, name, conn.body_params) + The catch-all route is required because v1 `/:name/:key` and v2 + `/:project_id/:name` collide at Phoenix router level. + """ + @spec dispatch(Plug.Conn.t(), map()) :: Plug.Conn.t() + def dispatch(%{assigns: %{api_version: :v1}} = conn, %{"path" => path}), + do: dispatch_v1(conn, conn.method, path) - {"DELETE", [pid, name, key]} -> - delete(conn, pid, name, key) + def dispatch(%{assigns: %{api_version: :v2}} = conn, %{"path" => path}), + do: dispatch_v2(conn, conn.method, path) - {"DELETE", [pid, name]} -> - delete_all( - conn, - pid, - name, - Map.merge(conn.body_params, conn.query_params) - ) + defp dispatch_v1(conn, "GET", [name]), + do: stream(conn, %{"name" => name}) - _ -> - send_resp(conn, 404, "Not found") - end - end + defp dispatch_v1(conn, "GET", [name, key]), + do: get(conn, %{"name" => name, "key" => key}) - defp authorize(conn, collection) do - subject = conn.assigns[:subject] || conn.assigns[:current_user] + defp dispatch_v1(conn, "PUT", [name, key]), + do: put(conn, body_with(conn, %{"name" => name, "key" => key})) - Permissions.can( - Lightning.Policies.Collections, - :access_collection, - subject, - collection - ) - end + defp dispatch_v1(conn, "POST", [name]), + do: put_all(conn, body_with(conn, %{"name" => name})) - defp resolve_collection(name) do - Collections.get_collection(name) - end - - defp resolve_collection(project_id, name) do - Collections.get_collection(project_id, name) - end + defp dispatch_v1(conn, "DELETE", [name, key]), + do: delete(conn, %{"name" => name, "key" => key}) - # V1 actions + defp dispatch_v1(conn, "DELETE", [name]), + do: delete_all(conn, params_with(conn, %{"name" => name})) - def put(conn, name, key, %{"value" => value}) do - with {:ok, collection} <- resolve_collection(name), - :ok <- authorize(conn, collection), - :ok <- Collections.put(collection, key, value) do - json(conn, %{upserted: 1, error: nil}) - else - {:error, %Ecto.Changeset{}} -> - json(conn, %{upserted: 0, error: "Format error"}) + defp dispatch_v1(conn, _method, _path), do: not_found(conn) - error -> - maybe_handle_limit_error(conn, error) - end - end + defp dispatch_v2(conn, "GET", [project_id, name]), + do: stream(conn, %{"project_id" => project_id, "name" => name}) - def put_all(conn, name, %{"items" => items}) do - with {:ok, collection} <- resolve_collection(name), - :ok <- authorize(conn, collection), - {:ok, count} <- Collections.put_all(collection, items) do - json(conn, %{upserted: count, error: nil}) - else - {:error, :duplicate_key} -> - conn - |> put_status(:unprocessable_entity) - |> json(%{upserted: 0, error: "Duplicate key found"}) + defp dispatch_v2(conn, "GET", [project_id, name, key]), + do: get(conn, %{"project_id" => project_id, "name" => name, "key" => key}) - error -> - maybe_handle_limit_error(conn, error) - end - end + defp dispatch_v2(conn, "PUT", [project_id, name, key]), + do: + put( + conn, + body_with(conn, %{ + "project_id" => project_id, + "name" => name, + "key" => key + }) + ) - def get(conn, name, key) do - with {:ok, collection} <- resolve_collection(name), - :ok <- authorize(conn, collection) do - case Collections.get(collection, key) do - nil -> resp(conn, :no_content, "") - item -> json(conn, item) - end - end - end + defp dispatch_v2(conn, "POST", [project_id, name]), + do: + put_all( + conn, + body_with(conn, %{"project_id" => project_id, "name" => name}) + ) - def delete(conn, name, key) do - with {:ok, collection} <- resolve_collection(name), - :ok <- authorize(conn, collection) do - case Collections.delete(collection, key) do - :ok -> - json(conn, %{key: key, deleted: 1, error: nil}) + defp dispatch_v2(conn, "DELETE", [project_id, name, key]), + do: + delete(conn, %{ + "project_id" => project_id, + "name" => name, + "key" => key + }) - {:error, :not_found} -> - json(conn, %{key: key, deleted: 0, error: "Item Not Found"}) - end - end - end + defp dispatch_v2(conn, "DELETE", [project_id, name]), + do: + delete_all( + conn, + params_with(conn, %{"project_id" => project_id, "name" => name}) + ) - def delete_all(conn, name, params) do - with {:ok, collection} <- resolve_collection(name), - :ok <- authorize(conn, collection) do - key_param = params["key"] - {:ok, n} = Collections.delete_all(collection, key_param) - json(conn, %{key: key_param, deleted: n, error: nil}) - end - end + defp dispatch_v2(conn, _method, _path), do: not_found(conn) - def stream(conn, name) do - with {:ok, collection, filters} <- validate_query(conn, name) do - key_pattern = conn.query_params["key"] - items_stream = stream_all_in_chunks(collection, filters, key_pattern) - response_limit = Map.fetch!(filters, :limit) + # Merges `extra` into the request body params — used for PUT/POST where we + # need to combine the JSON body with values parsed from the URL. + defp body_with(conn, extra), do: Map.merge(conn.body_params, extra) - case stream_chunked(conn, items_stream, response_limit) do - {:error, conn} -> conn - {:ok, conn} -> conn - end - end + # Merges `extra` into the combined query + body params — used for DELETE + # endpoints that accept a `key` filter in either place. + defp params_with(conn, extra) do + conn.query_params |> Map.merge(conn.body_params) |> Map.merge(extra) end - # V2 actions + defp not_found(conn), do: send_resp(conn, 404, "Not found") - def put(conn, project_id, name, key, %{"value" => value}) do - with {:ok, collection} <- resolve_collection(project_id, name), + @spec put(Plug.Conn.t(), map()) :: Plug.Conn.t() + def put(conn, %{"key" => key, "value" => value} = params) do + with {:ok, collection} <- resolve(params), :ok <- authorize(conn, collection), :ok <- Collections.put(collection, key, value) do json(conn, %{upserted: 1, error: nil}) @@ -206,8 +134,11 @@ defmodule LightningWeb.CollectionsController do end end - def put_all(conn, project_id, name, %{"items" => items}) do - with {:ok, collection} <- resolve_collection(project_id, name), + def put(conn, _params), do: missing_body(conn, "value") + + @spec put_all(Plug.Conn.t(), map()) :: Plug.Conn.t() + def put_all(conn, %{"items" => items} = params) do + with {:ok, collection} <- resolve(params), :ok <- authorize(conn, collection), {:ok, count} <- Collections.put_all(collection, items) do json(conn, %{upserted: count, error: nil}) @@ -222,8 +153,11 @@ defmodule LightningWeb.CollectionsController do end end - def get(conn, project_id, name, key) do - with {:ok, collection} <- resolve_collection(project_id, name), + def put_all(conn, _params), do: missing_body(conn, "items") + + @spec get(Plug.Conn.t(), map()) :: Plug.Conn.t() + def get(conn, %{"key" => key} = params) do + with {:ok, collection} <- resolve(params), :ok <- authorize(conn, collection) do case Collections.get(collection, key) do nil -> resp(conn, :no_content, "") @@ -232,8 +166,9 @@ defmodule LightningWeb.CollectionsController do end end - def delete(conn, project_id, name, key) do - with {:ok, collection} <- resolve_collection(project_id, name), + @spec delete(Plug.Conn.t(), map()) :: Plug.Conn.t() + def delete(conn, %{"key" => key} = params) do + with {:ok, collection} <- resolve(params), :ok <- authorize(conn, collection) do case Collections.delete(collection, key) do :ok -> @@ -245,8 +180,9 @@ defmodule LightningWeb.CollectionsController do end end - def delete_all(conn, project_id, name, params) do - with {:ok, collection} <- resolve_collection(project_id, name), + @spec delete_all(Plug.Conn.t(), map()) :: Plug.Conn.t() + def delete_all(conn, params) do + with {:ok, collection} <- resolve(params), :ok <- authorize(conn, collection) do key_param = params["key"] {:ok, n} = Collections.delete_all(collection, key_param) @@ -254,8 +190,11 @@ defmodule LightningWeb.CollectionsController do end end - def stream(conn, project_id, name) do - with {:ok, collection, filters} <- validate_query(conn, project_id, name) do + @spec stream(Plug.Conn.t(), map()) :: Plug.Conn.t() + def stream(conn, params) do + with {:ok, collection} <- resolve(params), + :ok <- authorize(conn, collection), + {:ok, filters} <- parse_query_params(conn.query_params) do key_pattern = conn.query_params["key"] items_stream = stream_all_in_chunks(collection, filters, key_pattern) response_limit = Map.fetch!(filters, :limit) @@ -267,10 +206,14 @@ defmodule LightningWeb.CollectionsController do end end - # Download (browser pipeline, v2 only) + @doc """ + Browser-pipeline download for a project-scoped collection. - def download(conn, %{"project_id" => project_id, "name" => col_name}) do - with {:ok, collection} <- resolve_collection(project_id, col_name), + Always v2 since the UI links to project-scoped download URLs. + """ + @spec download(Plug.Conn.t(), map()) :: Plug.Conn.t() + def download(conn, %{"project_id" => project_id, "name" => name}) do + with {:ok, collection} <- Collections.get_collection(project_id, name), :ok <- authorize(conn, collection) do items_stream = stream_all_in_chunks( @@ -283,12 +226,39 @@ defmodule LightningWeb.CollectionsController do |> put_resp_content_type("application/json") |> put_resp_header( "content-disposition", - ~s(attachment; filename="#{col_name}.json") + ~s(attachment; filename="#{name}.json") ) |> stream_as_json_array(items_stream) end end + # Resolves a collection by project + name when project_id is present, or by + # name alone otherwise. Any `{:error, reason}` is rendered by the fallback + # controller (404 for `:not_found`, 409 for `:conflict`). + @spec resolve(map()) :: {:ok, Collection.t()} | {:error, atom()} + defp resolve(%{"project_id" => project_id, "name" => name}), + do: Collections.get_collection(project_id, name) + + defp resolve(%{"name" => name}), + do: Collections.get_collection(name) + + defp authorize(conn, collection) do + subject = conn.assigns[:subject] || conn.assigns[:current_user] + + Permissions.can( + Lightning.Policies.Collections, + :access_collection, + subject, + collection + ) + end + + defp missing_body(conn, field) do + conn + |> put_status(:unprocessable_entity) + |> json(%{error: "Missing required field: #{field}"}) + end + defp stream_as_json_array(conn, items_stream) do conn = send_chunked(conn, 200) {:ok, conn} = Plug.Conn.chunk(conn, "[") @@ -355,30 +325,13 @@ defmodule LightningWeb.CollectionsController do end end - defp validate_query(conn, col_name) do - with {:ok, collection} <- resolve_collection(col_name), - :ok <- authorize(conn, collection), - {:ok, filters} <- parse_query_params(conn.query_params) do - {:ok, collection, filters} - end - end - - defp validate_query(conn, project_id, col_name) do - with {:ok, collection} <- resolve_collection(project_id, col_name), - :ok <- authorize(conn, collection), - {:ok, filters} <- parse_query_params(conn.query_params) do - {:ok, collection, filters} - end - end - defp parse_query_params(query_params) do - query_params = - Enum.into(query_params, %{ - "cursor" => nil, - "limit" => "#{@default_stream_limit}" - }) - - validate_query_params(query_params) + query_params + |> Enum.into(%{ + "cursor" => nil, + "limit" => "#{@default_stream_limit}" + }) + |> validate_query_params() end defp validate_query_params( diff --git a/lib/lightning_web/live/sandbox_live/components.ex b/lib/lightning_web/live/sandbox_live/components.ex index 9c065250ff3..44d4713bd0e 100644 --- a/lib/lightning_web/live/sandbox_live/components.ex +++ b/lib/lightning_web/live/sandbox_live/components.ex @@ -382,8 +382,7 @@ defmodule LightningWeb.SandboxLive.Components do > <:message> Sandbox merging is in beta. For production projects, use the CLI to merge locally and preview changes first. - Collection names will be synchronized: new collections in the sandbox will be created (empty) in the target, - and collections deleted in the sandbox will be permanently removed from the target along with all their data. + Collection names will be synced: new collections are added (empty) to the target, and collections missing from the sandbox are removed from the target. Collection data is never merged. diff --git a/lib/lightning_web/live/sandbox_live/index.ex b/lib/lightning_web/live/sandbox_live/index.ex index 1b5fc2d7b6d..38739296bae 100644 --- a/lib/lightning_web/live/sandbox_live/index.ex +++ b/lib/lightning_web/live/sandbox_live/index.ex @@ -6,6 +6,7 @@ defmodule LightningWeb.SandboxLive.Index do alias Lightning.Projects alias Lightning.Projects.MergeProjects alias Lightning.Projects.ProjectLimiter + alias Lightning.Projects.Sandboxes alias Lightning.Repo alias Lightning.VersionControl alias LightningWeb.SandboxLive.Components @@ -806,7 +807,7 @@ defmodule LightningWeb.SandboxLive.Index do case result do {:ok, _updated_target} = success -> - sync_collections(source, target) + Sandboxes.sync_collections(source, target) maybe_commit_to_github(target, "Merged sandbox #{source.name}") success @@ -815,44 +816,6 @@ defmodule LightningWeb.SandboxLive.Index do end end - defp sync_collections(sandbox, parent) do - sandbox_collections = Lightning.Collections.list_project_collections(sandbox) - parent_collections = Lightning.Collections.list_project_collections(parent) - - sandbox_names = MapSet.new(sandbox_collections, & &1.name) - parent_names = MapSet.new(parent_collections, & &1.name) - - to_create = MapSet.difference(sandbox_names, parent_names) - - if not Enum.empty?(to_create) do - current_time = DateTime.utc_now() |> DateTime.truncate(:second) - - rows = - Enum.map(to_create, fn name -> - %{ - id: Ecto.UUID.generate(), - name: name, - project_id: parent.id, - byte_size_sum: 0, - inserted_at: current_time, - updated_at: current_time - } - end) - - Repo.insert_all(Lightning.Collections.Collection, rows, - on_conflict: :nothing - ) - end - - to_delete = - parent_collections - |> Enum.filter( - &(&1.name in MapSet.difference(parent_names, sandbox_names)) - ) - - Enum.each(to_delete, &Lightning.Collections.delete_collection(&1.id)) - end - defp maybe_commit_to_github(project, commit_message) do with %{} = repo_connection <- VersionControl.get_repo_connection_for_project(project.id) do diff --git a/lib/lightning_web/plugs/api_version.ex b/lib/lightning_web/plugs/api_version.ex new file mode 100644 index 00000000000..7b0af115a9a --- /dev/null +++ b/lib/lightning_web/plugs/api_version.ex @@ -0,0 +1,40 @@ +defmodule LightningWeb.Plugs.ApiVersion do + @moduledoc """ + Resolves the API version for a request from the `x-api-version` header and + stores it in `conn.assigns[:api_version]`. + + Missing header or `"1"` resolves to `:v1`. `"2"` resolves to `:v2`. Any other + value is rejected with a 400 Bad Request so that unsupported versions fail + loudly instead of silently falling back. + """ + use Phoenix.Controller + import Plug.Conn + + @supported ~w(1 2) + + @type version :: :v1 | :v2 + + @spec init(keyword()) :: keyword() + def init(opts), do: opts + + @spec call(Plug.Conn.t(), keyword()) :: Plug.Conn.t() + def call(conn, _opts) do + case get_req_header(conn, "x-api-version") do + [] -> assign(conn, :api_version, :v1) + ["1"] -> assign(conn, :api_version, :v1) + ["2"] -> assign(conn, :api_version, :v2) + [value] -> reject(conn, value) + _many -> reject(conn, "multiple") + end + end + + defp reject(conn, value) do + conn + |> put_status(:bad_request) + |> json(%{ + error: + "Unsupported API version: #{inspect(value)}. Supported versions: #{Enum.join(@supported, ", ")}." + }) + |> halt() + end +end diff --git a/lib/lightning_web/router.ex b/lib/lightning_web/router.ex index d7963fb318c..f7679123b56 100644 --- a/lib/lightning_web/router.ex +++ b/lib/lightning_web/router.ex @@ -113,7 +113,7 @@ defmodule LightningWeb.Router do ## Collections scope "/collections", LightningWeb do - pipe_through [:authenticated_api] + pipe_through [:authenticated_api, LightningWeb.Plugs.ApiVersion] match :*, "/*path", CollectionsController, :dispatch end diff --git a/test/lightning/collections_test.exs b/test/lightning/collections_test.exs index 606e7c6c7ef..beb99d97ac4 100644 --- a/test/lightning/collections_test.exs +++ b/test/lightning/collections_test.exs @@ -75,7 +75,7 @@ defmodule Lightning.CollectionsTest do assert {:error, %{ errors: [ - project_id: + name: {"A collection with this name already exists", [ constraint: :unique, diff --git a/test/lightning/sandboxes_test.exs b/test/lightning/sandboxes_test.exs index f4513e9a4ae..423589ee572 100644 --- a/test/lightning/sandboxes_test.exs +++ b/test/lightning/sandboxes_test.exs @@ -610,6 +610,117 @@ defmodule Lightning.Projects.SandboxesTest do end end + describe "sync_collections/2" do + test "creates collections in target that exist in source but not target" do + source = insert(:project) + target = insert(:project) + + insert(:collection, project: source, name: "shared") + insert(:collection, project: source, name: "only-in-source") + insert(:collection, project: target, name: "shared") + + assert {:ok, %{created: 1, deleted: 0}} = + Sandboxes.sync_collections(source, target) + + target_names = + target + |> Lightning.Collections.list_project_collections() + |> Enum.map(& &1.name) + |> Enum.sort() + + assert target_names == ["only-in-source", "shared"] + end + + test "deletes collections in target that are missing from source, including items" do + source = insert(:project) + target = insert(:project) + + insert(:collection, project: source, name: "shared") + insert(:collection, project: target, name: "shared") + + dropped = + insert(:collection, + project: target, + name: "only-in-target", + items: [%{key: "k", value: "v"}] + ) + + assert {:ok, %{created: 0, deleted: 1}} = + Sandboxes.sync_collections(source, target) + + refute Lightning.Repo.get(Lightning.Collections.Collection, dropped.id) + + # items belonging to the deleted collection are removed with it + assert Lightning.Repo.all( + from i in Lightning.Collections.Item, + where: i.collection_id == ^dropped.id + ) == [] + end + + test "is a no-op when both projects have the same collections" do + source = insert(:project) + target = insert(:project) + + insert(:collection, project: source, name: "a") + insert(:collection, project: target, name: "a") + + assert {:ok, %{created: 0, deleted: 0}} = + Sandboxes.sync_collections(source, target) + end + + test "does not copy collection data across" do + source = insert(:project) + target = insert(:project) + + insert(:collection, + project: source, + name: "with-data", + items: [%{key: "k", value: "v"}] + ) + + assert {:ok, %{created: 1, deleted: 0}} = + Sandboxes.sync_collections(source, target) + + [new_collection] = Lightning.Collections.list_project_collections(target) + + assert Lightning.Collections.get_all( + new_collection, + %{cursor: nil, limit: 100}, + nil + ) == [] + end + + test "runs in a single transaction — either everything or nothing" do + source = insert(:project) + target = insert(:project) + + insert(:collection, project: source, name: "to-create") + insert(:collection, project: target, name: "to-delete") + + # Inject a failure inside the transaction by trying to insert a + # collection that will violate the unique constraint after we've done + # the work. We simulate this by wrapping the call in a parent + # transaction and forcing a rollback. + result = + Lightning.Repo.transaction(fn -> + {:ok, _summary} = + Sandboxes.sync_collections(source, target) + + Lightning.Repo.rollback(:simulated_failure) + end) + + assert result == {:error, :simulated_failure} + + # Target state must be unchanged + target_names = + target + |> Lightning.Collections.list_project_collections() + |> Enum.map(& &1.name) + + assert target_names == ["to-delete"] + end + end + describe "keychains" do test "clones only used keychains and rewires jobs to cloned keychains" do %{ diff --git a/test/lightning_web/collections_controller_test.exs b/test/lightning_web/collections_controller_test.exs index 50ea9f44670..62349a3538b 100644 --- a/test/lightning_web/collections_controller_test.exs +++ b/test/lightning_web/collections_controller_test.exs @@ -1080,6 +1080,81 @@ defmodule LightningWeb.API.CollectionsControllerTest do end end + describe "api version header" do + setup %{conn: conn} do + user = insert(:user) + project = insert(:project, project_users: [%{user: user}]) + collection = insert(:collection, project: project) + token = Lightning.Accounts.generate_api_token(user) + conn = assign_bearer(conn, token) + {:ok, conn: conn, project: project, collection: collection} + end + + test "returns 400 for an unsupported version", %{ + conn: conn, + collection: collection + } do + conn = + conn + |> put_req_header("x-api-version", "99") + |> get(~p"/collections/#{collection.name}") + + assert json_response(conn, 400)["error"] =~ "Unsupported API version" + end + + test "returns 400 for a garbage version value", %{ + conn: conn, + collection: collection + } do + conn = + conn + |> put_req_header("x-api-version", "not-a-version") + |> get(~p"/collections/#{collection.name}") + + assert json_response(conn, 400)["error"] =~ "Unsupported API version" + end + + test "treats no header as v1", %{conn: conn, collection: collection} do + conn = get(conn, ~p"/collections/#{collection.name}") + assert json_response(conn, 200)["items"] == [] + end + + test "treats an explicit v1 header as v1", %{ + conn: conn, + collection: collection + } do + conn = + conn + |> put_req_header("x-api-version", "1") + |> get(~p"/collections/#{collection.name}") + + assert json_response(conn, 200)["items"] == [] + end + end + + describe "malformed request bodies" do + setup %{conn: conn} do + user = insert(:user) + project = insert(:project, project_users: [%{user: user}]) + collection = insert(:collection, project: project) + token = Lightning.Accounts.generate_api_token(user) + conn = assign_bearer(conn, token) + {:ok, conn: conn, project: project, collection: collection} + end + + test "PUT with no value returns 422", %{conn: conn, collection: collection} do + conn = put(conn, ~p"/collections/#{collection.name}/some-key", %{}) + + assert json_response(conn, 422)["error"] =~ "Missing required field: value" + end + + test "POST with no items returns 422", %{conn: conn, collection: collection} do + conn = post(conn, ~p"/collections/#{collection.name}", %{}) + + assert json_response(conn, 422)["error"] =~ "Missing required field: items" + end + end + describe "v1 conflict — same collection name in multiple projects" do setup %{conn: conn} do user = insert(:user) From c8e7c5876961ce748da2959bd8b9027f2ef7baa8 Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Mon, 13 Apr 2026 16:32:46 +0000 Subject: [PATCH 11/25] Update changelog for sandbox collections support --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 762fa2afa7f..a0af6e2b40d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,14 @@ and this project adheres to ### Added +- Support collections in sandboxes. Collection names are now scoped per project, + empty collections are cloned into a sandbox on provision, and collection names + (not data) are synchronised when a sandbox is merged back into its parent. + Adds a v2 collections API at `/collections/:project_id/:name` selected via the + `x-api-version: 2` header. V1 continues to work and returns 409 when a name is + ambiguous across projects. + [#3548](https://github.com/OpenFn/lightning/issues/3548) + ### Changed - Bump `@openfn/ws-worker` from From ce0dee9ee2b9db46d8b0ee30dcad4a5feca77ebf Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Mon, 13 Apr 2026 16:53:01 +0000 Subject: [PATCH 12/25] Test dispatch fallback clauses for unsupported paths and methods --- .../collections_controller_test.exs | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/test/lightning_web/collections_controller_test.exs b/test/lightning_web/collections_controller_test.exs index 62349a3538b..6ce1e2a8cc1 100644 --- a/test/lightning_web/collections_controller_test.exs +++ b/test/lightning_web/collections_controller_test.exs @@ -1132,6 +1132,52 @@ defmodule LightningWeb.API.CollectionsControllerTest do end end + describe "unsupported paths and methods" do + setup %{conn: conn} do + user = insert(:user) + project = insert(:project, project_users: [%{user: user}]) + collection = insert(:collection, project: project) + token = Lightning.Accounts.generate_api_token(user) + conn = assign_bearer(conn, token) + {:ok, conn: conn, project: project, collection: collection} + end + + test "v1 returns 404 for unsupported HTTP methods", %{ + conn: conn, + collection: collection + } do + conn = patch(conn, ~p"/collections/#{collection.name}") + assert response(conn, 404) == "Not found" + end + + test "v1 returns 404 for unknown path shapes", %{conn: conn} do + conn = get(conn, "/collections/a/b/c/d") + assert response(conn, 404) == "Not found" + end + + test "v2 returns 404 for unsupported HTTP methods", %{ + conn: conn, + project: project, + collection: collection + } do + conn = + conn + |> put_req_header("x-api-version", "2") + |> patch(~p"/collections/#{project.id}/#{collection.name}") + + assert response(conn, 404) == "Not found" + end + + test "v2 returns 404 for unknown path shapes", %{conn: conn} do + conn = + conn + |> put_req_header("x-api-version", "2") + |> get("/collections/a/b/c/d/e") + + assert response(conn, 404) == "Not found" + end + end + describe "malformed request bodies" do setup %{conn: conn} do user = insert(:user) From 17aaa1c7ed415d57846f394e528a499e6c7aaccb Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Mon, 13 Apr 2026 17:04:01 +0000 Subject: [PATCH 13/25] Test api version plug rejects multiple header values --- .../collections_controller_test.exs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/lightning_web/collections_controller_test.exs b/test/lightning_web/collections_controller_test.exs index 6ce1e2a8cc1..a14bd9b3059 100644 --- a/test/lightning_web/collections_controller_test.exs +++ b/test/lightning_web/collections_controller_test.exs @@ -1130,6 +1130,22 @@ defmodule LightningWeb.API.CollectionsControllerTest do assert json_response(conn, 200)["items"] == [] end + + test "returns 400 when multiple x-api-version headers are sent", %{ + conn: conn, + collection: collection + } do + # Bypass Plug.Conn.put_req_header/3 (which replaces) to simulate a + # client that sends the header twice. + conn = + update_in(conn.req_headers, fn headers -> + headers ++ [{"x-api-version", "1"}, {"x-api-version", "2"}] + end) + + conn = get(conn, ~p"/collections/#{collection.name}") + + assert json_response(conn, 400)["error"] =~ "Unsupported API version" + end end describe "unsupported paths and methods" do From 16872600c355ec496ea7f825cb78fd36cea292e5 Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Tue, 14 Apr 2026 08:34:08 +0000 Subject: [PATCH 14/25] Use explicit up/down in collections migration with irreversible guard --- ...000000_scope_collection_name_uniqueness_to_project.exs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/priv/repo/migrations/20260410000000_scope_collection_name_uniqueness_to_project.exs b/priv/repo/migrations/20260410000000_scope_collection_name_uniqueness_to_project.exs index 71eeb5e42a5..1a67b3b33b5 100644 --- a/priv/repo/migrations/20260410000000_scope_collection_name_uniqueness_to_project.exs +++ b/priv/repo/migrations/20260410000000_scope_collection_name_uniqueness_to_project.exs @@ -1,8 +1,12 @@ defmodule Lightning.Repo.Migrations.ScopeCollectionNameUniquenessToProject do use Ecto.Migration - def change do - drop unique_index(:collections, [:name]) + def up do + drop_if_exists unique_index(:collections, [:name]) create unique_index(:collections, [:project_id, :name]) end + + def down do + raise "Cannot reverse: duplicate collection names may exist across projects" + end end From 57434b155653a01835bbfff9ff7462fd91ffcce0 Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Tue, 14 Apr 2026 08:38:15 +0000 Subject: [PATCH 15/25] Allow migration rollback when no duplicate collection names exist --- ...60410000000_scope_collection_name_uniqueness_to_project.exs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/priv/repo/migrations/20260410000000_scope_collection_name_uniqueness_to_project.exs b/priv/repo/migrations/20260410000000_scope_collection_name_uniqueness_to_project.exs index 1a67b3b33b5..f290e867229 100644 --- a/priv/repo/migrations/20260410000000_scope_collection_name_uniqueness_to_project.exs +++ b/priv/repo/migrations/20260410000000_scope_collection_name_uniqueness_to_project.exs @@ -7,6 +7,7 @@ defmodule Lightning.Repo.Migrations.ScopeCollectionNameUniquenessToProject do end def down do - raise "Cannot reverse: duplicate collection names may exist across projects" + drop_if_exists unique_index(:collections, [:project_id, :name]) + create unique_index(:collections, [:name]) end end From 27bdb15e0f5d6e78b8189a78d2fa6dc2bc3162b4 Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Tue, 14 Apr 2026 19:41:34 +0000 Subject: [PATCH 16/25] Handle sync_collections failure, remove dead code, fix test name - Log a warning when collection sync fails after merge instead of silently ignoring the error - Remove on_conflict: :nothing from insert_empty_collections since both callers guarantee no duplicates can exist - Rename misleading test to match what it actually verifies --- lib/lightning/projects/sandboxes.ex | 2 +- lib/lightning_web/live/sandbox_live/index.ex | 13 ++++++++++++- test/lightning/sandboxes_test.exs | 2 +- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/lib/lightning/projects/sandboxes.ex b/lib/lightning/projects/sandboxes.ex index a34390ad748..957347081b4 100644 --- a/lib/lightning/projects/sandboxes.ex +++ b/lib/lightning/projects/sandboxes.ex @@ -637,7 +637,7 @@ defmodule Lightning.Projects.Sandboxes do } end) - Repo.insert_all(Collection, rows, on_conflict: :nothing) + Repo.insert_all(Collection, rows) end end diff --git a/lib/lightning_web/live/sandbox_live/index.ex b/lib/lightning_web/live/sandbox_live/index.ex index 38739296bae..f75b93503e9 100644 --- a/lib/lightning_web/live/sandbox_live/index.ex +++ b/lib/lightning_web/live/sandbox_live/index.ex @@ -11,6 +11,8 @@ defmodule LightningWeb.SandboxLive.Index do alias Lightning.VersionControl alias LightningWeb.SandboxLive.Components + require Logger + defmodule MergeWorkflow do defstruct [:id, :name, :is_diverged, :is_new, :is_deleted] end @@ -807,7 +809,16 @@ defmodule LightningWeb.SandboxLive.Index do case result do {:ok, _updated_target} = success -> - Sandboxes.sync_collections(source, target) + case Sandboxes.sync_collections(source, target) do + {:ok, _} -> + :ok + + {:error, reason} -> + Logger.warning( + "Failed to sync collections from sandbox #{source.id} to #{target.id}: #{inspect(reason)}" + ) + end + maybe_commit_to_github(target, "Merged sandbox #{source.name}") success diff --git a/test/lightning/sandboxes_test.exs b/test/lightning/sandboxes_test.exs index 423589ee572..d73a2663a13 100644 --- a/test/lightning/sandboxes_test.exs +++ b/test/lightning/sandboxes_test.exs @@ -592,7 +592,7 @@ defmodule Lightning.Projects.SandboxesTest do assert Lightning.Collections.list_project_collections(sandbox) == [] end - test "re-provisioning is idempotent (on_conflict: :nothing)" do + test "each sandbox gets its own copy of parent collections" do actor = insert(:user) parent = insert(:project) ensure_member!(parent, actor, :owner) From e5c904b037322a86d75909384befed4143a5a3a3 Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Tue, 14 Apr 2026 19:44:29 +0000 Subject: [PATCH 17/25] Restore on_conflict: :nothing to guard against concurrent merges --- lib/lightning/projects/sandboxes.ex | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/lightning/projects/sandboxes.ex b/lib/lightning/projects/sandboxes.ex index 957347081b4..1e8314886c8 100644 --- a/lib/lightning/projects/sandboxes.ex +++ b/lib/lightning/projects/sandboxes.ex @@ -637,7 +637,9 @@ defmodule Lightning.Projects.Sandboxes do } end) - Repo.insert_all(Collection, rows) + # on_conflict: :nothing handles the rare case where two concurrent + # merges into the same target both try to create the same collection. + Repo.insert_all(Collection, rows, on_conflict: :nothing) end end From e02d9d58c6bce22b800b76e1f5fe5e4ed1afb9de Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Tue, 14 Apr 2026 21:48:59 +0000 Subject: [PATCH 18/25] Fix review findings for sandbox collections - Use project-scoped get_collection/2 in CollectionsComponent to prevent MatchError when collection names collide across projects - Guard migration rollback with duplicate name check so it fails with a clear message instead of a cryptic Postgres error - Make sync_collections failure fail the merge instead of silently logging, and skip the GitHub commit when collections fail to sync - Bind MapSet.difference outside the comprehension in sync_collections - Add v2 write-path test with run token to cover the primary worker path --- lib/lightning/projects/sandboxes.ex | 4 ++- .../project_live/collections_component.ex | 6 ++-- lib/lightning_web/live/sandbox_live/index.ex | 11 +++---- ..._collection_name_uniqueness_to_project.exs | 10 +++++++ .../collections_controller_test.exs | 30 +++++++++++++++++++ 5 files changed, 51 insertions(+), 10 deletions(-) diff --git a/lib/lightning/projects/sandboxes.ex b/lib/lightning/projects/sandboxes.ex index 1e8314886c8..70984c9de99 100644 --- a/lib/lightning/projects/sandboxes.ex +++ b/lib/lightning/projects/sandboxes.ex @@ -605,9 +605,11 @@ defmodule Lightning.Projects.Sandboxes do to_create = MapSet.difference(source_names, target_names) + names_to_delete = MapSet.difference(target_names, source_names) + to_delete_ids = for c <- target_collections, - c.name in MapSet.difference(target_names, source_names), + c.name in names_to_delete, do: c.id Repo.transaction(fn -> diff --git a/lib/lightning_web/live/project_live/collections_component.ex b/lib/lightning_web/live/project_live/collections_component.ex index e866b666c89..92a1cd6ff02 100644 --- a/lib/lightning_web/live/project_live/collections_component.ex +++ b/lib/lightning_web/live/project_live/collections_component.ex @@ -45,7 +45,8 @@ defmodule LightningWeb.ProjectLive.CollectionsComponent do socket ) do with :ok <- can_create_collection(socket) do - {:ok, collection} = Collections.get_collection(collection_name) + project_id = socket.assigns.project.id + {:ok, collection} = Collections.get_collection(project_id, collection_name) changeset = Collection.form_changeset(collection, %{raw_name: collection.name}) @@ -65,7 +66,8 @@ defmodule LightningWeb.ProjectLive.CollectionsComponent do socket ) do with :ok <- can_create_collection(socket) do - {:ok, collection} = Collections.get_collection(collection_name) + project_id = socket.assigns.project.id + {:ok, collection} = Collections.get_collection(project_id, collection_name) {:noreply, assign(socket, collection: collection, action: :delete)} end diff --git a/lib/lightning_web/live/sandbox_live/index.ex b/lib/lightning_web/live/sandbox_live/index.ex index f75b93503e9..6d7a8069b6b 100644 --- a/lib/lightning_web/live/sandbox_live/index.ex +++ b/lib/lightning_web/live/sandbox_live/index.ex @@ -811,17 +811,13 @@ defmodule LightningWeb.SandboxLive.Index do {:ok, _updated_target} = success -> case Sandboxes.sync_collections(source, target) do {:ok, _} -> - :ok + maybe_commit_to_github(target, "Merged sandbox #{source.name}") + success {:error, reason} -> - Logger.warning( - "Failed to sync collections from sandbox #{source.id} to #{target.id}: #{inspect(reason)}" - ) + {:error, "Failed to sync collections: #{inspect(reason)}"} end - maybe_commit_to_github(target, "Merged sandbox #{source.name}") - success - error -> error end @@ -900,5 +896,6 @@ defmodule LightningWeb.SandboxLive.Index do end defp format_merge_error(%{text: text}), do: text + defp format_merge_error(reason) when is_binary(reason), do: reason defp format_merge_error(reason), do: "Failed to merge: #{inspect(reason)}" end diff --git a/priv/repo/migrations/20260410000000_scope_collection_name_uniqueness_to_project.exs b/priv/repo/migrations/20260410000000_scope_collection_name_uniqueness_to_project.exs index f290e867229..cc21e88141f 100644 --- a/priv/repo/migrations/20260410000000_scope_collection_name_uniqueness_to_project.exs +++ b/priv/repo/migrations/20260410000000_scope_collection_name_uniqueness_to_project.exs @@ -7,6 +7,16 @@ defmodule Lightning.Repo.Migrations.ScopeCollectionNameUniquenessToProject do end def down do + dupes = + repo().query!("SELECT name FROM collections GROUP BY name HAVING count(*) > 1") + + if dupes.num_rows > 0 do + raise Ecto.MigrationError, + message: + "Cannot rollback: #{dupes.num_rows} collection name(s) exist in multiple projects. " <> + "Remove duplicates before rolling back." + end + drop_if_exists unique_index(:collections, [:project_id, :name]) create unique_index(:collections, [:name]) end diff --git a/test/lightning_web/collections_controller_test.exs b/test/lightning_web/collections_controller_test.exs index a14bd9b3059..c6ae8c66e9c 100644 --- a/test/lightning_web/collections_controller_test.exs +++ b/test/lightning_web/collections_controller_test.exs @@ -1429,6 +1429,36 @@ defmodule LightningWeb.API.CollectionsControllerTest do assert json_response(conn, 200)["key"] == "foo" end + test "with a valid run token, can write to own project's collection", %{ + conn: conn, + project: project, + collection: collection + } do + workflow = insert(:simple_workflow, project: project) + + workorder = + insert(:workorder, workflow: workflow, dataclip: insert(:dataclip)) + + run = + insert(:run, + work_order: workorder, + dataclip: workorder.dataclip, + starting_trigger: hd(workflow.triggers) + ) + + token = Lightning.Workers.generate_run_token(run) + + conn = + conn + |> assign_bearer(token) + |> put( + ~p"/collections/#{project.id}/#{collection.name}/new-key", + value: "new-val" + ) + + assert json_response(conn, 200) == %{"upserted" => 1, "error" => nil} + end + test "with a run token, cannot access a different project's collection", %{ conn: conn } do From bf8a54ebefb2d3b2d2cab4816ecdbe2b8438e80e Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Tue, 14 Apr 2026 22:02:01 +0000 Subject: [PATCH 19/25] Replace catch-all match with reusable VersionedRouter plug Introduces LightningWeb.Plugs.VersionedRouter, a reusable behaviour for header-based API versioning. Instead of `match :*, "/*path"` in the Phoenix router, the collections scope now uses `forward` to a router plug that resolves the version and delegates to V1Routes or V2Routes modules with explicit pattern matching per version. The controller keeps only business logic; routing and param assembly live in the version-specific route modules. The VersionedRouter plug handles fallback controller integration so actions can still return error tuples. --- .../controllers/collections/router.ex | 16 ++++ .../controllers/collections/v1_routes.ex | 35 +++++++ .../controllers/collections/v2_routes.ex | 56 +++++++++++ .../controllers/collections_controller.ex | 93 ------------------- lib/lightning_web/plugs/versioned_router.ex | 81 ++++++++++++++++ lib/lightning_web/router.ex | 6 +- .../collections_controller_test.exs | 8 +- 7 files changed, 195 insertions(+), 100 deletions(-) create mode 100644 lib/lightning_web/controllers/collections/router.ex create mode 100644 lib/lightning_web/controllers/collections/v1_routes.ex create mode 100644 lib/lightning_web/controllers/collections/v2_routes.ex create mode 100644 lib/lightning_web/plugs/versioned_router.ex diff --git a/lib/lightning_web/controllers/collections/router.ex b/lib/lightning_web/controllers/collections/router.ex new file mode 100644 index 00000000000..47eddf5cb64 --- /dev/null +++ b/lib/lightning_web/controllers/collections/router.ex @@ -0,0 +1,16 @@ +defmodule LightningWeb.Collections.Router do + @moduledoc """ + Version-aware router for the Collections API. + + Uses `LightningWeb.Plugs.VersionedRouter` to dispatch to v1 or v2 + route modules based on the `x-api-version` request header. + Mounted in the main router with `forward "/collections", ...`. + """ + use LightningWeb.Plugs.VersionedRouter, + version_plug: LightningWeb.Plugs.ApiVersion, + fallback: LightningWeb.FallbackController, + versions: %{ + v1: LightningWeb.Collections.V1Routes, + v2: LightningWeb.Collections.V2Routes + } +end diff --git a/lib/lightning_web/controllers/collections/v1_routes.ex b/lib/lightning_web/controllers/collections/v1_routes.ex new file mode 100644 index 00000000000..68c4b6773e0 --- /dev/null +++ b/lib/lightning_web/controllers/collections/v1_routes.ex @@ -0,0 +1,35 @@ +defmodule LightningWeb.Collections.V1Routes do + @moduledoc """ + V1 collection routes: name-scoped (`/:name`, `/:name/:key`). + """ + @behaviour LightningWeb.Plugs.VersionedRouter + + alias LightningWeb.CollectionsController, as: C + + @impl true + def route(conn, "GET", [name]), + do: C.stream(conn, %{"name" => name}) + + def route(conn, "GET", [name, key]), + do: C.get(conn, %{"name" => name, "key" => key}) + + def route(conn, "PUT", [name, key]), + do: C.put(conn, body_with(conn, %{"name" => name, "key" => key})) + + def route(conn, "POST", [name]), + do: C.put_all(conn, body_with(conn, %{"name" => name})) + + def route(conn, "DELETE", [name, key]), + do: C.delete(conn, %{"name" => name, "key" => key}) + + def route(conn, "DELETE", [name]), + do: C.delete_all(conn, all_params(conn, %{"name" => name})) + + def route(_conn, _method, _path), do: {:error, :not_found} + + defp body_with(conn, extra), do: Map.merge(conn.body_params, extra) + + defp all_params(conn, extra) do + conn.query_params |> Map.merge(conn.body_params) |> Map.merge(extra) + end +end diff --git a/lib/lightning_web/controllers/collections/v2_routes.ex b/lib/lightning_web/controllers/collections/v2_routes.ex new file mode 100644 index 00000000000..a045829478f --- /dev/null +++ b/lib/lightning_web/controllers/collections/v2_routes.ex @@ -0,0 +1,56 @@ +defmodule LightningWeb.Collections.V2Routes do + @moduledoc """ + V2 collection routes: project-scoped (`/:project_id/:name`, `/:project_id/:name/:key`). + """ + @behaviour LightningWeb.Plugs.VersionedRouter + + alias LightningWeb.CollectionsController, as: C + + @impl true + def route(conn, "GET", [project_id, name]), + do: C.stream(conn, %{"project_id" => project_id, "name" => name}) + + def route(conn, "GET", [project_id, name, key]), + do: C.get(conn, %{"project_id" => project_id, "name" => name, "key" => key}) + + def route(conn, "PUT", [project_id, name, key]), + do: + C.put( + conn, + body_with(conn, %{ + "project_id" => project_id, + "name" => name, + "key" => key + }) + ) + + def route(conn, "POST", [project_id, name]), + do: + C.put_all( + conn, + body_with(conn, %{"project_id" => project_id, "name" => name}) + ) + + def route(conn, "DELETE", [project_id, name, key]), + do: + C.delete(conn, %{ + "project_id" => project_id, + "name" => name, + "key" => key + }) + + def route(conn, "DELETE", [project_id, name]), + do: + C.delete_all( + conn, + all_params(conn, %{"project_id" => project_id, "name" => name}) + ) + + def route(_conn, _method, _path), do: {:error, :not_found} + + defp body_with(conn, extra), do: Map.merge(conn.body_params, extra) + + defp all_params(conn, extra) do + conn.query_params |> Map.merge(conn.body_params) |> Map.merge(extra) + end +end diff --git a/lib/lightning_web/controllers/collections_controller.ex b/lib/lightning_web/controllers/collections_controller.ex index 1fe3caff4ea..ac6b3654486 100644 --- a/lib/lightning_web/controllers/collections_controller.ex +++ b/lib/lightning_web/controllers/collections_controller.ex @@ -26,99 +26,6 @@ defmodule LightningWeb.CollectionsController do @valid_params ["key", "cursor", "limit" | @timestamp_params] - @doc """ - Entry point for all collection API routes. - - API version is resolved by `LightningWeb.Plugs.ApiVersion` and stored in - `conn.assigns.api_version`. Path shape is then used to route to the - appropriate action: - - * V1 is name-scoped: `/:name`, `/:name/:key` - * V2 is project-scoped: `/:project_id/:name`, `/:project_id/:name/:key` - - The catch-all route is required because v1 `/:name/:key` and v2 - `/:project_id/:name` collide at Phoenix router level. - """ - @spec dispatch(Plug.Conn.t(), map()) :: Plug.Conn.t() - def dispatch(%{assigns: %{api_version: :v1}} = conn, %{"path" => path}), - do: dispatch_v1(conn, conn.method, path) - - def dispatch(%{assigns: %{api_version: :v2}} = conn, %{"path" => path}), - do: dispatch_v2(conn, conn.method, path) - - defp dispatch_v1(conn, "GET", [name]), - do: stream(conn, %{"name" => name}) - - defp dispatch_v1(conn, "GET", [name, key]), - do: get(conn, %{"name" => name, "key" => key}) - - defp dispatch_v1(conn, "PUT", [name, key]), - do: put(conn, body_with(conn, %{"name" => name, "key" => key})) - - defp dispatch_v1(conn, "POST", [name]), - do: put_all(conn, body_with(conn, %{"name" => name})) - - defp dispatch_v1(conn, "DELETE", [name, key]), - do: delete(conn, %{"name" => name, "key" => key}) - - defp dispatch_v1(conn, "DELETE", [name]), - do: delete_all(conn, params_with(conn, %{"name" => name})) - - defp dispatch_v1(conn, _method, _path), do: not_found(conn) - - defp dispatch_v2(conn, "GET", [project_id, name]), - do: stream(conn, %{"project_id" => project_id, "name" => name}) - - defp dispatch_v2(conn, "GET", [project_id, name, key]), - do: get(conn, %{"project_id" => project_id, "name" => name, "key" => key}) - - defp dispatch_v2(conn, "PUT", [project_id, name, key]), - do: - put( - conn, - body_with(conn, %{ - "project_id" => project_id, - "name" => name, - "key" => key - }) - ) - - defp dispatch_v2(conn, "POST", [project_id, name]), - do: - put_all( - conn, - body_with(conn, %{"project_id" => project_id, "name" => name}) - ) - - defp dispatch_v2(conn, "DELETE", [project_id, name, key]), - do: - delete(conn, %{ - "project_id" => project_id, - "name" => name, - "key" => key - }) - - defp dispatch_v2(conn, "DELETE", [project_id, name]), - do: - delete_all( - conn, - params_with(conn, %{"project_id" => project_id, "name" => name}) - ) - - defp dispatch_v2(conn, _method, _path), do: not_found(conn) - - # Merges `extra` into the request body params — used for PUT/POST where we - # need to combine the JSON body with values parsed from the URL. - defp body_with(conn, extra), do: Map.merge(conn.body_params, extra) - - # Merges `extra` into the combined query + body params — used for DELETE - # endpoints that accept a `key` filter in either place. - defp params_with(conn, extra) do - conn.query_params |> Map.merge(conn.body_params) |> Map.merge(extra) - end - - defp not_found(conn), do: send_resp(conn, 404, "Not found") - @spec put(Plug.Conn.t(), map()) :: Plug.Conn.t() def put(conn, %{"key" => key, "value" => value} = params) do with {:ok, collection} <- resolve(params), diff --git a/lib/lightning_web/plugs/versioned_router.ex b/lib/lightning_web/plugs/versioned_router.ex new file mode 100644 index 00000000000..c534035627a --- /dev/null +++ b/lib/lightning_web/plugs/versioned_router.ex @@ -0,0 +1,81 @@ +defmodule LightningWeb.Plugs.VersionedRouter do + @moduledoc """ + A reusable plug for header-based API versioning with explicit route + definitions per version. + + Instead of a catch-all `match :*, "/*path"` in the Phoenix router, this + plug is used with `forward` and delegates to version-specific route + modules that pattern-match on `{method, path_segments}`. + + ## Usage + + Define a router module that `use`s this plug: + + defmodule LightningWeb.Collections.Router do + use LightningWeb.Plugs.VersionedRouter, + version_plug: LightningWeb.Plugs.ApiVersion, + fallback: LightningWeb.FallbackController, + versions: %{ + v1: LightningWeb.Collections.V1Routes, + v2: LightningWeb.Collections.V2Routes + } + end + + Then in the Phoenix router: + + forward "/collections", LightningWeb.Collections.Router + + ## Route modules + + Each version module must implement the `c:route/3` callback: + + defmodule LightningWeb.Collections.V1Routes do + @behaviour LightningWeb.Plugs.VersionedRouter + + @impl true + def route(conn, "GET", [name]), + do: MyController.stream(conn, %{"name" => name}) + + def route(conn, _method, _path), + do: {:error, :not_found} + end + + Actions may return either a `%Plug.Conn{}` (rendered directly) or an + error tuple like `{:error, :not_found}`, which is passed to the + configured fallback controller. + """ + + @callback route(Plug.Conn.t(), String.t(), [String.t()]) :: + Plug.Conn.t() | term() + + defmacro __using__(opts) do + quote do + @opts unquote(opts) + + def init(runtime_opts), do: Keyword.merge(@opts, runtime_opts) + + def call(conn, opts) do + version_plug = Keyword.fetch!(opts, :version_plug) + versions = Keyword.fetch!(opts, :versions) + fallback = Keyword.fetch!(opts, :fallback) + + conn = version_plug.call(conn, version_plug.init([])) + + if conn.halted do + conn + else + case Map.get(versions, conn.assigns[:api_version]) do + nil -> + Plug.Conn.send_resp(conn, 404, "Not found") + + handler -> + case handler.route(conn, conn.method, conn.path_info) do + %Plug.Conn{} = conn -> conn + error -> fallback.call(conn, error) + end + end + end + end + end + end +end diff --git a/lib/lightning_web/router.ex b/lib/lightning_web/router.ex index f7679123b56..be01a7016b7 100644 --- a/lib/lightning_web/router.ex +++ b/lib/lightning_web/router.ex @@ -112,9 +112,9 @@ defmodule LightningWeb.Router do end ## Collections - scope "/collections", LightningWeb do - pipe_through [:authenticated_api, LightningWeb.Plugs.ApiVersion] - match :*, "/*path", CollectionsController, :dispatch + scope "/collections" do + pipe_through [:authenticated_api] + forward "/", LightningWeb.Collections.Router end ## Authentication routes diff --git a/test/lightning_web/collections_controller_test.exs b/test/lightning_web/collections_controller_test.exs index c6ae8c66e9c..039190dfb07 100644 --- a/test/lightning_web/collections_controller_test.exs +++ b/test/lightning_web/collections_controller_test.exs @@ -1163,12 +1163,12 @@ defmodule LightningWeb.API.CollectionsControllerTest do collection: collection } do conn = patch(conn, ~p"/collections/#{collection.name}") - assert response(conn, 404) == "Not found" + assert json_response(conn, 404)["error"] == "Not Found" end test "v1 returns 404 for unknown path shapes", %{conn: conn} do conn = get(conn, "/collections/a/b/c/d") - assert response(conn, 404) == "Not found" + assert json_response(conn, 404)["error"] == "Not Found" end test "v2 returns 404 for unsupported HTTP methods", %{ @@ -1181,7 +1181,7 @@ defmodule LightningWeb.API.CollectionsControllerTest do |> put_req_header("x-api-version", "2") |> patch(~p"/collections/#{project.id}/#{collection.name}") - assert response(conn, 404) == "Not found" + assert json_response(conn, 404)["error"] == "Not Found" end test "v2 returns 404 for unknown path shapes", %{conn: conn} do @@ -1190,7 +1190,7 @@ defmodule LightningWeb.API.CollectionsControllerTest do |> put_req_header("x-api-version", "2") |> get("/collections/a/b/c/d/e") - assert response(conn, 404) == "Not found" + assert json_response(conn, 404)["error"] == "Not Found" end end From 1b2e4d4b1c5899546cad6b6ace65748caee59cd5 Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Tue, 14 Apr 2026 22:33:38 +0000 Subject: [PATCH 20/25] Simplify versioned routing into a single CollectionsRouter plug Replaces the 4-file VersionedRouter macro abstraction with a single plug at plugs/collections_router.ex. Inlines the ApiVersion logic so version resolution, routing, and fallback handling all live in one place. The router uses forward instead of match :*. --- .../controllers/collections/router.ex | 16 -- .../controllers/collections/v1_routes.ex | 35 ---- .../controllers/collections/v2_routes.ex | 56 ------- lib/lightning_web/plugs/api_version.ex | 40 ----- lib/lightning_web/plugs/collections_router.ex | 154 ++++++++++++++++++ lib/lightning_web/plugs/versioned_router.ex | 81 --------- lib/lightning_web/router.ex | 2 +- 7 files changed, 155 insertions(+), 229 deletions(-) delete mode 100644 lib/lightning_web/controllers/collections/router.ex delete mode 100644 lib/lightning_web/controllers/collections/v1_routes.ex delete mode 100644 lib/lightning_web/controllers/collections/v2_routes.ex delete mode 100644 lib/lightning_web/plugs/api_version.ex create mode 100644 lib/lightning_web/plugs/collections_router.ex delete mode 100644 lib/lightning_web/plugs/versioned_router.ex diff --git a/lib/lightning_web/controllers/collections/router.ex b/lib/lightning_web/controllers/collections/router.ex deleted file mode 100644 index 47eddf5cb64..00000000000 --- a/lib/lightning_web/controllers/collections/router.ex +++ /dev/null @@ -1,16 +0,0 @@ -defmodule LightningWeb.Collections.Router do - @moduledoc """ - Version-aware router for the Collections API. - - Uses `LightningWeb.Plugs.VersionedRouter` to dispatch to v1 or v2 - route modules based on the `x-api-version` request header. - Mounted in the main router with `forward "/collections", ...`. - """ - use LightningWeb.Plugs.VersionedRouter, - version_plug: LightningWeb.Plugs.ApiVersion, - fallback: LightningWeb.FallbackController, - versions: %{ - v1: LightningWeb.Collections.V1Routes, - v2: LightningWeb.Collections.V2Routes - } -end diff --git a/lib/lightning_web/controllers/collections/v1_routes.ex b/lib/lightning_web/controllers/collections/v1_routes.ex deleted file mode 100644 index 68c4b6773e0..00000000000 --- a/lib/lightning_web/controllers/collections/v1_routes.ex +++ /dev/null @@ -1,35 +0,0 @@ -defmodule LightningWeb.Collections.V1Routes do - @moduledoc """ - V1 collection routes: name-scoped (`/:name`, `/:name/:key`). - """ - @behaviour LightningWeb.Plugs.VersionedRouter - - alias LightningWeb.CollectionsController, as: C - - @impl true - def route(conn, "GET", [name]), - do: C.stream(conn, %{"name" => name}) - - def route(conn, "GET", [name, key]), - do: C.get(conn, %{"name" => name, "key" => key}) - - def route(conn, "PUT", [name, key]), - do: C.put(conn, body_with(conn, %{"name" => name, "key" => key})) - - def route(conn, "POST", [name]), - do: C.put_all(conn, body_with(conn, %{"name" => name})) - - def route(conn, "DELETE", [name, key]), - do: C.delete(conn, %{"name" => name, "key" => key}) - - def route(conn, "DELETE", [name]), - do: C.delete_all(conn, all_params(conn, %{"name" => name})) - - def route(_conn, _method, _path), do: {:error, :not_found} - - defp body_with(conn, extra), do: Map.merge(conn.body_params, extra) - - defp all_params(conn, extra) do - conn.query_params |> Map.merge(conn.body_params) |> Map.merge(extra) - end -end diff --git a/lib/lightning_web/controllers/collections/v2_routes.ex b/lib/lightning_web/controllers/collections/v2_routes.ex deleted file mode 100644 index a045829478f..00000000000 --- a/lib/lightning_web/controllers/collections/v2_routes.ex +++ /dev/null @@ -1,56 +0,0 @@ -defmodule LightningWeb.Collections.V2Routes do - @moduledoc """ - V2 collection routes: project-scoped (`/:project_id/:name`, `/:project_id/:name/:key`). - """ - @behaviour LightningWeb.Plugs.VersionedRouter - - alias LightningWeb.CollectionsController, as: C - - @impl true - def route(conn, "GET", [project_id, name]), - do: C.stream(conn, %{"project_id" => project_id, "name" => name}) - - def route(conn, "GET", [project_id, name, key]), - do: C.get(conn, %{"project_id" => project_id, "name" => name, "key" => key}) - - def route(conn, "PUT", [project_id, name, key]), - do: - C.put( - conn, - body_with(conn, %{ - "project_id" => project_id, - "name" => name, - "key" => key - }) - ) - - def route(conn, "POST", [project_id, name]), - do: - C.put_all( - conn, - body_with(conn, %{"project_id" => project_id, "name" => name}) - ) - - def route(conn, "DELETE", [project_id, name, key]), - do: - C.delete(conn, %{ - "project_id" => project_id, - "name" => name, - "key" => key - }) - - def route(conn, "DELETE", [project_id, name]), - do: - C.delete_all( - conn, - all_params(conn, %{"project_id" => project_id, "name" => name}) - ) - - def route(_conn, _method, _path), do: {:error, :not_found} - - defp body_with(conn, extra), do: Map.merge(conn.body_params, extra) - - defp all_params(conn, extra) do - conn.query_params |> Map.merge(conn.body_params) |> Map.merge(extra) - end -end diff --git a/lib/lightning_web/plugs/api_version.ex b/lib/lightning_web/plugs/api_version.ex deleted file mode 100644 index 7b0af115a9a..00000000000 --- a/lib/lightning_web/plugs/api_version.ex +++ /dev/null @@ -1,40 +0,0 @@ -defmodule LightningWeb.Plugs.ApiVersion do - @moduledoc """ - Resolves the API version for a request from the `x-api-version` header and - stores it in `conn.assigns[:api_version]`. - - Missing header or `"1"` resolves to `:v1`. `"2"` resolves to `:v2`. Any other - value is rejected with a 400 Bad Request so that unsupported versions fail - loudly instead of silently falling back. - """ - use Phoenix.Controller - import Plug.Conn - - @supported ~w(1 2) - - @type version :: :v1 | :v2 - - @spec init(keyword()) :: keyword() - def init(opts), do: opts - - @spec call(Plug.Conn.t(), keyword()) :: Plug.Conn.t() - def call(conn, _opts) do - case get_req_header(conn, "x-api-version") do - [] -> assign(conn, :api_version, :v1) - ["1"] -> assign(conn, :api_version, :v1) - ["2"] -> assign(conn, :api_version, :v2) - [value] -> reject(conn, value) - _many -> reject(conn, "multiple") - end - end - - defp reject(conn, value) do - conn - |> put_status(:bad_request) - |> json(%{ - error: - "Unsupported API version: #{inspect(value)}. Supported versions: #{Enum.join(@supported, ", ")}." - }) - |> halt() - end -end diff --git a/lib/lightning_web/plugs/collections_router.ex b/lib/lightning_web/plugs/collections_router.ex new file mode 100644 index 00000000000..8d25732d018 --- /dev/null +++ b/lib/lightning_web/plugs/collections_router.ex @@ -0,0 +1,154 @@ +defmodule LightningWeb.Plugs.CollectionsRouter do + @moduledoc """ + Versioned routing plug for the Collections API. + + Mounted via `forward` in the Phoenix router, this plug resolves the + API version from the `x-api-version` header and dispatches to the + appropriate controller action based on version, HTTP method, and path + segments. + + ## Version resolution + + The version is read from the `x-api-version` request header: + + * Missing or `"1"` -> v1 + * `"2"` -> v2 + * Any other value or multiple headers -> 400 Bad Request + + ## Routes + + * **V1** (name-scoped): `/:name`, `/:name/:key` + * **V2** (project-scoped): `/:project_id/:name`, `/:project_id/:name/:key` + + Controller actions may return a `%Plug.Conn{}` (rendered directly) or + an error tuple like `{:error, :not_found}`, which is passed to the + fallback controller. + """ + use Phoenix.Controller + import Plug.Conn + + alias LightningWeb.CollectionsController, as: C + alias LightningWeb.FallbackController + + @supported_versions ~w(1 2) + + def init(opts), do: opts + + def call(conn, _opts) do + case resolve_version(conn) do + {:ok, conn} -> + case route(conn, conn.assigns.api_version, conn.method, conn.path_info) do + %Plug.Conn{} = conn -> conn + error -> FallbackController.call(conn, error) + end + + {:error, conn} -> + conn + end + end + + # -- Version resolution -------------------------------------------------- + + defp resolve_version(conn) do + case get_req_header(conn, "x-api-version") do + [] -> {:ok, assign(conn, :api_version, :v1)} + ["1"] -> {:ok, assign(conn, :api_version, :v1)} + ["2"] -> {:ok, assign(conn, :api_version, :v2)} + [value] -> {:error, reject_version(conn, value)} + _many -> {:error, reject_version(conn, "multiple")} + end + end + + defp reject_version(conn, value) do + conn + |> put_status(:bad_request) + |> json(%{ + error: + "Unsupported API version: #{inspect(value)}. " <> + "Supported versions: #{Enum.join(@supported_versions, ", ")}." + }) + |> halt() + end + + # -- V1: name-scoped ----------------------------------------------------- + + defp route(conn, :v1, "GET", [name]), + do: C.stream(conn, %{"name" => name}) + + defp route(conn, :v1, "GET", [name, key]), + do: C.get(conn, %{"name" => name, "key" => key}) + + defp route(conn, :v1, "PUT", [name, key]), + do: C.put(conn, body_with(conn, %{"name" => name, "key" => key})) + + defp route(conn, :v1, "POST", [name]), + do: C.put_all(conn, body_with(conn, %{"name" => name})) + + defp route(conn, :v1, "DELETE", [name, key]), + do: C.delete(conn, %{"name" => name, "key" => key}) + + defp route(conn, :v1, "DELETE", [name]), + do: C.delete_all(conn, all_params(conn, %{"name" => name})) + + # -- V2: project-scoped -------------------------------------------------- + + defp route(conn, :v2, "GET", [project_id, name]), + do: C.stream(conn, %{"project_id" => project_id, "name" => name}) + + defp route(conn, :v2, "GET", [project_id, name, key]), + do: + C.get(conn, %{ + "project_id" => project_id, + "name" => name, + "key" => key + }) + + defp route(conn, :v2, "PUT", [project_id, name, key]), + do: + C.put( + conn, + body_with(conn, %{ + "project_id" => project_id, + "name" => name, + "key" => key + }) + ) + + defp route(conn, :v2, "POST", [project_id, name]), + do: + C.put_all( + conn, + body_with(conn, %{"project_id" => project_id, "name" => name}) + ) + + defp route(conn, :v2, "DELETE", [project_id, name, key]), + do: + C.delete(conn, %{ + "project_id" => project_id, + "name" => name, + "key" => key + }) + + defp route(conn, :v2, "DELETE", [project_id, name]), + do: + C.delete_all( + conn, + all_params(conn, %{"project_id" => project_id, "name" => name}) + ) + + # -- Fallback ------------------------------------------------------------- + + defp route(conn, _version, _method, _path) do + conn + |> put_resp_content_type("application/json") + |> send_resp(404, Jason.encode!(%{error: "Not Found"})) + end + + # -- Helpers -------------------------------------------------------------- + + defp body_with(conn, extra), do: Map.merge(conn.body_params, extra) + + defp all_params(conn, extra) do + conn.query_params |> Map.merge(conn.body_params) |> Map.merge(extra) + end +end diff --git a/lib/lightning_web/plugs/versioned_router.ex b/lib/lightning_web/plugs/versioned_router.ex deleted file mode 100644 index c534035627a..00000000000 --- a/lib/lightning_web/plugs/versioned_router.ex +++ /dev/null @@ -1,81 +0,0 @@ -defmodule LightningWeb.Plugs.VersionedRouter do - @moduledoc """ - A reusable plug for header-based API versioning with explicit route - definitions per version. - - Instead of a catch-all `match :*, "/*path"` in the Phoenix router, this - plug is used with `forward` and delegates to version-specific route - modules that pattern-match on `{method, path_segments}`. - - ## Usage - - Define a router module that `use`s this plug: - - defmodule LightningWeb.Collections.Router do - use LightningWeb.Plugs.VersionedRouter, - version_plug: LightningWeb.Plugs.ApiVersion, - fallback: LightningWeb.FallbackController, - versions: %{ - v1: LightningWeb.Collections.V1Routes, - v2: LightningWeb.Collections.V2Routes - } - end - - Then in the Phoenix router: - - forward "/collections", LightningWeb.Collections.Router - - ## Route modules - - Each version module must implement the `c:route/3` callback: - - defmodule LightningWeb.Collections.V1Routes do - @behaviour LightningWeb.Plugs.VersionedRouter - - @impl true - def route(conn, "GET", [name]), - do: MyController.stream(conn, %{"name" => name}) - - def route(conn, _method, _path), - do: {:error, :not_found} - end - - Actions may return either a `%Plug.Conn{}` (rendered directly) or an - error tuple like `{:error, :not_found}`, which is passed to the - configured fallback controller. - """ - - @callback route(Plug.Conn.t(), String.t(), [String.t()]) :: - Plug.Conn.t() | term() - - defmacro __using__(opts) do - quote do - @opts unquote(opts) - - def init(runtime_opts), do: Keyword.merge(@opts, runtime_opts) - - def call(conn, opts) do - version_plug = Keyword.fetch!(opts, :version_plug) - versions = Keyword.fetch!(opts, :versions) - fallback = Keyword.fetch!(opts, :fallback) - - conn = version_plug.call(conn, version_plug.init([])) - - if conn.halted do - conn - else - case Map.get(versions, conn.assigns[:api_version]) do - nil -> - Plug.Conn.send_resp(conn, 404, "Not found") - - handler -> - case handler.route(conn, conn.method, conn.path_info) do - %Plug.Conn{} = conn -> conn - error -> fallback.call(conn, error) - end - end - end - end - end - end -end diff --git a/lib/lightning_web/router.ex b/lib/lightning_web/router.ex index be01a7016b7..179eafb91b2 100644 --- a/lib/lightning_web/router.ex +++ b/lib/lightning_web/router.ex @@ -114,7 +114,7 @@ defmodule LightningWeb.Router do ## Collections scope "/collections" do pipe_through [:authenticated_api] - forward "/", LightningWeb.Collections.Router + forward "/", LightningWeb.Plugs.CollectionsRouter end ## Authentication routes From e0bfc461d4f7521b5fca3ce2382518473657b44c Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Tue, 14 Apr 2026 22:50:23 +0000 Subject: [PATCH 21/25] Fix dialyzer warnings for controller action specs The controller actions return error tuples from with chains (handled by the fallback controller), but their specs claimed Plug.Conn.t() only. This caused a pattern_match_cov warning in the CollectionsRouter plug. Broaden the return types to Plug.Conn.t() | term(). --- .../controllers/collections_controller.ex | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/lightning_web/controllers/collections_controller.ex b/lib/lightning_web/controllers/collections_controller.ex index ac6b3654486..b20f3b9e8d2 100644 --- a/lib/lightning_web/controllers/collections_controller.ex +++ b/lib/lightning_web/controllers/collections_controller.ex @@ -26,7 +26,7 @@ defmodule LightningWeb.CollectionsController do @valid_params ["key", "cursor", "limit" | @timestamp_params] - @spec put(Plug.Conn.t(), map()) :: Plug.Conn.t() + @spec put(Plug.Conn.t(), map()) :: Plug.Conn.t() | term() def put(conn, %{"key" => key, "value" => value} = params) do with {:ok, collection} <- resolve(params), :ok <- authorize(conn, collection), @@ -43,7 +43,7 @@ defmodule LightningWeb.CollectionsController do def put(conn, _params), do: missing_body(conn, "value") - @spec put_all(Plug.Conn.t(), map()) :: Plug.Conn.t() + @spec put_all(Plug.Conn.t(), map()) :: Plug.Conn.t() | term() def put_all(conn, %{"items" => items} = params) do with {:ok, collection} <- resolve(params), :ok <- authorize(conn, collection), @@ -62,7 +62,7 @@ defmodule LightningWeb.CollectionsController do def put_all(conn, _params), do: missing_body(conn, "items") - @spec get(Plug.Conn.t(), map()) :: Plug.Conn.t() + @spec get(Plug.Conn.t(), map()) :: Plug.Conn.t() | term() def get(conn, %{"key" => key} = params) do with {:ok, collection} <- resolve(params), :ok <- authorize(conn, collection) do @@ -73,7 +73,7 @@ defmodule LightningWeb.CollectionsController do end end - @spec delete(Plug.Conn.t(), map()) :: Plug.Conn.t() + @spec delete(Plug.Conn.t(), map()) :: Plug.Conn.t() | term() def delete(conn, %{"key" => key} = params) do with {:ok, collection} <- resolve(params), :ok <- authorize(conn, collection) do @@ -87,7 +87,7 @@ defmodule LightningWeb.CollectionsController do end end - @spec delete_all(Plug.Conn.t(), map()) :: Plug.Conn.t() + @spec delete_all(Plug.Conn.t(), map()) :: Plug.Conn.t() | term() def delete_all(conn, params) do with {:ok, collection} <- resolve(params), :ok <- authorize(conn, collection) do @@ -97,7 +97,7 @@ defmodule LightningWeb.CollectionsController do end end - @spec stream(Plug.Conn.t(), map()) :: Plug.Conn.t() + @spec stream(Plug.Conn.t(), map()) :: Plug.Conn.t() | term() def stream(conn, params) do with {:ok, collection} <- resolve(params), :ok <- authorize(conn, collection), From b15c804a17c475acb04e6faa1135ab23e59ea14b Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Tue, 14 Apr 2026 23:03:06 +0000 Subject: [PATCH 22/25] Move merge pipeline into Sandboxes.merge/4 The full merge sequence (workflow import + collection sync) now lives in the Sandboxes context instead of the LiveView. Any caller gets collection sync automatically, not just the UI. --- lib/lightning/projects/sandboxes.ex | 39 ++++++++++++++++++++ lib/lightning_web/live/sandbox_live/index.ex | 21 ++--------- 2 files changed, 42 insertions(+), 18 deletions(-) diff --git a/lib/lightning/projects/sandboxes.ex b/lib/lightning/projects/sandboxes.ex index 70984c9de99..2bf5a5cfb43 100644 --- a/lib/lightning/projects/sandboxes.ex +++ b/lib/lightning/projects/sandboxes.ex @@ -18,6 +18,7 @@ defmodule Lightning.Projects.Sandboxes do ## Operations * `provision/3` - Create a new sandbox from a parent project + * `merge/4` - Merge a sandbox into its target (workflows + collections) * `update_sandbox/3` - Update sandbox name, color, or environment * `delete_sandbox/2` - Delete a sandbox and all its descendants @@ -39,6 +40,8 @@ defmodule Lightning.Projects.Sandboxes do alias Lightning.Collections alias Lightning.Collections.Collection alias Lightning.Credentials.KeychainCredential + alias Lightning.Projects.MergeProjects + alias Lightning.Projects.Provisioner alias Lightning.Policies.Permissions alias Lightning.Projects.Project alias Lightning.Projects.ProjectCredential @@ -123,6 +126,42 @@ defmodule Lightning.Projects.Sandboxes do end end + @doc """ + Merges a sandbox into its target project. + + Applies the sandbox's workflow configuration to the target via the + provisioner, then synchronises collection names. Collection data is + never copied. + + ## Parameters + * `source` - The sandbox project being merged + * `target` - The project receiving the merge + * `actor` - The user performing the merge + * `opts` - Merge options (`:selected_workflow_ids`, `:deleted_target_workflow_ids`) + + ## Returns + * `{:ok, updated_target}` - Merge and collection sync succeeded + * `{:error, reason}` - Workflow merge or collection sync failed + """ + @spec merge(Project.t(), Project.t(), User.t(), map()) :: + {:ok, Project.t()} | {:error, term()} + def merge( + %Project{} = source, + %Project{} = target, + %User{} = actor, + opts \\ %{} + ) do + merge_doc = MergeProjects.merge_project(source, target, opts) + + with {:ok, updated_target} <- + Provisioner.import_document(target, actor, merge_doc, + allow_stale: true + ), + {:ok, _} <- sync_collections(source, target) do + {:ok, updated_target} + end + end + @doc """ Updates a sandbox project's basic attributes. diff --git a/lib/lightning_web/live/sandbox_live/index.ex b/lib/lightning_web/live/sandbox_live/index.ex index 6d7a8069b6b..17e67fb9866 100644 --- a/lib/lightning_web/live/sandbox_live/index.ex +++ b/lib/lightning_web/live/sandbox_live/index.ex @@ -798,25 +798,10 @@ defmodule LightningWeb.SandboxLive.Index do %{} end - result = - source - |> MergeProjects.merge_project(target, opts) - |> then( - &Lightning.Projects.Provisioner.import_document(target, actor, &1, - allow_stale: true - ) - ) - - case result do + case Sandboxes.merge(source, target, actor, opts) do {:ok, _updated_target} = success -> - case Sandboxes.sync_collections(source, target) do - {:ok, _} -> - maybe_commit_to_github(target, "Merged sandbox #{source.name}") - success - - {:error, reason} -> - {:error, "Failed to sync collections: #{inspect(reason)}"} - end + maybe_commit_to_github(target, "Merged sandbox #{source.name}") + success error -> error From 83a073a1c42157ceac5e43fd24547b17f9c5458c Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Tue, 14 Apr 2026 23:23:56 +0000 Subject: [PATCH 23/25] Fix alias ordering in Sandboxes module --- lib/lightning/projects/sandboxes.ex | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/lightning/projects/sandboxes.ex b/lib/lightning/projects/sandboxes.ex index 2bf5a5cfb43..089d73311e1 100644 --- a/lib/lightning/projects/sandboxes.ex +++ b/lib/lightning/projects/sandboxes.ex @@ -40,11 +40,11 @@ defmodule Lightning.Projects.Sandboxes do alias Lightning.Collections alias Lightning.Collections.Collection alias Lightning.Credentials.KeychainCredential - alias Lightning.Projects.MergeProjects - alias Lightning.Projects.Provisioner alias Lightning.Policies.Permissions + alias Lightning.Projects.MergeProjects alias Lightning.Projects.Project alias Lightning.Projects.ProjectCredential + alias Lightning.Projects.Provisioner alias Lightning.Projects.SandboxPromExPlugin alias Lightning.Repo alias Lightning.Workflows From a36bad316e81c95e7003618d797ff9295f5d0112 Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Tue, 14 Apr 2026 23:58:51 +0000 Subject: [PATCH 24/25] Test that collection sync failure shows flash error on merge --- .../live/sandbox_live/index_test.exs | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/test/lightning_web/live/sandbox_live/index_test.exs b/test/lightning_web/live/sandbox_live/index_test.exs index 3c52b8a67e1..2163e501ae3 100644 --- a/test/lightning_web/live/sandbox_live/index_test.exs +++ b/test/lightning_web/live/sandbox_live/index_test.exs @@ -1601,6 +1601,33 @@ defmodule LightningWeb.SandboxLive.IndexTest do assert length(parent_collections) == 1 assert hd(parent_collections).name == "shared" end + + test "merge fails with flash error when collection sync fails", %{ + conn: conn, + root: root, + sandbox: sandbox + } do + {:ok, view, _} = live(conn, ~p"/projects/#{root.id}/sandboxes") + + Mimic.expect(Lightning.Projects.Sandboxes, :merge, fn _src, + _tgt, + _actor, + _opts -> + {:error, "Failed to sync collections: :boom"} + end) + + Mimic.allow(Lightning.Projects.Sandboxes, self(), view.pid) + + view + |> element("#branch-rewire-sandbox-#{sandbox.id} button") + |> render_click() + + view |> form("#merge-sandbox-modal form") |> render_submit() + + html = render(view) + assert html =~ "Failed to sync collections" + refute has_element?(view, "#merge-sandbox-modal") + end end describe "Merge modal authorization" do From d1fa4e3ec22078e6878553b7df0c40a10a92aa2e Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Wed, 15 Apr 2026 00:02:00 +0000 Subject: [PATCH 25/25] Test Sandboxes.merge/4 including default opts --- test/lightning/sandboxes_test.exs | 50 ++++++++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/test/lightning/sandboxes_test.exs b/test/lightning/sandboxes_test.exs index d73a2663a13..3d7fd057566 100644 --- a/test/lightning/sandboxes_test.exs +++ b/test/lightning/sandboxes_test.exs @@ -690,7 +690,7 @@ defmodule Lightning.Projects.SandboxesTest do ) == [] end - test "runs in a single transaction — either everything or nothing" do + test "runs in a single transaction -- either everything or nothing" do source = insert(:project) target = insert(:project) @@ -721,6 +721,54 @@ defmodule Lightning.Projects.SandboxesTest do end end + describe "merge/4" do + test "imports the merge document and syncs collections" do + actor = insert(:user) + parent = insert(:project) + ensure_member!(parent, actor, :owner) + + insert(:simple_workflow, project: parent) + + sandbox = + insert(:project, + parent: parent, + project_users: [%{user: actor, role: :owner}] + ) + + insert(:simple_workflow, project: sandbox) + + insert(:collection, project: sandbox, name: "new-col") + + assert {:ok, _updated} = Sandboxes.merge(sandbox, parent, actor) + + parent_names = + parent + |> Lightning.Collections.list_project_collections() + |> Enum.map(& &1.name) + + assert "new-col" in parent_names + end + + test "defaults opts to empty map" do + actor = insert(:user) + parent = insert(:project) + ensure_member!(parent, actor, :owner) + + insert(:simple_workflow, project: parent) + + sandbox = + insert(:project, + parent: parent, + project_users: [%{user: actor, role: :owner}] + ) + + insert(:simple_workflow, project: sandbox) + + # Calling with 3 args exercises the \\ %{} default + assert {:ok, _updated} = Sandboxes.merge(sandbox, parent, actor) + end + end + describe "keychains" do test "clones only used keychains and rewires jobs to cloned keychains" do %{