JSON encoded connection doesn't report error on bad JSON

[aricart]

If a non-json client publishes a message that is invalid JSON, an encoded connection doesn't report the issue, this tests demonstrates the issue:

func TestJSONBad(t *testing.T) {
    var d interface{}
    err := json.Unmarshal([]byte(""), &d)
    if err == nil {
        t.Fatal("should be an error")
    }
}

func TestDecodeBadJSON(t *testing.T) {
    s := RunServerOnPort(TEST_PORT)
    defer s.Shutdown()

    nc := NewConnection(t, TEST_PORT)
    defer nc.Close()

    var errSub *nats.Subscription
    uc, err := nats.Connect(fmt.Sprintf("nats://127.0.0.1:%d", TEST_PORT),
        nats.ErrorHandler(func(_ *nats.Conn, sub *nats.Subscription, _ error) {
            errSub = sub
    }))
    if err != nil {
        t.Fatalf("Failed to create an connection: %v\n", err)
    }
    ec, err := nats.NewEncodedConn(uc, nats.JSON_ENCODER)
    if err != nil {
        t.Fatalf("Failed to create an encoded connection: %v\n", err)
    }
    defer ec.Close()

    sub, _ := ec.Subscribe("bad_json", func(s string) {})
    nc.Publish("bad_json", []byte(""))
    nc.Flush()
    ec.Flush()

    if errSub == nil || errSub != sub {
        t.Fatal("should have gotten an async error")
    }
}

The important issue is whether decoding errors should be an error for the client, I believe they should be because otherwise there's no visibility in that some client is generating bad JSON.

[derekcollison]

Agree. Although encoded connections need to be rethought out in general IMO to be way more generic like HTTP stacks have with middleware.

[kozlovic]

Few things:

• the test program is not really correct. I would replace current use of errSub with:

  errSub := make(chan *nats.Subscription, 1)
  uc, err := nats.Connect(fmt.Sprintf("nats://127.0.0.1:%d", TEST_PORT),
      nats.ErrorHandler(func(_ *nats.Conn, sub *nats.Subscription, _ error) {
          errSub <- sub
      }))

  then check at the end of test with something like:

  select {
  case esub := <-errSub:
      if esub != sub {
          t.Fatal("should have gotten the sub in the error handler")
      }
  case <-time.After(time.Second):
      t.Fatal("should have gotten an async error")
  }

  since the error handler is asynchronous and cannot be expected to execute right away

• Your use of ec for subscription (expected) and nc for publish is a tiny misleading. At first read I would have thought that you forgot to flush before doing the publish. It happens that this is the same underlying connection so you should be fine. No need for the nc.Flush() and ec.Flush() then.

Even with above changes, the test would fail. The reason is simple: in json_enc.go we say:

func (je *JsonEncoder) Decode(subject string, data []byte, vPtr interface{}) (err error) {
    switch arg := vPtr.(type) {
    case *string:
        // If they want a string and it is a JSON string, strip quotes
        // This allows someone to send a struct but receive as a plain string
        // This cast should be efficient for Go 1.3 and beyond.
        str := string(data)
        if strings.HasPrefix(str, `"`) && strings.HasSuffix(str, `"`) {
            *arg = str[1 : len(str)-1]
        } else {
            *arg = str
        }

which means that we intentionally return the given string if the provided callback in the Subscribe() call asks for a string. Would the callback expects an actual structure resulting in actual json unmarshal'ing, the test would have passed in that there would have been an error.

Changing this behavior now may break applications expecting this to work...

[aricart]

@derekcollison - I agree - In javascript and most other languages that support JSON easily, there's no complexity in encoding/decoding (JSON or otherwise). For NATS this is a bit more tricky because specific subjects are not associated with any particular schema or format, so having the decoding on the client is just a general option that may not be right depending on the source. I don't think it a burden to leave out the encoding/decoding to the client, and it would simplify the individual clients. This also allows the clients to be aware of any encoding/decoding they have to apply.

[aricart]

Furthermore, currently, we associate encoding with a connection to do the magic for publishers, instead of with a subscription. This makes it so that all messages via a connection need to use the same encoding or have multiple connections.

[khanakia]

How to make this work in case of Request and Reply Pattern. I want the client to know that they sent BAD JSON. Currently, when clients send BAD JSON there is no way I see to send the reply with error messages.

Right now I get only Request failed: nats: timeout