Repository: honeycombio/terraform-provider-honeycombio (language: python)

Issue:
Bug: Non string values (float, integer, etc.) in filters are not valid
We are seeing query filters fail to validate if we need to specify anything other than a string as a comparison value: float, int, etc.

**Versions**
Latest version of the provider.

**Steps to reproduce**
Use something like the following in the query definition:
```
filter {
   column 		= honeycombio_derived_column.<derived-column-name>.alias
   op     			= ">"
   value_float  	= 0.0
}
```

**Additional context**
None required.
Add filter value properties with explicit types.
As described in https://github.com/kvrhdn/terraform-provider-honeycombio/issues/27, the Honeycomb API currently fails to parse queries if a filter is sent with the wrong type in the JSON payload.

This PR adds new `value_*` options to the filter schema for each type in the Honeycomb API.
These properties are mutually exclusive with themselves and the original `value` field.


Feel free to either comment or edit @kvrhdn.
If you'd prefer to go with one of the other options enumerated in #27 that's fine too, just putting an option out there.

Fixes https://github.com/kvrhdn/terraform-provider-honeycombio/issues/27 


Discussion / hints:
I just reproduced this, and confirmed this is an issue with Terraform Plugin SDK based providers and "zero values" of underlying datatypes: https://discuss.hashicorp.com/t/explicit-zero-values-in-provider-schemas/18744

Looking into fixes now
Introduced by https://github.com/honeycombio/terraform-provider-honeycombio/issues/27
Hi @fitzoh, I think this is ready to merge (and release). Do you want to give the last commits I added a final review before merging?
>Hi @fitzoh, I think this is ready to merge (and release). Do you want to give the last commits I added a final review before merging?

Added a couple explicit warnings around `value` breaking w/ non-string columns, I'll leave merging those at your discretion.

Other than that I'd say :shipit: 

Also +1 for the list changes
Thanks again @cah-andrew-fitzgerald. I'll release this with v0.0.9 🚀 
And thank you for the cleanup/docs @kvrhdn 

Fix patch:
diff --git a/client/query_spec.go b/client/query_spec.go
index 3a8da796..c86e0e68 100644
--- a/client/query_spec.go
+++ b/client/query_spec.go
@@ -118,7 +118,7 @@ type FilterSpec struct {
 	// depends on the operator:
 	//  - 'exists' and 'does-not-exist': value should be nil
 	//  - 'in' and 'not-in': value should be a []string
-	//  - all other ops: value should be a string
+	//  - all other ops: value could be a string, int, bool or float
 	Value interface{} `json:"value,omitempty"`
 }
 
