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 description:
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)
+				}
+			}
+		})
+	}
+}