bugfix: query equivalence for time_range, filter ops, and calculations (

#282)

## Which problem is this PR solving?

- Closes #275

## Short description of the changes

The QuerySpec 'equivalence' functionality introduced in #236 to suppress
diffs failed to detect changes in `time_range`, `filter_combination`,
and `calculation`.

client/board_test.go

@@ -82,7 +82,7 @@ func TestBoards(t *testing.T) {
 					Op: CalculationOpCount,
 				},
 			},
-			TimeRange: ToPtr(7200), // 2 hours
+			TimeRange: ToPtr(DefaultQueryTimeRange),
 		})
 		assert.NoError(t, err)
 		b.ColumnLayout = BoardColumnStyleMulti

client/query_spec.go

@@ -50,6 +50,8 @@ type QuerySpec struct {
 	Granularity *int `json:"granularity,omitempty"`
 }
 
+const DefaultQueryTimeRange = 2 * 60 * 60
+
 // Determines if two QuerySpecs are equivalent
 func (qs *QuerySpec) EquivalentTo(other QuerySpec) bool {
 	// The order of Calculations is important for visualization rendering, so we're looking for equality
@@ -67,9 +69,14 @@ func (qs *QuerySpec) EquivalentTo(other QuerySpec) bool {
 	if !calcMatch {
 		// 'COUNT' is the default Calculation and equivalent to an empty Calculations -- check that before we give up
 		defaultCalc := []CalculationSpec{{Op: CalculationOpCount}}
-		if (other.Calculations != nil && reflect.DeepEqual(qs.Calculations, defaultCalc)) ||
-			(qs.Calculations != nil && reflect.DeepEqual(other.Calculations, defaultCalc)) ||
-			(len(qs.Calculations) > 1 || len(other.Calculations) > 1) {
+		qsC, oC := defaultCalc, defaultCalc
+		if len(qs.Calculations) != 0 {
+			qsC = qs.Calculations
+		}
+		if len(other.Calculations) != 0 {
+			oC = other.Calculations
+		}
+		if !reflect.DeepEqual(qsC, oC) {
 			return false
 		}
 	}
@@ -78,12 +85,8 @@ func (qs *QuerySpec) EquivalentTo(other QuerySpec) bool {
 	if !Equivalent(qs.Filters, other.Filters) {
 		return false
 	}
-	if qs.FilterCombination != other.FilterCombination {
-		// 'AND' is the default and equivalent to an empty Filter Combination
-		if (qs.FilterCombination == FilterCombinationAnd && other.FilterCombination != "") ||
-			(other.FilterCombination == FilterCombinationAnd && qs.FilterCombination != "") {
-			return false
-		}
+	if ValueOrDefault(qs.FilterCombination, DefaultFilterCombination) != ValueOrDefault(other.FilterCombination, DefaultFilterCombination) {
+		return false
 	}
 	if !reflect.DeepEqual(qs.Breakdowns, other.Breakdowns) {
 		return false
@@ -98,12 +101,8 @@ func (qs *QuerySpec) EquivalentTo(other QuerySpec) bool {
 	if !reflect.DeepEqual(qs.Limit, other.Limit) {
 		return false
 	}
-	if !reflect.DeepEqual(qs.TimeRange, other.TimeRange) {
-		// 7200 is the default, so an unset value is equivalent to 7200
-		if (reflect.DeepEqual(qs.TimeRange, 7200) && other.TimeRange != nil) ||
-			(reflect.DeepEqual(other.TimeRange, 7200) && qs.TimeRange != nil) {
-			return false
-		}
+	if PtrValueOrDefault(qs.TimeRange, DefaultQueryTimeRange) != PtrValueOrDefault(other.TimeRange, DefaultQueryTimeRange) {
+		return false
 	}
 	if !reflect.DeepEqual(qs.StartTime, other.StartTime) || !reflect.DeepEqual(qs.EndTime, other.EndTime) {
 		return false
@@ -241,8 +240,9 @@ type FilterCombination string
 
 // Declaration of filter combinations.
 const (
-	FilterCombinationOr  FilterCombination = "OR"
-	FilterCombinationAnd FilterCombination = "AND"
+	FilterCombinationOr      FilterCombination = "OR"
+	FilterCombinationAnd     FilterCombination = "AND"
+	DefaultFilterCombination                   = FilterCombinationAnd
 )
 
 // FilterCombinations returns an exhaustive list of filter combinations.

client/query_spec_test.go

@@ -88,7 +88,7 @@ func TestQuerySpec_EquivalentTo(t *testing.T) {
 					},
 				},
 				FilterCombination: "AND",
-				TimeRange:         ToPtr(7200),
+				TimeRange:         ToPtr(DefaultQueryTimeRange),
 			},
 			QuerySpec{},
 			true,
@@ -123,7 +123,7 @@ func TestQuerySpec_EquivalentTo(t *testing.T) {
 					},
 				},
 				Breakdowns: []string{"colB", "colA"},
-				TimeRange:  ToPtr(7200),
+				TimeRange:  ToPtr(DefaultQueryTimeRange),
 			},
 			QuerySpec{
 				Calculations: []CalculationSpec{
@@ -183,12 +183,93 @@ func TestQuerySpec_EquivalentTo(t *testing.T) {
 			},
 			false,
 		},
+		{
+			"Different time ranges",
+			QuerySpec{
+				Calculations: []CalculationSpec{
+					{
+						Op: "COUNT",
+					},
+				},
+				TimeRange: ToPtr(1800),
+			},
+			QuerySpec{
+				Calculations: []CalculationSpec{
+					{
+						Op: "COUNT",
+					},
+				},
+			},
+			false,
+		},
+		{
+			"Different FilterCombinations",
+			QuerySpec{
+				FilterCombination: "OR",
+			},
+			QuerySpec{},
+			false,
+		},
+		{
+			"Calculation different from DefaultCalc",
+			QuerySpec{
+				Calculations: []CalculationSpec{
+					{
+						Op:     "MIN",
+						Column: ToPtr("metrics.cpu.utilization"),
+					},
+				},
+			},
+			QuerySpec{},
+			false,
+		},
+		{
+			"Different Calculations",
+			QuerySpec{
+				Calculations: []CalculationSpec{
+					{
+						Op:     "MIN",
+						Column: ToPtr("metrics.cpu.utilization"),
+					},
+				},
+			},
+			QuerySpec{
+				Calculations: []CalculationSpec{
+					{
+						Op:     "MAX",
+						Column: ToPtr("metrics.cpu.utilization"),
+					},
+				},
+			},
+			false,
+		},
+		{
+			"Different Number of Calculations",
+			QuerySpec{
+				Calculations: []CalculationSpec{
+					{
+						Op:     "MIN",
+						Column: ToPtr("metrics.cpu.utilization"),
+					},
+				},
+			},
+			QuerySpec{
+				Calculations: []CalculationSpec{
+					{
+						Op: "COUNT_DISTINCT",
+					},
+					{
+						Op:     "MAX",
+						Column: ToPtr("metrics.cpu.utilization"),
+					},
+				},
+			},
+			false,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if got := tt.a.EquivalentTo(tt.b); got != tt.want {
-				t.Errorf("QuerySpec.EquivalentTo() = %v, want %v", got, tt.want)
-			}
+			assert.Equal(t, tt.want, tt.a.EquivalentTo(tt.b))
 		})
 	}
 }

client/type_helpers.go

@@ -46,3 +46,21 @@ func Equivalent[T any](a, b []T) bool {
 
 	return true
 }
+
+func IsZero[T comparable](v T) bool {
+	return v == *new(T)
+}
+
+func PtrValueOrDefault[T any](v *T, d T) T {
+	if v != nil {
+		return *v
+	}
+	return d
+}
+
+func ValueOrDefault[T comparable](v, d T) T {
+	if !IsZero(v) {
+		return v
+	}
+	return d
+}

honeycombio/data_source_query_specification.go

@@ -113,8 +113,8 @@ func dataSourceHoneycombioQuerySpec() *schema.Resource {
 			"filter_combination": {
 				Type:         schema.TypeString,
 				Optional:     true,
-				Default:      "AND",
-				ValidateFunc: validation.StringInSlice([]string{"AND", "OR"}, false),
+				Default:      honeycombio.DefaultFilterCombination,
+				ValidateFunc: validation.StringInSlice([]string{string(honeycombio.FilterCombinationAnd), string(honeycombio.FilterCombinationOr)}, false),
 			},
 			"breakdowns": {
 				Type:     schema.TypeList,
@@ -158,7 +158,7 @@ func dataSourceHoneycombioQuerySpec() *schema.Resource {
 				// Terraform isn't able to set the computed value causing a
 				// constant diff. By using a default value instead, we don't
 				// need the feedback from the API.
-				Default: 7200,
+				Default: honeycombio.DefaultQueryTimeRange,
 			},
 			"start_time": {
 				Type:     schema.TypeInt,
@@ -213,10 +213,10 @@ func dataSourceHoneycombioQuerySpecRead(ctx context.Context, d *schema.ResourceD
 		}
 	}
 
-	// The API doesn't return filter_combination if it is 'AND' (the default)
+	// The API doesn't return filter_combination if it's the default
 	var filterCombination honeycombio.FilterCombination
 	filterString := d.Get("filter_combination").(string)
-	if filterString != "" && filterString != "AND" {
+	if filterString != "" && filterString != string(honeycombio.DefaultFilterCombination) {
 		// doing it this way to support possible different filter types in future
 		// and having one less place to update them
 		filterCombination = honeycombio.FilterCombination(filterString)

honeycombio/resource_query_test.go

@@ -94,7 +94,7 @@ func testAccCheckQueryExists(t *testing.T, dataset string, name string, duration
 					Value:  float64(duration),
 				},
 			},
-			TimeRange: honeycombio.ToPtr(7200),
+			TimeRange: honeycombio.ToPtr(honeycombio.DefaultQueryTimeRange),
 		}
 
 		ok = assert.Equal(t, expectedQuery, createdQuery)