You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by "ocket8888 (via GitHub)" <gi...@apache.org> on 2024/01/16 18:27:56 UTC
[PR] To/api errors [trafficcontrol]
ocket8888 opened a new pull request, #7914:
URL: https://github.com/apache/trafficcontrol/pull/7914
## What does this PR (Pull Request) do?
This PR obsoletes #5162. I've made it smaller by not actually doing anything with the new structure yet (the plan is to introduce it to `api.Wrap`-ed handling and then things currently using the CRUDer will adopt it as they are rewritten), which should also reduce/eliminate conflicts in the future.
This adds a new, interface to Traffic Ops's `api` package. The interface encapsulates a user-facing error, a system-only error, and an HTTP response code, like so:
```go
type Errors interface {
Code() int
SystemError() error
UserError() error
// Some other methods omitted
}
```
This is meant to replace the `(error, error, int)` and `(int, error, error)` call signatures common throughout TO code. As well as making the TO codebase more compliant with the convention that "error values should be last function return signatures" - which is followed throughout the Go standard library, but is not actually enforced within our project - this eliminates any confusion about which error in the return signature is meant for clients and which is only meant for logs. It also makes it harder to make mistakes regarding those associations by making the assignment and use of such errors more explicit.
If this were adopted, it would simplify things greatly. For comparison:
```go
// The old way
userErr, sysErr, errCode := someFunction(inf)
if userErr != nil || sysErr != nil {
api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
return
}
// The new way (assuming in an api.Wrap-ed handler)
errs := someFunction(inf)
if errs != nil {
return errs
}
```
### Changes from #5162
Taking feedback into account, the new Errors type PR incorporates the following changes from the most recent draft of that PR
- `api.Errors` implements `error` and is an interface - and therefore "`nil`-able". This means that instead of checking e.g. `errs.Occurred()`, you can check if the returned value is `nil`.
- Rather than being directly constructed in a very verbose way that would encourage early initialization and setting fields later, helper functions have been added that should allow for simpler error construction in a single line (e.g. `return api.NewSystemErrorf("scanning CDNs: %w", err)`.
- Adds two files instead of changing a total of 153.
- Has 100% test coverage for the added lines.
## Which Traffic Control components are affected by this PR?
- Traffic Ops
## What is the best way to verify this PR?
Ensure the provided tests pass.
## The following criteria are ALL met by this PR
- [x] This PR includes tests
- [x] This PR includes documentation in the form of GoDocs - nothing external has changed, so no other documentation is necessary
- [x] An update to CHANGELOG.md is not necessary
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY**
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] To/api errors [trafficcontrol]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #7914:
URL: https://github.com/apache/trafficcontrol/pull/7914#issuecomment-1894299100
## [Codecov](https://app.codecov.io/gh/apache/trafficcontrol/pull/7914?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`adea91d`)](https://app.codecov.io/gh/apache/trafficcontrol/commit/adea91d814bb2d3ff04b167f9d79ecfc0dc9fbd6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 28.84% compared to head [(`521f8a7`)](https://app.codecov.io/gh/apache/trafficcontrol/pull/7914?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 38.98%.
> Report is 3 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #7914 +/- ##
=============================================
+ Coverage 28.84% 38.98% +10.13%
Complexity 98 98
=============================================
Files 602 424 -178
Lines 77445 32986 -44459
Branches 90 90
=============================================
- Hits 22342 12858 -9484
+ Misses 53012 18963 -34049
+ Partials 2091 1165 -926
```
| [Flag](https://app.codecov.io/gh/apache/trafficcontrol/pull/7914/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [golib_unit](https://app.codecov.io/gh/apache/trafficcontrol/pull/7914/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.84% <ø> (-0.01%)` | :arrow_down: |
| [grove_unit](https://app.codecov.io/gh/apache/trafficcontrol/pull/7914/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `12.02% <ø> (ø)` | |
| [t3c_unit](https://app.codecov.io/gh/apache/trafficcontrol/pull/7914/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `5.88% <ø> (+0.11%)` | :arrow_up: |
| [traffic_monitor_unit](https://app.codecov.io/gh/apache/trafficcontrol/pull/7914/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `25.47% <ø> (ø)` | |
| [traffic_ops_unit](https://app.codecov.io/gh/apache/trafficcontrol/pull/7914/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [traffic_stats_unit](https://app.codecov.io/gh/apache/trafficcontrol/pull/7914/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit_tests](https://app.codecov.io/gh/apache/trafficcontrol/pull/7914/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.09% <ø> (+7.42%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/trafficcontrol/pull/7914?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] To/api errors [trafficcontrol]
Posted by "zrhoffman (via GitHub)" <gi...@apache.org>.
zrhoffman merged PR #7914:
URL: https://github.com/apache/trafficcontrol/pull/7914
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org