richer api errors proposal

[ripienaar]

This creates richer errors in response to #1811 and numerous other requests.

The main idea is that when a 404 is raised in response to accessing a consumer we now include a unique identifier that would allow callers to distinguish between stream not found, consumer not found, no message found and other errors that might raise 404.

One could argue that users might parse the Description like stream not found however this makes our human friendly descriptions part of the API promises and we can never improve, bug fix or translate those strings. For example today there is an unfortunate mix of jetstream and JetStream in use in error descriptions and we would want the ability to improve errors without breaking peoples code.

Additionally I propose capturing help and urls in the code - not in actual API responses - to facilitate a single place where we maintain these errors and then adding tooling like nats error JS1 that will produce the error text, help, additional information and links to documentation that would help users resolve those errors. So the slow ApiErrorByKind would not be used in anything but the CLI to resolve these things, though even thats optional as the ApiErrors is exported.

Signed-off-by: R.I.Pienaar rip@devco.net

/cc @nats-io/core

[kozlovic]

I approve of this idea. Relying on error text is "error prone" :-)

[ripienaar]

@derekcollison I want to spend some time on this during this week, let me know what you think of the approach before I dive in on a wrong track, thanks!

[derekcollison]

Probably deserves a meeting between a few folks.

[ripienaar]

I added @matthiashanel and @aricart to reviewers for context, will have a call with all reviewers I guess.

[derekcollison]

Add me onto the list as well if you can.

[aricart]

I had looked at the PR before, and I agree that having a code that can be pointed at documentation can provide a richer experience to diagnose problems. The standard string that it represents is useful, but more so I think is the case where these values make it to a server log. Having documentation where the lookup can be performed is very useful.

[ripienaar]

@kozlovic @derekcollison this has grown quite big now - and I think we can go deeper by making some of the utility functions also return these errors to better detect causes but for now I am fairly happy with this.

Would you mind taking a peek through and let me know, I'll highlight a few specific areas via comments.

[ripienaar]

@kozlovic think I am good on this one, I updated MQTT code considering new errors and also added a ADR for how it works and how to maintain it (though I think that needs expanding in time).

for now keen to get this in so we can stop the rebasing pain :)

[kozlovic]

LGTM, except not sure what to do with exported JS errors: removing them may break some apps (but unlikely).

[kozlovic]

As we discussed once, should we leave those but with a doc section that says that they are "deprecated"? I am thinking of use-case where users embed the NATS Server. I assume that internally they will no longer be used or updated.

[ripienaar]

Yeah its a difficult one, we could say ErrJetStreamNotEnabled = ApiErrors[JSWhatever] but I am not 100% sure that will solve the problem in all cases tbh

[ripienaar]

Actually it should solve it for these - as they are essentially static, the ones where it wont solve it are the ones where we put in a different description via printf. So it might be fine

[derekcollison]

We should leave, no harm imo..

[kozlovic]

I think the point was that we would not use them anymore, but we may want to keep them as to possibly not break users that embed NATS Server and somehow were checking those error codes?

[ripienaar]

Yeah, I ppreciate why we would want to keep them - however, keeping them would not solve the problem completely.

For some we can keep them via an alias asignment but any of them that were dynamic would not be possible (the ones with tokens in them):

ErrJetStreamNotEnabled = ApiErrors[Foo]

Non of these had tokens of course so I need to look, might have to adjust NewT() and related functions to instead of a new instance return the existing pointer that is in ApiErrors, thats the only way it could possibly work. This though is risky someone could adjust the "source" of these errors and bad things will happen, as there's no way to really make them read only as such

[kozlovic]

Yes, it is silly of me to say to keep them not to "break" their code (because say ErrJetStreamNotEnabled is no longer defined), while it would break it in worse way by having the check for say ErrJetStreamNotEnabled be false because of the new errors returned.
So I would be ok if we remove in 2.3.0, but then if we have to release a 2.2.7 before that release, then we would have to create and maintain a branch_2_2_x to release from, which I would rather not have to do..

[ripienaar]

Old costants were restored in ee9d10f with appropriate comments warning people etc

I think this one is good to go now

[ripienaar]

While we wait for @derekcollison to give this a look the one thing I've been mulling over is how we format the errors that are of the ErrorF form.

ApiErrors[JSStreamExternalApiOverlapErrF].ErrOrNewF(foo, bar)

Where the ApiErr is:

{Code: 400, ErrCode: 10021, Description: "stream external api prefix %q must not overlap with %s"},

The problem here is that we want to be able to extend and change these errors - especially if we later need to translate them - then the order based replace becomes a problem.

The typical fix is to make the description something like "stream external api prefix {prefix} must not overlap with {api}" and then:

ApiErrors[JSStreamExternalApiOverlapErrF].ErrOrNewF("{prefix}", foo, "{api}", bar)

This is much more robust and future proof, personally I think it's worth it, but curious what @ColinSullivan1 (who like me also seem to care for translation) and @kozlovic thinks

[kozlovic]

This is much more robust and future proof, personally I think it's worth it, but curious what @ColinSullivan1 (who like me also seem to care for translation) and @kozlovic thinks

I agree.

[derekcollison]

In general LGTM. Went through them one by one so ignore repeated comments on codes need to be numbers.

[derekcollison]

We should leave, no harm imo..

[ripienaar]

I squashed this but I think I need more time to find a better solution to handle the named token replace things, forcing all things to be strings is terrible which is what strings.Replacer() wants. So will investigate more.

[derekcollison]

We could do the string replace solution at a different time as well.

[derekcollison]

I think we can merge once @kozlovic has signed off.

[kozlovic]

Last comment regarding removing server's existing ErrXXX...

[kozlovic]

I think the point was that we would not use them anymore, but we may want to keep them as to possibly not break users that embed NATS Server and somehow were checking those error codes?

[kozlovic]

New field deprecates is misspelled in one of the error, also comment regarding ordering of the error codes in the json file.

[kozlovic]

LGTM