From bebf28cab19742126ffedc2997b5faf5d783817d Mon Sep 17 00:00:00 2001 From: Jason Song Date: Wed, 16 Aug 2023 17:30:40 +0800 Subject: [PATCH 1/4] docs: comment for SearchOptions --- modules/indexer/issues/internal/model.go | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/modules/indexer/issues/internal/model.go b/modules/indexer/issues/internal/model.go index 31acd16bd44e..fed2c95d34a5 100644 --- a/modules/indexer/issues/internal/model.go +++ b/modules/indexer/issues/internal/model.go @@ -56,7 +56,22 @@ type SearchResult struct { Hits []Match } -// SearchOptions represents search options +// SearchOptions represents search options. +// +// It has a slightly different design from database query options. +// In database query options, a field is never a pointer, so it could be confusing when it's zero value: +// Do you want to search for data with a field value of 0, or do you not specify the field in the search options? +// To avoid this confusion, db introduced db.NoConditionID(-1). +// So zero value means the field is not specified in the search options, and db.NoConditionID means "== 0" or "id NOT IN (SELECT id FROM ...)" +// It's still not ideal, it trapped developers many times. +// And sometimes -1 could be a valid value, like issue ID, negative numbers indicate exclusion. +// Since db.NoConditionID is for "db" (the package name is db), it makes sense not to use it in the indexer: +// why do bleve/elasticsearch/meilisearch indexers need to know about db.NoConditionID? +// So in SearchOptions, we use pointer for fields which could be not specified, +// and always use the value to filter if it's not nil, even if it's zero or negative. +// It can handle almost all cases, if there is an exception, we can add a new field, like NoLabelOnly. +// Unfortunately, we still use db for the indexer and have to convert between db.NoConditionID and nil for legacy reasons. +// Sorry to confuse anyone who has been trapped by this. Maybe we could refactor the design in the future. type SearchOptions struct { Keyword string // keyword to search From b4b508ee9968de1c1fbfe0975fe74c2024389576 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Wed, 16 Aug 2023 17:42:28 +0800 Subject: [PATCH 2/4] fix: ToSearchOptions --- modules/indexer/issues/dboptions.go | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/modules/indexer/issues/dboptions.go b/modules/indexer/issues/dboptions.go index d0ef1c96b448..a819ec4c1977 100644 --- a/modules/indexer/issues/dboptions.go +++ b/modules/indexer/issues/dboptions.go @@ -17,10 +17,6 @@ func ToSearchOptions(keyword string, opts *issues_model.IssuesOptions) *SearchOp IsClosed: opts.IsClosed, } - if opts.ProjectID != 0 { - searchOpt.ProjectID = &opts.ProjectID - } - if len(opts.LabelIDs) == 1 && opts.LabelIDs[0] == 0 { searchOpt.NoLabelOnly = true } else { @@ -41,23 +37,47 @@ func ToSearchOptions(keyword string, opts *issues_model.IssuesOptions) *SearchOp searchOpt.MilestoneIDs = opts.MilestoneIDs } + zero := int64(0) + + if opts.ProjectID > 0 { + searchOpt.ProjectID = &opts.ProjectID + } else if opts.ProjectID == db.NoConditionID { + searchOpt.ProjectID = &zero + } + if opts.ProjectBoardID > 0 { + searchOpt.ProjectBoardID = &opts.ProjectBoardID + } else if opts.ProjectBoardID == db.NoConditionID { + searchOpt.ProjectBoardID = &zero + } if opts.AssigneeID > 0 { searchOpt.AssigneeID = &opts.AssigneeID + } else if opts.AssigneeID == db.NoConditionID { + searchOpt.AssigneeID = &zero } if opts.PosterID > 0 { searchOpt.PosterID = &opts.PosterID + } else if opts.PosterID == db.NoConditionID { + searchOpt.PosterID = &zero } if opts.MentionedID > 0 { searchOpt.MentionID = &opts.MentionedID + } else if opts.MentionedID == db.NoConditionID { + searchOpt.MentionID = &zero } if opts.ReviewedID > 0 { searchOpt.ReviewedID = &opts.ReviewedID + } else if opts.ReviewedID == db.NoConditionID { + searchOpt.ReviewedID = &zero } if opts.ReviewRequestedID > 0 { searchOpt.ReviewRequestedID = &opts.ReviewRequestedID + } else if opts.ReviewRequestedID == db.NoConditionID { + searchOpt.ReviewRequestedID = &zero } if opts.SubscriberID > 0 { searchOpt.SubscriberID = &opts.SubscriberID + } else if opts.SubscriberID == db.NoConditionID { + searchOpt.SubscriberID = &zero } if opts.UpdatedAfterUnix > 0 { From 8bde905ccc76e97da94a1735e619d24d235b07ca Mon Sep 17 00:00:00 2001 From: Jason Song Date: Wed, 16 Aug 2023 17:48:29 +0800 Subject: [PATCH 3/4] fix: use convertID --- modules/indexer/issues/db/options.go | 1 + modules/indexer/issues/dboptions.go | 60 +++++++++------------------- 2 files changed, 20 insertions(+), 41 deletions(-) diff --git a/modules/indexer/issues/db/options.go b/modules/indexer/issues/db/options.go index ebd672a6954a..0d6a8406d3b2 100644 --- a/modules/indexer/issues/db/options.go +++ b/modules/indexer/issues/db/options.go @@ -14,6 +14,7 @@ import ( ) func ToDBOptions(ctx context.Context, options *internal.SearchOptions) (*issue_model.IssuesOptions, error) { + // See the comment of issues_model.SearchOptions for the reason why we need to convert convertID := func(id *int64) int64 { if id == nil { return 0 diff --git a/modules/indexer/issues/dboptions.go b/modules/indexer/issues/dboptions.go index a819ec4c1977..a3b18fdcd17b 100644 --- a/modules/indexer/issues/dboptions.go +++ b/modules/indexer/issues/dboptions.go @@ -37,49 +37,27 @@ func ToSearchOptions(keyword string, opts *issues_model.IssuesOptions) *SearchOp searchOpt.MilestoneIDs = opts.MilestoneIDs } - zero := int64(0) - - if opts.ProjectID > 0 { - searchOpt.ProjectID = &opts.ProjectID - } else if opts.ProjectID == db.NoConditionID { - searchOpt.ProjectID = &zero - } - if opts.ProjectBoardID > 0 { - searchOpt.ProjectBoardID = &opts.ProjectBoardID - } else if opts.ProjectBoardID == db.NoConditionID { - searchOpt.ProjectBoardID = &zero - } - if opts.AssigneeID > 0 { - searchOpt.AssigneeID = &opts.AssigneeID - } else if opts.AssigneeID == db.NoConditionID { - searchOpt.AssigneeID = &zero - } - if opts.PosterID > 0 { - searchOpt.PosterID = &opts.PosterID - } else if opts.PosterID == db.NoConditionID { - searchOpt.PosterID = &zero - } - if opts.MentionedID > 0 { - searchOpt.MentionID = &opts.MentionedID - } else if opts.MentionedID == db.NoConditionID { - searchOpt.MentionID = &zero - } - if opts.ReviewedID > 0 { - searchOpt.ReviewedID = &opts.ReviewedID - } else if opts.ReviewedID == db.NoConditionID { - searchOpt.ReviewedID = &zero - } - if opts.ReviewRequestedID > 0 { - searchOpt.ReviewRequestedID = &opts.ReviewRequestedID - } else if opts.ReviewRequestedID == db.NoConditionID { - searchOpt.ReviewRequestedID = &zero - } - if opts.SubscriberID > 0 { - searchOpt.SubscriberID = &opts.SubscriberID - } else if opts.SubscriberID == db.NoConditionID { - searchOpt.SubscriberID = &zero + // See the comment of issues_model.SearchOptions for the reason why we need to convert + convertID := func(id int64) *int64 { + if id > 0 { + return &id + } + if id == db.NoConditionID { + var zero int64 + return &zero + } + return nil } + searchOpt.ProjectID = convertID(opts.ProjectID) + searchOpt.ProjectBoardID = convertID(opts.ProjectBoardID) + searchOpt.PosterID = convertID(opts.PosterID) + searchOpt.AssigneeID = convertID(opts.AssigneeID) + searchOpt.MentionID = convertID(opts.MentionedID) + searchOpt.ReviewedID = convertID(opts.ReviewedID) + searchOpt.ReviewRequestedID = convertID(opts.ReviewRequestedID) + searchOpt.SubscriberID = convertID(opts.SubscriberID) + if opts.UpdatedAfterUnix > 0 { searchOpt.UpdatedAfterUnix = &opts.UpdatedAfterUnix } From 73cb49f25b45d5eb6b7c5b231e0904f8d72efa13 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Wed, 16 Aug 2023 17:51:21 +0800 Subject: [PATCH 4/4] docs: update comments --- modules/indexer/issues/internal/model.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/modules/indexer/issues/internal/model.go b/modules/indexer/issues/internal/model.go index fed2c95d34a5..2de1e0e2bf20 100644 --- a/modules/indexer/issues/internal/model.go +++ b/modules/indexer/issues/internal/model.go @@ -60,18 +60,17 @@ type SearchResult struct { // // It has a slightly different design from database query options. // In database query options, a field is never a pointer, so it could be confusing when it's zero value: -// Do you want to search for data with a field value of 0, or do you not specify the field in the search options? +// Do you want to find data with a field value of 0, or do you not specify the field in the options? // To avoid this confusion, db introduced db.NoConditionID(-1). // So zero value means the field is not specified in the search options, and db.NoConditionID means "== 0" or "id NOT IN (SELECT id FROM ...)" // It's still not ideal, it trapped developers many times. // And sometimes -1 could be a valid value, like issue ID, negative numbers indicate exclusion. // Since db.NoConditionID is for "db" (the package name is db), it makes sense not to use it in the indexer: -// why do bleve/elasticsearch/meilisearch indexers need to know about db.NoConditionID? +// Why do bleve/elasticsearch/meilisearch indexers need to know about db.NoConditionID? // So in SearchOptions, we use pointer for fields which could be not specified, // and always use the value to filter if it's not nil, even if it's zero or negative. // It can handle almost all cases, if there is an exception, we can add a new field, like NoLabelOnly. // Unfortunately, we still use db for the indexer and have to convert between db.NoConditionID and nil for legacy reasons. -// Sorry to confuse anyone who has been trapped by this. Maybe we could refactor the design in the future. type SearchOptions struct { Keyword string // keyword to search