diff --git a/docs/data-sources/query_specification.md b/docs/data-sources/query_specification.md
index 7c18e42b..4c3758d8 100644
--- a/docs/data-sources/query_specification.md
+++ b/docs/data-sources/query_specification.md
@@ -65,11 +65,11 @@ Each query configuration may have zero or more `filter` blocks, which each accep
 
 * `column` - (Required) The column to apply the filter to.
 * `op` - (Required) The operator to apply, see the supported list of filter operators at [Filter Operators](https://docs.honeycomb.io/api/query-specification/#filter-operators). Not all operators require a value.
-* `value_string` - (Optional) The value used for the filter when the column is a string. Mutually exclusive with `value` and the other `value_*` options.
-* `value_integer` - (Optional) The value used for the filter when the column is an integer. Mutually exclusive with `value` and the other `value_*` options.
-* `value_float` - (Optional) The value used for the filter when the column is a float. Mutually exclusive with `value` and the other `value_*` options.
-* `value_boolean` - (Optional) The value used for the filter when the column is a boolean. Mutually exclusive with `value` and the other `value_*` options.
-* `value` - (Optional) Deprecated: use the explicitly typed `value_string` instead. This variant will break queries when used with non-string columns. Mutually exclusive with the other `value_*` options.
+* `value` - (Optional) The value used for the filter. Not needed if op is `exists`, `not-exists`, `in` or `not-in`. Mutually exclusive with the other `value_*` options.
+* `value_string` - (Optional) Deprecated: use 'value' instead. The value used for the filter when the column is a string. Mutually exclusive with `value` and the other `value_*` options.
+* `value_integer` - (Optional) Deprecated: use 'value' instead. The value used for the filter when the column is an integer. Mutually exclusive with `value` and the other `value_*` options.
+* `value_float` - (Optional) Deprecated: use 'value' instead. The value used for the filter when the column is a float. Mutually exclusive with `value` and the other `value_*` options.
+* `value_boolean` - (Optional) Deprecated: use 'value' instead. The value used for the filter when the column is a boolean. Mutually exclusive with `value` and the other `value_*` options.
 
 -> **NOTE** The type of the filter value should match with the type of the column. To determine the type of a column visit the dataset settings page, all the columns and their type are listed under _Schema_. This provider will not be able to detect invalid combinations.
 
diff --git a/honeycombio/data_source_query_specification.go b/honeycombio/data_source_query_specification.go
index a23c5151..6264c079 100644
--- a/honeycombio/data_source_query_specification.go
+++ b/honeycombio/data_source_query_specification.go
@@ -52,29 +52,32 @@ func dataSourceHoneycombioQuerySpec() *schema.Resource {
 						},
 						"value": {
 							Type:        schema.TypeString,
-							Description: "Deprecated: use the explicitly typed `value_string` instead. This variant will break queries when used with non-string columns. Mutually exclusive with the other `value_*` options.",
+							Description: "The value used for the filter. Not needed if op is `exists`, `not-exists`, `in` or `not-in`. Mutually exclusive with the other `value_*` options.",
 							Optional:    true,
-							Deprecated:  "Use of attribute `value` is discouraged and will break queries when used with non-string columns. The explicitly typed `value_*` variants are preferred instead.",
 						},
 						"value_string": {
 							Type:        schema.TypeString,
-							Description: "The value used for the filter when the column is a string. Mutually exclusive with `value` and the other `value_*` options",
+							Description: "Deprecated: use 'value' instead. The value used for the filter when the column is a string. Mutually exclusive with `value` and the other `value_*` options",
 							Optional:    true,
+							Deprecated:  "Use of attribute `value_string` is discouraged and will fail to plan if using the empty string. Use of `value` is encouraged.",
 						},
 						"value_integer": {
 							Type:        schema.TypeInt,
-							Description: "The value used for the filter when the column is an integer. Mutually exclusive with `value` and the other `value_*` options",
+							Description: "Deprecated: use 'value' instead. The value used for the filter when the column is an integer. Mutually exclusive with `value` and the other `value_*` options",
 							Optional:    true,
+							Deprecated:  "Use of attribute `value_integer` is discouraged and will fail to plan if using '0'. Use of `value` is encouraged.",
 						},
 						"value_float": {
 							Type:        schema.TypeFloat,
-							Description: "The value used for the filter when the column is a float. Mutually exclusive with `value` and the other `value_*` options",
+							Description: "Deprecated: use 'value' instead. The value used for the filter when the column is a float. Mutually exclusive with `value` and the other `value_*` options",
 							Optional:    true,
+							Deprecated:  "Use of attribute `value_float` is discouraged and will fail to plan if using '0'. Use of `value` is encouraged.",
 						},
 						"value_boolean": {
 							Type:        schema.TypeBool,
-							Description: "The value used for the filter when the column is a boolean. Mutually exclusive with `value` and the other `value_*` options",
+							Description: "Deprecated: use 'value' instead. The value used for the filter when the column is a boolean. Mutually exclusive with `value` and the other `value_*` options",
 							Optional:    true,
+							Deprecated:  "Use of attribute `value_boolean` is discouraged and will fail to plan if using 'false'. Use of `value` is encouraged.",
 						},
 					},
 				},
