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

Pipeliner's Do is a trap #2511

Closed
DaemonSnake opened this issue Mar 30, 2023 · 2 comments
Closed

Pipeliner's Do is a trap #2511

DaemonSnake opened this issue Mar 30, 2023 · 2 comments

Comments

@DaemonSnake
Copy link
Contributor

Expected Behavior

Using a method named Do on a Pipeliner object we expect to 'do' said Pipeliner instead of 'add new command'

Without reading the source, the impression, when seeing the prototype of the method and its name, is that it will produce a Cmd interface representing the Pipeliner.

The alternative expected behavior would be that if no arguments are given,
Do would return an error command.
This way the user knows he is misusing the Pipeliner object.

Current Behavior

Currently if given no arguments it creates a nop command, adds it to the list of commands and returns that nop command.
If the user tried to use .Result() on that command he will get an empty response.

Possible Solution

  • Rename Do into Create. Technically Do does nothing
  • If args is empty, create a new Cmd, use SetError on it and return it

Steps to Reproduce

	p := redisClient.Pipeline()
        cmd := redisClient.Exists(context.Background(), "someKey")
        if err := p.Process(context.Background(), cmd); err != nil {
           panic(err)
        }
	if _, err := p.Do(ctx).Result(); err != nil {
                panic(err)
	}
        rsp, err := cmd.Result()
        if  err != nil {
                panic(err)
        }

Context (Environment)

A similar piece of code turned in production for many months and only discovered after a week of investigation.
None of the reviewers caught the issue during the review process.

Detailed Description

Possible Implementation

If renaming is too much of a pain:

func (c *Pipeline) Do(ctx context.Context, args ...interface{}) *Cmd {
	cmd := NewCmd(ctx, args...)
        if len(args) == 0 {
            cmd.SetErr(errors.New(...)) //error that highlights miss-using of Pipeliners' `Do`
        }
	_ = c.Process(ctx, cmd)
	return cmd
}
@monkey92t
Copy link
Collaborator

fix by #2517

@vmihailenco
Copy link
Collaborator

Rename Do into Create. Technically Do does nothing

Do executes the command when used with the plain Redis client. It has a different meaning when used with a Pipeliner, but that's true for all other commands as well.

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

No branches or pull requests

3 participants