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

Implement named args and binding #104

Closed
wants to merge 7 commits into from
Closed

Conversation

stephenafamo
Copy link
Owner

Here is another attempt at binding named args.

Using it will look something like this:

package main

import (
	"context"
	"fmt"

	"github.com/stephenafamo/bob"

	_ "github.com/jackc/pgx/v5/stdlib"
	"github.com/stephenafamo/bob/dialect/psql"
	"github.com/stephenafamo/bob/dialect/psql/sm"
)

type Args struct {
	In1 int
	In2 int
	In3 int
	Id1 int
}

func main() {
	query := psql.Select(
		sm.Columns("id", "name"),
		sm.From("users"),
		sm.Where(psql.Quote("id").In(bob.Named("in1", "in2", "in3"))),
		sm.Where(psql.Raw("id >= ?", bob.Named("id1"))),
	)

	// Use as a regular query
	{
		bound, err := bob.BindNamed[*Args](query)
		if err != nil {
			panic(err)
		}

		queryStr, args, err := bound.Bind(nil).Build()
		if err != nil {
			panic(err)
		}

		fmt.Println(queryStr)
		fmt.Println(args)
	}

	// Use in prepared statements
	{
		ctx := context.Background()
		db, err := bob.Open("pgx", "postgres://postgres:postgres@localhost:5432/postgres?sslmode=disable")

		stmt, err := bob.Prepare[Args](ctx, db, query)
		if err != nil {
			panic(err)
		}

		_ = stmt
	}
}

@RangelReale let me know your thougts on this design.

  1. The binding process can be used on any query, not just statements.
  2. For a bob prepared statement, the bob.InTx method can be used to get a transaction specific version.

@RangelReale
Copy link
Contributor

Would it be possible for BindNamed to also accept a map[string]any?

@RangelReale
Copy link
Contributor

I did a first test with this design and it is very nice and clean, and this way I can use it with sqlx that we currently depend on 👍
Will do a more in-depth test.

@stephenafamo
Copy link
Owner Author

Would it be possible for BindNamed to also accept a map[string]any?

It is easy to create a mapBinder, similar to the structBinder. As a matter of fact, I had the implementation ready but deleted it for now.

However, it would have to use its own entry function, e.g. MapNamed, and I left it this way for now to keep things easier to maintain.

@RangelReale
Copy link
Contributor

Would it be possible for BindNamed to also accept a map[string]any?

The usecase is that I have some dynamic queries that can have more or less parameters depending on filters, otherwise I would need to create a struct with all possible parameters beforehand.

@stephenafamo
Copy link
Owner Author

I did a first test with this design and it is very nice and clean, and this way I can use it with sqlx that we currently depend on 👍
Will do a more in-depth test

Thank you. I'm looking forward to the in-depth test to help iron out any issues with the design.

@stephenafamo
Copy link
Owner Author

The usecase is that I have some dynamic queries that can have more or less parameters depending on filters, otherwise I would need to create a struct with all possible parameters beforehand.

But how would this work since the actual query will expect the same number of args every time?
Or would these be args that can be nil?

@RangelReale
Copy link
Contributor

The usecase is that I have some dynamic queries that can have more or less parameters depending on filters, otherwise I would need to create a struct with all possible parameters beforehand.

But how would this work since the actual query will expect the same number of args every time? Or would these be args that can be nil?

For these queries I don't use Prepare, I build it every time, and add parameters using Apply and add parameters to the map.

	if filter.Email != nil {
		query.Apply(
			sm.Where(`email = ?`, bob.Named("email")),
		)
		params["email"] = *filter.Email
	}

@stephenafamo
Copy link
Owner Author

For these queries I don't use Prepare, I build it every time, and add parameters using Apply and add parameters to the map.

	if filter.Email != nil {
		query.Apply(
			sm.Where(`email = ?`, bob.Named("email")),
		)
		params["email"] = *filter.Email
	}