@@ -333,7 +336,7 @@ func extractFilter(d *schema.ResourceData, index int) (honeycombio.FilterSpec, e
 	valueSet := false
 	v, vOk := d.GetOk(fmt.Sprintf("filter.%d.value", index))
 	if vOk {
-		filter.Value = v
+		filter.Value = coerceValueToType(v.(string))
 		valueSet = true
 	}
 	vs, vsOk := d.GetOk(fmt.Sprintf("filter.%d.value_string", index))
diff --git a/honeycombio/type_helpers.go b/honeycombio/type_helpers.go
index 7ea25ca2..abde6312 100644
--- a/honeycombio/type_helpers.go
+++ b/honeycombio/type_helpers.go
@@ -1,6 +1,10 @@
 package honeycombio
 
-import honeycombio "github.com/honeycombio/terraform-provider-honeycombio/client"
+import (
+	"strconv"
+
+	honeycombio "github.com/honeycombio/terraform-provider-honeycombio/client"
+)
 
 func boardStyleStrings() []string {
 	in := honeycombio.BoardStyles()
@@ -114,3 +118,19 @@ func triggerThresholdOpStrings() []string {
 
 	return out
 }
+
+func coerceValueToType(i string) interface{} {
+	// HCL really has three base types: bool, string, and number
+	// The Plugin SDK allows typing a schema field to Int or Float
+
+	// Plugin SDK assumes 64bit so we'll do the same
+	if v, err := strconv.ParseInt(i, 10, 64); err == nil {
+		return int(v)
+	} else if v, err := strconv.ParseFloat(i, 64); err == nil {
+		return v
+	} else if v, err := strconv.ParseBool(i); err == nil {
+		return v
+	}
+	// fallthrough to string
+	return i
+}


Test patch:
diff --git a/client/query_spec_test.go b/client/query_spec_test.go
index 620ef520..a14a3470 100644
--- a/client/query_spec_test.go
+++ b/client/query_spec_test.go
@@ -40,6 +40,11 @@ func TestQuerySpec(t *testing.T) {
 				Op:     FilterOpSmallerThan,
 				Value:  10000.0,
 			},
+			{
+				Column: "column_1",
+				Op:     FilterOpNotEquals,
+				Value:  "",
+			},
 		},
 		FilterCombination: FilterCombinationOr,
 		Breakdowns:        []string{"column_1", "column_2"},
diff --git a/honeycombio/data_source_query_specification_test.go b/honeycombio/data_source_query_specification_test.go
index 0226863c..c62717a5 100644
--- a/honeycombio/data_source_query_specification_test.go
+++ b/honeycombio/data_source_query_specification_test.go
@@ -38,9 +38,14 @@ data "honeycombio_query_specification" "test" {
         column = "trace.parent_id"
         op     = "does-not-exist"
     }
+    filter {
+        column = "duration_ms"
+        op     = ">"
+        value  = 0
+    }
     filter {
         column        = "duration_ms"
-        op            = ">"
+        op            = "<"
         value_integer = 100
     }
     filter {
@@ -97,6 +102,11 @@ const expectedJSON string = `{
     {
       "column": "duration_ms",
       "op": "\u003e",
+      "value": 0
+    },
+    {
+      "column": "duration_ms",
+      "op": "\u003c",
       "value": 100
     },
     {
@@ -350,3 +360,50 @@ data "honeycombio_query_specification" "test" {
 		},
 	})
 }
+
+func TestAccDataSourceHoneycombioQuery_zerovalue(t *testing.T) {
+	properZeroValueJSON := `{
+  "calculations": [
+    {
+      "op": "COUNT"
+    }
+  ],
+  "filters": [
+    {
+      "column": "duration_ms",
+      "op": "\u003e",
+      "value": 0
+    }
+  ],
+  "time_range": 7200
+}`
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:          testAccPreCheck(t),
+		ProviderFactories: testAccProviderFactories,
+		Steps: []resource.TestStep{
+			{
+				Config: `
+data "honeycombio_query_specification" "test" {
+  calculation {
+    op = "COUNT"
+  }
+
+  filter {
+    column = "duration_ms"
+    op     = ">"
+    value  = 0
+  }
+}
+
+output "query_json" {
+  value = data.honeycombio_query_specification.test.json
+}
+`,
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckOutput("query_json", properZeroValueJSON),
+				),
+			},
+		},
+	})
+}
diff --git a/honeycombio/resource_board_test.go b/honeycombio/resource_board_test.go
index b63585af..a1ee91e9 100644
--- a/honeycombio/resource_board_test.go
+++ b/honeycombio/resource_board_test.go
@@ -47,7 +47,7 @@ data "honeycombio_query_specification" "test" {
   filter {
     column = "duration_ms"
     op     = ">"
-    value  = count.index 
+    value  = count.index
   }
 }
 
@@ -100,6 +100,11 @@ func testAccCheckBoardExists(t *testing.T, name string) resource.TestCheckFunc {
 			return fmt.Errorf("could not find created board: %w", err)
 		}
 
+		for i := range createdBoard.Queries {
+			// we don't track the QuerySpec, just the IDs
+			createdBoard.Queries[i].Query = nil
+		}
+
 		expectedBoard := &honeycombio.Board{
 			ID:          createdBoard.ID,
 			Name:        "Test board from terraform-provider-honeycombio",
@@ -112,22 +117,6 @@ func testAccCheckBoardExists(t *testing.T, name string) resource.TestCheckFunc {
 					Dataset:           testAccDataset(),
 					QueryID:           createdBoard.Queries[0].QueryID,
 					QueryAnnotationID: createdBoard.Queries[0].QueryAnnotationID,
-					Query: &honeycombio.QuerySpec{
-						Calculations: []honeycombio.CalculationSpec{
-							{
-								Op:     honeycombio.CalculationOpAvg,
-								Column: honeycombio.StringPtr("duration_ms"),
-							},
-						},
-						Filters: []honeycombio.FilterSpec{
-							{
-								Column: "duration_ms",
-								Op:     ">",
-								Value:  "0",
-							},
-						},
-						TimeRange: honeycombio.IntPtr(7200),
-					},
 				},
 				{
 					Caption:           "test query 1",
@@ -135,22 +124,6 @@ func testAccCheckBoardExists(t *testing.T, name string) resource.TestCheckFunc {
 					Dataset:           testAccDataset(),
 					QueryID:           createdBoard.Queries[1].QueryID,
 					QueryAnnotationID: createdBoard.Queries[1].QueryAnnotationID,
-					Query: &honeycombio.QuerySpec{
-						Calculations: []honeycombio.CalculationSpec{
-							{
-								Op:     honeycombio.CalculationOpAvg,
-								Column: honeycombio.StringPtr("duration_ms"),
-							},
-						},
-						Filters: []honeycombio.FilterSpec{
-							{
-								Column: "duration_ms",
-								Op:     ">",
-								Value:  "1",
-							},
-						},
-						TimeRange: honeycombio.IntPtr(7200),
-					},
 				},
 			},
 		}
diff --git a/honeycombio/resource_query_test.go b/honeycombio/resource_query_test.go
index 59bb4377..df3b7192 100644
--- a/honeycombio/resource_query_test.go
+++ b/honeycombio/resource_query_test.go
@@ -4,7 +4,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"strconv"
 	"testing"
 
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
@@ -15,6 +14,7 @@ import (
 
 func TestAccHoneycombioQuery_update(t *testing.T) {
 	dataset := testAccDataset()
+	zeroDuration := 0
 	firstDuration := 20
 	secondDuration := 40
 
@@ -22,6 +22,12 @@ func TestAccHoneycombioQuery_update(t *testing.T) {
 		PreCheck:          testAccPreCheck(t),
 		ProviderFactories: testAccProviderFactories,
 		Steps: []resource.TestStep{
+			{
+				Config: testAccResourceQueryConfig(dataset, zeroDuration),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckQueryExists(t, dataset, "honeycombio_query.test", zeroDuration),
+				),
+			},
 			{
 				Config: testAccResourceQueryConfig(dataset, firstDuration),
 				Check: resource.ComposeTestCheckFunc(
@@ -85,7 +91,7 @@ func testAccCheckQueryExists(t *testing.T, dataset string, name string, duration
 				{
 					Column: "duration_ms",
 					Op:     ">",
-					Value:  strconv.Itoa(duration),
+					Value:  float64(duration),
 				},
 			},
 			TimeRange: honeycombio.IntPtr(7200),
diff --git a/honeycombio/type_helpers_test.go b/honeycombio/type_helpers_test.go
new file mode 100644
index 00000000..87d5ee17
--- /dev/null
+++ b/honeycombio/type_helpers_test.go
@@ -0,0 +1,57 @@
+package honeycombio
+
+import (
+	"reflect"
+	"testing"
+)
+
+func Test_coerceValueToType(t *testing.T) {
+	tests := []struct {
+		name     string
+		input    string
+		expected interface{}
+	}{
+		{
+			name:     "boolean true",
+			input:    "true",
+			expected: true,
+		},
+		{
+			name:     "boolean false",
+			input:    "false",
+			expected: false,
+		},
+		{
+			name:     "float",
+			input:    "10383.383",
+			expected: 10383.383,
+		},
+		{
+			name:     "int",
+			input:    "300",
+			expected: 300,
+		},
+		{
+			name:     "zero",
+			input:    "0",
+			expected: 0,
+		},
+		{
+			name:     "stringy number",
+			input:    "10.special",
+			expected: "10.special",
+		},
+		{
+			name:     "empty string",
+			input:    "",
+			expected: "",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := coerceValueToType(tt.input); !reflect.DeepEqual(got, tt.expected) {
+				t.Errorf("coerceInputToType() = %v<%T>, want %v<%T>", got, got, tt.expected, tt.expected)
+			}
+		})
+	}
+}