From 8fd134dec2a42d693006662d6b3c11704ee678b9 Mon Sep 17 00:00:00 2001 From: Joe Clark Date: Fri, 10 Apr 2026 16:52:24 +0100 Subject: [PATCH 01/38] 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/38] 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/38] 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/38] 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/38] 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/38] 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/38] 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/38] 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/38] 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/38] 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/38] 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/38] 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/38] 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/38] 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/38] 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/38] 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/38] 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/38] 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/38] 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/38] 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/38] 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/38] 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/38] 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/38] 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/38] 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 %{ From 73a92a37010da74313ef3a4d8e32bba21315fb37 Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Fri, 17 Apr 2026 05:01:41 +0000 Subject: [PATCH 26/38] Add SandboxSettingsBanner component and assign sandbox? in settings Introduces a small reusable banner component with three variants (Local, Editable, Inherited) that explains how settings on a tab will or won't flow on merge. Settings LiveView preloads the parent project and exposes both sandbox? and parent_project as assigns so the template can render the sandbox-specific UI. --- .../components/sandbox_settings_banner.ex | 71 +++++++++++++++++++ .../live/project_live/settings.ex | 46 ++++++++++-- 2 files changed, 113 insertions(+), 4 deletions(-) create mode 100644 lib/lightning_web/components/sandbox_settings_banner.ex diff --git a/lib/lightning_web/components/sandbox_settings_banner.ex b/lib/lightning_web/components/sandbox_settings_banner.ex new file mode 100644 index 00000000000..89535f41d78 --- /dev/null +++ b/lib/lightning_web/components/sandbox_settings_banner.ex @@ -0,0 +1,71 @@ +defmodule LightningWeb.Components.SandboxSettingsBanner do + @moduledoc """ + Banner shown at the top of a Project Settings tab when the project is a + sandbox, communicating how changes on that tab will (or will not) flow + to the parent project on merge. + + Three variants: + + * `:local` — changes apply only to this sandbox and do not sync on merge + * `:editable` — changes will sync to the parent on merge + * `:inherited` — settings are read-only, managed in the parent project + + ## Examples + + <.sandbox_settings_banner variant={:local} /> + + <.sandbox_settings_banner + variant={:inherited} + parent_project={@parent_project} + /> + """ + use LightningWeb, :component + + alias LightningWeb.Components.Common + + attr :variant, :atom, required: true, values: [:local, :editable, :inherited] + attr :id, :string, required: true + attr :parent_project, :map, default: nil + + def sandbox_settings_banner(%{variant: :local} = assigns) do + ~H""" + + <:message> + Changes you make here only apply to this sandbox and won't sync to the parent project on merge. + + + """ + end + + def sandbox_settings_banner(%{variant: :editable} = assigns) do + ~H""" + + <:message> + Changes you make here will sync to the parent project on merge. + + + """ + end + + def sandbox_settings_banner(%{variant: :inherited} = assigns) do + ~H""" + + <:message> + These settings are inherited from the parent project + <.parent_link :if={@parent_project} project={@parent_project} />and can't be changed here. + + + """ + end + + attr :project, :map, required: true + + defp parent_link(assigns) do + ~H""" + (<.link + navigate={~p"/projects/#{@project.id}/settings"} + class="font-medium underline" + >{@project.name}) + """ + end +end diff --git a/lib/lightning_web/live/project_live/settings.ex b/lib/lightning_web/live/project_live/settings.ex index 4f4b134e635..018e68a7eb3 100644 --- a/lib/lightning_web/live/project_live/settings.ex +++ b/lib/lightning_web/live/project_live/settings.ex @@ -6,6 +6,7 @@ defmodule LightningWeb.ProjectLive.Settings do import LightningWeb.LayoutComponents + alias Lightning.Accounts.User alias Lightning.Collections alias Lightning.Credentials alias Lightning.Helpers @@ -18,6 +19,8 @@ defmodule LightningWeb.ProjectLive.Settings do alias Lightning.WebhookAuthMethods alias LightningWeb.Components.GithubComponents + import LightningWeb.Components.SandboxSettingsBanner + require Logger on_mount {LightningWeb.Hooks, :project_scope} @@ -34,6 +37,10 @@ defmodule LightningWeb.ProjectLive.Settings do VersionControl.subscribe(current_user) end + project = Lightning.Repo.preload(project, :parent) + sandbox? = Project.sandbox?(project) + parent_project = if sandbox?, do: project.parent + project_user = Projects.get_project_user(project, current_user) project_files = Projects.list_project_files(project) @@ -126,6 +133,8 @@ defmodule LightningWeb.ProjectLive.Settings do current_user: socket.assigns.current_user, github_enabled: VersionControl.github_enabled?(), name: project.name, + parent_project: parent_project, + project: project, project_changeset: Project.form_changeset(project, %{raw_name: project.name}), project_files: project_files, @@ -133,6 +142,7 @@ defmodule LightningWeb.ProjectLive.Settings do project_user: project_user, project_users: [], projects: projects, + sandbox?: sandbox?, selected_credential_type: nil, show_collaborators_modal: false, show_invite_collaborators_modal: false, @@ -463,7 +473,9 @@ defmodule LightningWeb.ProjectLive.Settings do if user_removable?( project_user, assigns.current_user, - assigns.can_remove_project_user + assigns.can_remove_project_user, + assigns.project, + assigns.sandbox? ) do Projects.delete_project_user!(project_user) @@ -637,7 +649,13 @@ defmodule LightningWeb.ProjectLive.Settings do """ end - defp remove_user_tooltip(project_user, current_user, can_remove_project_user) do + defp remove_user_tooltip( + project_user, + current_user, + can_remove_project_user, + project, + sandbox? + ) do cond do !can_remove_project_user -> "You do not have permission to remove a user" @@ -648,14 +666,34 @@ defmodule LightningWeb.ProjectLive.Settings do project_user.role == :owner -> "You cannot remove an owner" + sandbox? and parent_admin?(project, project_user) -> + "Cannot remove a user who is admin or owner on the parent project" + true -> "" end end - defp user_removable?(project_user, current_user, can_remove_project_user) do + defp user_removable?( + project_user, + current_user, + can_remove_project_user, + project, + sandbox? + ) do can_remove_project_user and project_user.role != :owner and - project_user.user_id != current_user.id + project_user.user_id != current_user.id and + not (sandbox? and parent_admin?(project, project_user)) + end + + defp parent_admin?(project, %{user: %User{} = user}), + do: Lightning.Projects.Sandboxes.parent_admin?(project, user) + + defp parent_admin?(project, %{user_id: user_id}) do + case Lightning.Accounts.get_user(user_id) do + %User{} = user -> Lightning.Projects.Sandboxes.parent_admin?(project, user) + nil -> false + end end defp user_has_valid_oauth_token(user) do From c5357a2e458b0221500d42ca943a40c34fb1ed30 Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Fri, 17 Apr 2026 05:01:50 +0000 Subject: [PATCH 27/38] Apply sandbox-aware UI per settings tab Per Joe's per-page proposal in #3398: * project: Sandbox Identity panel with parent project link, hide Delete button, show Local banner * credentials, collections: Editable banner * webhook_security: replace auth methods table with explanatory message in sandboxes (V1 doesn't support webhook security in sandboxes) * collaboration: Local banner * security (MFA): Inherited banner, MFA toggle disabled * vcs, data-storage, history-exports: Local banner --- .../live/project_live/settings.html.heex | 411 +++++++++++------- 1 file changed, 249 insertions(+), 162 deletions(-) diff --git a/lib/lightning_web/live/project_live/settings.html.heex b/lib/lightning_web/live/project_live/settings.html.heex index 5c4024cd491..774acfa4b0a 100644 --- a/lib/lightning_web/live/project_live/settings.html.heex +++ b/lib/lightning_web/live/project_live/settings.html.heex @@ -62,9 +62,20 @@ History Exports <:panel hash="project" class="space-y-4"> + <.sandbox_settings_banner + :if={@sandbox?} + id="sandbox-banner-project" + variant={:local} + /> <.section_header - title="Project setup" - subtitle="Projects are isolated workspaces that contain workflows, accessible to certain users." + title={if @sandbox?, do: "Sandbox setup", else: "Project setup"} + subtitle={ + if @sandbox?, + do: + "Sandboxes are isolated copies of a project for safe experimentation.", + else: + "Projects are isolated workspaces that contain workflows, accessible to certain users." + } permissions_message="basic settings, but you can export a copy." can_perform_action={@can_edit_project} /> @@ -72,10 +83,22 @@
- Project Identity + {if @sandbox?, do: "Sandbox Identity", else: "Project Identity"}
- This metadata helps you identify the types of workflows managed in this project and the people that have access. + <%= if @sandbox? do %> + Identifies this sandbox within the parent project. + + Parent project: <.link + navigate={~p"/projects/#{@parent_project.id}/settings"} + class="font-medium text-indigo-600 hover:text-indigo-900" + > + {@parent_project.name} + . + + <% else %> + This metadata helps you identify the types of workflows managed in this project and the people that have access. + <% end %>
<.form @@ -233,7 +256,7 @@ Export project
- <%= if @can_delete_project do %> + <%= if @can_delete_project and not @sandbox? do %>
The danger zone
@@ -268,6 +291,11 @@
<:panel hash="credentials" class="space-y-4 block"> + <.sandbox_settings_banner + :if={@sandbox?} + id="sandbox-banner-credentials" + variant={:editable} + /> <.section_header title="Project credentials" subtitle="Manage OAuth 2.0 Clients and Credentials accessible to this project." @@ -315,6 +343,11 @@ /> <:panel hash="collections" class="space-y-4"> + <.sandbox_settings_banner + :if={@sandbox?} + id="sandbox-banner-collections" + variant={:editable} + /> <.live_component module={LightningWeb.ProjectLive.CollectionsComponent} id="collections" @@ -325,171 +358,198 @@ /> <:panel hash="webhook_security" class="space-y-4"> - <.section_header - title="Webhook security" - subtitle="Webhook authentication methods that are used with the starting trigger in workflows." - permissions_message="webhook auth methods." - can_perform_action={@can_write_webhook_auth_method} - action_button_text="New auth method" - action_button_click={ - JS.push("show_modal", value: %{target: "new_webhook_auth_method"}) - } - action_button_disabled={!@can_write_webhook_auth_method} - action_button_id="add_new_auth_method" - /> - <.modal - :if={ - @active_modal in [ - :new_webhook_auth_method, - :edit_webhook_auth_method - ] - } - id="webhook_auth_method_modal" - width="min-w-1/3 max-w-xl" - on_close={JS.push("close_active_modal")} - show={true} - > + <%= if @sandbox? do %> + <.section_header + title="Webhook security" + subtitle="Webhook authentication methods that are used with the starting trigger in workflows." + permissions_message="webhook auth methods." + can_perform_action={false} + /> +
+

+ Webhook security is not available in sandboxes for now. Manage these methods in the parent project <.link + :if={@parent_project} + navigate={ + ~p"/projects/#{@parent_project.id}/settings#webhook_security" + } + class="font-medium text-indigo-600 hover:text-indigo-900" + > + ({@parent_project.name}) + . +

+
+ <% else %> + <.section_header + title="Webhook security" + subtitle="Webhook authentication methods that are used with the starting trigger in workflows." + permissions_message="webhook auth methods." + can_perform_action={@can_write_webhook_auth_method} + action_button_text="New auth method" + action_button_click={ + JS.push("show_modal", value: %{target: "new_webhook_auth_method"}) + } + action_button_disabled={!@can_write_webhook_auth_method} + action_button_id="add_new_auth_method" + /> + <.modal + :if={ + @active_modal in [ + :new_webhook_auth_method, + :edit_webhook_auth_method + ] + } + id="webhook_auth_method_modal" + width="min-w-1/3 max-w-xl" + on_close={JS.push("close_active_modal")} + show={true} + > + <.live_component + :if={@active_modal == :new_webhook_auth_method} + module={LightningWeb.WorkflowLive.WebhookAuthMethodFormComponent} + id="new_auth_method" + action={:new} + current_user={@current_user} + webhook_auth_method={@active_modal_assigns.webhook_auth_method} + trigger={nil} + on_close={JS.push("close_active_modal")} + return_to={~p"/projects/#{@project.id}/settings#webhook_security"} + /> + <.live_component + :if={@active_modal == :edit_webhook_auth_method} + module={LightningWeb.WorkflowLive.WebhookAuthMethodFormComponent} + id={"edit_auth_method_#{@active_modal_assigns.webhook_auth_method.id}"} + action={:edit} + current_user={@current_user} + webhook_auth_method={@active_modal_assigns.webhook_auth_method} + trigger={nil} + on_close={JS.push("close_active_modal")} + return_to={~p"/projects/#{@project.id}/settings#webhook_security"} + /> + <.live_component - :if={@active_modal == :new_webhook_auth_method} - module={LightningWeb.WorkflowLive.WebhookAuthMethodFormComponent} - id="new_auth_method" - action={:new} + :if={@active_modal == :delete_webhook_auth_method} + module={LightningWeb.WorkflowLive.WebhookAuthMethodDeleteModal} + id={"delete_auth_method_#{@active_modal_assigns.webhook_auth_method.id}"} current_user={@current_user} webhook_auth_method={@active_modal_assigns.webhook_auth_method} - trigger={nil} on_close={JS.push("close_active_modal")} return_to={~p"/projects/#{@project.id}/settings#webhook_security"} /> - <.live_component - :if={@active_modal == :edit_webhook_auth_method} - module={LightningWeb.WorkflowLive.WebhookAuthMethodFormComponent} - id={"edit_auth_method_#{@active_modal_assigns.webhook_auth_method.id}"} - action={:edit} - current_user={@current_user} + - - <.live_component - :if={@active_modal == :delete_webhook_auth_method} - module={LightningWeb.WorkflowLive.WebhookAuthMethodDeleteModal} - id={"delete_auth_method_#{@active_modal_assigns.webhook_auth_method.id}"} - current_user={@current_user} - webhook_auth_method={@active_modal_assigns.webhook_auth_method} - on_close={JS.push("close_active_modal")} - return_to={~p"/projects/#{@project.id}/settings#webhook_security"} - /> - - - <:empty_state> - <.empty_state - icon="hero-plus-circle" - message="No auth methods found." - button_text="Create a new auth method" - button_id="open-create-auth-method-modal" - button_click={ - JS.push("show_modal", - value: %{target: "new_webhook_auth_method"} - ) - } - button_disabled={!@can_write_webhook_auth_method} - /> - - <:linked_triggers :let={auth_method}> - - + <:empty_state> + <.empty_state + icon="hero-plus-circle" + message="No auth methods found." + button_text="Create a new auth method" + button_id="open-create-auth-method-modal" + button_click={ JS.push("show_modal", - value: %{ - target: "linked_triggers_for_webhook_auth_method", - id: auth_method.id - } + value: %{target: "new_webhook_auth_method"} ) } - > - {Enum.count(auth_method.triggers)} - - - No associated triggers... + button_disabled={!@can_write_webhook_auth_method} + /> + + <:linked_triggers :let={auth_method}> + + + {Enum.count(auth_method.triggers)} + + + No associated triggers... + - - - <:action :let={auth_method}> - <%= if @can_write_webhook_auth_method do %> - - View - - <% else %> - - Edit - - <% end %> - - <:action :let={auth_method}> - <%= if @can_write_webhook_auth_method do %> - - Delete - - <% else %> - - Delete - - <% end %> - - + + <:action :let={auth_method}> + <%= if @can_write_webhook_auth_method do %> + + View + + <% else %> + + Edit + + <% end %> + + <:action :let={auth_method}> + <%= if @can_write_webhook_auth_method do %> + + Delete + + <% else %> + + Delete + + <% end %> + + + <% end %> <:panel hash="collaboration" class="space-y-4"> + <.sandbox_settings_banner + :if={@sandbox?} + id="sandbox-banner-collaboration" + variant={:local} + /> <.section_header title="Project collaboration" subtitle="View collaborators and manage alert settings for this project." @@ -547,14 +607,18 @@ remove_user_tooltip( project_user, @current_user, - @can_remove_project_user + @can_remove_project_user, + @project, + @sandbox? ) } disabled={ !user_removable?( project_user, @current_user, - @can_remove_project_user + @can_remove_project_user, + @project, + @sandbox? ) } > @@ -580,7 +644,9 @@ user_removable?( project_user, @current_user, - @can_remove_project_user + @can_remove_project_user, + @project, + @sandbox? ) } id={"remove_#{project_user.id}_modal"} @@ -589,11 +655,17 @@ <% end %> <:panel hash="security" class="space-y-4"> + <.sandbox_settings_banner + :if={@sandbox?} + id="sandbox-banner-security" + variant={:inherited} + parent_project={@parent_project} + /> <.section_header title="Project security" subtitle="View and manage security settings for this project." permissions_message="multi-factor authentication settings." - can_perform_action={@can_edit_project} + can_perform_action={@can_edit_project and not @sandbox?} />
<%= if assigns[:mfa_banner] do %> @@ -628,10 +700,10 @@
<:panel hash="data-storage" class="space-y-4"> + <.sandbox_settings_banner + :if={@sandbox?} + id="sandbox-banner-data-storage" + variant={:local} + /> <.section_header title="Data storage" subtitle="View or modify data storage settings for this project." @@ -945,6 +1027,11 @@
<:panel hash="history-exports" class="space-y-4"> + <.sandbox_settings_banner + :if={@sandbox?} + id="sandbox-banner-history-exports" + variant={:local} + /> <.section_header title="History exports" subtitle="View export status and download work order history for this project." From 39527bb325df85fa21756e72bdc5b954b43e72c9 Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Fri, 17 Apr 2026 05:01:59 +0000 Subject: [PATCH 28/38] Enforce parent admin floor rule for sandbox collaborators Adds Sandboxes.parent_admin?/2 which walks the parent chain and returns true when the user is admin or owner on any ancestor project. Projects.delete_project_user!/1 now raises when removing such a user from a sandbox, so the protection holds even if the LiveView UI guard is bypassed. --- lib/lightning/projects.ex | 9 +++++++++ lib/lightning/projects/sandboxes.ex | 29 +++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/lib/lightning/projects.ex b/lib/lightning/projects.ex index 32d663ac7e5..400436b2f54 100644 --- a/lib/lightning/projects.ex +++ b/lib/lightning/projects.ex @@ -574,6 +574,15 @@ defmodule Lightning.Projects do %{user_id: user_id, project_id: project_id} = Repo.preload(project_user, [:user, :project]) + if Project.sandbox?(project_user.project) and + Lightning.Projects.Sandboxes.parent_admin?( + project_user.project, + project_user.user + ) do + raise ArgumentError, + "Cannot remove a parent project admin from a sandbox" + end + Repo.transaction(fn -> from(pc in Lightning.Projects.ProjectCredential, join: c in Lightning.Credentials.Credential, diff --git a/lib/lightning/projects/sandboxes.ex b/lib/lightning/projects/sandboxes.ex index 089d73311e1..bf430e536df 100644 --- a/lib/lightning/projects/sandboxes.ex +++ b/lib/lightning/projects/sandboxes.ex @@ -209,6 +209,35 @@ defmodule Lightning.Projects.Sandboxes do end end + @doc """ + Returns `true` when `user` has an `:admin` or `:owner` role on any ancestor + of `project`, walking the parent chain. + + Used to enforce the parent-admin floor rule: a user who is admin/owner on + any ancestor project cannot be removed from, or downgraded within, a + sandbox descended from that project. + """ + @spec parent_admin?(Project.t(), User.t()) :: boolean() + def parent_admin?(%Project{} = project, %User{} = user) do + project + |> ancestors() + |> Enum.any?(fn ancestor -> + Lightning.Projects.get_project_user_role(user, ancestor) in [ + :admin, + :owner + ] + end) + end + + defp ancestors(%Project{parent_id: nil}), do: [] + + defp ancestors(%Project{parent_id: parent_id}) do + case Lightning.Projects.get_project(parent_id) do + nil -> [] + %Project{} = parent -> [parent | ancestors(parent)] + end + end + @doc """ Deletes a sandbox and all its descendant projects. From ecf49508c4c93b6946ed92df2b141fbb0e528eea Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Fri, 17 Apr 2026 05:02:09 +0000 Subject: [PATCH 29/38] Test sandbox settings page UI and merge-layer field protection LiveView tests cover the per-tab banners, Sandbox Identity panel, disabled MFA toggle, webhook security message, and the parent admin floor rule (UI guard + Projects.delete_project_user! enforcement). Regression tests in sandboxes_test.exs document that the merge pipeline does not propagate Local/Inherited project-level fields (requires_mfa, concurrency, retention, name, description, collaborators) from a sandbox to its parent. These guard against future MergeProjects or Provisioner changes that could accidentally start syncing these fields. --- test/lightning/sandboxes_test.exs | 112 +++++++ .../project_live/sandbox_settings_test.exs | 292 ++++++++++++++++++ 2 files changed, 404 insertions(+) create mode 100644 test/lightning_web/live/project_live/sandbox_settings_test.exs diff --git a/test/lightning/sandboxes_test.exs b/test/lightning/sandboxes_test.exs index 3d7fd057566..c183f81a5a1 100644 --- a/test/lightning/sandboxes_test.exs +++ b/test/lightning/sandboxes_test.exs @@ -769,6 +769,118 @@ defmodule Lightning.Projects.SandboxesTest do end end + # These tests document that the merge pipeline does not propagate + # project-level Local or Inherited fields from sandbox to parent. They + # guard against future changes to MergeProjects or Provisioner that + # could accidentally start syncing these fields. + describe "merge/4 does not propagate Local/Inherited fields" do + setup do + actor = insert(:user) + + parent = + insert(:project, + name: "parent", + description: "parent description", + requires_mfa: false, + concurrency: 5, + retention_policy: :retain_all, + history_retention_period: 30, + dataclip_retention_period: 30 + ) + + ensure_member!(parent, actor, :owner) + insert(:simple_workflow, project: parent) + + sandbox = + insert(:project, + name: "sandbox", + description: "sandbox description", + parent: parent, + requires_mfa: true, + concurrency: 1, + retention_policy: :erase_all, + history_retention_period: 7, + dataclip_retention_period: nil, + project_users: [%{user: actor, role: :owner}] + ) + + insert(:simple_workflow, project: sandbox) + {:ok, actor: actor, parent: parent, sandbox: sandbox} + end + + test "requires_mfa stays at parent's value", %{ + actor: actor, + parent: parent, + sandbox: sandbox + } do + {:ok, _} = Sandboxes.merge(sandbox, parent, actor) + assert Repo.reload(parent).requires_mfa == false + end + + test "concurrency stays at parent's value", %{ + actor: actor, + parent: parent, + sandbox: sandbox + } do + {:ok, _} = Sandboxes.merge(sandbox, parent, actor) + assert Repo.reload(parent).concurrency == 5 + end + + test "retention settings stay at parent's values", %{ + actor: actor, + parent: parent, + sandbox: sandbox + } do + {:ok, _} = Sandboxes.merge(sandbox, parent, actor) + reloaded = Repo.reload(parent) + assert reloaded.retention_policy == :retain_all + assert reloaded.history_retention_period == 30 + assert reloaded.dataclip_retention_period == 30 + end + + test "name and description stay at parent's values", %{ + actor: actor, + parent: parent, + sandbox: sandbox + } do + {:ok, _} = Sandboxes.merge(sandbox, parent, actor) + reloaded = Repo.reload(parent) + assert reloaded.name == "parent" + assert reloaded.description == "parent description" + end + + test "collaborators are not synced from sandbox to parent", %{ + actor: actor, + parent: parent, + sandbox: sandbox + } do + sandbox_only_user = insert(:user) + + insert(:project_user, + project: sandbox, + user: sandbox_only_user, + role: :editor + ) + + parent_user_ids_before = + parent.id + |> Lightning.Projects.get_project_users!() + |> Enum.map(& &1.user_id) + + {:ok, _} = Sandboxes.merge(sandbox, parent, actor) + + parent_user_ids_after = + parent.id + |> Lightning.Projects.get_project_users!() + |> Enum.map(& &1.user_id) + + refute sandbox_only_user.id in parent_user_ids_after + + assert Enum.sort(parent_user_ids_before) == + Enum.sort(parent_user_ids_after) + end + end + describe "keychains" do test "clones only used keychains and rewires jobs to cloned keychains" do %{ diff --git a/test/lightning_web/live/project_live/sandbox_settings_test.exs b/test/lightning_web/live/project_live/sandbox_settings_test.exs new file mode 100644 index 00000000000..ed2d784d09e --- /dev/null +++ b/test/lightning_web/live/project_live/sandbox_settings_test.exs @@ -0,0 +1,292 @@ +defmodule LightningWeb.ProjectLive.SandboxSettingsTest do + use LightningWeb.ConnCase, async: true + + import Phoenix.LiveViewTest + import Lightning.Factories + + alias Lightning.Projects + alias Lightning.Projects.Sandboxes + + setup :stub_usage_limiter_ok + setup :register_and_log_in_user + + defp setup_parent_and_sandbox(%{user: user}) do + parent = + insert(:project, + name: "parent-project", + project_users: [%{user: user, role: :owner}] + ) + + sandbox = + insert(:project, + name: "sandbox-test", + parent: parent, + project_users: [%{user: user, role: :owner}] + ) + + {:ok, parent: parent, sandbox: sandbox} + end + + describe "non-sandbox project (parent project)" do + setup [:setup_parent_and_sandbox] + + test "does not show any sandbox banners", %{conn: conn, parent: parent} do + {:ok, _view, html} = live(conn, ~p"/projects/#{parent.id}/settings") + + refute html =~ "sandbox-banner-" + end + + test "shows 'Project Identity' header on project tab", %{ + conn: conn, + parent: parent + } do + {:ok, _view, html} = live(conn, ~p"/projects/#{parent.id}/settings") + assert html =~ "Project Identity" + refute html =~ "Sandbox Identity" + end + + test "shows the danger zone delete button", %{conn: conn, parent: parent} do + {:ok, _view, html} = live(conn, ~p"/projects/#{parent.id}/settings") + assert html =~ "The danger zone" + assert html =~ "Delete project" + end + + test "shows webhook auth methods table on webhook_security tab", %{ + conn: conn, + parent: parent + } do + {:ok, _view, html} = live(conn, ~p"/projects/#{parent.id}/settings") + refute html =~ "Webhook security is not available in sandboxes" + end + end + + describe "sandbox project" do + setup [:setup_parent_and_sandbox] + + test "shows Editable banner on credentials and collections tabs", %{ + conn: conn, + sandbox: sandbox + } do + {:ok, _view, html} = live(conn, ~p"/projects/#{sandbox.id}/settings") + + assert html =~ ~s(id="sandbox-banner-credentials") + assert html =~ ~s(id="sandbox-banner-collections") + + assert html =~ + "Changes you make here will sync to the parent project on merge." + end + + test "shows Local banner on project, collaboration, vcs, data-storage, history-exports tabs", + %{conn: conn, sandbox: sandbox} do + {:ok, _view, html} = live(conn, ~p"/projects/#{sandbox.id}/settings") + + for tab <- ~w(project collaboration vcs data-storage history-exports) do + assert html =~ ~s(id="sandbox-banner-#{tab}") + end + + assert html =~ + "Changes you make here only apply to this sandbox and won't sync" + end + + test "shows Inherited banner on security tab", %{ + conn: conn, + sandbox: sandbox + } do + {:ok, _view, html} = live(conn, ~p"/projects/#{sandbox.id}/settings") + + assert html =~ ~s(id="sandbox-banner-security") + assert html =~ "These settings are inherited from the parent project" + end + + test "shows 'Sandbox Identity' header instead of 'Project Identity'", %{ + conn: conn, + sandbox: sandbox + } do + {:ok, _view, html} = live(conn, ~p"/projects/#{sandbox.id}/settings") + + assert html =~ "Sandbox Identity" + assert html =~ "Sandbox setup" + assert html =~ "Parent project:" + assert html =~ "parent-project" + end + + test "hides the danger zone delete button", %{conn: conn, sandbox: sandbox} do + {:ok, _view, html} = live(conn, ~p"/projects/#{sandbox.id}/settings") + + refute html =~ "The danger zone" + refute html =~ "Delete project" + end + + test "shows webhook security explanatory message instead of auth methods", + %{conn: conn, sandbox: sandbox} do + {:ok, _view, html} = live(conn, ~p"/projects/#{sandbox.id}/settings") + + assert html =~ "Webhook security is not available in sandboxes for now" + end + + test "MFA toggle is disabled in sandbox", %{conn: conn, sandbox: sandbox} do + {:ok, view, _html} = live(conn, ~p"/projects/#{sandbox.id}/settings") + html = render(view) + + assert html =~ ~s(id="toggle-mfa-switch") + assert html =~ ~s(disabled) + assert html =~ "cursor-not-allowed" + end + end + + describe "Sandboxes.parent_admin?/2" do + test "returns true when user is admin on direct parent" do + user = insert(:user) + parent = insert(:project, project_users: [%{user: user, role: :admin}]) + sandbox = insert(:project, parent: parent, project_users: []) + + assert Sandboxes.parent_admin?(sandbox, user) + end + + test "returns true when user is owner on direct parent" do + user = insert(:user) + parent = insert(:project, project_users: [%{user: user, role: :owner}]) + sandbox = insert(:project, parent: parent, project_users: []) + + assert Sandboxes.parent_admin?(sandbox, user) + end + + test "returns false when user is editor on parent" do + user = insert(:user) + parent = insert(:project, project_users: [%{user: user, role: :editor}]) + sandbox = insert(:project, parent: parent, project_users: []) + + refute Sandboxes.parent_admin?(sandbox, user) + end + + test "returns false when user has no role on parent" do + user = insert(:user) + parent = insert(:project, project_users: []) + sandbox = insert(:project, parent: parent, project_users: []) + + refute Sandboxes.parent_admin?(sandbox, user) + end + + test "walks the chain — admin on grandparent counts" do + user = insert(:user) + + grandparent = + insert(:project, project_users: [%{user: user, role: :admin}]) + + parent = insert(:project, parent: grandparent, project_users: []) + sandbox = insert(:project, parent: parent, project_users: []) + + assert Sandboxes.parent_admin?(sandbox, user) + end + + test "returns false for projects with no parent" do + user = insert(:user) + project = insert(:project, project_users: [%{user: user, role: :admin}]) + + refute Sandboxes.parent_admin?(project, user) + end + end + + describe "delete_project_user! parent admin protection" do + test "raises when removing a parent admin from a sandbox" do + admin = insert(:user) + other = insert(:user) + + parent = + insert(:project, + project_users: [ + %{user: admin, role: :admin}, + %{user: other, role: :owner} + ] + ) + + sandbox = + insert(:project, + parent: parent, + project_users: [ + %{user: admin, role: :editor}, + %{user: other, role: :owner} + ] + ) + + sandbox_pu = Projects.get_project_user(sandbox, admin) + + assert_raise ArgumentError, + ~r/Cannot remove a parent project admin/, + fn -> + Projects.delete_project_user!(sandbox_pu) + end + end + + test "allows removing a non-parent-admin from a sandbox" do + regular = insert(:user) + owner = insert(:user) + + parent = + insert(:project, project_users: [%{user: owner, role: :owner}]) + + sandbox = + insert(:project, + parent: parent, + project_users: [ + %{user: regular, role: :editor}, + %{user: owner, role: :owner} + ] + ) + + sandbox_pu = Projects.get_project_user(sandbox, regular) + + assert %Lightning.Projects.ProjectUser{} = + Projects.delete_project_user!(sandbox_pu) + end + + test "allows removing any user from a non-sandbox project" do + admin = insert(:user) + other = insert(:user) + + project = + insert(:project, + project_users: [ + %{user: admin, role: :admin}, + %{user: other, role: :owner} + ] + ) + + pu = Projects.get_project_user(project, admin) + + assert %Lightning.Projects.ProjectUser{} = + Projects.delete_project_user!(pu) + end + end + + describe "Remove Collaborator UI guard for parent admins" do + test "Remove button is disabled for a parent admin in sandbox", %{ + conn: conn, + user: user + } do + parent_admin = insert(:user, email: "parent-admin@example.com") + + parent = + insert(:project, + project_users: [ + %{user: user, role: :owner}, + %{user: parent_admin, role: :admin} + ] + ) + + sandbox = + insert(:project, + parent: parent, + project_users: [ + %{user: user, role: :owner}, + %{user: parent_admin, role: :editor} + ] + ) + + {:ok, _view, html} = live(conn, ~p"/projects/#{sandbox.id}/settings") + + assert html =~ + "Cannot remove a user who is admin or owner on the parent project" + end + end +end From 0b211c843234981d83ec424ba0547c6ff1f9a957 Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Fri, 17 Apr 2026 05:02:14 +0000 Subject: [PATCH 30/38] Update changelog for sandbox settings page --- CHANGELOG.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a0af6e2b40d..dd86974a4d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,13 @@ and this project adheres to `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) +- Sandbox-aware Project Settings page. Each tab shows a banner explaining how + changes will (or will not) flow on merge: Local (sandbox-only), Editable + (syncs on merge), or Inherited (read-only, managed in the parent). The Sandbox + Identity panel links back to the parent project, the MFA toggle is read-only, + webhook security is unavailable in V1, and parent project admins cannot be + removed from a sandbox. + [#3398](https://github.com/OpenFn/lightning/issues/3398) ### Changed From d860833c50d879fda4c8f3260abf650ecf240cd2 Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Fri, 17 Apr 2026 05:25:10 +0000 Subject: [PATCH 31/38] Fix credo: import order and nested module aliases --- lib/lightning_web/live/project_live/settings.ex | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/lightning_web/live/project_live/settings.ex b/lib/lightning_web/live/project_live/settings.ex index 018e68a7eb3..10673119768 100644 --- a/lib/lightning_web/live/project_live/settings.ex +++ b/lib/lightning_web/live/project_live/settings.ex @@ -4,8 +4,10 @@ defmodule LightningWeb.ProjectLive.Settings do """ use LightningWeb, :live_view + import LightningWeb.Components.SandboxSettingsBanner import LightningWeb.LayoutComponents + alias Lightning.Accounts alias Lightning.Accounts.User alias Lightning.Collections alias Lightning.Credentials @@ -15,12 +17,11 @@ defmodule LightningWeb.ProjectLive.Settings do alias Lightning.Projects.Project alias Lightning.Projects.ProjectLimiter alias Lightning.Projects.ProjectUser + alias Lightning.Projects.Sandboxes alias Lightning.VersionControl alias Lightning.WebhookAuthMethods alias LightningWeb.Components.GithubComponents - import LightningWeb.Components.SandboxSettingsBanner - require Logger on_mount {LightningWeb.Hooks, :project_scope} @@ -687,11 +688,11 @@ defmodule LightningWeb.ProjectLive.Settings do end defp parent_admin?(project, %{user: %User{} = user}), - do: Lightning.Projects.Sandboxes.parent_admin?(project, user) + do: Sandboxes.parent_admin?(project, user) defp parent_admin?(project, %{user_id: user_id}) do - case Lightning.Accounts.get_user(user_id) do - %User{} = user -> Lightning.Projects.Sandboxes.parent_admin?(project, user) + case Accounts.get_user(user_id) do + %User{} = user -> Sandboxes.parent_admin?(project, user) nil -> false end end From 8f083feba69a2be84489c3508e5ec3726e1ab22f Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Fri, 17 Apr 2026 05:38:13 +0000 Subject: [PATCH 32/38] Remove dead nil clauses from parent admin code paths The ancestors/1 helper handled a parent project lookup returning nil, but the parent_id FK uses on_delete: :nilify_all so a stale parent_id can't exist. The LiveView's parent_admin?/2 had a second clause fetching a user by user_id when the user wasn't preloaded, but get_project_users!/1 always preloads :user. Both were defensive code for scenarios prevented by FK constraints. Removed per CLAUDE.md guidance not to handle scenarios that can't happen. --- lib/lightning/projects/sandboxes.ex | 6 ++---- lib/lightning_web/live/project_live/settings.ex | 8 -------- 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/lib/lightning/projects/sandboxes.ex b/lib/lightning/projects/sandboxes.ex index bf430e536df..2422540bdab 100644 --- a/lib/lightning/projects/sandboxes.ex +++ b/lib/lightning/projects/sandboxes.ex @@ -232,10 +232,8 @@ defmodule Lightning.Projects.Sandboxes do defp ancestors(%Project{parent_id: nil}), do: [] defp ancestors(%Project{parent_id: parent_id}) do - case Lightning.Projects.get_project(parent_id) do - nil -> [] - %Project{} = parent -> [parent | ancestors(parent)] - end + parent = Lightning.Projects.get_project!(parent_id) + [parent | ancestors(parent)] end @doc """ diff --git a/lib/lightning_web/live/project_live/settings.ex b/lib/lightning_web/live/project_live/settings.ex index 10673119768..10ef72c4bdf 100644 --- a/lib/lightning_web/live/project_live/settings.ex +++ b/lib/lightning_web/live/project_live/settings.ex @@ -7,7 +7,6 @@ defmodule LightningWeb.ProjectLive.Settings do import LightningWeb.Components.SandboxSettingsBanner import LightningWeb.LayoutComponents - alias Lightning.Accounts alias Lightning.Accounts.User alias Lightning.Collections alias Lightning.Credentials @@ -690,13 +689,6 @@ defmodule LightningWeb.ProjectLive.Settings do defp parent_admin?(project, %{user: %User{} = user}), do: Sandboxes.parent_admin?(project, user) - defp parent_admin?(project, %{user_id: user_id}) do - case Accounts.get_user(user_id) do - %User{} = user -> Sandboxes.parent_admin?(project, user) - nil -> false - end - end - defp user_has_valid_oauth_token(user) do VersionControl.oauth_token_valid?(user.github_oauth_token) end From 662da084cc8d5c7c544b8f9e14787918173c778a Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Fri, 17 Apr 2026 05:42:28 +0000 Subject: [PATCH 33/38] Restore nil parent handling and preload :user in remove handler Two bugs from the previous "remove dead code" commit: 1. Sandboxes.ancestors/1 needs the nil clause back. The parent_id FK uses :nilify_all but that fires at delete-time in the database. An in-memory project struct loaded before the parent was deleted will still have the old parent_id. The nil clause handles this race gracefully instead of crashing the LiveView. Added a regression test that exercises this branch. 2. The handle_event remove path loaded the project_user without preloading :user, so removing the second parent_admin?/2 clause would have crashed it with FunctionClauseError. Preloading :user at the call site is more targeted than restoring the dead clause. --- lib/lightning/projects/sandboxes.ex | 6 ++++-- lib/lightning_web/live/project_live/settings.ex | 4 +++- .../live/project_live/sandbox_settings_test.exs | 13 +++++++++++++ 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/lib/lightning/projects/sandboxes.ex b/lib/lightning/projects/sandboxes.ex index 2422540bdab..bf430e536df 100644 --- a/lib/lightning/projects/sandboxes.ex +++ b/lib/lightning/projects/sandboxes.ex @@ -232,8 +232,10 @@ defmodule Lightning.Projects.Sandboxes do defp ancestors(%Project{parent_id: nil}), do: [] defp ancestors(%Project{parent_id: parent_id}) do - parent = Lightning.Projects.get_project!(parent_id) - [parent | ancestors(parent)] + case Lightning.Projects.get_project(parent_id) do + nil -> [] + %Project{} = parent -> [parent | ancestors(parent)] + end end @doc """ diff --git a/lib/lightning_web/live/project_live/settings.ex b/lib/lightning_web/live/project_live/settings.ex index 10ef72c4bdf..6bbd41209e3 100644 --- a/lib/lightning_web/live/project_live/settings.ex +++ b/lib/lightning_web/live/project_live/settings.ex @@ -17,6 +17,7 @@ defmodule LightningWeb.ProjectLive.Settings do alias Lightning.Projects.ProjectLimiter alias Lightning.Projects.ProjectUser alias Lightning.Projects.Sandboxes + alias Lightning.Repo alias Lightning.VersionControl alias Lightning.WebhookAuthMethods alias LightningWeb.Components.GithubComponents @@ -468,7 +469,8 @@ defmodule LightningWeb.ProjectLive.Settings do %{"project_user_id" => project_user_id}, %{assigns: assigns} = socket ) do - project_user = Projects.get_project_user!(project_user_id) + project_user = + Projects.get_project_user!(project_user_id) |> Repo.preload(:user) if user_removable?( project_user, diff --git a/test/lightning_web/live/project_live/sandbox_settings_test.exs b/test/lightning_web/live/project_live/sandbox_settings_test.exs index ed2d784d09e..47fa5fca723 100644 --- a/test/lightning_web/live/project_live/sandbox_settings_test.exs +++ b/test/lightning_web/live/project_live/sandbox_settings_test.exs @@ -185,6 +185,19 @@ defmodule LightningWeb.ProjectLive.SandboxSettingsTest do refute Sandboxes.parent_admin?(project, user) end + + test "returns false when parent_id points to a deleted project" do + # Race condition: in-memory sandbox struct has a parent_id that + # was nilified in the database after we loaded the struct (because + # the parent project was deleted). The function should treat the + # missing ancestor as no ancestor, not crash. + user = insert(:user) + sandbox = insert(:project) + stale_parent_id = Ecto.UUID.generate() + stale_sandbox = %{sandbox | parent_id: stale_parent_id} + + refute Sandboxes.parent_admin?(stale_sandbox, user) + end end describe "delete_project_user! parent admin protection" do From 4b7d0c5c162114d4c126ac11dadcc2c58f138aff Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Fri, 17 Apr 2026 05:44:45 +0000 Subject: [PATCH 34/38] Move :user preload into Projects.get_project_user!/2 The view shouldn't reach into Repo. Adds an include opt to get_project_user!/2 so callers can request preloaded associations through the context (mirrors Ecto's :preload but named for the caller's intent). Drops the now-unused Repo alias in the settings LiveView. --- lib/lightning/projects.ex | 5 ++++- lib/lightning_web/live/project_live/settings.ex | 4 +--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/lightning/projects.ex b/lib/lightning/projects.ex index 400436b2f54..1cde89e49f8 100644 --- a/lib/lightning/projects.ex +++ b/lib/lightning/projects.ex @@ -319,7 +319,10 @@ defmodule Lightning.Projects do ** (Ecto.NoResultsError) """ - def get_project_user!(id), do: Repo.get!(ProjectUser, id) + def get_project_user!(id, opts \\ []) do + include = Keyword.get(opts, :include, []) + ProjectUser |> Repo.get!(id) |> Repo.preload(include) + end @spec get_project_user(Ecto.UUID.t()) :: ProjectUser.t() | nil def get_project_user(id) when is_binary(id), do: Repo.get(ProjectUser, id) diff --git a/lib/lightning_web/live/project_live/settings.ex b/lib/lightning_web/live/project_live/settings.ex index 6bbd41209e3..02339e98841 100644 --- a/lib/lightning_web/live/project_live/settings.ex +++ b/lib/lightning_web/live/project_live/settings.ex @@ -17,7 +17,6 @@ defmodule LightningWeb.ProjectLive.Settings do alias Lightning.Projects.ProjectLimiter alias Lightning.Projects.ProjectUser alias Lightning.Projects.Sandboxes - alias Lightning.Repo alias Lightning.VersionControl alias Lightning.WebhookAuthMethods alias LightningWeb.Components.GithubComponents @@ -469,8 +468,7 @@ defmodule LightningWeb.ProjectLive.Settings do %{"project_user_id" => project_user_id}, %{assigns: assigns} = socket ) do - project_user = - Projects.get_project_user!(project_user_id) |> Repo.preload(:user) + project_user = Projects.get_project_user!(project_user_id, include: :user) if user_removable?( project_user, From 6916e934266c07850f162edf805a953585bd6aff Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Sun, 19 Apr 2026 12:47:29 +0000 Subject: [PATCH 35/38] Address Joe's review + polish sandbox settings UX Copy and styling (Joe's inline review): - Drop "won't sync" / "can't be changed here" in favour of "do not sync" / "cannot be changed here" in the sandbox settings banner. - Add a coloured border to each banner variant so they don't look ghosted. - Reword the Sandbox Identity parent line to "Identifies this sandbox within its parent: ". - Drop "for now" from the Webhook Security copy. Banner placement: - Move the banner under the tab's section_header across all sandbox panels so the reading order is page -> section -> context -> content. - For Collections, the section_header lives inside CollectionsComponent, so pass `sandbox?` into the component and render the banner there. Fix a misleading "Role based permissions" message: - The Webhook Security and Security (MFA) sandbox branches were passing `can_perform_action={false}` to section_header, which forced the role message even for admins. The block isn't role-based in either case: webhook security is categorically disabled, MFA is inherited. - Make `permissions_message` on section_header optional (default nil). When nil, the component does not render the role message even if `can_perform_action` is false. Existing callers are unaffected. - Webhook Security sandbox branch: render a regular content card (matching other settings cards on the page) with accurate copy that explains webhook auth methods are shared with and enforced on the sandbox but only manageable from the parent. Avoids the misleading "disabled" framing. - Security sandbox branch: `can_perform_action={@can_edit_project}` with `permissions_message` nilified in sandbox mode. Role message still fires for viewers outside sandbox; in sandbox, the Inherited banner and the disabled MFA toggle carry the meaning without double-messaging. Tests: - New regression: the sandbox settings page never renders the role permissions line on the webhook_security or security tabs. --- CHANGELOG.md | 4 +- .../components/layout_components.ex | 4 +- .../components/sandbox_settings_banner.ex | 10 +- .../project_live/collections_component.ex | 15 +- .../live/project_live/settings.html.heex | 129 +++++++++--------- .../project_live/sandbox_settings_test.exs | 15 +- 6 files changed, 98 insertions(+), 79 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dd86974a4d6..fdb7d974112 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,8 +28,8 @@ and this project adheres to changes will (or will not) flow on merge: Local (sandbox-only), Editable (syncs on merge), or Inherited (read-only, managed in the parent). The Sandbox Identity panel links back to the parent project, the MFA toggle is read-only, - webhook security is unavailable in V1, and parent project admins cannot be - removed from a sandbox. + webhook authentication methods are managed from the parent project, and parent + project admins cannot be removed from a sandbox. [#3398](https://github.com/OpenFn/lightning/issues/3398) ### Changed diff --git a/lib/lightning_web/components/layout_components.ex b/lib/lightning_web/components/layout_components.ex index cc5c9024022..df608976857 100644 --- a/lib/lightning_web/components/layout_components.ex +++ b/lib/lightning_web/components/layout_components.ex @@ -388,7 +388,7 @@ defmodule LightningWeb.LayoutComponents do attr :title, :string, required: true attr :subtitle, :string, required: true - attr :permissions_message, :string, required: true + attr :permissions_message, :string, default: nil attr :can_perform_action, :boolean, default: true attr :action_button_text, :string, default: nil attr :action_button_click, :any, default: nil @@ -409,7 +409,7 @@ defmodule LightningWeb.LayoutComponents do {@subtitle} - <%= if !@can_perform_action do %> + <%= if !@can_perform_action and @permissions_message do %> <.permissions_message section={@permissions_message} /> <% end %>
diff --git a/lib/lightning_web/components/sandbox_settings_banner.ex b/lib/lightning_web/components/sandbox_settings_banner.ex index 89535f41d78..e9f6617a7ad 100644 --- a/lib/lightning_web/components/sandbox_settings_banner.ex +++ b/lib/lightning_web/components/sandbox_settings_banner.ex @@ -29,9 +29,9 @@ defmodule LightningWeb.Components.SandboxSettingsBanner do def sandbox_settings_banner(%{variant: :local} = assigns) do ~H""" - + <:message> - Changes you make here only apply to this sandbox and won't sync to the parent project on merge. + Changes you make here only apply to this sandbox and do not sync to the parent project on merge. """ @@ -39,7 +39,7 @@ defmodule LightningWeb.Components.SandboxSettingsBanner do def sandbox_settings_banner(%{variant: :editable} = assigns) do ~H""" - + <:message> Changes you make here will sync to the parent project on merge. @@ -49,10 +49,10 @@ defmodule LightningWeb.Components.SandboxSettingsBanner do def sandbox_settings_banner(%{variant: :inherited} = assigns) do ~H""" - + <:message> These settings are inherited from the parent project - <.parent_link :if={@parent_project} project={@parent_project} />and can't be changed here. + <.parent_link :if={@parent_project} project={@parent_project} />and cannot be changed here. """ diff --git a/lib/lightning_web/live/project_live/collections_component.ex b/lib/lightning_web/live/project_live/collections_component.ex index 92a1cd6ff02..f8c698b5ba7 100644 --- a/lib/lightning_web/live/project_live/collections_component.ex +++ b/lib/lightning_web/live/project_live/collections_component.ex @@ -3,6 +3,7 @@ defmodule LightningWeb.ProjectLive.CollectionsComponent do use LightningWeb, :live_component + import LightningWeb.Components.SandboxSettingsBanner import LightningWeb.LayoutComponents alias Lightning.Collections @@ -16,8 +17,13 @@ defmodule LightningWeb.ProjectLive.CollectionsComponent do @impl true def update( - %{can_create_collection: _, collections: _, return_to: _, project: _} = - assigns, + %{ + can_create_collection: _, + collections: _, + return_to: _, + project: _, + sandbox?: _ + } = assigns, socket ) do {:ok, @@ -178,6 +184,11 @@ defmodule LightningWeb.ProjectLive.CollectionsComponent do action_button_disabled={!@can_create_collection} action_button_id="open-create-collection-modal-button" /> + <.sandbox_settings_banner + :if={@sandbox?} + id="sandbox-banner-collections" + variant={:editable} + /> <.form_modal_component :if={@action == :new} diff --git a/lib/lightning_web/live/project_live/settings.html.heex b/lib/lightning_web/live/project_live/settings.html.heex index 774acfa4b0a..2d3aab8eb70 100644 --- a/lib/lightning_web/live/project_live/settings.html.heex +++ b/lib/lightning_web/live/project_live/settings.html.heex @@ -62,11 +62,6 @@ History Exports <:panel hash="project" class="space-y-4"> - <.sandbox_settings_banner - :if={@sandbox?} - id="sandbox-banner-project" - variant={:local} - /> <.section_header title={if @sandbox?, do: "Sandbox setup", else: "Project setup"} subtitle={ @@ -79,6 +74,11 @@ permissions_message="basic settings, but you can export a copy." can_perform_action={@can_edit_project} /> + <.sandbox_settings_banner + :if={@sandbox?} + id="sandbox-banner-project" + variant={:local} + />
@@ -87,15 +87,13 @@ <%= if @sandbox? do %> - Identifies this sandbox within the parent project. - - Parent project: <.link - navigate={~p"/projects/#{@parent_project.id}/settings"} - class="font-medium text-indigo-600 hover:text-indigo-900" - > - {@parent_project.name} - . - + Identifies this sandbox within its parent: <.link + :if={@parent_project} + navigate={~p"/projects/#{@parent_project.id}/settings"} + class="font-medium text-indigo-600 hover:text-indigo-900" + > + {@parent_project.name} + . <% else %> This metadata helps you identify the types of workflows managed in this project and the people that have access. <% end %> @@ -291,11 +289,6 @@
<:panel hash="credentials" class="space-y-4 block"> - <.sandbox_settings_banner - :if={@sandbox?} - id="sandbox-banner-credentials" - variant={:editable} - /> <.section_header title="Project credentials" subtitle="Manage OAuth 2.0 Clients and Credentials accessible to this project." @@ -332,6 +325,11 @@ + <.sandbox_settings_banner + :if={@sandbox?} + id="sandbox-banner-credentials" + variant={:editable} + /> <:panel hash="collections" class="space-y-4"> - <.sandbox_settings_banner - :if={@sandbox?} - id="sandbox-banner-collections" - variant={:editable} - /> <.live_component module={LightningWeb.ProjectLive.CollectionsComponent} id="collections" project={@project} collections={@collections} can_create_collection={@can_create_collection} + sandbox?={@sandbox?} return_to={~p"/projects/#{@project.id}/settings#collections"} /> @@ -362,21 +356,26 @@ <.section_header title="Webhook security" subtitle="Webhook authentication methods that are used with the starting trigger in workflows." - permissions_message="webhook auth methods." - can_perform_action={false} /> -
-

- Webhook security is not available in sandboxes for now. Manage these methods in the parent project <.link - :if={@parent_project} - navigate={ - ~p"/projects/#{@parent_project.id}/settings#webhook_security" - } - class="font-medium text-indigo-600 hover:text-indigo-900" - > - ({@parent_project.name}) - . -

+
+
+ Webhook authentication is managed in the parent project +
+ + Methods are shared with this sandbox and enforced on its webhook triggers, but can only be created, edited, or deleted from the parent. + + <.link + :if={@parent_project} + navigate={ + ~p"/projects/#{@parent_project.id}/settings#webhook_security" + } + class="mt-2 inline-block text-xs font-medium text-indigo-600 hover:text-indigo-900" + > + Manage them in the parent project ({@parent_project.name}) +
<% else %> <.section_header @@ -545,11 +544,6 @@ <% end %> <:panel hash="collaboration" class="space-y-4"> - <.sandbox_settings_banner - :if={@sandbox?} - id="sandbox-banner-collaboration" - variant={:local} - /> <.section_header title="Project collaboration" subtitle="View collaborators and manage alert settings for this project." @@ -569,6 +563,11 @@ } action_button_id="show_collaborators_modal_button" /> + <.sandbox_settings_banner + :if={@sandbox?} + id="sandbox-banner-collaboration" + variant={:local} + /> <.support_access_toggle can_edit_project={@can_edit_project} project={@project} @@ -655,18 +654,20 @@ <% end %> <:panel hash="security" class="space-y-4"> + <.section_header + title="Project security" + subtitle="View and manage security settings for this project." + permissions_message={ + unless @sandbox?, do: "multi-factor authentication settings." + } + can_perform_action={@can_edit_project} + /> <.sandbox_settings_banner :if={@sandbox?} id="sandbox-banner-security" variant={:inherited} parent_project={@parent_project} /> - <.section_header - title="Project security" - subtitle="View and manage security settings for this project." - permissions_message="multi-factor authentication settings." - can_perform_action={@can_edit_project and not @sandbox?} - />
<%= if assigns[:mfa_banner] do %> {Phoenix.LiveView.TagEngine.component( @@ -730,17 +731,17 @@
<:panel hash="vcs" class="space-y-4"> - <.sandbox_settings_banner - :if={@sandbox?} - id="sandbox-banner-vcs" - variant={:local} - /> <.section_header title="Version control" subtitle="View or modify external version control settings for this project." permissions_message={"version control configuration#{@can_initiate_github_sync && " but you can initiate a sync." || "."}"} can_perform_action={@can_install_github} /> + <.sandbox_settings_banner + :if={@sandbox?} + id="sandbox-banner-vcs" + variant={:local} + />
<%= if assigns[:github_banner] do %> {Phoenix.LiveView.TagEngine.component( @@ -834,17 +835,17 @@
<:panel hash="data-storage" class="space-y-4"> - <.sandbox_settings_banner - :if={@sandbox?} - id="sandbox-banner-data-storage" - variant={:local} - /> <.section_header title="Data storage" subtitle="View or modify data storage settings for this project." permissions_message="data storage settings." can_perform_action={@can_edit_data_retention} /> + <.sandbox_settings_banner + :if={@sandbox?} + id="sandbox-banner-data-storage" + variant={:local} + />
<.form :let={f} @@ -1027,17 +1028,17 @@
<:panel hash="history-exports" class="space-y-4"> - <.sandbox_settings_banner - :if={@sandbox?} - id="sandbox-banner-history-exports" - variant={:local} - /> <.section_header title="History exports" subtitle="View export status and download work order history for this project." permissions_message="history exports." can_perform_action={true} /> + <.sandbox_settings_banner + :if={@sandbox?} + id="sandbox-banner-history-exports" + variant={:local} + /> Date: Sun, 19 Apr 2026 21:20:52 +0000 Subject: [PATCH 36/38] Polish project settings page UX Unify the card pattern across panels, tighten labels, and wire the MFA toggle through the shared <.input type="toggle"> component so every on/off control on the page uses the same primitive. - Shared toggle component now accepts %JS{} commands on on_click. - Version switcher in workflow editor uses the shared toggle too. - Sentence-case tab labels, panel titles, and sub-section headings. - Security panel: MFA + webhook auth presented as sibling cards. - Setup panel: identity/concurrency/export/danger zone cards. - Data storage: each sub-setting in its own card, shared save footer. - Danger zone: red-bordered card with "What gets deleted" detail. - Exports tab title aligned with its tab label. - Layout: reserve scrollbar gutter to stop horizontal jitter. - Shared table container gets overflow-hidden to clip white tbody corners inside the rounded outer ring. --- .../components/layout_components.ex | 2 +- lib/lightning_web/components/new_inputs.ex | 28 +- lib/lightning_web/components/table.ex | 5 +- .../live/project_live/settings.ex | 10 +- .../live/project_live/settings.html.heex | 931 ++++++++---------- lib/lightning_web/live/workflow_live/edit.ex | 49 +- .../project_live/sandbox_settings_test.exs | 21 +- test/lightning_web/live/project_live_test.exs | 74 +- .../live/workflow_live/edit_test.exs | 4 +- 9 files changed, 526 insertions(+), 598 deletions(-) diff --git a/lib/lightning_web/components/layout_components.ex b/lib/lightning_web/components/layout_components.ex index df608976857..9afb8ffd968 100644 --- a/lib/lightning_web/components/layout_components.ex +++ b/lib/lightning_web/components/layout_components.ex @@ -186,7 +186,7 @@ defmodule LightningWeb.LayoutComponents do
{render_slot(@inner_block)}
diff --git a/lib/lightning_web/components/new_inputs.ex b/lib/lightning_web/components/new_inputs.ex index f54fc8b0c3d..e775c6b0ecf 100644 --- a/lib/lightning_web/components/new_inputs.ex +++ b/lib/lightning_web/components/new_inputs.ex @@ -415,7 +415,9 @@ defmodule LightningWeb.Components.NewInputs do attr :tooltip, :any, default: nil - attr :on_click, :string, default: nil + attr :on_click, :any, default: nil + + attr :on_click_target, :any, default: nil attr :value_key, :any, default: nil @@ -875,13 +877,7 @@ defmodule LightningWeb.Components.NewInputs do