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

Document pattern of checking for zero rows #85

Open
flowchartsman opened this issue Mar 25, 2024 · 6 comments · May be fixed by #89
Open

Document pattern of checking for zero rows #85

flowchartsman opened this issue Mar 25, 2024 · 6 comments · May be fixed by #89
Labels
documentation Improvements or additions to documentation

Comments

@flowchartsman
Copy link

flowchartsman commented Mar 25, 2024

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.

This was referenced Mar 26, 2024
@flowchartsman
Copy link
Author

flowchartsman commented Mar 26, 2024

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 zombiezen added the documentation Improvements or additions to documentation label Mar 27, 2024
@zombiezen
Copy link
Owner

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?

@zombiezen zombiezen changed the title sqlitex.Execute - provide option to return an error if 0 rows are returned. Document pattern of checking for zero rows Mar 27, 2024
@flowchartsman
Copy link
Author

flowchartsman commented Mar 28, 2024

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 flowchartsman linked a pull request Mar 28, 2024 that will close this issue
@flowchartsman
Copy link
Author

flowchartsman commented Mar 30, 2024

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
Copy link
Owner

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
Copy link
Author

As requested! #90

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

Successfully merging a pull request may close this issue.

2 participants