From 3b129aaa80e752dd2e0e007fc28c0db652af6b5c Mon Sep 17 00:00:00 2001
From: Jason Song <i@wolfogre.com>
Date: Wed, 16 Aug 2023 23:40:13 +0800
Subject: [PATCH] Explain SearchOptions and fix ToSearchOptions (#26542)

Follow #26012 #26490.

A detailed description has been added to the comment.
---
 modules/indexer/issues/db/options.go     |  1 +
 modules/indexer/issues/dboptions.go      | 40 +++++++++++-------------
 modules/indexer/issues/internal/model.go | 16 +++++++++-
 3 files changed, 35 insertions(+), 22 deletions(-)

diff --git a/modules/indexer/issues/db/options.go b/modules/indexer/issues/db/options.go
index ebd672a695..0d6a8406d3 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 d0ef1c96b4..a3b18fdcd1 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,25 +37,27 @@ func ToSearchOptions(keyword string, opts *issues_model.IssuesOptions) *SearchOp
 		searchOpt.MilestoneIDs = opts.MilestoneIDs
 	}
 
-	if opts.AssigneeID > 0 {
-		searchOpt.AssigneeID = &opts.AssigneeID
-	}
-	if opts.PosterID > 0 {
-		searchOpt.PosterID = &opts.PosterID
-	}
-	if opts.MentionedID > 0 {
-		searchOpt.MentionID = &opts.MentionedID
-	}
-	if opts.ReviewedID > 0 {
-		searchOpt.ReviewedID = &opts.ReviewedID
-	}
-	if opts.ReviewRequestedID > 0 {
-		searchOpt.ReviewRequestedID = &opts.ReviewRequestedID
-	}
-	if opts.SubscriberID > 0 {
-		searchOpt.SubscriberID = &opts.SubscriberID
+	// 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
 	}
diff --git a/modules/indexer/issues/internal/model.go b/modules/indexer/issues/internal/model.go
index 31acd16bd4..2de1e0e2bf 100644
--- a/modules/indexer/issues/internal/model.go
+++ b/modules/indexer/issues/internal/model.go
@@ -56,7 +56,21 @@ 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 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?
+// 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.
 type SearchOptions struct {
 	Keyword string // keyword to search