Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Errors returned from server should be lowered cased if passed to application #1121

Closed
derekcollison opened this issue Nov 3, 2022 · 3 comments · Fixed by #1125
Closed

Errors returned from server should be lowered cased if passed to application #1121

derekcollison opened this issue Nov 3, 2022 · 3 comments · Fixed by #1125
Assignees
Labels
feature New feature request

Comments

@derekcollison
Copy link
Member

I encountered this with invalidating pull requests. The header description is properly title case, but the Go client should lower case it if it passes the string up to the application.

@derekcollison derekcollison added the feature New feature request label Nov 3, 2022
@piotrpio
Copy link
Collaborator

piotrpio commented Nov 5, 2022

@derekcollison I am not sure if that is a good idea. 2 main reasons for that:

  1. It may break existing users (e.g. we depend on literal error messages in tests sometimes).
  2. It makes some error messages look off, e.g. Exceeded MaxRequestExpires is changed to exceeded maxrequestexpires

Additionally, I'm not sure why is this beneficial - by convention errors in go should start with lowercase, but we always have the nats: prefix anyway.

@wallyqs
Copy link
Member

wallyqs commented Nov 5, 2022

Maybe using errors.Is instead of the error string verbatim is what would have helped here? The nats protocol error implements Is interface and does a ToLower to avoid depending on the case, and the JS APIErrors instead of the error string use the APIError code.

nats.go/nats.go

Lines 2335 to 2337 in 980f955

func (nerr *natsProtoErr) Is(err error) bool {
return strings.ToLower(nerr.Error()) == err.Error()
}

https://github.com/nats-io/nats.go/blob/main/jserrors.go#L143-L154

@piotrpio
Copy link
Collaborator

piotrpio commented Nov 7, 2022

@wallyqs that is not a bad idea, but I'm not sure whether we want to create a new error type here, unless it's something applicable to other errors (basically the whole bunch of errors in nats.go). I think for this scenario it's sufficient to add a new error variable (it can be a JetStreamError I suppose) and return that to be able to easily compare.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants