Repository: mbrt/gmailctl (go)

Issue:
download command doesn't support filters with HasAttachment
When running the `download` subcommand, it fails with 

```
$ gmailctl download
error importing filter #0: empty criteria
```

In looking at it, my first filter is one that labels things with File Attachments:

<img width="421" alt="image" src="https://user-images.githubusercontent.com/4735/78503014-ad1dd780-7729-11ea-95ce-73137c47dba1.png">

As that has no criteria around those defaulted or assumed filters, it fails, even though it's something you can create a filter around.

It would be nice to be able to have it skip over these if they aren't currently downloadable, or to add support for other criteria fields.

Pull request:
Has attachment
Close #97.

Patch:
diff --git a/pkg/export/api/api_import.go b/pkg/export/api/api_import.go
index 644c6d2..1557b1e 100644
--- a/pkg/export/api/api_import.go
+++ b/pkg/export/api/api_import.go
@@ -3,6 +3,8 @@ package api
 import (
 	"errors"
 	"fmt"
+	"reflect"
+	"strings"
 
 	"github.com/hashicorp/go-multierror"
 	gmailv1 "google.golang.org/api/gmail/v1"
@@ -11,6 +13,39 @@ import (
 	"github.com/mbrt/gmailctl/pkg/gmail"
 )
 
+var (
+	// keep sorted
+	knownCriteriaFields = map[string]bool{
+		"ExcludeChats":    true,
+		"ForceSendFields": true,
+		"From":            true,
+		"HasAttachment":   true,
+		"NegatedQuery":    true,
+		"NullFields":      true,
+		"Query":           true,
+		"Size":            true,
+		"SizeComparison":  true,
+		"Subject":         true,
+		"To":              true,
+	}
+	// keep sorted
+	knownActionFields = map[string]bool{
+		"AddLabelIds":     true,
+		"ForceSendFields": true,
+		"Forward":         true,
+		"NullFields":      true,
+		"RemoveLabelIds":  true,
+	}
+	// keep sorted
+	unsupportedCriteriaFields = map[string]bool{
+		"ExcludeChats":   true,
+		"Size":           true,
+		"SizeComparison": true,
+	}
+	// keep sorted
+	unsupportedActionFields = map[string]bool{}
+)
+
 // Import exports Gmail filters into Gmail API objects.
 //
 // If some filter is invalid, the import skips it and returns only the
@@ -54,6 +89,10 @@ func importAction(action *gmailv1.FilterAction, lmap LabelMap) (filter.Actions,
 	if action == nil {
 		return res, errors.New("empty action")
 	}
+	if err := checkUnsupportedFields(*action, unsupportedActionFields); err != nil {
+		return res, fmt.Errorf("criteria: %w", err)
+	}
+
 	if err := importAddLabels(&res, action.AddLabelIds, lmap); err != nil {
 		return res, err
 	}
@@ -135,24 +174,62 @@ func importCategory(labelID string) gmail.Category {
 }
 
 func importCriteria(criteria *gmailv1.FilterCriteria) (filter.Criteria, error) {
+	// We don't ever generate queries that touch certain fields, so supporting them
+	// only for the import phase is not worth the effort. Instead update the regular
+	// query field with an equivalent expression.
 	if criteria == nil {
 		return filter.Criteria{}, errors.New("empty criteria")
 	}
-	query := criteria.Query
+	if err := checkUnsupportedFields(*criteria, unsupportedCriteriaFields); err != nil {
+		return filter.Criteria{}, fmt.Errorf("criteria: %w", err)
+	}
+
+	query := appendQuery(nil, criteria.Query)
 
-	// We don't ever generate negated queries, so supporting them only for the import
-	// is not worth the effort. Instead update the regular query field with an
-	// equivalent expression.
+	// Negated queries:
 	// Note that elements in the negated query are by default in OR together, according
 	// to GMail behavior.
 	if criteria.NegatedQuery != "" {
-		query = fmt.Sprintf("%s -{%s}", query, criteria.NegatedQuery)
+		query = appendQuery(query, fmt.Sprintf("-{%s}", criteria.NegatedQuery))
+	}
+
+	// HasAttachment:
+	if criteria.HasAttachment {
+		query = appendQuery(query, "has:attachment")
 	}
 
 	return filter.Criteria{
 		From:    criteria.From,
 		To:      criteria.To,
 		Subject: criteria.Subject,
-		Query:   query,
+		Query:   strings.Join(query, " "),
 	}, nil
 }
+
+func checkUnsupportedFields(a interface{}, unsupported map[string]bool) error {
+	t := reflect.TypeOf(a)
+	v := reflect.ValueOf(a)
+
+	for i := 0; i < v.NumField(); i++ {
+		field := v.Field(i)
+		name := t.Field(i).Name
+
+		if !unsupported[name] {
+			continue
+		}
+		// Check that the value for unsupported fields is zero.
+		fv := field.Interface()
+		if fv != reflect.Zero(field.Type()).Interface() {
+			return fmt.Errorf("usage of unsupported field %q (value %v)", name, fv)
+		}
+	}
+
+	return nil
+}
+
+func appendQuery(q []string, a string) []string {
+	if a == "" {
+		return q
+	}
+	return append(q, a)
+}


Test patch:
diff --git a/pkg/export/api/api_import_test.go b/pkg/export/api/api_import_test.go
index f590c99..2a1bb18 100644
--- a/pkg/export/api/api_import_test.go
+++ b/pkg/export/api/api_import_test.go
@@ -64,10 +64,11 @@ func TestImportCriteria(t *testing.T) {
 				RemoveLabelIds: []string{},
 			},
 			Criteria: &gmailv1.FilterCriteria{
-				From:    "foo@bar.com",
-				To:      "baz@zuz.it",
-				Subject: "baz",
-				Query:   "my query",
+				From:          "foo@bar.com",
+				To:            "baz@zuz.it",
+				Subject:       "baz",
+				Query:         "my query",
+				HasAttachment: true,
 			},
 		},
 	}
