fix: added validations for having expression

pkg/types/querybuildertypes/querybuildertypesv5/validation.go

@@ -209,6 +209,12 @@ func (q *QueryBuilderQuery[T]) validateAggregations() error {
 				}
 				aliases[v.Alias] = true
 			}
+			if strings.Contains(strings.ToLower(v.Expression), " as ") {
+				return errors.NewInvalidInputf(
+					errors.CodeInvalidInput,
+					"aliasing is not allowed in expression. Use `alias` field instead",
+				)
+			}
 		case LogAggregation:
 			if v.Expression == "" {
 				aggId := fmt.Sprintf("aggregation #%d", i+1)
@@ -231,6 +237,12 @@ func (q *QueryBuilderQuery[T]) validateAggregations() error {
 				}
 				aliases[v.Alias] = true
 			}
+			if strings.Contains(strings.ToLower(v.Expression), " as ") {
+				return errors.NewInvalidInputf(
+					errors.CodeInvalidInput,
+					"aliasing is not allowed in expression. Use `alias` field instead",
+				)
+			}
 		}
 	}
 
@@ -414,7 +426,48 @@ func (q *QueryBuilderQuery[T]) validateHaving() error {
 		)
 	}
 
-	return nil
+	validExpressions := []string{}
+	// ensure that having expression uses aggregation expressions or alias
+	for _, agg := range q.Aggregations {
+		switch v := any(agg).(type) {
+		case TraceAggregation:
+			if strings.Contains(q.Having.Expression, v.Expression) {
+				return nil
+			} else {
+				validExpressions = append(validExpressions, v.Expression)
+			}
+			if v.Alias != "" {
+				if strings.Contains(q.Having.Expression, v.Alias) {
+					return nil
+				} else {
+					validExpressions = append(validExpressions, v.Alias)
+				}
+			}
+		case LogAggregation:
+			if strings.Contains(q.Having.Expression, v.Expression) {
+				return nil
+			} else {
+				validExpressions = append(validExpressions, v.Expression)
+			}
+			if v.Alias != "" {
+				if strings.Contains(q.Having.Expression, v.Alias) {
+					return nil
+				} else {
+					validExpressions = append(validExpressions, v.Alias)
+				}
+			}
+		case MetricAggregation:
+			// TODO: (@srikanthccv) validate having expression for metric aggregations
+			return nil
+		}
+	}
+
+	return errors.NewInvalidInputf(
+		errors.CodeInvalidInput,
+		"Having expression is not using valid identifiers",
+	).WithAdditional(
+		fmt.Sprintf("Having expression can only reference aggregation aliases/expressions. Valid keys are: %s", strings.Join(validExpressions, ", ")),
+	)
 }
 
 // ValidateQueryRangeRequest validates the entire query range request

pkg/types/querybuildertypes/querybuildertypesv5/validation_test.go

@@ -4,6 +4,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/SigNoz/signoz/pkg/types/metrictypes"
 	"github.com/SigNoz/signoz/pkg/types/telemetrytypes"
 )
 
@@ -512,4 +513,388 @@ func TestQueryRangeRequest_ValidateOrderByForAggregation(t *testing.T) {
 			}
 		})
 	}