If you're rebuilding the query every time, isn't it just easier to do this?

 	if filter.Email != nil {
 		query.Apply(
 			sm.Where(psql.Quote("email").EQ(*filter.Email)),
 		)
 	}

This way you wouldn't even need to rebind the parameters.

@RangelReale
Copy link
Contributor

RangelReale commented Sep 12, 2023

For these queries I don't use Prepare, I build it every time, and add parameters using Apply and add parameters to the map.

	if filter.Email != nil {
		query.Apply(
			sm.Where(`email = ?`, bob.Named("email")),
		)
		params["email"] = *filter.Email
	}

If you're rebuilding the query every time, isn't it just easier to do this?

 	if filter.Email != nil {
 		query.Apply(
 			sm.Where(psql.Quote("email").EQ(*filter.Email)),
 		)
 	}

This way you wouldn't even need to rebind the parameters.

Will this generate an SQL argument or the text will be hardcoded on the SQL?

I tried to test it, but it seems that calling BindNamed expects that all arguments be bob.Named, this one which is not gave me an error:

panic: missing arg

@stephenafamo
Copy link
Owner Author

stephenafamo commented Sep 12, 2023

Will this generate an SQL argument or the text will be hardcoded on the SQL?

My bad 🤦🏾 . It should be like this to make it an arg. (the old code wouldn't even compile).

 	if filter.Email != nil {
 		query.Apply(
 			sm.Where(psql.Quote("email").EQ(psql.Arg(*filter.Email))),
 		)
 	}

I tried to test it, but it seems that calling BindNamed expects that all arguments be bob.Named, this one which is not gave me an error:

It shouldn't. Any unnamed arg will be "saved" and returned in the final list after binding.

@RangelReale
Copy link
Contributor

I tried to test it, but it seems that calling BindNamed expects that all arguments be bob.Named, this one which is not gave me an error:

It shouldn't. Any unnamed arg will be "saved" and returned in the final list after binding.

Seems to be not working like this:

	query := psql.Select(
		sm.Columns("id", "name"),
		sm.From("users"),
		sm.Where(psql.Raw("id >= ?", bob.Named("id1"))),
		sm.Where(psql.Raw("id <= ?", 12)),
	)

	type paramst struct {
		Id1 int
	}

	queryBound, err := bob.BindNamed[paramst](query)
	if err != nil {
		panic(err)
	}

	queryStr, args, err := queryBound.Bind(paramst{
		Id1: 400,
	}).Build()
	if err != nil {
		panic(err)
	}
[id1]
panic: missing arg 

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Sep 12, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 0859bb0
Status: ✅  Deploy successful!
Preview URL: https://daba63b7.bob-8vc.pages.dev
Branch Preview URL: https://bind-named-args.bob-8vc.pages.dev

View logs

@stephenafamo
Copy link
Owner Author

Seems to be not working like this:

Try now.

@RangelReale
Copy link
Contributor

Seems to be not working like this:

Try now.

This seems fixed now, will resume my tests.

@RangelReale
Copy link
Contributor

I'm finding that not having map parameters is somewhat limiting for the majority of queries, for example, GetByID:

		query := psql.Select(
			sm.Columns(getDefaultTenantColumns()...),
			sm.From("tenant"),
			sm.Where(psql.Raw(`tenant_id = ?`, bob.Named("tenant_id"))),
			sm.Where(psql.Raw(`deleted_at IS NULL`)),
		)

In this case I would need to create a top level struct (as it needs to be declared on the type of Stmt stored on the Storage struct) just for this specific query, and I would need to create similar things for all queries expect the ones that uses all fields, like INSERT or UPDATE.

I don't thing arguments are reusable enough to require they always be a top-level declared struct, arguments vary a lot between queries.

@stephenafamo
Copy link
Owner Author

@RangelReale after more than a year, I have returned to working on this 😅

See details in #298

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.

2 participants