From 3eb7d53fe8466241acbbc7532f89071d1d3867ec Mon Sep 17 00:00:00 2001
From: unkin-agent <unkin-agent@unkin.net>
Date: Mon, 7 Sep 2026 00:24:57 +1000
Subject: [PATCH] Refuse a group_by naming an aggregate column

A group_by key that repeats a folded column made the column a grouping
key and an aggregate at once, and for avg it left the upstream query
grouping on a column the rewrite had removed, so the request failed as
an opaque 502.

- refuse a group_by field that names a folded aggregate or the avg column
- cover the function-then-field ordering of the existing clash check
---
 aggregate.go      | 10 ++++++++++
 aggregate_test.go | 43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/aggregate.go b/aggregate.go
index ec93b38..41da8b2 100644
--- a/aggregate.go
+++ b/aggregate.go
@@ -138,6 +138,9 @@ func parseAggregate(query string) (*aggregateSpec, error) {
 	}
 	for _, node := range ast[2:] {
 		for _, f := range groupByFields(node) {
+			if hasAgg(spec.aggs, f) || (spec.avg && f == avgColumn) {
+				return nil, groupByClash(f)
+			}
 			spec.keys = appendUnique(spec.keys, f)
 		}
 	}
@@ -157,6 +160,13 @@ func columnClash(name string) error {
 	return fmt.Errorf("extract projects the column %q more than once: openvoxdb returns the repeat as %q, which pdbmux can neither key on nor fold", name, name+"_2")
 }
 
+// groupByClash refuses a group_by naming an aggregate column: the same key
+// cannot both identify a row and be folded across backends. openvoxdb rejects
+// the shape too, so refusing here is a 400 instead of a failed upstream query.
+func groupByClash(name string) error {
+	return fmt.Errorf("group_by names the aggregate column %q: pdbmux folds that column across backends, so it cannot also be a grouping key", name)
+}
+
 // rewriteAvg replaces the client's avg column with the sum and count of the same
 // expression, so the true weighted average can be computed from the shards:
 // Postgres avg(x) is sum(x)/count(x), and both of those do combine.
diff --git a/aggregate_test.go b/aggregate_test.go
index 1e05552..3cbe8f4 100644
--- a/aggregate_test.go
+++ b/aggregate_test.go
@@ -400,6 +400,49 @@ func TestParseAggregate_FieldClashingWithAFunctionIsRefused(t *testing.T) {
 	}
 }
 
+// The ordering the sibling case does not cover: the function column comes first
+// and a later plain field takes the response key it already named.
+func TestParseAggregate_FunctionClashingWithALaterFieldIsRefused(t *testing.T) {
+	const q = `["extract",[["function","count","certname"],"count"],["=","certname","h1"]]`
+	spec, err := parseAggregate(q)
+	if err == nil {
+		t.Fatalf("parseAggregate(%s) = %+v, want a refusal", q, spec)
+	}
+	if !strings.Contains(err.Error(), "count") {
+		t.Errorf("error %q does not name the clashing column", err)
+	}
+}
+
+// A group_by naming a folded column would make it a grouping key and an
+// aggregate at once, so it is refused rather than sent upstream to fail.
+func TestParseAggregate_GroupByOnAnAggregateColumnIsRefused(t *testing.T) {
+	for _, fn := range []string{"count", "sum", "min", "max", "avg"} {
+		q := `["extract",[["function","` + fn + `","line"]],["group_by","` + fn + `"]]`
+		spec, err := parseAggregate(q)
+		if err == nil {
+			t.Errorf("parseAggregate(%s) = %+v, want a refusal", q, spec)
+			continue
+		}
+		if !strings.Contains(err.Error(), fn) {
+			t.Errorf("error %q does not name the clashing column %q", err, fn)
+		}
+	}
+}
+
+// The refusal is limited to the folded columns: grouping on a plain field or on
+// a row function's own column stays legitimate.
+func TestParseAggregate_GroupByOnANonAggregateColumnIsKept(t *testing.T) {
+	for _, q := range []string{
+		`["extract",[["function","count","certname"],"status"],["group_by","status"]]`,
+		`["extract",[["function","to_string","producer_timestamp","FMDD"],["function","count"]],["group_by",["function","to_string","producer_timestamp","FMDD"]]]`,
+		`["extract",[["function","avg","line"],"type"],["group_by","type"]]`,
+	} {
+		if spec, err := parseAggregate(q); err != nil || spec == nil {
+			t.Errorf("parseAggregate(%s) = %+v, %v, want an accepted spec", q, spec, err)
+		}
+	}
+}
+
 // A repeated plain field projects the same value twice, so it is no clash: the
 // duplicate carries nothing the grouping key has not already got.
 func TestParseAggregate_RepeatedPlainFieldIsKept(t *testing.T) {