From 5f92509042979a27c75d5549dfa01ed0d4b3b51d Mon Sep 17 00:00:00 2001 From: Coteh <3276350+Coteh@users.noreply.github.com> Date: Fri, 21 Aug 2020 12:48:30 -0400 Subject: [PATCH 01/11] Emit errors through Search API --- backend/app/adapter/routing/handle/search.go | 2 +- backend/app/usecase/search/search.go | 70 ++++++++++++++------ backend/app/usecase/search/search_test.go | 14 ++-- 3 files changed, 59 insertions(+), 27 deletions(-) diff --git a/backend/app/adapter/routing/handle/search.go b/backend/app/adapter/routing/handle/search.go index 5b49abc66..d4cb68532 100644 --- a/backend/app/adapter/routing/handle/search.go +++ b/backend/app/adapter/routing/handle/search.go @@ -168,7 +168,7 @@ func (f *Filter) resourcesString() []string { return resources } -func newSearchResponse(result search.Result) SearchResponse { +func newSearchResponse(result search.ResourceResult) SearchResponse { shortLinks := make([]ShortLink, len(result.ShortLinks)) for i := 0; i < len(result.ShortLinks); i++ { shortLinks[i] = newShortLink(result.ShortLinks[i]) diff --git a/backend/app/usecase/search/search.go b/backend/app/usecase/search/search.go index d5c760221..e0c2769b0 100644 --- a/backend/app/usecase/search/search.go +++ b/backend/app/usecase/search/search.go @@ -1,7 +1,6 @@ package search import ( - "errors" "strings" "time" @@ -22,12 +21,32 @@ type Search struct { // Result represents the result of a search query. type Result struct { + Resources ResourceResult + Err error +} + +// ResourceResult represents the resources obtained from a search query. +type ResourceResult struct { ShortLinks []entity.ShortLink Users []entity.User } +// ErrUnknownResource represents unknown search resource error. +type ErrUnknownResource string + +func (e ErrUnknownResource) Error() string { + return string(e) +} + +// ErrUserNotProvided represents user not provided for search query. +type ErrUserNotProvided string + +func (e ErrUserNotProvided) Error() string { + return string(e) +} + // Search finds resources based on specified criteria. -func (s Search) Search(query Query, filter Filter) (Result, error) { +func (s Search) Search(query Query, filter Filter) (ResourceResult, error) { resultCh := make(chan Result) defer close(resultCh) @@ -38,50 +57,63 @@ func (s Search) Search(query Query, filter Filter) (Result, error) { go func() { result, err := s.searchResource(filter.resources[i], orders[i], query, filter) if err != nil { - // TODO(issue#865): Handle errors of Search API s.logger.Error(err) - resultCh <- Result{} + resultCh <- Result{ + Resources: ResourceResult{}, + Err: err, + } return } - resultCh <- result + resultCh <- Result{ + Resources: result, + Err: nil, + } }() } timeout := time.After(s.timeout) - var results []Result + var results []ResourceResult + var resultErr error for i := 0; i < len(filter.resources); i++ { select { case result := <-resultCh: - results = append(results, result) + // Only return the first error encountered + if resultErr == nil { + resultErr = result.Err + } + results = append(results, result.Resources) case <-timeout: return mergeResults(results), nil } } - return mergeResults(results), nil + return mergeResults(results), resultErr } -func (s Search) searchResource(resource Resource, orderBy order.Order, query Query, filter Filter) (Result, error) { +func (s Search) searchResource(resource Resource, orderBy order.Order, query Query, filter Filter) (ResourceResult, error) { switch resource { case ShortLink: return s.searchShortLink(query, orderBy, filter) case User: return s.searchUser(query, orderBy, filter) default: - return Result{}, errors.New("unknown resource") + // TODO add tests for unknown resource + return ResourceResult{}, ErrUnknownResource("unknown resource") } } // TODO(issue#866): Simplify searchShortLink function -func (s Search) searchShortLink(query Query, orderBy order.Order, filter Filter) (Result, error) { +func (s Search) searchShortLink(query Query, orderBy order.Order, filter Filter) (ResourceResult, error) { if query.User == nil { - s.logger.Error(errors.New("user not provided")) - return Result{}, nil + // TODO add a test for user not provided + err := ErrUserNotProvided("user not provided") + s.logger.Error(err) + return ResourceResult{}, err } shortLinks, err := s.getShortLinkByUser(*query.User) if err != nil { - return Result{}, err + return ResourceResult{}, err } var matchedAliasByAll, matchedAliasByAny, matchedLongLinkByAll, matchedLongLinkByAny []entity.ShortLink @@ -115,14 +147,14 @@ func (s Search) searchShortLink(query Query, orderBy order.Order, filter Filter) filteredShortLinks := filterShortLinks(mergedShortLinks, filter) - return Result{ + return ResourceResult{ ShortLinks: filteredShortLinks, Users: nil, }, nil } -func (s Search) searchUser(query Query, orderBy order.Order, filter Filter) (Result, error) { - return Result{}, nil +func (s Search) searchUser(query Query, orderBy order.Order, filter Filter) (ResourceResult, error) { + return ResourceResult{}, nil } func (s Search) getShortLinkByUser(user entity.User) ([]entity.ShortLink, error) { @@ -171,8 +203,8 @@ func toOrders(ordersBy []order.By) []order.Order { return orders } -func mergeResults(results []Result) Result { - var mergedResult Result +func mergeResults(results []ResourceResult) ResourceResult { + var mergedResult ResourceResult for _, result := range results { mergedResult.ShortLinks = append(mergedResult.ShortLinks, result.ShortLinks...) diff --git a/backend/app/usecase/search/search_test.go b/backend/app/usecase/search/search_test.go index 96e9a33c9..eae2d1d8e 100644 --- a/backend/app/usecase/search/search_test.go +++ b/backend/app/usecase/search/search_test.go @@ -25,7 +25,7 @@ func TestSearch(t *testing.T) { orders []order.By relationUsers []entity.User relationShortLinks []entity.ShortLink - expectedResult Result + expectedResult ResourceResult }{ { name: "search without user", @@ -97,7 +97,7 @@ func TestSearch(t *testing.T) { LongLink: "https://facebook.com", }, }, - expectedResult: Result{}, + expectedResult: ResourceResult{}, }, { name: "search without query", @@ -172,7 +172,7 @@ func TestSearch(t *testing.T) { LongLink: "https://facebook.com", }, }, - expectedResult: Result{ + expectedResult: ResourceResult{ ShortLinks: []entity.ShortLink{ { Alias: "facebook", @@ -259,7 +259,7 @@ func TestSearch(t *testing.T) { LongLink: "https://facebook.com", }, }, - expectedResult: Result{ + expectedResult: ResourceResult{ ShortLinks: []entity.ShortLink{ { Alias: "google", @@ -347,7 +347,7 @@ func TestSearch(t *testing.T) { LongLink: "https://facebook.com", }, }, - expectedResult: Result{ + expectedResult: ResourceResult{ ShortLinks: nil, Users: nil, }, @@ -426,7 +426,7 @@ func TestSearch(t *testing.T) { LongLink: "https://facebook.com", }, }, - expectedResult: Result{ + expectedResult: ResourceResult{ ShortLinks: []entity.ShortLink{ { Alias: "short", @@ -510,7 +510,7 @@ func TestSearch(t *testing.T) { LongLink: "https://facebook.com", }, }, - expectedResult: Result{ + expectedResult: ResourceResult{ ShortLinks: []entity.ShortLink{ { Alias: "short", From 3f99e4b3da47de0e67c4e63995614c41480ec3b9 Mon Sep 17 00:00:00 2001 From: Coteh <3276350+Coteh@users.noreply.github.com> Date: Fri, 21 Aug 2020 12:52:45 -0400 Subject: [PATCH 02/11] Run script formatter --- .../adapter/sqldb/short_link_integration_test.go | 16 ++++++++-------- backend/app/usecase/search/search.go | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/backend/app/adapter/sqldb/short_link_integration_test.go b/backend/app/adapter/sqldb/short_link_integration_test.go index 2349fd75f..eb776065f 100644 --- a/backend/app/adapter/sqldb/short_link_integration_test.go +++ b/backend/app/adapter/sqldb/short_link_integration_test.go @@ -726,10 +726,10 @@ func TestShortLinkSql_DeleteShortLink(t *testing.T) { name: "delete exisiting shortlink", tableRows: []shortLinkTableRow{ { - alias: "short_is_great", - longLink: "https://short-d.com", - createdAt: ptr.Time(must.Time(t, "2018-05-01T08:02:16-07:00")), - expireAt: ptr.Time(must.Time(t, "2020-05-01T08:02:16-07:00")), + alias: "short_is_great", + longLink: "https://short-d.com", + createdAt: ptr.Time(must.Time(t, "2018-05-01T08:02:16-07:00")), + expireAt: ptr.Time(must.Time(t, "2020-05-01T08:02:16-07:00")), }, }, alias: "short_is_great", @@ -739,10 +739,10 @@ func TestShortLinkSql_DeleteShortLink(t *testing.T) { name: "shortlink does not exist", tableRows: []shortLinkTableRow{ { - alias: "i_luv_short", - longLink: "https://short-d.com", - createdAt: ptr.Time(must.Time(t, "2018-05-01T08:02:16-07:00")), - expireAt: ptr.Time(must.Time(t, "2020-05-01T08:02:16-07:00")), + alias: "i_luv_short", + longLink: "https://short-d.com", + createdAt: ptr.Time(must.Time(t, "2018-05-01T08:02:16-07:00")), + expireAt: ptr.Time(must.Time(t, "2020-05-01T08:02:16-07:00")), }, }, alias: "short_is_great", diff --git a/backend/app/usecase/search/search.go b/backend/app/usecase/search/search.go index e0c2769b0..800fe6024 100644 --- a/backend/app/usecase/search/search.go +++ b/backend/app/usecase/search/search.go @@ -66,7 +66,7 @@ func (s Search) Search(query Query, filter Filter) (ResourceResult, error) { } resultCh <- Result{ Resources: result, - Err: nil, + Err: nil, } }() } From 54918ef87a32d7cf588bea871b91949b4bbe8a26 Mon Sep 17 00:00:00 2001 From: Coteh <3276350+Coteh@users.noreply.github.com> Date: Fri, 21 Aug 2020 14:26:51 -0400 Subject: [PATCH 03/11] Emit JSON from HTTP error, and emit appropriate status codes --- backend/app/adapter/routing/handle/search.go | 37 +++++++++++++++++--- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/backend/app/adapter/routing/handle/search.go b/backend/app/adapter/routing/handle/search.go index d4cb68532..3dde39354 100644 --- a/backend/app/adapter/routing/handle/search.go +++ b/backend/app/adapter/routing/handle/search.go @@ -2,6 +2,7 @@ package handle import ( "encoding/json" + "errors" "io/ioutil" "net/http" "time" @@ -47,6 +48,11 @@ type SearchResponse struct { Users []User `json:"users,omitempty"` } +// SearchError represents an error with the Search API request. +type SearchError struct { + Message string `json:"message"` +} + // ShortLink represents the short_link field of Search API respond. type ShortLink struct { Alias string `json:"alias,omitempty"` @@ -79,7 +85,7 @@ func Search( defer r.Body.Close() if err != nil { i.SearchFailed(err) - http.Error(w, err.Error(), http.StatusInternalServerError) + emitSearchError(w, err) return } @@ -87,7 +93,7 @@ func Search( err = json.Unmarshal(buf, &body) if err != nil { i.SearchFailed(err) - http.Error(w, err.Error(), http.StatusInternalServerError) + emitSearchError(w, err) return } @@ -99,14 +105,14 @@ func Search( filter, err := search.NewFilter(body.Filter.MaxResults, body.Filter.Resources, body.Filter.Orders) if err != nil { i.SearchFailed(err) - http.Error(w, err.Error(), http.StatusInternalServerError) + emitSearchError(w, err) return } results, err := searcher.Search(query, filter) if err != nil { i.SearchFailed(err) - http.Error(w, err.Error(), http.StatusInternalServerError) + emitSearchError(w, err) return } @@ -114,7 +120,7 @@ func Search( respBody, err := json.Marshal(&response) if err != nil { i.SearchFailed(err) - http.Error(w, err.Error(), http.StatusInternalServerError) + emitSearchError(w, err) return } @@ -205,3 +211,24 @@ func newUser(user entity.User) User { UpdatedAt: user.UpdatedAt, } } + +func emitSearchError(w http.ResponseWriter, err error) { + var code = http.StatusInternalServerError + var ( + u search.ErrUserNotProvided + r search.ErrUnknownResource + ) + if errors.As(err, &u) { + code = http.StatusUnauthorized + } + if errors.As(err, &r) { + code = http.StatusNotFound + } + errResp, err := json.Marshal(SearchError{ + Message: err.Error(), + }) + if err != nil { + return + } + http.Error(w, string(errResp), code) +} From 39d4dfeeab2b90f500eaa8e26ad0b3ef7ca8415a Mon Sep 17 00:00:00 2001 From: Coteh <3276350+Coteh@users.noreply.github.com> Date: Fri, 21 Aug 2020 16:41:08 -0400 Subject: [PATCH 04/11] Add tests --- backend/app/usecase/search/search_test.go | 156 ++++++++++++---------- 1 file changed, 85 insertions(+), 71 deletions(-) diff --git a/backend/app/usecase/search/search_test.go b/backend/app/usecase/search/search_test.go index eae2d1d8e..cfa7b36c3 100644 --- a/backend/app/usecase/search/search_test.go +++ b/backend/app/usecase/search/search_test.go @@ -26,78 +26,22 @@ func TestSearch(t *testing.T) { relationUsers []entity.User relationShortLinks []entity.ShortLink expectedResult ResourceResult + // TODO(issue#803): Check error types in tests. + expHasErr bool }{ { - name: "search without user", - shortLinks: shortLinks{ - "git-google": entity.ShortLink{ - Alias: "git-google", - LongLink: "http://github.com/google", - }, - "google": entity.ShortLink{ - Alias: "google", - LongLink: "https://google.com", - }, - "short": entity.ShortLink{ - Alias: "short", - LongLink: "https://short-d.com", - }, - "facebook": entity.ShortLink{ - Alias: "facebook", - LongLink: "https://facebook.com", - }, - }, + name: "search without user", + shortLinks: shortLinks{}, Query: Query{ Query: "http google", }, - maxResults: 2, - resources: []Resource{ShortLink}, - orders: []order.By{order.ByCreatedTimeASC}, - relationUsers: []entity.User{ - { - ID: "alpha", - Email: "alpha@example.com", - }, - { - ID: "alpha", - Email: "alpha@example.com", - }, - { - ID: "beta", - Email: "beta@example.com", - }, - { - ID: "alpha", - Email: "alpha@example.com", - }, - { - ID: "alpha", - Email: "alpha@example.com", - }, - }, - relationShortLinks: []entity.ShortLink{ - { - Alias: "git-google", - LongLink: "http://github.com/google", - }, - { - Alias: "google", - LongLink: "https://google.com", - }, - { - Alias: "google", - LongLink: "https://google.com", - }, - { - Alias: "short", - LongLink: "https://short-d.com", - }, - { - Alias: "facebook", - LongLink: "https://facebook.com", - }, - }, - expectedResult: ResourceResult{}, + maxResults: 2, + resources: []Resource{ShortLink}, + orders: []order.By{order.ByCreatedTimeASC}, + relationUsers: []entity.User{}, + relationShortLinks: []entity.ShortLink{}, + expectedResult: ResourceResult{}, + expHasErr: true, }, { name: "search without query", @@ -348,8 +292,8 @@ func TestSearch(t *testing.T) { }, }, expectedResult: ResourceResult{ - ShortLinks: nil, - Users: nil, + //ShortLinks: nil, + //Users: nil, }, }, { @@ -464,8 +408,8 @@ func TestSearch(t *testing.T) { }, }, maxResults: 2, - resources: []Resource{ShortLink, User, Unknown}, - orders: []order.By{order.ByCreatedTimeASC, order.ByUnsorted, order.ByCreatedTimeASC}, + resources: []Resource{ShortLink, User}, + orders: []order.By{order.ByCreatedTimeASC, order.ByUnsorted}, relationUsers: []entity.User{ { ID: "alpha", @@ -520,6 +464,72 @@ func TestSearch(t *testing.T) { Users: nil, }, }, + { + name: "unknown resource query", + shortLinks: shortLinks{ + "short": entity.ShortLink{ + Alias: "short", + LongLink: "https://short-d.com", + }, + }, + Query: Query{ + Query: "short", + User: &entity.User{ + ID: "alpha", + Email: "alpha@example.com", + }, + }, + maxResults: 1, + resources: []Resource{Unknown}, + orders: []order.By{order.ByCreatedTimeASC}, + relationUsers: []entity.User{ + { + ID: "alpha", + Email: "alpha@example.com", + }, + }, + relationShortLinks: []entity.ShortLink{ + { + Alias: "short", + LongLink: "https://short-d.com", + }, + }, + expectedResult: ResourceResult{}, + expHasErr: true, + }, + { + name: "both known and unknown resource queries", + shortLinks: shortLinks{ + "short": entity.ShortLink{ + Alias: "short", + LongLink: "https://short-d.com", + }, + }, + Query: Query{ + Query: "short", + User: &entity.User{ + ID: "alpha", + Email: "alpha@example.com", + }, + }, + maxResults: 1, + resources: []Resource{ShortLink, Unknown}, + orders: []order.By{order.ByCreatedTimeASC, order.ByUnsorted}, + relationUsers: []entity.User{ + { + ID: "alpha", + Email: "alpha@example.com", + }, + }, + relationShortLinks: []entity.ShortLink{ + { + Alias: "short", + LongLink: "https://short-d.com", + }, + }, + expectedResult: ResourceResult{}, + expHasErr: true, + }, } for _, testCase := range testCases { @@ -540,6 +550,10 @@ func TestSearch(t *testing.T) { assert.Equal(t, nil, err) result, err := search.Search(testCase.Query, filter) + if testCase.expHasErr { + assert.NotEqual(t, nil, err) + return + } assert.Equal(t, nil, err) assert.Equal(t, testCase.expectedResult, result) From b2050c25f9b3ee0a0b7273dbafe6da86fe5e4d48 Mon Sep 17 00:00:00 2001 From: Coteh <3276350+Coteh@users.noreply.github.com> Date: Fri, 21 Aug 2020 16:45:46 -0400 Subject: [PATCH 05/11] Improve search without user test --- backend/app/usecase/search/search_test.go | 33 ++++++++++++++++------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/backend/app/usecase/search/search_test.go b/backend/app/usecase/search/search_test.go index cfa7b36c3..abfad6c03 100644 --- a/backend/app/usecase/search/search_test.go +++ b/backend/app/usecase/search/search_test.go @@ -30,18 +30,33 @@ func TestSearch(t *testing.T) { expHasErr bool }{ { - name: "search without user", - shortLinks: shortLinks{}, + name: "search without user", + shortLinks: shortLinks{ + "short": entity.ShortLink{ + Alias: "short", + LongLink: "https://short-d.com", + }, + }, Query: Query{ Query: "http google", }, - maxResults: 2, - resources: []Resource{ShortLink}, - orders: []order.By{order.ByCreatedTimeASC}, - relationUsers: []entity.User{}, - relationShortLinks: []entity.ShortLink{}, - expectedResult: ResourceResult{}, - expHasErr: true, + maxResults: 1, + resources: []Resource{ShortLink}, + orders: []order.By{order.ByCreatedTimeASC}, + relationUsers: []entity.User{ + { + ID: "alpha", + Email: "alpha@example.com", + }, + }, + relationShortLinks: []entity.ShortLink{ + { + Alias: "short", + LongLink: "https://short-d.com", + }, + }, + expectedResult: ResourceResult{}, + expHasErr: true, }, { name: "search without query", From 0ed8474fa4813ee6d3bc765f6c460bc9391e86dc Mon Sep 17 00:00:00 2001 From: James Cote <3276350+Coteh@users.noreply.github.com> Date: Sat, 22 Aug 2020 16:42:34 -0400 Subject: [PATCH 06/11] Apply suggestions from code review Co-authored-by: Arber Avdullahu --- backend/app/adapter/routing/handle/search.go | 2 +- backend/app/usecase/search/search.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/backend/app/adapter/routing/handle/search.go b/backend/app/adapter/routing/handle/search.go index 3dde39354..ae34129d8 100644 --- a/backend/app/adapter/routing/handle/search.go +++ b/backend/app/adapter/routing/handle/search.go @@ -213,8 +213,8 @@ func newUser(user entity.User) User { } func emitSearchError(w http.ResponseWriter, err error) { - var code = http.StatusInternalServerError var ( + code http.StatusInternalServerError u search.ErrUserNotProvided r search.ErrUnknownResource ) diff --git a/backend/app/usecase/search/search.go b/backend/app/usecase/search/search.go index 800fe6024..4b6374588 100644 --- a/backend/app/usecase/search/search.go +++ b/backend/app/usecase/search/search.go @@ -39,10 +39,10 @@ func (e ErrUnknownResource) Error() string { } // ErrUserNotProvided represents user not provided for search query. -type ErrUserNotProvided string +type ErrUserNotProvided struct{} func (e ErrUserNotProvided) Error() string { - return string(e) + return "user not provided" } // Search finds resources based on specified criteria. From 3c74029a03844e43171555d5ed98744f8063721d Mon Sep 17 00:00:00 2001 From: James Cote <3276350+Coteh@users.noreply.github.com> Date: Sat, 22 Aug 2020 16:43:32 -0400 Subject: [PATCH 07/11] Apply suggestions from code review --- backend/app/usecase/search/search.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/backend/app/usecase/search/search.go b/backend/app/usecase/search/search.go index 4b6374588..f17700999 100644 --- a/backend/app/usecase/search/search.go +++ b/backend/app/usecase/search/search.go @@ -32,10 +32,10 @@ type ResourceResult struct { } // ErrUnknownResource represents unknown search resource error. -type ErrUnknownResource string +type ErrUnknownResource struct{} func (e ErrUnknownResource) Error() string { - return string(e) + return "unknown resource" } // ErrUserNotProvided represents user not provided for search query. @@ -97,16 +97,14 @@ func (s Search) searchResource(resource Resource, orderBy order.Order, query Que case User: return s.searchUser(query, orderBy, filter) default: - // TODO add tests for unknown resource - return ResourceResult{}, ErrUnknownResource("unknown resource") + return ResourceResult{}, ErrUnknownResource{} } } // TODO(issue#866): Simplify searchShortLink function func (s Search) searchShortLink(query Query, orderBy order.Order, filter Filter) (ResourceResult, error) { if query.User == nil { - // TODO add a test for user not provided - err := ErrUserNotProvided("user not provided") + err := ErrUserNotProvided{} s.logger.Error(err) return ResourceResult{}, err } From 072ffd2b504c3a7e693ad090fb51a512616a4d77 Mon Sep 17 00:00:00 2001 From: Coteh <3276350+Coteh@users.noreply.github.com> Date: Mon, 24 Aug 2020 20:26:04 -0400 Subject: [PATCH 08/11] Fix typo with var declaration --- backend/app/adapter/routing/handle/search.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/backend/app/adapter/routing/handle/search.go b/backend/app/adapter/routing/handle/search.go index ae34129d8..e85db4ba0 100644 --- a/backend/app/adapter/routing/handle/search.go +++ b/backend/app/adapter/routing/handle/search.go @@ -214,9 +214,9 @@ func newUser(user entity.User) User { func emitSearchError(w http.ResponseWriter, err error) { var ( - code http.StatusInternalServerError - u search.ErrUserNotProvided - r search.ErrUnknownResource + code = http.StatusInternalServerError + u search.ErrUserNotProvided + r search.ErrUnknownResource ) if errors.As(err, &u) { code = http.StatusUnauthorized From 09599a10badafe6e3f0458dee120526e1daf720e Mon Sep 17 00:00:00 2001 From: Coteh <3276350+Coteh@users.noreply.github.com> Date: Sun, 13 Sep 2020 13:32:39 -0400 Subject: [PATCH 09/11] Add error channel for sending search errors - Also remove extra logging statement in searchShortLink --- backend/app/adapter/routing/handle/search.go | 2 +- backend/app/usecase/search/search.go | 57 +++++++++----------- 2 files changed, 26 insertions(+), 33 deletions(-) diff --git a/backend/app/adapter/routing/handle/search.go b/backend/app/adapter/routing/handle/search.go index e85db4ba0..975564831 100644 --- a/backend/app/adapter/routing/handle/search.go +++ b/backend/app/adapter/routing/handle/search.go @@ -174,7 +174,7 @@ func (f *Filter) resourcesString() []string { return resources } -func newSearchResponse(result search.ResourceResult) SearchResponse { +func newSearchResponse(result search.Result) SearchResponse { shortLinks := make([]ShortLink, len(result.ShortLinks)) for i := 0; i < len(result.ShortLinks); i++ { shortLinks[i] = newShortLink(result.ShortLinks[i]) diff --git a/backend/app/usecase/search/search.go b/backend/app/usecase/search/search.go index f17700999..ae5c3fe44 100644 --- a/backend/app/usecase/search/search.go +++ b/backend/app/usecase/search/search.go @@ -21,12 +21,6 @@ type Search struct { // Result represents the result of a search query. type Result struct { - Resources ResourceResult - Err error -} - -// ResourceResult represents the resources obtained from a search query. -type ResourceResult struct { ShortLinks []entity.ShortLink Users []entity.User } @@ -46,9 +40,11 @@ func (e ErrUserNotProvided) Error() string { } // Search finds resources based on specified criteria. -func (s Search) Search(query Query, filter Filter) (ResourceResult, error) { +func (s Search) Search(query Query, filter Filter) (Result, error) { resultCh := make(chan Result) + errCh := make(chan error) defer close(resultCh) + defer close(errCh) orders := toOrders(filter.orders) @@ -58,60 +54,57 @@ func (s Search) Search(query Query, filter Filter) (ResourceResult, error) { result, err := s.searchResource(filter.resources[i], orders[i], query, filter) if err != nil { s.logger.Error(err) - resultCh <- Result{ - Resources: ResourceResult{}, - Err: err, - } + resultCh <- Result{} + errCh <- err return } - resultCh <- Result{ - Resources: result, - Err: nil, - } + resultCh <- result + errCh <- nil }() } timeout := time.After(s.timeout) - var results []ResourceResult + var results []Result var resultErr error for i := 0; i < len(filter.resources); i++ { select { case result := <-resultCh: + results = append(results, result) + case <-timeout: + return mergeResults(results), nil + } + select { + case err := <-errCh: // Only return the first error encountered if resultErr == nil { - resultErr = result.Err + resultErr = err } - results = append(results, result.Resources) - case <-timeout: - return mergeResults(results), nil } } return mergeResults(results), resultErr } -func (s Search) searchResource(resource Resource, orderBy order.Order, query Query, filter Filter) (ResourceResult, error) { +func (s Search) searchResource(resource Resource, orderBy order.Order, query Query, filter Filter) (Result, error) { switch resource { case ShortLink: return s.searchShortLink(query, orderBy, filter) case User: return s.searchUser(query, orderBy, filter) default: - return ResourceResult{}, ErrUnknownResource{} + return Result{}, ErrUnknownResource{} } } // TODO(issue#866): Simplify searchShortLink function -func (s Search) searchShortLink(query Query, orderBy order.Order, filter Filter) (ResourceResult, error) { +func (s Search) searchShortLink(query Query, orderBy order.Order, filter Filter) (Result, error) { if query.User == nil { - err := ErrUserNotProvided{} - s.logger.Error(err) - return ResourceResult{}, err + return Result{}, ErrUserNotProvided{} } shortLinks, err := s.getShortLinkByUser(*query.User) if err != nil { - return ResourceResult{}, err + return Result{}, err } var matchedAliasByAll, matchedAliasByAny, matchedLongLinkByAll, matchedLongLinkByAny []entity.ShortLink @@ -145,14 +138,14 @@ func (s Search) searchShortLink(query Query, orderBy order.Order, filter Filter) filteredShortLinks := filterShortLinks(mergedShortLinks, filter) - return ResourceResult{ + return Result{ ShortLinks: filteredShortLinks, Users: nil, }, nil } -func (s Search) searchUser(query Query, orderBy order.Order, filter Filter) (ResourceResult, error) { - return ResourceResult{}, nil +func (s Search) searchUser(query Query, orderBy order.Order, filter Filter) (Result, error) { + return Result{}, nil } func (s Search) getShortLinkByUser(user entity.User) ([]entity.ShortLink, error) { @@ -201,8 +194,8 @@ func toOrders(ordersBy []order.By) []order.Order { return orders } -func mergeResults(results []ResourceResult) ResourceResult { - var mergedResult ResourceResult +func mergeResults(results []Result) Result { + var mergedResult Result for _, result := range results { mergedResult.ShortLinks = append(mergedResult.ShortLinks, result.ShortLinks...) From b3df32915bfb1c4184d06526aeb614e4ca067b4e Mon Sep 17 00:00:00 2001 From: Coteh <3276350+Coteh@users.noreply.github.com> Date: Sun, 13 Sep 2020 13:37:26 -0400 Subject: [PATCH 10/11] Add documentation for search errors --- backend/app/adapter/routing/api.yml | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/backend/app/adapter/routing/api.yml b/backend/app/adapter/routing/api.yml index 359562ea4..f5946753e 100644 --- a/backend/app/adapter/routing/api.yml +++ b/backend/app/adapter/routing/api.yml @@ -117,6 +117,28 @@ paths: type: array items: $ref: '#/components/schemas/User' + '401': + description: unauthorized user + content: + application/json: + schema: + type: object + required: + - message + properties: + message: + type: string + '404': + description: unknown resource + content: + application/json: + schema: + type: object + required: + - message + properties: + message: + type: string security: - web_api: [] /oauth/github/sign-in: From 0073d1fae1acafb3641df4fafacaf359cc3eef9f Mon Sep 17 00:00:00 2001 From: Coteh <3276350+Coteh@users.noreply.github.com> Date: Sun, 13 Sep 2020 13:50:55 -0400 Subject: [PATCH 11/11] Fix search test --- backend/app/usecase/search/search_test.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/backend/app/usecase/search/search_test.go b/backend/app/usecase/search/search_test.go index abfad6c03..be4d0c290 100644 --- a/backend/app/usecase/search/search_test.go +++ b/backend/app/usecase/search/search_test.go @@ -25,7 +25,7 @@ func TestSearch(t *testing.T) { orders []order.By relationUsers []entity.User relationShortLinks []entity.ShortLink - expectedResult ResourceResult + expectedResult Result // TODO(issue#803): Check error types in tests. expHasErr bool }{ @@ -55,7 +55,7 @@ func TestSearch(t *testing.T) { LongLink: "https://short-d.com", }, }, - expectedResult: ResourceResult{}, + expectedResult: Result{}, expHasErr: true, }, { @@ -131,7 +131,7 @@ func TestSearch(t *testing.T) { LongLink: "https://facebook.com", }, }, - expectedResult: ResourceResult{ + expectedResult: Result{ ShortLinks: []entity.ShortLink{ { Alias: "facebook", @@ -218,7 +218,7 @@ func TestSearch(t *testing.T) { LongLink: "https://facebook.com", }, }, - expectedResult: ResourceResult{ + expectedResult: Result{ ShortLinks: []entity.ShortLink{ { Alias: "google", @@ -306,7 +306,7 @@ func TestSearch(t *testing.T) { LongLink: "https://facebook.com", }, }, - expectedResult: ResourceResult{ + expectedResult: Result{ //ShortLinks: nil, //Users: nil, }, @@ -385,7 +385,7 @@ func TestSearch(t *testing.T) { LongLink: "https://facebook.com", }, }, - expectedResult: ResourceResult{ + expectedResult: Result{ ShortLinks: []entity.ShortLink{ { Alias: "short", @@ -469,7 +469,7 @@ func TestSearch(t *testing.T) { LongLink: "https://facebook.com", }, }, - expectedResult: ResourceResult{ + expectedResult: Result{ ShortLinks: []entity.ShortLink{ { Alias: "short", @@ -509,7 +509,7 @@ func TestSearch(t *testing.T) { LongLink: "https://short-d.com", }, }, - expectedResult: ResourceResult{}, + expectedResult: Result{}, expHasErr: true, }, { @@ -542,7 +542,7 @@ func TestSearch(t *testing.T) { LongLink: "https://short-d.com", }, }, - expectedResult: ResourceResult{}, + expectedResult: Result{}, expHasErr: true, }, }