Merge branch 'main' into nv/layout-validations

pkg/types/dashboardtypes/perses_dashboard_data.go

@@ -138,14 +138,33 @@ func validateQueryAllowedForPanel(plugin QueryPlugin, allowed []QueryPluginKind,
 	return nil
 }
 
-// validateLayouts rejects grid items referencing a panel that doesn't exist.
+const maxLayoutsPerDashboard = 500
+
+// validateLayouts validates the dashboard's layouts: bounded section count,
+// per-item geometry, resolvable panel references, and no panel placed twice.
+// Geometry (validateGridLayoutGeometry) needs only each layout's own data but
+// runs here so its errors can name the layout by index.
 func (d *DashboardSpec) validateLayouts() error {
+	if len(d.Layouts) > maxLayoutsPerDashboard {
+		return errors.NewInvalidInputf(ErrCodeDashboardInvalidInput, "spec.layouts: dashboard has %d layouts; maximum is %d", len(d.Layouts), maxLayoutsPerDashboard)
+	}
+
+	// Could enforce this but skipping for now: panels in no grid item (orphans)
+	// are allowed.
+
+	// The frontend keys each grid item by its panel id, so placing one panel in
+	// two grid items collides; reject duplicate references dashboard-wide. Maps
+	// each referenced panel key to the path of the item that first placed it.
+	referencedPanels := make(map[string]string, len(d.Panels))
 	for li, layout := range d.Layouts {
 		grid, ok := layout.Spec.(*dashboard.GridLayoutSpec)
 		if !ok {
 			// Unreachable via UnmarshalJSON; reaching here means a Go caller broke the Kind/Spec pairing.
 			return errors.NewInternalf(errors.CodeInternal, "spec.layouts[%d].spec: unexpected layout spec type %T", li, layout.Spec)
 		}
+		if err := validateGridLayoutGeometry(grid, li); err != nil {
+			return err
+		}
 		for ii, item := range grid.Items {
 			path := fmt.Sprintf("spec.layouts[%d].spec.items[%d].content", li, ii)
 			if item.Content == nil {
@@ -158,6 +177,10 @@ func (d *DashboardSpec) validateLayouts() error {
 			if _, ok := d.Panels[key]; !ok {
 				return errors.NewInvalidInputf(ErrCodeDashboardInvalidInput, "%s: references unknown panel %q", path, key)
 			}
+			if firstPath, dup := referencedPanels[key]; dup {
+				return errors.NewInvalidInputf(ErrCodeDashboardInvalidInput, "%s: panel %q is already placed by %s", path, key, firstPath)
+			}
+			referencedPanels[key] = path
 		}
 	}
 	return nil

pkg/types/dashboardtypes/perses_dashboard_patch_test.go

@@ -299,19 +299,22 @@ func TestPatchableDashboardV2_Apply(t *testing.T) {
 	// Layout edits
 	// ─────────────────────────────────────────────────────────────────
 
-	t.Run("move panel by editing layout x coordinate", func(t *testing.T) {
-		out, err := decode(t, `[{"op": "replace", "path": "/spec/layouts/0/spec/items/0/x", "value": 6}]`).Apply(base)
+	t.Run("move panel by editing layout y coordinate", func(t *testing.T) {
+		// p2 fills the right half of row 0, so p1 can only move to a fresh row
+		// without tripping overlap validation.
+		out, err := decode(t, `[{"op": "replace", "path": "/spec/layouts/0/spec/items/0/y", "value": 6}]`).Apply(base)
 		require.NoError(t, err)
 		raw := jsonOf(t, out)
-		// The first item used to live at x=0, now lives at x=6.
-		assert.Contains(t, raw, `"x":6,"y":0,"width":6,"height":6,"content":{"$ref":"#/spec/panels/p1"}`)
+		// The first item used to live at y=0, now lives at y=6.
+		assert.Contains(t, raw, `"x":0,"y":6,"width":6,"height":6,"content":{"$ref":"#/spec/panels/p1"}`)
 	})
 
 	t.Run("resize panel by editing layout width", func(t *testing.T) {
-		out, err := decode(t, `[{"op": "replace", "path": "/spec/layouts/0/spec/items/0/width", "value": 12}]`).Apply(base)
+		// p2 sits at x=6, so p1 (at x=0) can only shrink; widening it would overlap.
+		out, err := decode(t, `[{"op": "replace", "path": "/spec/layouts/0/spec/items/0/width", "value": 3}]`).Apply(base)
 		require.NoError(t, err)
 		raw := jsonOf(t, out)
-		assert.Contains(t, raw, `"width":12`)
+		assert.Contains(t, raw, `"width":3`)
 	})
 
 	t.Run("rename layout row title", func(t *testing.T) {
@@ -321,11 +324,12 @@ func TestPatchableDashboardV2_Apply(t *testing.T) {
 	})
 
 	t.Run("append layout item", func(t *testing.T) {
-		out, err := decode(t, `[{
-			"op": "add",
-			"path": "/spec/layouts/0/spec/items/-",
-			"value": {"x": 0, "y": 6, "width": 12, "height": 6, "content": {"$ref": "#/spec/panels/p1"}}
-		}]`).Apply(base)
+		// Appending needs a not-yet-placed panel, so add one in the same patch;
+		// re-placing p1 or p2 would be a duplicate reference.
+		out, err := decode(t, `[
+			{"op": "add", "path": "/spec/panels/p3", "value": {"kind": "Panel", "spec": {"plugin": {"kind": "signoz/TablePanel", "spec": {}}, "queries": [{"kind": "time_series", "spec": {"plugin": {"kind": "signoz/BuilderQuery", "spec": {"name": "A", "signal": "logs", "aggregations": [{"expression": "count()"}]}}}}]}}},
+			{"op": "add", "path": "/spec/layouts/0/spec/items/-", "value": {"x": 0, "y": 6, "width": 12, "height": 6, "content": {"$ref": "#/spec/panels/p3"}}}
+		]`).Apply(base)
 		require.NoError(t, err)
 		// Item count went 2 → 3.
 		raw := jsonOf(t, out)

pkg/types/dashboardtypes/perses_dashboard_test.go

@@ -8,6 +8,7 @@ import (
 	"time"
 
 	"github.com/SigNoz/signoz/pkg/errors"
+	"github.com/perses/spec/go/dashboard"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"k8s.io/apimachinery/pkg/util/validation"
@@ -1442,3 +1443,123 @@ func TestPanelTypeQueryTypeCompatibility(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateGridGeometry(t *testing.T) {
+	tests := []struct {
+		scenario         string
+		items            []dashboard.GridItem
+		expectErrContain string
+	}{
+		{
+			scenario:         "valid side-by-side items",
+			items:            []dashboard.GridItem{{X: 0, Y: 0, Width: 6, Height: 6}, {X: 6, Y: 0, Width: 6, Height: 6}},
+			expectErrContain: "",
+		},
+		{
+			scenario:         "valid full-width item",
+			items:            []dashboard.GridItem{{X: 0, Y: 0, Width: 12, Height: 6}},
+			expectErrContain: "",
+		},
+		{
+			scenario:         "stacked items do not overlap",
+			items:            []dashboard.GridItem{{X: 0, Y: 0, Width: 6, Height: 6}, {X: 0, Y: 6, Width: 6, Height: 6}},
+			expectErrContain: "",
+		},
+		{
+			scenario:         "zero width",
+			items:            []dashboard.GridItem{{X: 0, Y: 0, Width: 0, Height: 6}},
+			expectErrContain: "width must be at least 1",
+		},
+		{
+			scenario:         "zero height",
+			items:            []dashboard.GridItem{{X: 0, Y: 0, Width: 6, Height: 0}},
+			expectErrContain: "height must be at least 1",
+		},
+		{
+			scenario:         "negative x",
+			items:            []dashboard.GridItem{{X: -1, Y: 0, Width: 6, Height: 6}},
+			expectErrContain: "x must not be negative",
+		},
+		{
+			scenario:         "negative y",
+			items:            []dashboard.GridItem{{X: 0, Y: -1, Width: 6, Height: 6}},
+			expectErrContain: "y must not be negative",
+		},
+		{
+			scenario:         "width wider than grid",
+			items:            []dashboard.GridItem{{X: 0, Y: 0, Width: 13, Height: 6}},
+			expectErrContain: "width (13) exceeds grid width 12",
+		},
+		{
+			scenario:         "x at grid width",
+			items:            []dashboard.GridItem{{X: 12, Y: 0, Width: 1, Height: 6}},
+			expectErrContain: "x (12) must be less than grid width 12",
+		},
+		{
+			scenario:         "x plus width overflows grid",
+			items:            []dashboard.GridItem{{X: 8, Y: 0, Width: 6, Height: 6}},
+			expectErrContain: "x (8) + width (6) exceeds grid width 12",
+		},
+		{
+			scenario:         "overlapping items",
+			items:            []dashboard.GridItem{{X: 0, Y: 0, Width: 6, Height: 6}, {X: 3, Y: 3, Width: 6, Height: 6}},
+			expectErrContain: "items[0] and items[1] overlap",
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.scenario, func(t *testing.T) {
+			err := validateGridLayoutGeometry(&dashboard.GridLayoutSpec{Items: test.items}, 0)
+			if test.expectErrContain == "" {
+				require.NoError(t, err)
+				return
+			}
+			require.Error(t, err)
+			require.Contains(t, err.Error(), test.expectErrContain)
+		})
+	}
+}
+
+func TestValidateGridItemLimit(t *testing.T) {
+	err := validateGridLayoutGeometry(&dashboard.GridLayoutSpec{Items: make([]dashboard.GridItem, maxItemsPerGridLayout+1)}, 0)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "maximum is")
+}
+
+// Both panel refs are valid, so this errors only if geometry validation runs on
+// the unmarshal path — it does, via DashboardSpec.Validate -> validateLayouts.
+func TestInvalidateLayoutOverlapViaUnmarshal(t *testing.T) {
+	data := []byte(`{
+		"panels": {
+			"p1": {"kind": "Panel", "spec": {"plugin": {"kind": "signoz/TablePanel", "spec": {}}, "queries": [{"kind": "time_series", "spec": {"plugin": {"kind": "signoz/BuilderQuery", "spec": {"name": "A", "signal": "logs", "aggregations": [{"expression": "count()"}]}}}}]}},
+			"p2": {"kind": "Panel", "spec": {"plugin": {"kind": "signoz/TablePanel", "spec": {}}, "queries": [{"kind": "time_series", "spec": {"plugin": {"kind": "signoz/BuilderQuery", "spec": {"name": "A", "signal": "logs", "aggregations": [{"expression": "count()"}]}}}}]}}
+		},
+		"layouts": [{"kind": "Grid", "spec": {"items": [
+			{"x": 0, "y": 0, "width": 6, "height": 6, "content": {"$ref": "#/spec/panels/p1"}},
+			{"x": 3, "y": 3, "width": 6, "height": 6, "content": {"$ref": "#/spec/panels/p2"}}
+		]}}]
+	}`)
+	_, err := unmarshalDashboard(data)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "overlap")
+}
+
+// The frontend keys each grid item by its panel id, so the same panel placed by
+// two grid items crashes the section; the backend rejects it dashboard-wide. The
+// two items are side by side so they clear the overlap check first.
+func TestInvalidateDuplicatePanelReference(t *testing.T) {
+	data := []byte(`{
+		"panels": {
+			"p1": {"kind": "Panel", "spec": {"plugin": {"kind": "signoz/TablePanel", "spec": {}}, "queries": [{"kind": "time_series", "spec": {"plugin": {"kind": "signoz/BuilderQuery", "spec": {"name": "A", "signal": "logs", "aggregations": [{"expression": "count()"}]}}}}]}}
+		},
+		"layouts": [{"kind": "Grid", "spec": {"items": [
+			{"x": 0, "y": 0, "width": 6, "height": 6, "content": {"$ref": "#/spec/panels/p1"}},
+			{"x": 6, "y": 0, "width": 6, "height": 6, "content": {"$ref": "#/spec/panels/p1"}}
+		]}}]
+	}`)
+	_, err := unmarshalDashboard(data)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "already placed")
+	// Both offending grid items are named.
+	require.Contains(t, err.Error(), "spec.layouts[0].spec.items[0].content")
+	require.Contains(t, err.Error(), "spec.layouts[0].spec.items[1].content")
+}

pkg/types/dashboardtypes/perses_replicas.go

@@ -322,6 +322,55 @@ func (l *Layout) UnmarshalJSON(data []byte) error {
 	return nil
 }
 
+const (
+	gridColumnCount       = 12
+	maxItemsPerGridLayout = 100
+)
+
+// validateGridLayoutGeometry checks a single grid layout's item geometry (size,
+// position, and intra-section overlap), which Perses does not. It reads only the
+// layout's own items; layoutIndex is supplied by the caller (validateLayouts)
+// solely to name the layout in error paths.
+func validateGridLayoutGeometry(spec *dashboard.GridLayoutSpec, layoutIndex int) error {
+	if len(spec.Items) > maxItemsPerGridLayout {
+		return errors.NewInvalidInputf(ErrCodeDashboardInvalidInput, "spec.layouts[%d].spec.items: has %d items; maximum is %d", layoutIndex, len(spec.Items), maxItemsPerGridLayout)
+	}
+	for i, item := range spec.Items {
+		// The width/x bounds keep x+width small enough not to overflow.
+		switch {
+		case item.Width < 1:
+			return errors.NewInvalidInputf(ErrCodeDashboardInvalidInput, "spec.layouts[%d].spec.items[%d]: width must be at least 1, got %d", layoutIndex, i, item.Width)
+		case item.Height < 1:
+			return errors.NewInvalidInputf(ErrCodeDashboardInvalidInput, "spec.layouts[%d].spec.items[%d]: height must be at least 1, got %d", layoutIndex, i, item.Height)
+		case item.X < 0:
+			return errors.NewInvalidInputf(ErrCodeDashboardInvalidInput, "spec.layouts[%d].spec.items[%d]: x must not be negative, got %d", layoutIndex, i, item.X)
+		case item.Y < 0:
+			return errors.NewInvalidInputf(ErrCodeDashboardInvalidInput, "spec.layouts[%d].spec.items[%d]: y must not be negative, got %d", layoutIndex, i, item.Y)
+		case item.Width > gridColumnCount:
+			return errors.NewInvalidInputf(ErrCodeDashboardInvalidInput, "spec.layouts[%d].spec.items[%d]: width (%d) exceeds grid width %d", layoutIndex, i, item.Width, gridColumnCount)
+		case item.X >= gridColumnCount:
+			return errors.NewInvalidInputf(ErrCodeDashboardInvalidInput, "spec.layouts[%d].spec.items[%d]: x (%d) must be less than grid width %d", layoutIndex, i, item.X, gridColumnCount)
+		case item.X+item.Width > gridColumnCount:
+			return errors.NewInvalidInputf(ErrCodeDashboardInvalidInput, "spec.layouts[%d].spec.items[%d]: x (%d) + width (%d) exceeds grid width %d", layoutIndex, i, item.X, item.Width, gridColumnCount)
+		}
+		// Could cap y/height but skipping for now: the grid grows vertically
+		// without limit (frontend autoSize), so "too big" has no natural bound.
+	}
+	// Two items overlap iff their rectangles intersect on both axes.
+	overlap := func(a, b dashboard.GridItem) bool {
+		return a.X < b.X+b.Width && b.X < a.X+a.Width &&
+			a.Y < b.Y+b.Height && b.Y < a.Y+a.Height
+	}
+	for i := 0; i < len(spec.Items); i++ {
+		for j := i + 1; j < len(spec.Items); j++ {
+			if overlap(spec.Items[i], spec.Items[j]) {
+				return errors.NewInvalidInputf(ErrCodeDashboardInvalidInput, "spec.layouts[%d].spec.items[%d] and items[%d] overlap", layoutIndex, i, j)
+			}
+		}
+	}
+	return nil
+}
+
 func (Layout) JSONSchemaOneOf() []any {
 	return []any{
 		LayoutEnvelope[dashboard.GridLayoutSpec]{Kind: string(dashboard.KindGridLayout)},

tests/integration/tests/dashboard/03_v2_dashboard.py

@@ -173,6 +173,125 @@ def test_create_rejects_too_many_tags(
     assert response.json()["error"]["code"] == "dashboard_invalid_input"
 
 
+def test_create_rejects_invalid_grid_layout(
+    signoz: SigNoz,
+    create_user_admin: Operation,  # pylint: disable=unused-argument
+    get_token: Callable[[str, str], str],
+):
+    token = get_token(USER_ADMIN_EMAIL, USER_ADMIN_PASSWORD)
+
+    def panel(name: str) -> dict:
+        return {
+            "kind": "Panel",
+            "spec": {
+                "display": {"name": name},
+                "plugin": {"kind": "signoz/TablePanel", "spec": {}},
+                "queries": [
+                    {
+                        "kind": "time_series",
+                        "spec": {
+                            "plugin": {
+                                "kind": "signoz/BuilderQuery",
+                                "spec": {
+                                    "name": "A",
+                                    "signal": "logs",
+                                    "aggregations": [{"expression": "count()"}],
+                                },
+                            }
+                        },
+                    }
+                ],
+            },
+        }
+
+    # Two grid items reference valid, distinct panels but share cells, so the
+    # overlap is the only violation.
+    response = requests.post(
+        signoz.self.host_configs["8080"].get(BASE_URL),
+        json={
+            "schemaVersion": "v6",
+            "name": "rejects-overlap",
+            "spec": {
+                "display": {"name": "Rejects Overlap"},
+                "panels": {"p1": panel("P1"), "p2": panel("P2")},
+                "layouts": [
+                    {
+                        "kind": "Grid",
+                        "spec": {
+                            "items": [
+                                {"x": 0, "y": 0, "width": 6, "height": 6, "content": {"$ref": "#/spec/panels/p1"}},
+                                {"x": 3, "y": 3, "width": 6, "height": 6, "content": {"$ref": "#/spec/panels/p2"}},
+                            ]
+                        },
+                    }
+                ],
+            },
+            "tags": [],
+        },
+        headers={"Authorization": f"Bearer {token}"},
+        timeout=5,
+    )
+    assert response.status_code == HTTPStatus.BAD_REQUEST
+    assert response.json()["error"]["code"] == "dashboard_invalid_input"
+    assert "overlap" in response.json()["error"]["message"]
+
+    # One panel placed by two grid items (side by side, so they clear the overlap
+    # check first). The frontend keys grid items by panel id, so this is rejected.
+    response = requests.post(
+        signoz.self.host_configs["8080"].get(BASE_URL),
+        json={
+            "schemaVersion": "v6",
+            "name": "rejects-multiref",
+            "spec": {
+                "display": {"name": "Rejects Multiref"},
+                "panels": {"p1": panel("P1")},
+                "layouts": [
+                    {
+                        "kind": "Grid",
+                        "spec": {
+                            "items": [
+                                {"x": 0, "y": 0, "width": 6, "height": 6, "content": {"$ref": "#/spec/panels/p1"}},
+                                {"x": 6, "y": 0, "width": 6, "height": 6, "content": {"$ref": "#/spec/panels/p1"}},
+                            ]
+                        },
+                    }
+                ],
+            },
+            "tags": [],
+        },
+        headers={"Authorization": f"Bearer {token}"},
+        timeout=5,
+    )
+    assert response.status_code == HTTPStatus.BAD_REQUEST
+    assert response.json()["error"]["code"] == "dashboard_invalid_input"
+    assert "already placed" in response.json()["error"]["message"]
+
+    # More grid items than allowed. The item-count check runs before the
+    # panel-ref check, so content-less items suffice here.
+    response = requests.post(
+        signoz.self.host_configs["8080"].get(BASE_URL),
+        json={
+            "schemaVersion": "v6",
+            "name": "rejects-too-many-items",
+            "spec": {
+                "display": {"name": "Rejects Too Many"},
+                "layouts": [
+                    {
+                        "kind": "Grid",
+                        "spec": {"items": [{"x": 0, "y": 0, "width": 1, "height": 1} for _ in range(101)]},
+                    }
+                ],
+            },
+            "tags": [],
+        },
+        headers={"Authorization": f"Bearer {token}"},
+        timeout=5,
+    )
+    assert response.status_code == HTTPStatus.BAD_REQUEST
+    assert response.json()["error"]["code"] == "dashboard_invalid_input"
+    assert "maximum" in response.json()["error"]["message"]
+
+
 @pytest.mark.parametrize(
     "params",
     [