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

Make Durable mandatory for PullSubscribe #691

Merged
merged 1 commit into from Mar 30, 2021
Merged

Conversation

variadico
Copy link
Contributor

nats.Durable is a SubOpt that may be passed to PullSubscribe or other
methods. If a durable name isn't passed to PullSubscribe, then we return
a runtime error.

Instead of returning a runtime error for this case, this change updates
the PullSubscribe method to require a durable name parameter.

@coveralls
Copy link

coveralls commented Mar 29, 2021

Coverage Status

Coverage increased (+0.06%) to 86.558% when pulling 077cf10 on pull-durable-req into d9ad37d on master.

Copy link
Member

@wallyqs wallyqs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

func (js *js) PullSubscribe(subj string, opts ...SubOpt) (*Subscription, error) {
return js.subscribe(subj, _EMPTY_, nil, nil, opts)
func (js *js) PullSubscribe(subj, durable string, opts ...SubOpt) (*Subscription, error) {
return js.subscribe(subj, _EMPTY_, nil, nil, append(opts, Durable(durable)))
Copy link
Member

@wallyqs wallyqs Mar 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we prevent here that nats.Durable is passed so that it is not ambiguous?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh, yeah. Good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated nats.Durable to check if it has already been set. If so, it returns an error.

func Durable(name string) SubOpt {
	return subOptFn(func(opts *subOpts) error {
		if opts.cfg.Durable != "" {
			return fmt.Errorf("nats: option Durable set more than once")
		}
		if strings.Contains(name, ".") {
			return ErrInvalidDurableName
		}

		opts.cfg.Durable = name
		return nil
	})
}

I also added this new test code.

sub, err = js.PullSubscribe("bar", "foo", nats.Durable("bar"))
if err == nil {
	t.Fatalf("Unexpected success")
}
if err != nil && err.Error() != `nats: option Durable set more than once` {
	t.Errorf("Expected consumer in pull mode error, got %v", err)
}

nats.Durable is a SubOpt that may be passed to PullSubscribe or other
methods. If a durable name isn't passed to PullSubscribe, then we return
a runtime error.

Instead of returning a runtime error for this case, this change updates
the PullSubscribe method to require a durable name parameter.
@wallyqs wallyqs merged commit 882e98e into master Mar 30, 2021
@wallyqs wallyqs deleted the pull-durable-req branch March 30, 2021 00:26
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants