stephenafamo/bob

Questions about generated query builders

inoc603 opened this issue · 6 comments

The generated query builder function only take bob.Mod[dialect.SelectQuery], but returns TableQuery, and methods like UpdateAll are still available on the type. For example:

	models.Videos(ctx, db).UpdateAll(&models.VideoSetter{
		SponsorID: omitnull.From(1),
	})

will result in the following query and error:

UPDATE `videos` AS `videos` SET
`sponsor_id` = ?
WHERE ((`id`) IN (SELECT
`id`
FROM `videos` AS `videos`
))
0: omitnull.Val[int]{value:1, state:2}

Error 1093 (HY000): You can't specify target table 'videos' for update in FROM clause

While this is understandable, it doesn't quite match the 'find error in compile time' design style as in separating sm/im/um packages for query builder. Maybe we can make the methods on query more restricted by having more types of query? e.g. TableSelectQuery, TableUpdateQuery.

Also I couldn't find any genereated code (without using raw dialect specific query builder) as alternative to the following sqlboiler experience:

models.Videos(mods...).UpdateAll(ctx, db, models.M{models.VideoColums.SponsorID: 1})

I end up generating code like this (using the test database in bob as example):

usage:

	rowsAffected, err := models.UpdateVideos(
		ctx, bob.Debug(db),
		models.UpdateWhere.Videos.UserID.GT(10),
	).Set(models.VideoSetter{
		SponsorID: omitnull.From(1),
	})

in singleton:

type updateQueryMod interface {
	applyUpdateQuery(q *dialect.UpdateQuery) int
}

type UpdateQuery[Tset updateQueryMod] struct {
	q bob.BaseQuery[*dialect.UpdateQuery]
	ctx context.Context
	exec bob.Executor
}

func (uq *UpdateQuery[Tset]) Set(s Tset) (int64, error) {
	if s.applyUpdateQuery(uq.q.Expression) == 0 {
		return 0, nil
	}
	query, args, err := uq.q.Build()
	if err != nil {
		return 0, err
	}
	res, err := uq.exec.ExecContext(uq.ctx, query, args...)
	if err != nil {
		return 0, err
	}
	return res.RowsAffected()
}

in model:

func (vs VideoSetter) applyUpdateQuery(q *dialect.UpdateQuery) (applied int) {
	if vs.ID.IsSet() {
		um.Set(TableNames.Videos, ColumnNames.Videos.ID).ToArg(vs.ID).Apply(q)
		applied++
	}
	if vs.UserID.IsSet() {
		um.Set(TableNames.Videos, ColumnNames.Videos.UserID).ToArg(vs.UserID).Apply(q)
		applied++
	}
	if vs.SponsorID.IsSet() {
		um.Set(TableNames.Videos, ColumnNames.Videos.SponsorID).ToArg(vs.SponsorID).Apply(q)
		applied++
	}
	return applied
}

func UpdateVideos(
	ctx context.Context, 
	exec bob.Executor, 
	mods ...bob.Mod[*dialect.UpdateQuery],
) *UpdateQuery[VideoSetter] {
	q := mysql.Update(um.Table(TableNames.Videos))
	q.Apply(mods...)
	return &UpdateQuery[VideoSetter]{
		q: q,
		ctx: ctx,
		exec: exec,
	}
}

I wonder if there's a better way to support these types for query out of the box with bob.

models.Videos(mods...).UpdateAll(ctx, db, models.M{models.VideoColums.SponsorID: 1})

I wonder if there's a better way to support these types for query out of the box with bob.

Yes this is a bug. It is supposed to work, but there is an error in the way the update query was generated. I'll fix this as soon as I can.

If we keep the current function signature unchanged, the working code might be:

models.Videos(
        ctx, db, 
        models.SelectWhere.ID.LT(10),
).UpdateAll(models.VIdeoSetter{
        SponsorID: omit.From(1),
})

Although I think this is fine, the generated UpdateWhere makes me thinks maybe we should use SelectWhere in All, and UpdateWhere in UpdateAll? It would be tricky since the current mod is typed bob.Mod[*dialect.SelectQuery].

It would be tricky since the current mod is typed bob.Mod[*dialect.SelectQuery].

Yes, this is why the UpdateAll and DeleteAll methods work the way they do.
To be honest, I also like the UpdateQuery idea too, but I'm not sure if it is critical to include it.

I end up generating code like this (using the test database in bob as example):

The more I look at this, the more I like it. However, it would be better to make the ModelSetter implement bob.Mod[*dialect.UpdateQuery], that way it can be directly passed to models.UpdateVideos.

Can you add a new tag for the latest fixed version please? Thanks. @stephenafamo

I want to include a few other things. Should do so over the next few days.