Emit errors through Search API #1000
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1000 +/- ##
==========================================
+ Coverage 52.58% 52.62% +0.04%
==========================================
Files 142 142
Lines 3564 3576 +12
Branches 168 168
==========================================
+ Hits 1874 1882 +8
- Misses 1623 1627 +4
Partials 67 67
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| ShortLinks []entity.ShortLink | ||
| Users []entity.User | ||
| } | ||
|
|
||
| // ErrUnknownResource represents unknown search resource error. |
There was a problem hiding this comment.
Maybe we can move this new error.go file. Wdty?
There was a problem hiding this comment.
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.
Co-authored-by: Arber Avdullahu <[email protected]>
| ShortLinks []entity.ShortLink | ||
| Users []entity.User | ||
| } | ||
|
|
||
| // ErrUnknownResource represents unknown search resource error. |
There was a problem hiding this comment.
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.
|
Please hold this PR. We need to discuss what are the expected behaviors. |
| // SearchError represents an error with the Search API request. | ||
| type SearchError struct { | ||
| Message string `json:"message"` | ||
| } |
There was a problem hiding this comment.
I wonder what is this for? Are we going handle it in the frontend?
There was a problem hiding this comment.
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?
| 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) | ||
| } |
There was a problem hiding this comment.
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.
backend/app/usecase/search/search.go
Outdated
| // 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) { |
There was a problem hiding this comment.
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.
| }() | ||
| } | ||
|
|
||
| timeout := time.After(s.timeout) | ||
| var results []Result | ||
| var results []ResourceResult | ||
| var resultErr error |
There was a problem hiding this comment.
This is not needed based on the previous comment.
There was a problem hiding this comment.
I think this is still needed, because we would like to return only the first error that occurred. What do you think?
backend/app/usecase/search/search.go
Outdated
| 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{} |
There was a problem hiding this comment.
Let's revert this. Didn't seems to provide value here.
backend/app/usecase/search/search.go
Outdated
| s.logger.Error(errors.New("user not provided")) | ||
| return Result{}, nil | ||
| err := ErrUserNotProvided{} | ||
| s.logger.Error(err) |
There was a problem hiding this comment.
Let's directly return error here and log it at the caller.
- Also remove extra logging statement in searchShortLink
Fixes #865
Tests not added yet, so this will be in draft until they're added.
Emit errors through search API and add new types for unknown resource and user not provided.
TODO
two unknown resourcesedit: redundant, other two cover ithandle/search.go(edit: going to skip them, beyond the scope of this PR imo)