-}
+}
+
+func TestQueryRangeRequest_ValidateHaving(t *testing.T) {
+	tests := []struct {
+		name    string
+		query   any // either QueryBuilderQuery[TraceAggregation] or QueryBuilderQuery[LogAggregation]
+		wantErr bool
+		errMsg  string
+	}{
+		// nil having
+		{
+			name: "trace nil having should pass",
+			query: QueryBuilderQuery[TraceAggregation]{
+				Name:   "A",
+				Signal: telemetrytypes.SignalTraces,
+				Aggregations: []TraceAggregation{
+					{Expression: "count()"},
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "log nil having should pass",
+			query: QueryBuilderQuery[LogAggregation]{
+				Name:   "A",
+				Signal: telemetrytypes.SignalLogs,
+				Aggregations: []LogAggregation{
+					{Expression: "count()"},
+				},
+			},
+			wantErr: false,
+		},
+		// empty having expression
+		{
+			name: "trace empty having expression should pass",
+			query: QueryBuilderQuery[TraceAggregation]{
+				Name:   "A",
+				Signal: telemetrytypes.SignalTraces,
+				Aggregations: []TraceAggregation{
+					{Expression: "count()"},
+				},
+				Having: &Having{Expression: ""},
+			},
+			wantErr: false,
+		},
+		{
+			name: "log empty having expression should pass",
+			query: QueryBuilderQuery[LogAggregation]{
+				Name:   "A",
+				Signal: telemetrytypes.SignalLogs,
+				Aggregations: []LogAggregation{
+					{Expression: "count()"},
+				},
+				Having: &Having{Expression: ""},
+			},
+			wantErr: false,
+		},
+		// having without aggregations
+		{
+			name: "trace having without aggregations should fail",
+			query: QueryBuilderQuery[TraceAggregation]{
+				Name:   "A",
+				Signal: telemetrytypes.SignalTraces,
+				Having: &Having{Expression: "count() > 10"},
+			},
+			wantErr: true,
+			errMsg:  "at least one aggregation is required",
+		},
+		{
+			name: "log having without aggregations should fail",
+			query: QueryBuilderQuery[LogAggregation]{
+				Name:   "A",
+				Signal: telemetrytypes.SignalLogs,
+				Having: &Having{Expression: "count() > 10"},
+			},
+			wantErr: true,
+			errMsg:  "at least one aggregation is required",
+		},
+		// having with matching expression
+		{
+			name: "trace having with matching expression should pass",
+			query: QueryBuilderQuery[TraceAggregation]{
+				Name:   "A",
+				Signal: telemetrytypes.SignalTraces,
+				Aggregations: []TraceAggregation{
+					{Expression: "count()"},
+				},
+				Having: &Having{Expression: "count() > 100"},
+			},
+			wantErr: false,
+		},
+		{
+			name: "log having with matching expression should pass",
+			query: QueryBuilderQuery[LogAggregation]{
+				Name:   "A",
+				Signal: telemetrytypes.SignalLogs,
+				Aggregations: []LogAggregation{
+					{Expression: "count()"},
+				},
+				Having: &Having{Expression: "count() > 50"},
+			},
+			wantErr: false,
+		},
+		// having with matching alias
+		{
+			name: "trace having with matching alias should pass",
+			query: QueryBuilderQuery[TraceAggregation]{
+				Name:   "A",
+				Signal: telemetrytypes.SignalTraces,
+				Aggregations: []TraceAggregation{
+					{Expression: "count()", Alias: "total"},
+				},
+				Having: &Having{Expression: "total > 100"},
+			},
+			wantErr: false,
+		},
+		{
+			name: "log having with matching alias should pass",
+			query: QueryBuilderQuery[LogAggregation]{
+				Name:   "A",
+				Signal: telemetrytypes.SignalLogs,
+				Aggregations: []LogAggregation{
+					{Expression: "count()", Alias: "log_count"},
+				},
+				Having: &Having{Expression: "log_count > 50"},
+			},
+			wantErr: false,
+		},
+		// having with invalid identifier
+		{
+			name: "trace having with invalid identifier should fail",
+			query: QueryBuilderQuery[TraceAggregation]{
+				Name:   "A",
+				Signal: telemetrytypes.SignalTraces,
+				Aggregations: []TraceAggregation{
+					{Expression: "count()", Alias: "total"},
+				},
+				Having: &Having{Expression: "unknown_col > 100"},
+			},
+			wantErr: true,
+			errMsg:  "Having expression is not using valid identifiers",
+		},
+		{
+			name: "log having with invalid identifier should fail",
+			query: QueryBuilderQuery[LogAggregation]{
+				Name:   "A",
+				Signal: telemetrytypes.SignalLogs,
+				Aggregations: []LogAggregation{
+					{Expression: "count()", Alias: "log_count"},
+				},
+				Having: &Having{Expression: "bad_identifier > 50"},
+			},
+			wantErr: true,
+			errMsg:  "Having expression is not using valid identifiers",
+		},
+		// having matching second aggregation expression
+		{
+			name: "trace having matching second aggregation should pass",
+			query: QueryBuilderQuery[TraceAggregation]{
+				Name:   "A",
+				Signal: telemetrytypes.SignalTraces,
+				Aggregations: []TraceAggregation{
+					{Expression: "count()", Alias: "total"},
+					{Expression: "sum(duration)", Alias: "total_duration"},
+				},
+				Having: &Having{Expression: "sum(duration) > 1000"},
+			},
+			wantErr: false,
+		},
+		// having matching second aggregation alias
+		{
+			name: "trace having matching second aggregation alias should pass",
+			query: QueryBuilderQuery[TraceAggregation]{
+				Name:   "A",
+				Signal: telemetrytypes.SignalTraces,
+				Aggregations: []TraceAggregation{
+					{Expression: "count()", Alias: "total"},
+					{Expression: "sum(duration)", Alias: "total_duration"},
+				},
+				Having: &Having{Expression: "total_duration > 1000"},
+			},
+			wantErr: false,
+		},
+		// metric having (validation not yet implemented)
+		{
+			name: "metric having should pass (validation not yet implemented)",
+			query: QueryBuilderQuery[MetricAggregation]{
+				Name:   "A",
+				Signal: telemetrytypes.SignalMetrics,
+				Aggregations: []MetricAggregation{
+					{
+						MetricName:       "http_requests_total",
+						TimeAggregation:  metrictypes.TimeAggregationRate,
+						SpaceAggregation: metrictypes.SpaceAggregationSum,
+					},
+				},
+				Having: &Having{Expression: "anything_goes > 100"},
+			},
+			wantErr: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var err error
+			switch q := tt.query.(type) {
+			case QueryBuilderQuery[TraceAggregation]:
+				err = q.Validate(RequestTypeTimeSeries)
+			case QueryBuilderQuery[LogAggregation]:
+				err = q.Validate(RequestTypeTimeSeries)
+			case QueryBuilderQuery[MetricAggregation]:
+				err = q.Validate(RequestTypeTimeSeries)
+			}
+			if tt.wantErr {
+				if err == nil {
+					t.Errorf("validateHaving() expected error but got none")
+					return
+				}
+				if tt.errMsg != "" && !contains(err.Error(), tt.errMsg) {
+					t.Errorf("validateHaving() error = %v, want to contain %v", err.Error(), tt.errMsg)
+				}
+			} else {
+				if err != nil {
+					t.Errorf("validateHaving() unexpected error = %v", err)
+				}
+			}
+		})
+	}
+}
+
+func TestQueryRangeRequest_ValidateAggregations(t *testing.T) {
+	tests := []struct {
+		name    string
+		wantErr bool
+		errMsg  string
+		query   any // either QueryBuilderQuery[TraceAggregation] or QueryBuilderQuery[LogAggregation]
+	}{
+		// AS in expression (lowercase)
+		{
+			name: "trace aggregation with AS in expression should fail",
+			query: QueryBuilderQuery[TraceAggregation]{
+				Name:   "A",
+				Signal: telemetrytypes.SignalTraces,
+				Aggregations: []TraceAggregation{
+					{Expression: "count() as total"},
+				},
+			},
+			wantErr: true,
+			errMsg:  "aliasing is not allowed in expression",
+		},
+		{
+			name: "log aggregation with AS in expression should fail",
+			query: QueryBuilderQuery[LogAggregation]{
+				Name:   "A",
+				Signal: telemetrytypes.SignalLogs,
+				Aggregations: []LogAggregation{
+					{Expression: "sum(bytes) as total_bytes"},
+				},
+			},
+			wantErr: true,
+			errMsg:  "aliasing is not allowed in expression",
+		},
+		// AS in expression (uppercase)
+		{
+			name: "trace aggregation with AS (uppercase) in expression should fail",
+			query: QueryBuilderQuery[TraceAggregation]{
+				Name:   "A",
+				Signal: telemetrytypes.SignalTraces,
+				Aggregations: []TraceAggregation{
+					{Expression: "count() AS total"},
+				},
+			},
+			wantErr: true,
+			errMsg:  "aliasing is not allowed in expression",
+		},
+		{
+			name: "log aggregation with AS (uppercase) in expression should fail",
+			query: QueryBuilderQuery[LogAggregation]{
+				Name:   "A",
+				Signal: telemetrytypes.SignalLogs,
+				Aggregations: []LogAggregation{
+					{Expression: "sum(bytes) AS total_bytes"},
+				},
+			},
+			wantErr: true,
+			errMsg:  "aliasing is not allowed in expression",
+		},
+		// AS in expression (mixed case)
+		{
+			name: "trace aggregation with AS (mixed case) in expression should fail",
+			query: QueryBuilderQuery[TraceAggregation]{
+				Name:   "A",
+				Signal: telemetrytypes.SignalTraces,
+				Aggregations: []TraceAggregation{
+					{Expression: "count() As total"},
+				},
+			},
+			wantErr: true,
+			errMsg:  "aliasing is not allowed in expression",
+		},
+		{
+			name: "log aggregation with AS (mixed case) in expression should fail",
+			query: QueryBuilderQuery[LogAggregation]{
+				Name:   "A",
+				Signal: telemetrytypes.SignalLogs,
+				Aggregations: []LogAggregation{
+					{Expression: "sum(bytes) As total_bytes"},
+				},
+			},
+			wantErr: true,
+			errMsg:  "aliasing is not allowed in expression",
+		},
+		// without AS
+		{
+			name: "trace aggregation without AS should pass",
+			query: QueryBuilderQuery[TraceAggregation]{
+				Name:   "A",
+				Signal: telemetrytypes.SignalTraces,
+				Aggregations: []TraceAggregation{
+					{Expression: "count()"},
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "log aggregation without AS should pass",
+			query: QueryBuilderQuery[LogAggregation]{
+				Name:   "A",
+				Signal: telemetrytypes.SignalLogs,
+				Aggregations: []LogAggregation{
+					{Expression: "sum(bytes)"},
+				},
+			},
+			wantErr: false,
+		},
+		// alias in alias field
+		{
+			name: "trace aggregation with alias in alias field should pass",
+			query: QueryBuilderQuery[TraceAggregation]{
+				Name:   "A",
+				Signal: telemetrytypes.SignalTraces,
+				Aggregations: []TraceAggregation{
+					{Expression: "count()", Alias: "total"},
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "log aggregation with alias in alias field should pass",
+			query: QueryBuilderQuery[LogAggregation]{
+				Name:   "A",
+				Signal: telemetrytypes.SignalLogs,
+				Aggregations: []LogAggregation{
+					{Expression: "count()", Alias: "total"},
+				},
+			},
+			wantErr: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var err error
+			switch q := tt.query.(type) {
+			case QueryBuilderQuery[TraceAggregation]:
+				err = q.Validate(RequestTypeTimeSeries)
+			case QueryBuilderQuery[LogAggregation]:
+				err = q.Validate(RequestTypeTimeSeries)
+			}
+			if tt.wantErr {
+				if err == nil {
+					t.Errorf("validateAggregations() expected error but got none")
+					return
+				}
+				if tt.errMsg != "" && !contains(err.Error(), tt.errMsg) {
+					t.Errorf("validateAggregations() error = %v, want to contain %v", err.Error(), tt.errMsg)
+				}
+			} else {
+				if err != nil {
+					t.Errorf("validateAggregations() unexpected error = %v", err)
+				}
+			}
+		})
+	}
+}