Deliver Subject field in Push Consumer Config should not have a default or warn/error about it being empty

[simon-connektica]

Proposed change

The deliver_subject field is required in order for a push consumer to work properly, at least mine was not working properly without it. The issue is that pull::Config implements Default and #[serde(default)] for the deliver_subject field. It's really easy to mess up the configuration such as in the code below:

.get_or_create_consumer(
    "bob",
    jetstream::push::Config {
        name: "bob"
        durable_name: Some(
            "bob"
        ),
        ..Default::default()
    },
)

Use case

Just making the API more fool proof because it's really easy to miss that this field is required from the docs.

Contribution

I could do it if the changes are deemed worthwhile.

[simon-connektica]

It seem like the Go SDK creates a new inbox if the DeliverSubject field is empty. Going to dig and try to see what async_nats does.

It does not seem to use a new_inbox() if deliver_subject.is_empty(). I found another commit related to this issue here

nats.rs/async-nats/CHANGELOG.md

Line 707 in a1876c4

* Make `deliver_subject` required for `push::Config` by @caspervonb in https://github.com/nats-io/nats.rs/pull/531

[Jarema]

This is the old JS API that uses defaults for everything when calling Subscribe method, not if creating a Consumer directly.
I need to think about this one for a bit. I would prefer to avoid "magic" in the background.

At the same time, removing Default implementation does not seem sensible, as this is the way to specify only required fields in Consumer Config.

I don't like it, but maybe throwing a runtime error that delivery subject cannot be empty is the way to go here.

[simon-connektica]

Returning an error from get_or_create_consumer would satisfy me!

[simon-connektica]

After doing quite a bit of reading I came to the conclusion that the best option for deliver_subject is to set it to new_inbox() at least in the general case we have here.

In any case I would highlight in the crate's docs that deliver_subject must be set to something! The docs on the NATS website don't clearly explain what deliver_subject is for and that it's mandatory. I figured out eventually that it's so the jetstream client can use Core NATS to subscribe to deliver_subject but it took me a while to figure out. It's kind of a leaky implementation detail.

This is just my feedback as a new async_nats & NATS user, I'm taking the time to write these issues in the hope that it's valuable feedback for you, but I understand if not all of my notes are actionable from a holistic point of view.

[Jarema]

@simon-connektica I disagree with the idea of automatically setting the deliver subject and here is the reason:

If someone have strick permissions for subjects, and forgets to setup the delivery subject, this consumer will not work properly.
I prefer the error.

However, there already is an error returned:

Error { kind: Other, source: Some(Custom { kind: Other, error: "push consumer must have delivery subject" }) }

[simon-connektica]

Was this error added after the release of 0.33? I did not get this error in my service.

[Jarema]

This is the error returned by the server. What happened when you tried to create a push consumer without a delivery subject?