Skip to content
This repository was archived by the owner on Oct 17, 2020. It is now read-only.

Emit errors through Search API #1000

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Emit errors through Search API #1000

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
5f92509
Emit errors through Search API
Coteh Aug 21, 2020
3f99e4b
Run script formatter
Coteh Aug 21, 2020
54918ef
Emit JSON from HTTP error, and emit appropriate status codes
Coteh Aug 21, 2020
39d4dfe
Add tests
Coteh Aug 21, 2020
b2050c2
Improve search without user test
Coteh Aug 21, 2020
0ed8474
Apply suggestions from code review
Coteh Aug 22, 2020
3c74029
Apply suggestions from code review
Coteh Aug 22, 2020
072ffd2
Fix typo with var declaration
Coteh Aug 25, 2020
5c5f93d
Merge branch 'master' into search-error-handling
Coteh Aug 25, 2020
09599a1
Add error channel for sending search errors
Coteh Sep 13, 2020
b3df329
Add documentation for search errors
Coteh Sep 13, 2020
0073d1f
Fix search test
Coteh Sep 13, 2020
54a7071
Merge branch 'master' into search-error-handling
Coteh Sep 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions backend/app/adapter/routing/api.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
37 changes: 32 additions & 5 deletions backend/app/adapter/routing/handle/search.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package handle

import (
"encoding/json"
"errors"
"io/ioutil"
"net/http"
"time"
Expand Down Expand Up @@ -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"`
}
Comment on lines +51 to +54
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder what is this for? Are we going handle it in the frontend?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will provide information about the search error that occurred. So far, we have "user not provided" and "unknown resource" errors. I think it would make sense to emit these errors in the frontend, even though they may only show up in rare circumstances (e.g. user somehow gets logged out right before they search for something). What do you think?


// ShortLink represents the short_link field of Search API respond.
type ShortLink struct {
Alias string `json:"alias,omitempty"`
Expand Down Expand Up @@ -79,15 +85,15 @@ func Search(
defer r.Body.Close()
if err != nil {
i.SearchFailed(err)
http.Error(w, err.Error(), http.StatusInternalServerError)
emitSearchError(w, err)
return
}

var body SearchRequest
err = json.Unmarshal(buf, &body)
if err != nil {
i.SearchFailed(err)
http.Error(w, err.Error(), http.StatusInternalServerError)
emitSearchError(w, err)
return
}

Expand All @@ -99,22 +105,22 @@ 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
}

response := newSearchResponse(results)
respBody, err := json.Marshal(&response)
if err != nil {
i.SearchFailed(err)
http.Error(w, err.Error(), http.StatusInternalServerError)
emitSearchError(w, err)
return
}

Expand Down Expand Up @@ -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)
}
Comment on lines +215 to +234
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each search error is different in the code. I recommend putting the content of this function back to the code so that the reader can follow the error handling logic. This is an example when abstraction is reducing readability.

16 changes: 8 additions & 8 deletions backend/app/adapter/sqldb/short_link_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand Down
35 changes: 29 additions & 6 deletions backend/app/usecase/search/search.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package search

import (
"errors"
"strings"
"time"

Expand All @@ -26,10 +25,26 @@ type Result struct {
Users []entity.User
}

// ErrUnknownResource represents unknown search resource error.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can move this new error.go file. Wdty?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other usecase files, such as creator.go the error types are embedded within the file. I am not sure what the benefit of having it in a separate file would be, especially since there's only two custom error types at the moment.

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)

Expand All @@ -38,27 +53,36 @@ 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed based on the previous comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is still needed, because we would like to return only the first error that occurred. What do you think?

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 = err
}
}
}

return mergeResults(results), nil
return mergeResults(results), resultErr
}

func (s Search) searchResource(resource Resource, orderBy order.Order, query Query, filter Filter) (Result, error) {
Expand All @@ -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)
Expand Down
Loading