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 9 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
39 changes: 33 additions & 6 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 @@ -168,7 +174,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])
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
68 changes: 49 additions & 19 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 @@ -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.
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) {
func (s Search) Search(query Query, filter Filter) (ResourceResult, error) {
Copy link
Member

Choose a reason for hiding this comment

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

We are already returning error here. I wonder why are we putting extract error in ResourceResult. Go is making error an interface. By design, the language wants us to return whatever error we get directly.

resultCh := make(chan Result)
defer close(resultCh)

Expand All @@ -38,50 +57,61 @@ 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
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)
// 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")
return ResourceResult{}, ErrUnknownResource{}
Copy link
Member

Choose a reason for hiding this comment

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

Let's revert this. Didn't seems to provide value here.

}
}

// 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
err := ErrUserNotProvided{}
s.logger.Error(err)
Copy link
Member

Choose a reason for hiding this comment

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

Let's directly return error here and log it at the caller.

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
Expand Down Expand Up @@ -115,14 +145,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) {
Expand Down Expand Up @@ -171,8 +201,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...)
Expand Down
Loading