@@ -81,7 +82,7 @@ func TestImportCriteria(t *testing.T) {
 				From:    "foo@bar.com",
 				To:      "baz@zuz.it",
 				Subject: "baz",
-				Query:   "my query",
+				Query:   "my query has:attachment",
 			},
 		},
 	}
@@ -162,6 +163,14 @@ func TestImportBad(t *testing.T) {
 				From: "foo@bar.com",
 			},
 		},
+		{
+			Action: &gmailv1.FilterAction{
+				AddLabelIds: []string{labelIDTrash},
+			},
+			Criteria: &gmailv1.FilterCriteria{
+				Size: 123,
+			},
+		},
 	}
 	imported, err := Import(filters, emptyLabelMap())
 	assert.NotNil(t, err)
diff --git a/pkg/export/api/api_test.go b/pkg/export/api/api_test.go
new file mode 100644
index 0000000..4b8d836
--- /dev/null
+++ b/pkg/export/api/api_test.go
@@ -0,0 +1,40 @@
+package api
+
+import (
+	"reflect"
+	"testing"
+
+	gmailv1 "google.golang.org/api/gmail/v1"
+)
+
+func TestUnknownFilterFields(t *testing.T) {
+	// Test that new versions of the API don't add fields that we don't know
+	// about.
+	// This makes sure we acknowledge the existence of new fields so that we
+	// don't screw up user filters on updates.
+	tests := []struct {
+		value       interface{}
+		knownFields map[string]bool
+	}{
+		{
+			value:       gmailv1.FilterAction{},
+			knownFields: knownActionFields,
+		},
+		{
+			value:       gmailv1.FilterCriteria{},
+			knownFields: knownCriteriaFields,
+		},
+	}
+
+	for _, tc := range tests {
+		tp := reflect.TypeOf(tc.value)
+		t.Run(tp.Name(), func(t *testing.T) {
+			for i := 0; i < tp.NumField(); i++ {
+				field := tp.Field(i)
+				if !tc.knownFields[field.Name] {
+					t.Errorf("Field %q is unknown", field.Name)
+				}
+			}
+		})
+	}
+}