nats-io / nats.go

Golang client for NATS, the cloud native messaging system.
https://nats.io
Apache License 2.0
5.54k stars 696 forks source link

incorrect error message when creating KV watches #1354

Open critterjohnson opened 1 year ago

critterjohnson commented 1 year ago

Defect

Versions of nats.go and the nats-server if one was involved:

nats-server: v2.9.19 github.com/nats-io/nats.go v1.27.1

OS/Container environment:

MacOS 13.4.1 go version go1.20.5 darwin/arm64

Steps or code to reproduce the issue:

Start a JetStream enabled NATS server:

nats-server -js

Spawn a watch on an invalid key:

package main

import (
    "github.com/nats-io/nats.go"
)

func main() {
    nc, _ := nats.Connect(nats.DefaultURL)

    js, err := nc.JetStream()
    if err != nil {
        panic(err)
    }

    kv, err := js.KeyValue("bucket")
    if err != nil {
        panic(err)
    }

    _, err = kv.Watch("test..")
    if err != nil {
        panic(err)
    }
}

Expected result:

An error message similar to the one the CLI provides:

$ nats kv watch bucket test..
nats: error: nats: consumer filter subject is not a valid subset of the interest subjects

Actual result:

panic: nats: jetstream not enabled
mdawar commented 1 year ago

It's also worth noting that the nats CLI current main branch also shows the error:

nats: error: nats: jetstream not enabled

I can only get the error:

nats: error: nats: consumer filter subject is not a valid subset of the interest subjects

With nats CLI v0.0.35 uses nats.go version 1.19.0:

go install github.com/nats-io/natscli/nats@v0.0.35
ripienaar commented 1 year ago

It is because the trailing “..” is an invalid subject and it causes the API subjects being used to also be invalid.

We should probably fail for such invalid sibjects earlier - but that’s why you get jetstream not enabled, the jetstream API can’t be reached.

mdawar commented 1 year ago

It is because the trailing “..” is an invalid subject and it causes the API subjects being used to also be invalid.

We should probably fail for such invalid sibjects earlier - but that’s why you get jetstream not enabled, the jetstream API can’t be reached.

Yes the Watch() method does not validate the keys pattern, the other KV methods use the keyValid() function to validate the key which is going to fail if in Watch() if the pattern contains wildcards.

critterjohnson commented 1 year ago

It is because the trailing “..” is an invalid subject and it causes the API subjects being used to also be invalid.

We should probably fail for such invalid sibjects earlier - but that’s why you get jetstream not enabled, the jetstream API can’t be reached.

I understand that - I found where it's coming from in the source code but I figured I should file an issue here.

FWIW, this came up when I was accidentally passing an empty string to a format string that constructed the key for me. The bad error message led me to believe that there was some kind of server-side error, and it took me a good half hour to figure out what was going on.

a-h commented 1 year ago

I think it's an issue with the Go API design. I would prefer that if the NATS server is going to complain about subject formatting, then it should accept a Subject type instead of a string so it's harder to receive an invalid one.

For example, in the subject package, it would be possible to have a Parse(s string) (s Subject, err error) function that checks that the subject is valid using the same rules as the server. You'd use it with subject.Parse("my_subject").

It might have helper methods on it to build up valid subjects, wildcards etc.