Add ErrKeyExists on kv.Create

[bruth]

Fix #1134

[bruth]

Let me know if there is a better way to identify this particular error type.

[wallyqs]

maybe add it as one of the constants here: https://github.com/nats-io/nats.go/blob/main/jserrors.go#L112
so that it represents this error from the server:
https://github.com/nats-io/nats-server/blob/e42260cc80f793006ef7f90cc91ede26fe14073f/server/jetstream_errors_generated.go#L536

[coveralls]

Coverage decreased (-0.2%) to 85.599% when pulling 7230ca7 on issue-1134 into 1cc4427 on main.

[wallyqs]

something that we started doing in the nats.go client is to move these codes into jserrors.go:
https://github.com/nats-io/nats.go/blob/main/jserrors.go#L121

then to make it compatible with errors.Is defining something like:

ErrWrongLastSequence JetStreamError = &jsError{apiErr: &APIError{ErrorCode: JSErrStreamWrongLastSequence}}

then this check would become:

if errors.Is(err, ErrWrongLastSequence) {
 return ErrKeyExists
}

What I think might be helpful eventually is to have something like another KeyValueError type that embeds the original JS APIError to be able to inspect the original JS API Error which resulted into the KeyValueError by using errors.As.

[wallyqs]

LGTM, left some comments on improving the way we represent errors from KeyValue API but can be done later.

[bruth]

Thanks @wallyqs I can make a subsequent PR for that. I don't have access to merge, so feel free.

[ripienaar]

Is this is a API breaking change @wallyqs @bruth ?

[bruth]

@ripienaar Good call out.. technically yes given that it changes how a person would need to check for an error and the existing manual method of checking for the code would now not work. A user on Slack called it out since the error not intuitive without understanding the implementation, so it debatable if we could frame it as such.

All that said, how has this been handled in the past? Should we defer all error improvements to a separate branch and then bump the major version?

[ripienaar]

The way the coded errors are handled now is really awkard as you mentioned, we could have something like nats.IsNatsErr(err, 1234) to check the error code as well as standard go wrapping checks in there, all the casting and IsError etc we do now is super verbose.

I don't know the anwer but I am generally concerned about backward compatibility so that was my main concern.

In this case, obviously your commit is an improvement and one could even argue surfacing a underlying nats error in the kv api is just a bug.

[wallyqs]

that's a good point, to avoid being a breaking change we could go back to returning the raw APIError (return err) and instead define ErrKeyExists with the embedded APIError something like this:

ErrKeyExists JetStreamError = &jsError{apiErr: &APIError{ErrorCode: JSErrStreamWrongLastSequence, Code: 400}}

then that way those checking with errors.Is(err, ErrKeyExists) would be able to handle with the new behavior and previous usage with the raw APIError type would still work without changes.

[bruth]

argue surfacing a underlying nats error in the kv api is just a bug.

Yea, that is sort of how it felt and I think would be acceptable framing in this case..

then that way those checking with errors.Is(err, ErrKeyExists) would be able to handle with the new behavior and previous usage

I think wrapping is a good idea for the existing set of ErrXX values in order to migrate everything over to jserrors.go and expecting users to use errors.Is going forward. Looking at the distinction of the ApiError vs. JetStreamError, where does this distinction get used in practice? Likewise, if we were to introduce a KVError, I am not sure if having different types are useful compared to have different named values, e.g. ErrKeyValueKeyExists (following the ErrJetStream... naming convention). Just thinking out loud..

[ripienaar]

We liked the idea of the error codes in the JetStream errors (see nats error ls for example as a great source of information that we could lean into, links to doc site etc).

So we started with JetStream work as a place to bake the error system and discover what work and doesn't, it went through a few iterations before we ended up where we are now.

I'd personally like to see this now spread out to other server errors and into clients with deeper awareness of the metadata that goes with the errors etc

For KV though, surfacing a coded error is almost always a bug imo, KV API is small and easy to understand and focussed on one thing. It's errors should be a constrained enough set that you can have constants defined for them like the current not found ones and that those will remain pretty barebones to not end up painting us into corners when it comes to supporting other storage backends which seems inevitable.

$ nats errors lookup 10121
NATS Error Code: 10121

        Description: consumer max ack pending exceeds system limit of {limit}
          HTTP Code: 400
  Go Index Constant: JSConsumerMaxPendingAckExcessErrF

No further information available

[bruth]

We liked the idea of the error codes in the JetStream errors [...] I'd personally like to see this now spread out to other server errors and into clients with deeper awareness of the metadata that goes with the errors etc

Yes I definitely agree with that, richer errors with more metadata the better and we definitely want to lean into a source of truth of errors for docs and and clients to stay consistent.

My question was more around what value the JetStreamError type provides since it wraps ApiError.

[ripienaar]

Others might be better to comment there - but since they are essentially only produced by JetStream today, and we dont know how it would look for non JS errors - seperating them might be a way to improve long term api stability should the errors in non JS uses from the server deviate a bit (we dont always have like HTTP status error codes etc)