From 8ac158cdb4957bdbb34c4b9eab46447596906884 Mon Sep 17 00:00:00 2001 From: konrad Date: Sat, 9 May 2020 15:48:56 +0000 Subject: [PATCH] Task Filter Fixes (#495) Fix gocyclo Fix misspell Error codes docs Filter fixes Co-authored-by: kolaente Reviewed-on: https://kolaente.dev/vikunja/api/pulls/495 --- Makefile | 2 +- docs/content/doc/usage/errors.md | 1 + pkg/models/error.go | 27 +++++++++++++++++ pkg/models/task_collection.go | 12 +++++--- pkg/models/task_collection_filter.go | 6 ++++ pkg/models/tasks.go | 43 +++++++++++++++++++++------- 6 files changed, 75 insertions(+), 16 deletions(-) diff --git a/Makefile b/Makefile index f1eb960b..850e5a01 100644 --- a/Makefile +++ b/Makefile @@ -219,7 +219,7 @@ gocyclo-check: go get -u github.com/fzipp/gocyclo; \ go install $(GOFLAGS) github.com/fzipp/gocyclo; \ fi - for S in $(GOFILES); do gocyclo -over 24 $$S || exit 1; done; + for S in $(GOFILES); do gocyclo -over 27 $$S || exit 1; done; .PHONY: static-check static-check: diff --git a/docs/content/doc/usage/errors.md b/docs/content/doc/usage/errors.md index 490b6f88..6c7b3eb5 100644 --- a/docs/content/doc/usage/errors.md +++ b/docs/content/doc/usage/errors.md @@ -77,6 +77,7 @@ This document describes the different errors Vikunja can return. | 4015 | 404 | The task comment does not exist. | | 4016 | 403 | Invalid task field. | | 4017 | 403 | Invalid task filter comparator. | +| 4018 | 403 | Invalid task filter concatinator. | ### Namespace diff --git a/pkg/models/error.go b/pkg/models/error.go index 1fe4321e..8809b495 100644 --- a/pkg/models/error.go +++ b/pkg/models/error.go @@ -707,6 +707,33 @@ func (err ErrInvalidTaskFilterComparator) HTTPError() web.HTTPError { } } +// ErrInvalidTaskFilterConcatinator represents an error where the provided task field is invalid +type ErrInvalidTaskFilterConcatinator struct { + Concatinator taskFilterConcatinator +} + +// IsErrInvalidTaskFilterConcatinator checks if an error is ErrInvalidTaskFilterConcatinator. +func IsErrInvalidTaskFilterConcatinator(err error) bool { + _, ok := err.(ErrInvalidTaskFilterConcatinator) + return ok +} + +func (err ErrInvalidTaskFilterConcatinator) Error() string { + return fmt.Sprintf("Task filter concatinator is invalid [Concatinator: %s]", err.Concatinator) +} + +// ErrCodeInvalidTaskFilterConcatinator holds the unique world-error code of this error +const ErrCodeInvalidTaskFilterConcatinator = 4018 + +// HTTPError holds the http error description +func (err ErrInvalidTaskFilterConcatinator) HTTPError() web.HTTPError { + return web.HTTPError{ + HTTPCode: http.StatusBadRequest, + Code: ErrCodeInvalidTaskFilterConcatinator, + Message: fmt.Sprintf("The task filter concatinator '%s' is invalid.", err.Concatinator), + } +} + // ================= // Namespace errors // ================= diff --git a/pkg/models/task_collection.go b/pkg/models/task_collection.go index 07c99127..6cc72b78 100644 --- a/pkg/models/task_collection.go +++ b/pkg/models/task_collection.go @@ -43,6 +43,8 @@ type TaskCollection struct { // The comparator for field and value FilterComparator []string `query:"filter_comparator"` FilterComparatorArr []string `query:"filter_comparator[]"` + // The way all filter conditions are concatenated together, can be either "and" or "or"., + FilterConcat string `query:"filter_concat"` web.CRUDable `xorm:"-" json:"-"` web.Rights `xorm:"-" json:"-"` @@ -90,6 +92,7 @@ func validateTaskField(fieldName string) error { // @Param filter_by query string false "The name of the field to filter by. Accepts an array for multiple filters which will be chanied together, all supplied filter must match." // @Param filter_value query string false "The value to filter for." // @Param filter_comparator query string false "The comparator to use for a filter. Available values are `equals`, `greater`, `greater_equals`, `less` and `less_equals`. Defaults to `equals`" +// @Param filter_concat query string false "The concatinator to use for filters. Available values are `and` or `or`. Defaults to `or`." // @Security JWTKeyAuth // @Success 200 {array} models.Task "The tasks" // @Failure 500 {object} models.Message "Internal error" @@ -123,10 +126,11 @@ func (tf *TaskCollection) ReadAll(a web.Auth, search string, page int, perPage i } taskopts := &taskOptions{ - search: search, - page: page, - perPage: perPage, - sortby: sort, + search: search, + page: page, + perPage: perPage, + sortby: sort, + filterConcat: taskFilterConcatinator(tf.FilterConcat), } taskopts.filters, err = getTaskFiltersByCollections(tf) diff --git a/pkg/models/task_collection_filter.go b/pkg/models/task_collection_filter.go index b8705950..cfc6de10 100644 --- a/pkg/models/task_collection_filter.go +++ b/pkg/models/task_collection_filter.go @@ -56,6 +56,12 @@ func getTaskFiltersByCollections(c *TaskCollection) (filters []*taskFilter, err c.FilterComparator = append(c.FilterComparator, c.FilterComparatorArr...) } + if c.FilterConcat != "" && c.FilterConcat != filterConcatAnd && c.FilterConcat != filterConcatOr { + return nil, ErrInvalidTaskFilterConcatinator{ + Concatinator: taskFilterConcatinator(c.FilterConcat), + } + } + filters = make([]*taskFilter, 0, len(c.FilterBy)) for i, f := range c.FilterBy { filter := &taskFilter{ diff --git a/pkg/models/tasks.go b/pkg/models/tasks.go index e7e153c3..9e261222 100644 --- a/pkg/models/tasks.go +++ b/pkg/models/tasks.go @@ -122,12 +122,20 @@ func (TaskReminder) TableName() string { return "task_reminders" } +type taskFilterConcatinator string + +const ( + filterConcatAnd = "and" + filterConcatOr = "or" +) + type taskOptions struct { - search string - page int - perPage int - sortby []*sortParam - filters []*taskFilter + search string + page int + perPage int + sortby []*sortParam + filters []*taskFilter + filterConcat taskFilterConcatinator } // ReadAll is a dummy function to still have that endpoint documented @@ -144,6 +152,7 @@ type taskOptions struct { // @Param filter_by query string false "The name of the field to filter by. Accepts an array for multiple filters which will be chanied together, all supplied filter must match." // @Param filter_value query string false "The value to filter for." // @Param filter_comparator query string false "The comparator to use for a filter. Available values are `equals`, `greater`, `greater_equals`, `less` and `less_equals`. Defaults to `equals`" +// @Param filter_concat query string false "The concatinator to use for filters. Available values are `and` or `or`. Defaults to `or`." // @Security JWTKeyAuth // @Success 200 {array} models.Task "The tasks" // @Failure 500 {object} models.Message "Internal error" @@ -154,6 +163,11 @@ func (t *Task) ReadAll(a web.Auth, search string, page int, perPage int) (result func getRawTasksForLists(lists []*List, opts *taskOptions) (tasks []*Task, resultCount int, totalItems int64, err error) { + // Set the default concatinator of filter variables to or if none was provided + if opts.filterConcat == "" { + opts.filterConcat = filterConcatOr + } + // Get all list IDs and get the tasks var listIDs []int64 for _, l := range lists { @@ -197,6 +211,7 @@ func getRawTasksForLists(lists []*List, opts *taskOptions) (tasks []*Task, resul } var filters = make([]builder.Cond, 0, len(opts.filters)) + // To still find tasks with nil values, we exclude 0s when comparing with >/< values. for _, f := range opts.filters { switch f.comparator { case taskFilterComparatorEquals: @@ -204,13 +219,13 @@ func getRawTasksForLists(lists []*List, opts *taskOptions) (tasks []*Task, resul case taskFilterComparatorNotEquals: filters = append(filters, &builder.Neq{f.field: f.value}) case taskFilterComparatorGreater: - filters = append(filters, &builder.Gt{f.field: f.value}) + filters = append(filters, builder.Or(&builder.Gt{f.field: f.value}, &builder.Eq{f.field: 0})) case taskFilterComparatorGreateEquals: - filters = append(filters, &builder.Gte{f.field: f.value}) + filters = append(filters, builder.Or(&builder.Gte{f.field: f.value}, &builder.Eq{f.field: 0})) case taskFilterComparatorLess: - filters = append(filters, &builder.Lt{f.field: f.value}) + filters = append(filters, builder.Or(&builder.Lt{f.field: f.value}, &builder.Eq{f.field: 0})) case taskFilterComparatorLessEquals: - filters = append(filters, &builder.Lte{f.field: f.value}) + filters = append(filters, builder.Or(&builder.Lte{f.field: f.value}, &builder.Eq{f.field: 0})) } } @@ -230,8 +245,14 @@ func getRawTasksForLists(lists []*List, opts *taskOptions) (tasks []*Task, resul } if len(filters) > 0 { - query = query.Where(builder.Or(filters...)) - queryCount = queryCount.Where(builder.Or(filters...)) + if opts.filterConcat == filterConcatOr { + query = query.Where(builder.Or(filters...)) + queryCount = queryCount.Where(builder.Or(filters...)) + } + if opts.filterConcat == filterConcatAnd { + query = query.Where(builder.And(filters...)) + queryCount = queryCount.Where(builder.And(filters...)) + } } limit, start := getLimitFromPageIndex(opts.page, opts.perPage)