Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JS: APIs that accept JSOpt don't make use the prefix #742

Open
kozlovic opened this issue Jun 3, 2021 · 2 comments
Open

JS: APIs that accept JSOpt don't make use the prefix #742

kozlovic opened this issue Jun 3, 2021 · 2 comments
Assignees
Labels
bug Confirmed reproducible bug

Comments

@kozlovic
Copy link
Member

kozlovic commented Jun 3, 2021

Unless my understanding is wrong, all APIs that accept an JSOpt array mean that one could call say AddStream(cfg, nats.APIPrefix("MyPrefix.")) and expect this call to make use of the given prefix. They do not, all those calls use the JS context's prefix that was set when calling JetStream().

If that is the intent (to use the context's prefix), then making possible for users to provide nats.APIPrefix() as an option is wrong since it will create confusion.

If the intent was that the prefix can be specified per API call, then there is a bug which stems from apiSubj() that is based on the context instead of the option that is computed in each API call.
Current code looks like this:

o, cancel, err := getJSContextOpts(js.opts, opts...)
...
csSubj := js.apiSubj(fmt.Sprintf(apiStreamCreateT, cfg.Name))
r, err := js.nc.RequestWithContext(o.ctx, csSubj, req)

As seen above, the context is based on the computed option (a mix of default and user override), but the api subject is based on the context prefix. A solution would be to change apiSubj() to make it a receiver of *jsOpts and call it wit o:

o, cancel, err := getJSContextOpts(js.opts, opts...)
...
csSubj := o.apiSubj(fmt.Sprintf(apiStreamCreateT, cfg.Name))
r, err := js.nc.RequestWithContext(o.ctx, csSubj, req)

with:

func (opts *jsOpts) apiSubj(subj string) string {
	if opts.pre == _EMPTY_ {
		return subj
	}
	var b strings.Builder
	b.WriteString(opts.pre)
	b.WriteString(subj)
	return b.String()
}

Since I am not sure of the original intent, I wanted first to confirm the expected behavior.

@wallyqs
Copy link
Member

wallyqs commented Jun 3, 2021

I think would make things more flexible if we allow APIPrefix to redefine the prefix for a single call.

@kozlovic
Copy link
Member Author

kozlovic commented Jun 3, 2021

I agree that the context is lightweight and one could create one specifically for a given call to another prefix:

js, _ := nc.JetStream()
js.AddStream(cfg)

jsSpoke := nc.JetStream(APIPrefix("Spoke."))
jsSpoke.AddSream(otherCfg)

but again, my issue is that we allow to pass APIPrefix() to the JS calls but we don't use it, which is bad. The following code is not doing what one would expect:

js, _ := nc.JetStream()
js.AddStream(cfg)

js.AddStream(otherCfg, APPrefix("Spoke."))

@wallyqs wallyqs added the bug Confirmed reproducible bug label Jul 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed reproducible bug
Projects
None yet
Development

No branches or pull requests

3 participants