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: diff --git a/backend/app/adapter/routing/handle/search.go b/backend/app/adapter/routing/handle/search.go index 5b49abc66..975564831 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 + 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) +} 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 d5c760221..ae5c3fe44 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" @@ -26,10 +25,26 @@ type Result struct { Users []entity.User } +// ErrUnknownResource represents unknown search resource error. +type ErrUnknownResource struct{} + +func (e ErrUnknownResource) Error() string { + return "unknown resource" +} + +// ErrUserNotProvided represents user not provided for search query. +type ErrUserNotProvided struct{} + +func (e ErrUserNotProvided) Error() string { + return "user not provided" +} + // Search finds resources based on specified criteria. 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) @@ -38,17 +53,19 @@ 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{} + errCh <- err return } resultCh <- result + errCh <- nil }() } timeout := time.After(s.timeout) var results []Result + var resultErr error for i := 0; i < len(filter.resources); i++ { select { case result := <-resultCh: @@ -56,9 +73,16 @@ func (s Search) Search(query Query, filter Filter) (Result, error) { case <-timeout: return mergeResults(results), nil } + select { + case err := <-errCh: + // Only return the first error encountered + if resultErr == nil { + resultErr = err + } + } } - return mergeResults(results), nil + return mergeResults(results), resultErr } func (s Search) searchResource(resource Resource, orderBy order.Order, query Query, filter Filter) (Result, error) { @@ -68,15 +92,14 @@ func (s Search) searchResource(resource Resource, orderBy order.Order, query Que case User: return s.searchUser(query, orderBy, filter) default: - return Result{}, errors.New("unknown resource") + return Result{}, ErrUnknownResource{} } } // TODO(issue#866): Simplify searchShortLink function func (s Search) searchShortLink(query Query, orderBy order.Order, filter Filter) (Result, error) { if query.User == nil { - s.logger.Error(errors.New("user not provided")) - return Result{}, nil + return Result{}, ErrUserNotProvided{} } shortLinks, err := s.getShortLinkByUser(*query.User) diff --git a/backend/app/usecase/search/search_test.go b/backend/app/usecase/search/search_test.go index 96e9a33c9..be4d0c290 100644 --- a/backend/app/usecase/search/search_test.go +++ b/backend/app/usecase/search/search_test.go @@ -26,31 +26,21 @@ func TestSearch(t *testing.T) { relationUsers []entity.User relationShortLinks []entity.ShortLink expectedResult Result + // 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", - }, }, Query: Query{ Query: "http google", }, - maxResults: 2, + maxResults: 1, resources: []Resource{ShortLink}, orders: []order.By{order.ByCreatedTimeASC}, relationUsers: []entity.User{ @@ -58,46 +48,15 @@ func TestSearch(t *testing.T) { 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: Result{}, + expHasErr: true, }, { name: "search without query", @@ -348,8 +307,8 @@ func TestSearch(t *testing.T) { }, }, expectedResult: Result{ - ShortLinks: nil, - Users: nil, + //ShortLinks: nil, + //Users: nil, }, }, { @@ -464,8 +423,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 +479,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: Result{}, + 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: Result{}, + expHasErr: true, + }, } for _, testCase := range testCases { @@ -540,6 +565,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)