Add error if Consumer Name and Durable are not equal

[Jarema]

• Link to issue, e.g. Resolves #NNN
• Documentation added (if applicable)
• Tests added
• Branch rebased on top of current main (git pull --rebase origin main)
• Changes squashed to a single commit (described here)
• Build is green in Travis CI
• You have certified that the contribution is your original work and that you license the work to the project under the Apache 2 license

Resolves #
When creating a Consumer with specified both Durable and Name, eats-server was overriding Name with Durable value.

Changes proposed in this pull request:

• Error if both Name and Durable are specified, but not equal

/cc @nats-io/core

[derekcollison]

LGTM

[kozlovic]

Nit about the error message (and not sure if I am correct or not).

[kozlovic]

nit: should it be "have to be" instead of "has to be" since we are talking about Durable and Name? (take it from a non native so really not sure :-) )

[derekcollison]

should be

[kozlovic]

Looking at it with @aricart, the server was indeed already making this check and return an error:

nats-server/server/jetstream_api.go

Line 3776 in f7cb5b1

if req.Config.Durable != _EMPTY_ {

[Jarema]

TL;DR It breaks if you do CONSUMER.CREATE.stream.DurableName instead of CONSUMER.CREATE.stream.Name while both Durable and Name are provided and not equal.

@kozlovic & @aricart that lines do not check if config.Durable != config.Name.
It checks if consumerName from request subject != config.Durable.

@aricart client gets error properly, because he does:

consumerName = cfg.name ?? cfg.durable_name ?? "";

So if user specifies both name and durable, name will be put in request subject. Even if its durable consumer.

If both name and durable are provided, but durable is put into the subject consumer name, this check will not detect missconfig.
That means, that any client implementing new API has to know that if both values are provides, he has to use name.

What is worse, is that if library will actually provide durable as a consumer name in subject in above case, server will override name with durable. here:

https://github.com/nats-io/nats-server/blob/main/server/jetstream_api.go#L3784

All of that without an error.

This can be reproduced with this test:

    async fn create_consumer() {
        let subscriber = tracing_subscriber::FmtSubscriber::builder()
            .with_max_level(Level::DEBUG)
            .finish();
        // use that subscriber to process traces emitted after this point
        tracing::subscriber::set_global_default(subscriber).unwrap();

        let server = nats_server::run_server("tests/configs/jetstream.conf");
        let client = async_nats::connect(server.client_url()).await.unwrap();
        let context = async_nats::jetstream::new(client);


        let mut consumer = context
            .get_or_create_stream("events")
            .await
            .unwrap()
            .create_consumer(consumer::pull::Config {
                durable_name: Some("namex".to_string()),
                name: Some("namey".to_string()),
                ..Default::default()
            })
            .await
            .unwrap();
        let info = consumer.info().await.unwrap();
        assert_eq!(info.config.name, info.config.durable_name);
    }

As you can see, name and durable_name becomes equal after creation.
All because Rust client uses durable_name in subject if both values are provided.

I can obviously easily fix it, but this PR protect other clients from hitting this.
Better to throw explicit error saying what's wrong rather than explain that behavior in ADR/docs.

[aricart]

A little more context - I onlyl set consumerName IF the server supports the new API, if it doesn't it simply does the old API. The configuration though will have whatever is set on it (name and durable) in this case. If name is specified, and the new API is not available, the client tosses.

const { min, ok: newAPI } = nci.protocol.features.get(
      Feature.JS_NEW_CONSUMER_CREATE_API,
    );

    const name = cfg.name === "" ? undefined : cfg.name;
    if (name && !newAPI) {
      throw new Error(`consumer 'name' requires server ${min}`);
    }

    let subj;
    let consumerName = "";
    if (newAPI) {
      consumerName = cfg.name ?? cfg.durable_name ?? "";
    }
    if (consumerName !== "") {
      let fs = cfg.filter_subject ?? undefined;
      if (fs === ">") {
        fs = undefined;
      }
      subj = fs !== undefined
        ? `${this.prefix}.CONSUMER.CREATE.${stream}.${consumerName}.${fs}`
        : `${this.prefix}.CONSUMER.CREATE.${stream}.${consumerName}`;
    } else {
      subj = cfg.durable_name
        ? `${this.prefix}.CONSUMER.DURABLE.CREATE.${stream}.${cfg.durable_name}`
        : `${this.prefix}.CONSUMER.CREATE.${stream}`;
    }

[kozlovic]

@Jarema I have the NATS C client PR here where I go by: if Name is specified, then use the new API: https://github.com/nats-io/nats.c/pull/580/files#diff-40734a3f51f4aaaf82dbb31446990052221354380724b30ebf9a1e1cab522449R2340. If the Durable is specified in the config and does not match the subject request, then we get the error message Alberto referred to, which was then captured in that test: nats-io/nats.c@2179f21#diff-e259e818dba4950aedfda772de80aaf4737e6e24a364f543f398597aeaa84651R23399

But I understand what you are saying about js.Subscribe(nats.Durable()), since Name would not be specified, then we would use old API.

[kozlovic]

It breaks if you do CONSUMER.CREATE.stream.DurableName instead of CONSUMER.CREATE.stream.Name while both Durable and Name are provided and not equal.

From the subject perspective, "DurableName" and "Name" are just a consumer name. There is nothing in the subject space that can tell if this is going to be a durable or not, this information is in the request itself. But we agree that if the consumer name in the subject does not match the "Durable" in the request, we should get an error and our argument is that it already happens (see Alberto and my C client PR to prove it).

So if user specifies both name and durable, name will be put in request subject. Even if its durable consumer.

The name goes into the subject, and durable is in the request, they need to match (see point above) and that gives server indication that this is a durable consumer.

If both name and durable are provided, but durable is put into the subject consumer name, this check will not detect missconfig.

I am not sure I understand: if the library puts the durable name in the subject, that the library's choice, so I then expect the library to check that this is matching with name. Instead, if you always use name in the subject and durable in the request, then server already checks that they match (see point 1 again).

That means, that any client implementing new API has to know that if both values are provides, he has to use name.

Yes. But @piotrpio made a point (that I attributed to you in previous response of mine), is that with client library: js.Subscribe("...", nats.Durable("dur")) would use the old API, unless we change this to set the Name too (equal to durable name), which seem ok to me.

[Jarema]

@kozlovic you are right.

I should use name, but I used durableName in subject.

It was a bug I found only because I wrote test that expected error from the server on name/durable_name mismatch (which did not happen because server overriden name with consumer name from subject).

Would be quicker and easier if server would told me I did something dumb 😊.

Hence this PR. To improve JetStream API and make it guide client implementers if they do a mistake, instead of silently accepting request and changing it.

But if you think thats too defensive - we can close this one. I just try to treat our NATS Server API like a public one - to save time on client side fixing such things. Easier to catch misbehavior once in the server than in every client lib 😊.

[kozlovic]

But if you think thats too defensive - we can close this one. I just try to treat our NATS Server API like a public one - to save time on client side fixing such things. Easier to catch misbehavior once in the server than in every client lib

No, we can keep it if this prevents other libs maintainer some headache :-). I tried with your branch and my C client test and I don't have to change it, I still get the other error, so I think that is fine to keep this PR. Maybe update the error text? (and I assume you know that you need to edit the json file - actually use the NATS cli tool for that - to add a new error and then call "go generate")

[Jarema]

Ok.
Will update the error message.

I did update the .json and run go generate, but I updated it manually instead of using CLI. Will check if the output is exactly the same to avoid any future errors.

[kozlovic]

LGTM