[IMPROVED] Add JetStream error codes, extract ErrConsumerNameAlreadyInUse #1044

piotrpio · 2022-08-11T16:05:05Z

Resolves #977

Additional changes in this PR:

Make *APIError implement error interface
Return JetStream error codes in error message
Verify error messages returned by server instead of parsing error messages as strings
Remove ErrInvalidDurable and checkDurName() in favor of ErrInvalidConsumerName and checkConsumerName()
Use ErrJetStreamNotEnabledForAccount error with error code when not enabled for an account instead of general ErrJetStreamNotEnabled

coveralls · 2022-08-11T16:09:51Z

Coverage decreased (-0.05%) to 85.55% when pulling 82272a0 on js-error-codes into fb5ca2c on main.

piotrpio · 2022-08-11T16:58:24Z

One thing I'm not certain about is removing ErrInvalidDurableName error, as this is a breaking change. Not sure what the impact would be, although I'm not a fan of having both ErrInvalidDurableName and ErrInvalidConsumerName` used interchangeably.

wallyqs

Nice work @piotrpio 👍, left some comments

wallyqs · 2022-08-11T16:16:18Z

js.go

-	return js.subscribe(subj, _EMPTY_, nil, mch, true, true, append(opts, Durable(durable)))
+	if durable != "" {
+		opts = append(opts, Durable(durable))
+	}


Does this change behavior to support the ephemerals?

No, pull ephemerals are already supported. This change is introduced because nats.Durable() should not accept empty consumer name (or else checkConsumerName() will fail)

wallyqs · 2022-08-11T16:27:21Z

jsm.go

+			return nil, err
+		}
+		if consInfo != nil {
+			return nil, fmt.Errorf("creating consumer %q on stream %q: %w", cfg.Durable, stream, ErrConsumerNameAlreadyInUse)


think need to add nats: prefix here? maybe not an error if the intended config is the same as the result of Info (idempotent) ?

Hmm, idempotency check is a good idea. As to nats: prefix, I'll just reverse the order of the message and put ErrConsumerNameAlreadyInUse at the beginning.

wallyqs · 2022-08-11T16:32:52Z

jsm.go

@@ -268,6 +285,30 @@ type consumerResponse struct {

 // AddConsumer will add a JetStream consumer.
 func (js *js) AddConsumer(stream string, cfg *ConsumerConfig, opts ...JSOpt) (*ConsumerInfo, error) {
+	if cfg != nil && cfg.Durable != _EMPTY_ {
+		consInfo, err := js.ConsumerInfo(stream, cfg.Durable)


I think this is good to call Info before, but also changes the behavior slightly since need to give permissions to info in across account cases which they probably would have anyway.

Yeah, I would lean towards leaving that, as without that AddConsumer() I feel does not work as the name suggests (it will simply update a consumer if it already exists).

wallyqs · 2022-08-11T16:39:30Z

jsm.go

+	ConsumerNameExists    ErrorCode = 10013
+	ConsumerAlreadyExists ErrorCode = 10105
+
+	MessageNotFound ErrorCode = 10037


A bit worried that we are putting too many of these into the nats namespace package, wonder if it is time to move them into their own package... Or maybe suffix them like MessageNotFoundErrorCode? code in the client then would read like: esp.Error.ErrorCode == MessageNotFoundErrorCode

I was thinking about extracting those codes together with errors to a separate package, but moving already defined errors would be a breaking change, so I decided against it. I think the format with ErrorCode prefix makes sense, I'll change that.

wallyqs · 2022-08-11T16:52:52Z

nats.go

@@ -144,7 +144,6 @@ var (
 	ErrNoStreamResponse             = errors.New("nats: no response from stream")
 	ErrNotJSMessage                 = errors.New("nats: not a jetstream message")
 	ErrInvalidStreamName            = errors.New("nats: invalid stream name")
-	ErrInvalidDurableName           = errors.New("nats: invalid durable name")


Probably fine to remove though... maybe safer to mark as deprecated and that it will be removed since not used and use instead ErrConsumerNameAlreadyInUse?

wallyqs · 2022-08-11T17:15:32Z

jsm.go

@@ -384,10 +414,10 @@ func (js *js) DeleteConsumer(stream, consumer string, opts ...JSOpt) error {
 	}

 	if resp.Error != nil {
-		if resp.Error.Code == 404 {
+		if resp.Error.ErrorCode == ConsumerNotFound {


Wonder if we could make these ErrConsumerNotFound plain error types, instead conform to the Error interface and maybe a JetStreamAPIError interface and stamp them with their error codes there, something like:

ErrConsumerNotFound = &apiError{ErrorCode: ...}

Good call. I have an idea on how to make the implementation a bit cleaner, but that would require changing the way errors are compared:

As you mentioned, change the defined error types to APIError, like so:
ErrConsumerNotFound = &APIError{ErrorCode: 10014, Code: 404, Description: "nats: consumer not found"}

Implement custom Is() method on APIError, comparing error codes:

func (e *APIError) Is(target error) bool { err, ok := target.(*APIError) if !ok { return false } return err.ErrorCode == e.ErrorCode && err.Code == e.Code }

Simplify client method implementations so that we do not have to compare the error codes manually at all, e.g.:

if info.Error != nil { return nil, info.Error }

instead of

if info.Error != nil { if info.Error.ErrorCode == ConsumerNotFound { return nil, ErrConsumerNotFound } if info.Error.ErrorCode == StreamNotFound { return nil, ErrStreamNotFound } return nil, info.Error }

However, that means user would not be able to check errors by simple comparison (err == ErrConsumerNotFound), but instead rely on errors.Is(err, ErrConsumerNotFound) (which usually should be the preferred way to go).

The other approach would be to leave the client method implementations untouched and simply change the variable definitions to be &APIError{} instead of errors.New().

What do you think?

Also, what do you mean by implementing JetStreamAPIError interface (i.e. what would it do). The error interface is implemented as part of this PR.

I put together an example of what we could do via an interface here: #1047

wallyqs · 2022-08-13T05:17:23Z

jsm.go

-		} else {
-			err = errors.New(info.Error.Description)
+		if info.Error.ErrorCode == JetStreamNotEnabledForAccount {
+			return nil, ErrJetStreamNotEnabled


In the server not enabled for account and not enabled looks like they are two different errors actually... maybe we should start distinguishing them too and use ErrJetStreamNotEnabledForAccount?

https://github.com/nats-io/nats-server/blob/52c4872666a26acdf475cb89ae9b5f367324fadd/server/jetstream_errors_generated.go#L472-L474

Another factor why maybe we should clean it up is that JetStreamNotEnabledForAccount is a hard error since means that account can never use JetStream in current setup, whereas things are a bit less certain in ErrJetStreamNotEnabled, it can be either a temporary error due to JS not ready yet.

Yes, distinguishing these errors is a good idea.

wallyqs

LGTM!

kozlovic

LGTM

Use JetStream error codes, extract ErrConsumerNameAlredyInUse

4d7a668

piotrpio requested review from kozlovic, wallyqs and Jarema August 11, 2022 16:12

wallyqs reviewed Aug 11, 2022

View reviewed changes

wallyqs mentioned this pull request Aug 12, 2022

js: Add JetStreamError with more API response details #1047

Merged

wallyqs reviewed Aug 13, 2022

View reviewed changes

Address review comments

82272a0

piotrpio requested a review from wallyqs August 16, 2022 08:07

wallyqs changed the title ~~[IMPROVED] Add JetStream error codes, extract ErrConsumerNameAlredyInUse~~ [IMPROVED] Add JetStream error codes, extract ErrConsumerNameAlreadyInUse Aug 16, 2022

wallyqs approved these changes Aug 16, 2022

View reviewed changes

kozlovic approved these changes Aug 16, 2022

View reviewed changes

kozlovic merged commit b81c9e7 into main Aug 16, 2022

kozlovic deleted the js-error-codes branch August 16, 2022 17:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IMPROVED] Add JetStream error codes, extract ErrConsumerNameAlreadyInUse #1044

[IMPROVED] Add JetStream error codes, extract ErrConsumerNameAlreadyInUse #1044

piotrpio commented Aug 11, 2022 •

edited by wallyqs

Loading

coveralls commented Aug 11, 2022 •

edited

Loading

piotrpio commented Aug 11, 2022

wallyqs left a comment

wallyqs Aug 11, 2022

piotrpio Aug 12, 2022

wallyqs Aug 11, 2022

piotrpio Aug 12, 2022

wallyqs Aug 11, 2022

piotrpio Aug 12, 2022

wallyqs Aug 11, 2022

piotrpio Aug 12, 2022

wallyqs Aug 11, 2022

wallyqs Aug 11, 2022 •

edited

Loading

piotrpio Aug 12, 2022

piotrpio Aug 12, 2022

wallyqs Aug 12, 2022

wallyqs Aug 13, 2022

wallyqs Aug 13, 2022

piotrpio Aug 15, 2022

wallyqs left a comment

kozlovic left a comment

[IMPROVED] Add JetStream error codes, extract ErrConsumerNameAlreadyInUse #1044

[IMPROVED] Add JetStream error codes, extract ErrConsumerNameAlreadyInUse #1044

Conversation

piotrpio commented Aug 11, 2022 • edited by wallyqs Loading

coveralls commented Aug 11, 2022 • edited Loading

piotrpio commented Aug 11, 2022

wallyqs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wallyqs Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wallyqs left a comment

Choose a reason for hiding this comment

kozlovic left a comment

Choose a reason for hiding this comment

piotrpio commented Aug 11, 2022 •

edited by wallyqs

Loading

coveralls commented Aug 11, 2022 •

edited

Loading

wallyqs Aug 11, 2022 •

edited

Loading