Document pattern of checking for zero rows

[flowchartsman]

I am using sqlitex.Execute for both queries and updates, and I can't seem to figure out if there is a package-supported way to instruct these methods to return an error if zero rows are returned. Currently, I am doing something like the following:

/* ... */
	found := false
	err = sqlitex.ExecuteFS(conn, sqlFS, "path/to/embedded.sql", &sqlitex.ExecOptions{
		Named: args{
			":key": key,
		},
		ResultFunc: func(stmt *sqlite.Stmt) error {
			found = true
			r := stmt.ColumnReader(0)
                        // parameter v is an any to be gob decoded or json decoded to
			return gob.NewDecoder(r).Decode(v)
		},
	})
	if err != nil {
		return err
	}
	if !found {
		return ErrNotFound
	}

But this is annoying to keep writing. Have I missed something? I know we have sqlitex.Result* methods, but there isn't one for Bytes, which I'm using, and they don't really fit the workflow of Execute and friends.

Provided there is no other way, I propose an additional field in sqlitex.ExecOptions called ExpectRows bool or ExpectResults bool that, when true, returns either a sentinel error from sqlitex (like sqlitex.ErrNoRows) or a sqlite error whose code resolves to sqlite.ResultNotFound if no rows exist.

[flowchartsman]

Went ahead and tossed a couple PRs your way, if they suit you. I could go either way between ExpectResults and ExpectRows, but ExpectResults is at least symmetrical with the (newly-exported) ErrNoResults.

I think they could both be useful, but I can also continue to work around the issue, so please feel free to close if they don't fit the project goals. 🫡

[zombiezen]

Thanks for the PR. I agree this is a common pattern, but having this in the library seems like it would make application control flow less clear.

I'm going to close #87 and retitle this issue to match new scope, but I'd like to have an example of this pattern in the documentation. Would you want to send a PR over for that?

[flowchartsman]

I'd like to push back a little on this being just a doc change before I give up the ghost.

I see your point wrt control flow; however, I also think that forcing two statements in different scopes to address such a common use case creates friction. In my case, it was exacerbated by the presence of other conveniences that seemed to do what I wanted, but didn't.

To elaborate: I didn't discover that this might bite me until I was writing tests, only to find my zero values untouched, and I ended up beating my head against the wall for a couple hours trying to find a way to do something declarative in ResultFunc before I dug into the code and realized it wasn't getting called at all. It's true that you can read between the lines in the docs and realize that ResultFunc won't get called if there are no results, and I might have made that leap sooner but for the presence of the Result* functions, which I initially assumed were what I should be using as a convenience for the one-result use case, since they consume a *sqlite.Stmt, and are in the same package. Eventually I realized they serve a different usage pattern entirely with a different level of convenience. I suppose you could argue that a grab-bag of convenience workflows is the whole raison d'être for x packages, but it was confusing.

So, essentially, the gap I was left with was how to use the otherwise elegant pattern of embedding my .sql files with the convenience of the Result* functions, which is how #86 got started. Unfortunately, it doesn't actually help me much, since there's no bridge to preparing the statement and binding values as there is with Execute and friends.

I considered a PR for something like PrepareFS(conn *sqlite.Conn, fsys fs.FS, filename string) (*sqlite.Stmt, error), but that's relatively clunky, since it can't slot neatly into a Result* call, and it loses out on the convenience of ExecOptions bindings. This is why I opted for the bool field approach in #87, since it's backwards-compatible and had the smallest blast radius.

Another option might be to use a little closure magic to automate the process with a SingleRow family of functions:

// SingleRow is [Exec], but it returns an error if there is not exactly one result returned.
func SingleRow(conn *sqlite.Conn, query string, opts *ExecOptions) error {
	oOpts, gotResult := oneResult(opts)
	err := Execute(conn, query, oOpts)
	if err != nil {
		return err
	}
	if !gotResult() {
		return errNoResults
	}
	return nil
}

// SingleRowFS is [ExecuteFS], but but it returns an error if there is not exactly one result returned.
func SingleRowFS(conn *sqlite.Conn, fsys fs.FS, filename string, opts *ExecOptions) error {
	oOpts, gotResult := oneResult(opts)
	err := ExecuteFS(conn, fsys, filename, oOpts)
	if err != nil {
		return err
	}
	if !gotResult() {
		return errNoResults
	}
	return nil
}

func oneResult(opts *ExecOptions) (*ExecOptions, func() bool) {
	if opts == nil {
		opts = &ExecOptions{}
	}
	if opts.ResultFunc== nil {
		opts.ResultFunc=func(*sqlite.Stmt) error { return nil }
	}
	called := false
	opts.ResultFunc=func(stmt *sqlite.Stmt) error {
		if called {
			return errMultipleResults
		}
		called = true
		return opts.ResultFunc(stmt)
	}
	return opts, func()bool{return called}
}

[flowchartsman]

In light of the contribution guide rework, now I'm worried I'm being too presumptuous 😔. If you'd prefer, I can close the PR and move the above to a discussion.

If so, I'll be happy to proceed with the doc PR in the meantime.

[zombiezen]

Thanks for the detailed experience report, and I'm sorry to hear that the library's behavior led to a frustrating debugging experience. I'm in agreement that this is an area that can be improved, and I'm genuinely appreciative for the conversation and code contribution.

Just to set expectations, I'm currently travelling, so I won't be able to review this for a week or so.

In light of the contribution guide rework, now I'm worried I'm being too presumptuous 😔.

I reworked the contribution guide mostly to remove the line "this is a project that fills my own needs" because it's definitely grown beyond that. I hope the new guide is not off-putting. I would appreciate any feedback you have on it (feel free to email privately if that's more comfortable to you).

As stated above, I appreciate you reaching out on this issue. I'm in agreement with you that this experience can be improved, but I'm trying to start with the smallest maintenance cost change to address, then working up.

If you'd prefer, I can close the PR and move the above to a discussion.

Yeah, let's start with a discussion. 👍 There's some interesting options here and I don't want you to churn on code until we have more alignment.

[flowchartsman]

As requested! #90