Merge pull request #937 from Shopify/logpush-null-filter

fix marshalling of LogpushJob with no filter
cloudflare · Jun 18, 2022 · 2909e7e · 2909e7e
2 parents 604e5b8 + d07e8d6
commit 2909e7e
Show file tree

Hide file tree

Showing 3 changed files with 79 additions and 21 deletions.
diff --git a/logpush.go b/logpush.go
@@ -12,18 +12,18 @@ import (
 
 // LogpushJob describes a Logpush job.
 type LogpushJob struct {
-	ID                 int               `json:"id,omitempty"`
-	Dataset            string            `json:"dataset"`
-	Enabled            bool              `json:"enabled"`
-	Name               string            `json:"name"`
-	LogpullOptions     string            `json:"logpull_options"`
-	DestinationConf    string            `json:"destination_conf"`
-	OwnershipChallenge string            `json:"ownership_challenge,omitempty"`
-	LastComplete       *time.Time        `json:"last_complete,omitempty"`
-	LastError          *time.Time        `json:"last_error,omitempty"`
-	ErrorMessage       string            `json:"error_message,omitempty"`
-	Frequency          string            `json:"frequency,omitempty"`
-	Filter             LogpushJobFilters `json:"filter,omitempty"`
+	ID                 int                `json:"id,omitempty"`
+	Dataset            string             `json:"dataset"`
+	Enabled            bool               `json:"enabled"`
+	Name               string             `json:"name"`
+	LogpullOptions     string             `json:"logpull_options"`
+	DestinationConf    string             `json:"destination_conf"`
+	OwnershipChallenge string             `json:"ownership_challenge,omitempty"`
+	LastComplete       *time.Time         `json:"last_complete,omitempty"`
+	LastError          *time.Time         `json:"last_error,omitempty"`
+	ErrorMessage       string             `json:"error_message,omitempty"`
+	Frequency          string             `json:"frequency,omitempty"`
+	Filter             *LogpushJobFilters `json:"filter,omitempty"`
 }
 
 type LogpushJobFilters struct {
@@ -131,17 +131,23 @@ type LogpushDestinationExistsRequest struct {
 func (f LogpushJob) MarshalJSON() ([]byte, error) {
 	type Alias LogpushJob
 
-	filter, err := json.Marshal(f.Filter)
+	var filter string
 
-	if err != nil {
-		return nil, err
+	if f.Filter != nil {
+		b, err := json.Marshal(f.Filter)
+
+		if err != nil {
+			return nil, err
+		}
+
+		filter = string(b)
 	}
 
 	return json.Marshal(&struct {
 		Filter string `json:"filter,omitempty"`
 		Alias
 	}{
-		Filter: string(filter),
+		Filter: filter,
 		Alias:  (Alias)(f),
 	})
 }
@@ -167,7 +173,7 @@ func (f *LogpushJob) UnmarshalJSON(data []byte) error {
 		if err := filter.Where.Validate(); err != nil {
 			return err
 		}
-		f.Filter = filter
+		f.Filter = &filter
 	}
 	return nil
 }

diff --git a/logpush_example_test.go b/logpush_example_test.go
@@ -180,7 +180,7 @@ func ExampleLogpushJob_MarshalJSON() {
 		LogpullOptions:  "fields=RayID,ClientIP,EdgeStartTimestamp&timestamps=rfc3339&CVE-2021-44228=true",
 		Dataset:         "http_requests",
 		DestinationConf: "s3://<BUCKET_PATH>?region=us-west-2/",
-		Filter: cloudflare.LogpushJobFilters{
+		Filter: &cloudflare.LogpushJobFilters{
 			Where: cloudflare.LogpushJobFilter{
 				And: []cloudflare.LogpushJobFilter{
 					{Key: "ClientRequestPath", Operator: cloudflare.Contains, Value: "/static\\"},

diff --git a/logpush_test.go b/logpush_test.go
@@ -17,8 +17,8 @@ import (
 const (
 	jobID                       = 1
 	serverLogpushJobDescription = `{
-		"id": %d,
-		"dataset": "http_requests",
+	"id": %d,
+	"dataset": "http_requests",
     "enabled": false,
 	"name": "example.com",
     "logpull_options": "fields=RayID,ClientIP,EdgeStartTimestamp&timestamps=rfc3339",
@@ -389,7 +389,7 @@ func TestLogpushJob_Unmarshall(t *testing.T) {
 			LogpullOptions:  "fields=RayID,ClientIP,EdgeStartTimestamp&timestamps=rfc3339&CVE-2021-44228=true",
 			Dataset:         "http_requests",
 			DestinationConf: "s3://<BUCKET_PATH>?region=us-west-2/",
-			Filter: LogpushJobFilters{
+			Filter: &LogpushJobFilters{
 				Where: LogpushJobFilter{
 					And: []LogpushJobFilter{
 						{Key: "ClientRequestPath", Operator: Contains, Value: "/static\\"},
@@ -423,3 +423,55 @@ func TestLogpushJob_Unmarshall(t *testing.T) {
 		}, job)
 	})
 }
+
+func TestLogPushJob_Marshall(t *testing.T) {
+	testCases := []struct {
+		job  LogpushJob
+		want string
+	}{
+		{
+			job: LogpushJob{
+				Dataset:         "http_requests",
+				Name:            "valid filter",
+				LogpullOptions:  "fields=RayID,ClientIP,EdgeStartTimestamp&timestamps=rfc3339",
+				DestinationConf: "https://example.com",
+				Filter: &LogpushJobFilters{
+					Where: LogpushJobFilter{Key: "ClientRequestHost", Operator: Equal, Value: "example.com"},
+				},
+			},
+			want: `{
+				"dataset": "http_requests",
+				"enabled": false,
+				"name": "valid filter",
+				"logpull_options": "fields=RayID,ClientIP,EdgeStartTimestamp&timestamps=rfc3339",
+				"destination_conf": "https://example.com",
+				"filter":"{\"where\":{\"key\":\"ClientRequestHost\",\"operator\":\"eq\",\"value\":\"example.com\"}}"
+			}`,
+		},
+		{
+			job: LogpushJob{
+				Dataset:         "http_requests",
+				Name:            "no filter",
+				LogpullOptions:  "fields=RayID,ClientIP,EdgeStartTimestamp&timestamps=rfc3339",
+				DestinationConf: "https://example.com",
+			},
+			want: `{
+				"dataset": "http_requests",
+				"enabled": false,
+				"name": "no filter",
+				"logpull_options": "fields=RayID,ClientIP,EdgeStartTimestamp&timestamps=rfc3339",
+				"destination_conf": "https://example.com"
+			}`,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.job.Name, func(t *testing.T) {
+			got, err := json.Marshal(tc.job)
+
+			if assert.NoError(t, err) {
+				assert.JSONEq(t, tc.want, string(got))
+			}
+		})
+	}
+}