You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2020/10/16 18:19:47 UTC
[GitHub] [trafficcontrol] ocket8888 opened a new pull request #5162: To/api errors
ocket8888 opened a new pull request #5162:
URL: https://github.com/apache/trafficcontrol/pull/5162
## What does this PR (Pull Request) do?
- [x] This PR is not related to any Issue
This adds a new, fairly small package to Traffic Ops: `apierrors`. It exposes a structure for containing a user-facing error, a system-only error, and an HTTP response code, like so:
```go
type Errors struct {
Code int
SystemError error
UserError error
}
```
This is meant to replace the `(error, error, int)` and `(int, error, error)` call signatures common throughout TO code. In fact, this PR changes such signatures to unify them all to return just that `Errors` structure instead. 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.
It also adds helper functions similar to those that already exist for the old, "exploded" format of passing in all three bits of information separately, which allows one to change comparatively complex error handling into something much simpler:
```go
// The old way
inf, userErr, sysErr, errCode := api.NewInfo(r, nil, nil)
if userErr != nil || sysErr != nil {
api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
}
// The new way
inf, errs := api.NewInfo(r, nil, nil)
if errs.Occurred() {
inf.HandleErrs(w, r, errs)
return
}
```
## Which Traffic Control components are affected by this PR?
- Traffic Ops
## What is the best way to verify this PR?
All existing API and unit tests should pass. This PR adds a few tests related to the new `Errors` structure, as well.
## 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 for refactors
- [x] This PR includes any and all required license headers
- [x] This PR does not include a database migration
- [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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c edited a comment on pull request #5162: Traffic Ops API Errors structure
Posted by GitBox <gi...@apache.org>.
rob05c edited a comment on pull request #5162:
URL: https://github.com/apache/trafficcontrol/pull/5162#issuecomment-712269928
> Are you sure? The Go standard library always returns errors last in multi-value returns
Sure. As long as it's consistent, I don't have a strong opinion. I like the code last, but you're right, the error being last is more "idiomatic Go."
> No - they do need to both be checked, but I don't need to do it.
That's my concern: that nobody will do it, i.e. the `error` will get passed to something that just logs or returns it without doing the type switch, which is always a bug. Likewise, that something will return an `error` that isn't an `APIError` which fails the type switch, also having no correct answer and resulting in a bug.
> So call the interface APIError consisting of String() string and StatusCode() int. What's important is that we have a single interface, with two concrete implementations (SysErr and UserErr)
To be clear, I'm not opposed to a single return type that implements other interfaces, e.g. `APIErr`, or `SysErr` and `UserErr`. I just think if that type happens to implement `error` it'll be prone to bugs.
Anyway, I changed my mind, changing my vote:
`+0` on any single return that doesn't implement error. I'm not convinced it's better, but I'm open to seeing what it looks like.
`-0` on any single return that implements error. I still think it's dangerous and prone to bugs, but if everyone else agrees with it, I'm not going to be a binding vote against.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c edited a comment on pull request #5162: Traffic Ops API Errors structure
Posted by GitBox <gi...@apache.org>.
rob05c edited a comment on pull request #5162:
URL: https://github.com/apache/trafficcontrol/pull/5162#issuecomment-712269928
> Are you sure? The Go standard library always returns errors last in multi-value returns
Sure. As long as it's consistent, I don't have a strong opinion. I like the code last, but you're right, the error being last is more "idiomatic Go."
> No - they do need to both be checked, but I don't need to do it.
That's my concern: that nobody will do it, i.e. the `error` will get passed to something that just logs or returns it without doing the type switch, which is always a bug. Likewise, that something will return an `error` that isn't an `APIError` which fails the type switch, also having no correct answer and resulting in a bug.
> So call the interface APIError consisting of String() string and StatusCode() int. What's important is that we have a single interface, with two concrete implementations (SysErr and UserErr)
To be clear, I'm not opposed to a single return type that implements other interfaces, e.g. `APIErr`, or `SysErr` and `UserErr`. I just think if that type happens to implement `error` it'll be prone to bugs.
Anyway, I changed my mind, changing my vote:
`+0` on any single return that doesn't implement `error`. I'm not convinced it's better, but I'm open to seeing what it looks like.
`-0` on any single return that implements `error`. I still think it's dangerous and prone to bugs, but if everyone else agrees with it, I'm not going to be a binding vote against.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c removed a comment on pull request #5162: Traffic Ops API Errors structure
Posted by GitBox <gi...@apache.org>.
rob05c removed a comment on pull request #5162:
URL: https://github.com/apache/trafficcontrol/pull/5162#issuecomment-710558680
Also FWIW, the `userErr, sysErr, errCode`, `if userErr != nil || sysErr != nil { api.HandleErr(userErr, sysErr, errCode)`, etc was born out of pragmatism. We tried a bunch of things, and that seemed to be the one that was the easiest to work with and generally safe enough.
That doesn't mean there isn't something better out there. Just saying, it did come from pragmatism, not some grand theory edict 🤷
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on pull request #5162: Traffic Ops API Errors structure
Posted by GitBox <gi...@apache.org>.
rob05c commented on pull request #5162:
URL: https://github.com/apache/trafficcontrol/pull/5162#issuecomment-712269928
> Are you sure? The Go standard library always returns errors last in multi-value returns
Sure. As long as it's consistent, I don't have a strong opinion. I like the code last, but you're right, the error being last is more "idiomatic Go."
> No - they do need to both be checked, but I don't need to do it.
That's my concern: that nobody will do it, i.e. the `error` will get passed to something that just logs or returns it without doing the type switch, which is always a bug. Likewise, that something will return an `error` that isn't an `APIError` which fails the type switch, also having no correct answer and resulting in a bug.
> So call the interface APIError consisting of String() string and StatusCode() int. What's important is that we have a single interface, with two concrete implementations (SysErr and UserErr)
To be clear, I'm not opposed to a single return type that implements other interfaces, e.g. `APIErr`, or `SysErr` and `UserErr`. I just think if that type happens to implement `error` it'll be prone to bugs.
Anyway, I changed my mind, changing my vote:
`+0` on any symbol Errs that doesn't implement error. I'm not convinced it's better, but I'm open to seeing what it looks like.
`-0` on any single return that implements error. I still think it's dangerous and prone to bugs, but if everyone else agrees with it, I'm not going to be a binding vote against.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on pull request #5162: Traffic Ops API Errors structure
Posted by GitBox <gi...@apache.org>.
rob05c commented on pull request #5162:
URL: https://github.com/apache/trafficcontrol/pull/5162#issuecomment-710554447
Personally, I don't like either a custom non-`error` struct, or a struct that constains both and implements `error`.
The API really does need to do completely different things for user vs system errors.
A single `error` interface seems more idiomatic-Go. But it's not really very useful, if at all. Because you always need to check and handle both cases. And if you ever don't, it's very easy to accidentally return a system error to a user, or to log and not return a user error. You always need to check. It's dangerous and bug-prone to hide that.
Moreover, if the common helper funcs just return an `error`, what does a helper func that handles it do when it gets an `error` that isn't a `UserAndSystemError`? Does it log? Or return to the user? Any answer is also dangerous and bug-prone.
> any confusion about which error in the return signature is meant for clients and which is only meant for logs
Thinking about it some more, I guess I'm not _that_ opposed to the `struct Errors`. I can see the argument that it makes accidental swaps harder. But I am opposed to that struct, or any single return, implementing `error`. I'm very worried that anything that actually takes advantage of that is a bug.
> the (error, error, int) and (int, error, error)
That was my mistake, and I apologize. IMO the idiom should always be "userErr, sysErr, errCode". The funcs that got it backward are my fault, and should be fixed.
>The apierrors.Errors struct holds data for system errors, user errors, and status codes, but we would only ever want to populate either SystemError or UserError, never both. We need a solution that only allows us to return one or the other, and the success status code should have nothing to do with the error cases.
You want a Sum Type. Go doesn't do Monads sanely. Alas. I don't have a strong opinion on whether any Errors type is `{UserErr, SystemErr}` or `{Err, ErrIsUser}`. It does seem conceivable that an error could be both user and system; but I don't think we've ever had that in practice.
I have no strong opinion on whether the return code is included in any Error object. I do wish it could conveniently strongly-typed, but all the standard library funcs take `int`, which is why it isn't. An `api.HTTPCode` would unfortunately make composing with the standard library painful.
TLDR
`-0` on `struct Errors` that doesn't implement `error`.
`-1` on any single "UserAndSystemErrAndOrCode" return that implements `error`.
Also, can I request we name any new symbols `*Err` not `*Error`? IMO the shorter name is more idiomatic Go, marginally easier to read and write, and there's no other English word it would ever be confused with.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on pull request #5162: Traffic Ops API Errors structure
Posted by GitBox <gi...@apache.org>.
rob05c commented on pull request #5162:
URL: https://github.com/apache/trafficcontrol/pull/5162#issuecomment-710558680
Also FWIW, the `userErr, sysErr, errCode`, `if userErr != nil || sysErr != nil { api.HandleErr(userErr, sysErr, errCode)`, etc was born out of pragmatism. We tried a bunch of things, and that seemed to be the one that was the easiest to work with and generally safe enough.
That doesn't mean there isn't something better out there. Just saying, it did come from pragmatism, not some grand theory edict 🤷
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 edited a comment on pull request #5162: Traffic Ops API Errors structure
Posted by GitBox <gi...@apache.org>.
ocket8888 edited a comment on pull request #5162:
URL: https://github.com/apache/trafficcontrol/pull/5162#issuecomment-710634613
> ... we would only ever want to populate either `SystemError` or `UserError`, never both
That sounds pretty reasonable, but there are multiple cases right now where both are explicitly populated.
> It smells funny to return a non-nil Errors struct in both the success and errors cases -- it goes against the Go idiom of "nil error means success". Instead of checking for nil, you have to call a special method on the return value
Well it's not actually an `error`, it just contains them. The `error`s it contains actually don't go against that idiom, and you can do the check yourself if you want; you don't "have" to call a special method on the return value. It just seemed easier and cleaner.
> I don't really like creating the Errors struct at the beginning of the method then just setting its fields before returning it. If possible, I think errors should be declared in the same line they're returned. If the entire function/method has access to the error struct, it can be modified in unexpected ways.
There are definitely many places where it could just return a struct literal. In most cases I don't really have a preference for one over the other, but there might be a few places where I'd want to try to make a case for modifying values.
To conclude: I don't really have a problem with reworking this to be a single `error` if people like that solution better. It does seem more elegant, but the problem is that would entail changing logic, because there are places that return both errors, and I don't think that should be done without examining those use-cases.
> A single error interface seems more idiomatic-Go. But it's not really very useful, if at all. Because you always need to check and handle both cases.
No - they do need to both be checked, but _I_ don't need to do it. That's what `api.HandleErrs` is for, it can all be checked in one place so I don't need to do a switch everywhere, or check two values every time. I really do think collapsing that would be an improvement.
> If the common helper funcs just return an error, what does a helper func that handles it do when it gets an error that isn't a UserAndSystemError?
That would ideally impossible. I think, instead of using types to represent the concept as Rawlin suggested - because you're right, Go doesn't do those complex types very well - we could instead store that information in the structure itself:
```go
type Err struct {
RawError error
Code int
UserSafe bool
}
```
Then helpers just return that instead of `error`, so we maintain type-safety.
>That was my mistake, and I apologize. IMO the idiom should always be "userErr, sysErr, errCode". The funcs that got it backward are my fault, and should be fixed.
Are you sure? The Go standard library always returns errors last in multi-value returns, and go-lint's default ruleset would have a problem with all of our call signatures if they didn't conform to that convention. If we wind up not making any new structure at all, to be honest I'd prefer "errCode, userErr, sysErr".
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on pull request #5162: Traffic Ops API Errors structure
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5162:
URL: https://github.com/apache/trafficcontrol/pull/5162#issuecomment-710634613
> ... we would only ever want to populate either `SystemError` or `UserError`, never both
That sounds pretty reasonable, but there are multiple cases right now where both are explicitly populated.
> It smells funny to return a non-nil Errors struct in both the success and errors cases -- it goes against the Go idiom of "nil error means success". Instead of checking for nil, you have to call a special method on the return value
Well it's not actually an `error`, it just contains them. The `error`s it contains actually don't go against that idiom, and you can do the check yourself if you want; you don't "have" to call a special method on the return value. It just seemed easier and cleaner.
> I don't really like creating the Errors struct at the beginning of the method then just setting its fields before returning it. If possible, I think errors should be declared in the same line they're returned. If the entire function/method has access to the error struct, it can be modified in unexpected ways.
There are definitely many places where it could just return a struct literal. In most cases I don't really have a preference for one over the other, but there might be a few places where I'd want to try to make a case for modifying values.
To conclude: I don't really have a problem with reworking this to be a single `error` if people like that solution better. It does seem more elegant, but the problem is that would entail changing logic, because there are places that return both errors, and I don't think that should be done without examining those use-cases.
> A single error interface seems more idiomatic-Go. But it's not really very useful, if at all. Because you always need to check and handle both cases.
No - they do need to both be checked, but _I_ don't need to do it. That's what `api.HandleErrs` is for, it can all be checked in one place so I don't need to do a switch everywhere, or check two values every time. I really do think collapsing that would be an improvement.
> f the common helper funcs just return an error, what does a helper func that handles it do when it gets an error that isn't a UserAndSystemError
That would ideally impossible. I think, instead of using types to represent the concept as Rawlin suggested - because you're right, Go doesn't do those complex types very well - we could instead store that information in the structure itself:
```go
type Err struct {
RawError error
Code int
UserSafe bool
}
```
>That was my mistake, and I apologize. IMO the idiom should always be "userErr, sysErr, errCode". The funcs that got it backward are my fault, and should be fixed.
Are you sure? The Go standard library always returns errors last in multi-value returns, and go-lint's default ruleset would have a problem with all of our call signatures if they didn't conform to that convention.
Then helpers just return that instead of `error`, so we maintain type-safety.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 edited a comment on pull request #5162: Traffic Ops API Errors structure
Posted by GitBox <gi...@apache.org>.
ocket8888 edited a comment on pull request #5162:
URL: https://github.com/apache/trafficcontrol/pull/5162#issuecomment-710634613
> ... we would only ever want to populate either `SystemError` or `UserError`, never both
That sounds pretty reasonable, but there are multiple cases right now where both are explicitly populated.
> It smells funny to return a non-nil Errors struct in both the success and errors cases -- it goes against the Go idiom of "nil error means success". Instead of checking for nil, you have to call a special method on the return value
Well it's not actually an `error`, it just contains them. The `error`s it contains actually don't go against that idiom, and you can do the check yourself if you want; you don't "have" to call a special method on the return value. It just seemed easier and cleaner.
> I don't really like creating the Errors struct at the beginning of the method then just setting its fields before returning it. If possible, I think errors should be declared in the same line they're returned. If the entire function/method has access to the error struct, it can be modified in unexpected ways.
There are definitely many places where it could just return a struct literal. In most cases I don't really have a preference for one over the other, but there might be a few places where I'd want to try to make a case for modifying values.
To conclude: I don't really have a problem with reworking this to be a single `error` if people like that solution better. It does seem more elegant, but the problem is that would entail changing logic, because there are places that return both errors, and I don't think that should be done without examining those use-cases.
> A single error interface seems more idiomatic-Go. But it's not really very useful, if at all. Because you always need to check and handle both cases.
No - they do need to both be checked, but _I_ don't need to do it. That's what `api.HandleErrs` is for, it can all be checked in one place so I don't need to do a switch everywhere, or check two values every time. I really do think collapsing that would be an improvement.
> If the common helper funcs just return an error, what does a helper func that handles it do when it gets an error that isn't a UserAndSystemError
That would ideally impossible. I think, instead of using types to represent the concept as Rawlin suggested - because you're right, Go doesn't do those complex types very well - we could instead store that information in the structure itself:
```go
type Err struct {
RawError error
Code int
UserSafe bool
}
```
>That was my mistake, and I apologize. IMO the idiom should always be "userErr, sysErr, errCode". The funcs that got it backward are my fault, and should be fixed.
Are you sure? The Go standard library always returns errors last in multi-value returns, and go-lint's default ruleset would have a problem with all of our call signatures if they didn't conform to that convention.
Then helpers just return that instead of `error`, so we maintain type-safety.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on pull request #5162: Traffic Ops API Errors structure
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5162:
URL: https://github.com/apache/trafficcontrol/pull/5162#issuecomment-710741987
> Maybe context would only be added if it was a SysErr, in which case you could use a helper function that only adds context if it's a SysErr.
Yeah that sounds great. I was actually considering doing something similar to that in the current version by the time I was done seeing all the use-cases.
> ...are there any cases where we actually set both errors?
Yes. I think there's only two or three, but here's one: [lines 174 through 178 in login/register.go](https://github.com/apache/trafficcontrol/blob/master/traffic_ops/traffic_ops_golang/login/register.go#L174-L178)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman edited a comment on pull request #5162: Traffic Ops API Errors structure
Posted by GitBox <gi...@apache.org>.
zrhoffman edited a comment on pull request #5162:
URL: https://github.com/apache/trafficcontrol/pull/5162#issuecomment-710670282
> > > the (error, error, int) and (int, error, error)
> >
> > That was my mistake, and I apologize. IMO the idiom should always be "userErr, sysErr, errCode". The funcs that got it backward are my fault, and should be fixed.
>
> Are you sure? The Go standard library always returns errors last in multi-value returns, and go-lint's default ruleset would have a problem with all of our call signatures if they didn't conform to that convention. If we wind up not making any new structure at all, to be honest I'd prefer "errCode, userErr, sysErr".
As I [said in #5071](https://github.com/apache/trafficcontrol/pull/5071#discussion_r501175965), I think `(error, error, int)` should be exempt from that rule because an HTTP status code is effectively an error code.
Golint rules exist to guide developers for whom conventions would otherwise be arbitrary. We have a good reason not to do it that way, so we should do it our way in this case.
All we need to do for `golangci-lint` to ignore that particular `golint` rule is to add it to `issues.exclude-rules` in our `.golangci.yml`. We could also go a step further and enforce `userErr, sysErr, errCode` in our linting.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on pull request #5162: Traffic Ops API Errors structure
Posted by GitBox <gi...@apache.org>.
rawlinp commented on pull request #5162:
URL: https://github.com/apache/trafficcontrol/pull/5162#issuecomment-712222812
> Yes. I think there's only two or three, but here's one: lines 174 through 178 in login/register.go
Ah, interesting. I guess in that case it might make sense to just log sysErr immediately, then pass up the userErr. Since those cases seem to be very rare, maybe that would be alright.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c edited a comment on pull request #5162: Traffic Ops API Errors structure
Posted by GitBox <gi...@apache.org>.
rob05c edited a comment on pull request #5162:
URL: https://github.com/apache/trafficcontrol/pull/5162#issuecomment-710554447
Personally, I don't like either a custom non-`error` struct, or a struct that constains both and implements `error`.
The API really does need to do completely different things for user vs system errors.
A single `error` interface seems more idiomatic-Go. But it's not really very useful, if at all. Because you always need to check and handle both cases. And if you ever don't, it's very easy to accidentally return a system error to a user, or to log and not return a user error. You always need to check. It's dangerous and bug-prone to hide that.
Moreover, if the common helper funcs just return an `error`, what does a helper func that handles it do when it gets an `error` that isn't a `UserAndSystemError`? Does it log? Or return to the user? Any answer is also dangerous and bug-prone.
> any confusion about which error in the return signature is meant for clients and which is only meant for logs
Thinking about it some more, I guess I'm not _that_ opposed to the `struct Errors`. I can see the argument that it makes accidental swaps harder. But I am opposed to that struct, or any single return, implementing `error`. I'm very worried that anything that actually takes advantage of that is a bug.
> the (error, error, int) and (int, error, error)
That was my mistake, and I apologize. IMO the idiom should always be "userErr, sysErr, errCode". The funcs that got it backward are my fault, and should be fixed.
>The apierrors.Errors struct holds data for system errors, user errors, and status codes, but we would only ever want to populate either SystemError or UserError, never both. We need a solution that only allows us to return one or the other, and the success status code should have nothing to do with the error cases.
You want a Sum Type. Go doesn't do Monads sanely. Alas. I don't have a strong opinion on whether any Errors type is `{UserErr, SystemErr}` or `{Err, ErrIsUser}`. It does seem conceivable that an error could be both user and system; but I don't think we've ever had that in practice.
I have no strong opinion on whether the return code is included in any Error object. I do wish it could be conveniently strongly-typed, but all the standard library funcs take `int`, which is why it isn't. An `api.HTTPCode` would unfortunately make composing with the standard library painful.
TLDR
`-0` on `struct Errors` that doesn't implement `error`.
`-1` on any single "UserAndSystemErrAndOrCode" return that implements `error`.
Also, can I request we name any new symbols `*Err` not `*Error`? IMO the shorter name is more idiomatic Go, marginally easier to read and write, and there's no other English word it would ever be confused with.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on pull request #5162: Traffic Ops API Errors structure
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5162:
URL: https://github.com/apache/trafficcontrol/pull/5162#issuecomment-710670282
> > > the (error, error, int) and (int, error, error)
> >
> > That was my mistake, and I apologize. IMO the idiom should always be "userErr, sysErr, errCode". The funcs that got it backward are my fault, and should be fixed.
>
> Are you sure? The Go standard library always returns errors last in multi-value returns, and go-lint's default ruleset would have a problem with all of our call signatures if they didn't conform to that convention. If we wind up not making any new structure at all, to be honest I'd prefer "errCode, userErr, sysErr".
As I [said in #5071](https://github.com/apache/trafficcontrol/pull/5071#discussion_r501175965), I think `(error, error, int)` should be exempt from that rule because an HTTP status code is effectively an error code.
Golint rules exist to guide developers for whom conventions would otherwise be arbitrary. We have a good reason not to do it that way, so we should do it our way in this case.
All we need to do for `golangci-lint` to ignore that particular `golint` rule is to add it to `issues.exclude-rules` in our `.golangci.yml`. We could also go a step further and enforce `errCode, userErr, sysErr` in our linting.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on pull request #5162: Traffic Ops API Errors structure
Posted by GitBox <gi...@apache.org>.
rawlinp commented on pull request #5162:
URL: https://github.com/apache/trafficcontrol/pull/5162#issuecomment-710672368
>>A single error interface seems more idiomatic-Go. But it's not really very useful, if at all. Because you always need to check and handle both cases.
> No - they do need to both be checked, but I don't need to do it. That's what api.HandleErrs is for, it can all be checked in one place so I don't need to do a switch everywhere, or check two values every time. I really do think collapsing that would be an improvement.
I agree, very few places would actually need to type switch. Most code would simply be checking `err != nil`, optionally adding context, then passing it up the chain. Maybe context would only be added if it was a `SysErr`, in which case you could use a helper function that only adds context if it's a `SysErr`.
> It does seem more elegant, but the problem is that would entail changing logic, because there are places that return both errors, and I don't think that should be done without examining those use-cases.
I know there are places where we simply return both errors if either are non-nil, but are there any cases where we actually _set_ both errors? I'm trying to think of a reason why an error would be both a system and a user error, and I'm coming up empty.
> But I am opposed to that struct, or any single return, implementing error. I'm very worried that anything that actually takes advantage of that is a bug.
So call the interface `APIError` consisting of `String() string` and `StatusCode() int`. What's important is that we have a single interface, with two concrete implementations (`SysErr` and `UserErr`). All functions would return `APIError`. Once the error is declared, you can't mutate `statusCode`, and you can't accidentally cast it to a plain `error` either.
> If we wind up not making any new structure at all, to be honest I'd prefer "errCode, userErr, sysErr".
I can get on board with that, assuming we can't come to a consensus on this `APIError` interface. Typing the error code first helps you remember what kind of error type you're dealing with to type next, and it's pretty easy to remember "user error first, sys error second" after typing about 12 return statements like that.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org