sqlc icon indicating copy to clipboard operation
sqlc copied to clipboard

Configure positional parameter logic

Open kyleconroy opened this issue 2 years ago • 6 comments

What do you want to change?

For Go, sqlc will use a positional argument for a query if there's a single parameter. If there's more than one, it will generate a parameters struct. While this works for me, a few other folks would like different behavior.

@brentd would like to use positional arguments for up to three parameters (https://github.com/kyleconroy/sqlc/issues/1496) @Threpio would like to always use parameter structs (https://github.com/kyleconroy/sqlc/issues/1373)

How should we do it?

We can solve this using a single configuration parameter called query_parameter_limit, which defaults to 1 (the current behavior). We can set the value to a pointer in the configuration YAML / JSON so that zero is a valid option. Anything less than zero should be an error.

What programming language backends need to be changed?

Go

kyleconroy avatar Mar 19 '22 02:03 kyleconroy

I am very new to sqlc, given I put some research in it I would like to pick this.

iAziz786 avatar Mar 19 '22 05:03 iAziz786

Just to chime in - I really like the positional argument for only 1 parameter - I would even like positional arguments for more parameters (maybe up to 3?), but only if their types are distinct. That's what I would like out of a Go generator.

When there are more than 3 parameters, then a struct is more ergonomic anyway. But when there are <= 3 parameters, of all distinct types, the compiler will protect you from transposing them, and it should be self-documenting enough.

Crazy Idea Another kind of bizzare pattern that could be applied would be to apply the functional options pattern(But applied to arguments, not options). The boilerplate to facilitate it is rather tedious, but it would be generated anyway. Here's what I'd be shooting for (in user-written code)

_, _ := queries.CreateAuthor(ctx,
  db.Name("Stephen King"),
  db.Bio(sql.NullString{Valid: true, String: "wrote books"}))
  
// or maybe by using some better naming for the package names
_, _ := queries.CreateAuthor(ctx,
  params.Name("Stephen King"),
  params.Bio(sql.NullString{Valid: true, String: "wrote books"}))

This has a few side effects:

  1. You would get full compile-time safety. Forgot a parameter? compile error. Reordered parameters? compile error (but easy to fix, or maybe make them alphabetical?)
  2. It would be slower than it is now, but still significantly faster than any reflection-based one
  3. It is self-documenting at the call site.
  4. You have to write the parameters in exactly the right order :(. But I am not sure how to get around that without sacrificing compile safety.

Would anyone be interested in something like that?

And here is a little template for what I think would be required to do that. Generated Boilerplate

// we start with private structs
type createAuthorParams struct {
	name string
	bio  sql.NullString
}

func (c *createAuthorParams) setName(name string) {
	c.name = name
}

func (c *createAuthorParams) setBio(bio sql.NullString) {
	c.bio = bio
}


type nameParam = func (nameSetter)

type nameSetter interface {
	setName(string)
}

type bioParam = func (bioSetter)

type bioSetter interface {
	setBio(sql.NullString)
}

func Bio(b sql.NullString) bioParam {
	return func (s bioSetter) {
		s.setBio(b)
	}
}

func Name(name string) nameParam {
	return func (s nameSetter) {
		s.setName(name)
	}
}

func (q *Queries) CreateAuthor(ctx context.Context, name nameParam, bio bioParam) (sql.Result, error) {
	var p createAuthorParams
	name(&p)
	bio(&p)
	return q.db.ExecContext(ctx, createAuthor, p.name, p.bio)
}

skabbes avatar May 01 '22 01:05 skabbes

I think having the option be configurable would be a great advantage. Reason being - sometimes we want to decode JSON directly into the struct emitted by sqlc, and in that case having the option to set it to 0 would be quite helpful.

farazfazli avatar May 23 '22 22:05 farazfazli

Hi @kyleconroy - Is this still open or has it been resolved?

Happy to pick this up and code this as work has given me a bit of time to do open-source work.

Threpio avatar Jun 13 '22 07:06 Threpio

@skabbes That's an interesting proposal, but a bit beyond what this issue is tackling. Want to split it out into a separate proposal?

@Threpio Yes, this is still open. There are a few open PRs that I need to review.

kyleconroy avatar Aug 29 '22 04:08 kyleconroy

Since the time I wrote the proposal, I think I learned a lot more about my own use cases - and also the thing others are struggling with - with regard to sqlc.

I (personally) think that "sqlc core engine" (parser, "typechecking", etc) could use some love before trying to double-down on anything code-gen related. I think I'll "retract" my proposal, so I can help out on those areas first. I'd really want to avoid 'death by 1000 paper cuts', and I don't think my proposal is really limiting anyone from using sqlc - it would just be a nice to have.

skabbes avatar Aug 29 '22 14:08 skabbes

Hey @kyleconroy - any update on this?

farazfazli avatar Mar 30 '23 01:03 farazfazli

@farazfazli the first proposal is already merged

go-mez avatar Apr 07 '23 19:04 go-mez

@go-mez That's awesome! Good job.

farazfazli avatar Apr 11 '23 03:04 farazfazli

query_parameter_limit is now live :)

kyleconroy avatar Sep 22 '23 20:09 kyleconroy