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

Query builder should add components rather than replacing them. Example: Calling WHERE multiple times should add additional clauses, rather than replace the only clause #378

Open
veqryn opened this issue Aug 30, 2024 · 6 comments
Labels
bug Something isn't working

Comments

@veqryn
Copy link

veqryn commented Aug 30, 2024

Describe the bug
The query builder replaces rather than adds components and clauses.
This goes against the behavior of every single query builder I've ever worked with, and is rather surprising and unintuitive, while also making some situations difficult or impossible.

Environment (please complete the following information):

  • OS: linux and mac
  • Database: postgres
  • Database driver: pgx v5
  • Jet version: 2.11.1

Code snippet

func (d DAO) SelectAllAccountsByFilter(ctx context.Context, filters models.Filters) ([]model.Accounts, error) {
	query := SELECT(
		Accounts.AllColumns,
	).FROM(
		Accounts,
	)

	if len(filters.Names) > 0 {
		query = query.WHERE(Accounts.Name.IN(Strings(filters.Names)...))
	}
	if filters.Active != nil {
		query = query.WHERE(Accounts.Active.EQ(Bool(*filters.Active)))
	}
	if len(filters.FavColors) > 0 {
		query = query.WHERE(Accounts.FavColor.IN(Strings(filters.FavColors)...))
	}

Expected behavior
When passed a Filters struct with all three values set, I expected to see an SQL statement that included 3 clauses in the WHERE portion.
Instead, only the last one was set.

I expect this kind of "builder" behavior for all parts of the query, including the SELECT, the FROM, the ORDER_BY, everything. (I should be able to call SELECT multiple times, to add new columns to be selected. Same for FROM, ORDER_BY, everything, etc.)

Why? Because building complex queries or queries for complex api's, are often dependent on logic, so there is often a need to change parts of the query based if statements.

Yes, I realize an ugly workaround would be to build it separately, then pass it into JET when finished, but besides being ugly, it defeats a lot of the purpose of a query builder.

@veqryn veqryn added the bug Something isn't working label Aug 30, 2024
@go-jet
Copy link
Owner

go-jet commented Aug 31, 2024

What you describe as expected behavior is not an SQL builder but an ORM. SQL builder tries to mimics the underling sql without hiding any query details.

For instance:

	if len(filters.Names) > 0 {
		query = query.WHERE(Accounts.Name.IN(Strings(filters.Names)...))
	}
	if filters.Active != nil {
		query = query.WHERE(Accounts.Active.EQ(Bool(*filters.Active)))
	}

ORM will add silently AND between these two conditions, but sql builder should not. My only regret is not adding a panic if WHERE or any statement clause is called twice.

SQL builder tries to map golang DLS code to sql, one to one. Since sql SELECT query can have only one WHERE clause, sql builder SELECT statement should have only one WHERE clause.

condition := Bool(true)

if len(filters.Names) > 0 {
	condition = condition.AND(Accounts.Name.IN(Strings(filters.Names)...))  // AND is not hidden
}
if filters.Active != nil {
	condition = condition.AND(Accounts.Active.EQ(Bool(*filters.Active)))
}
if len(filters.FavColors) > 0 {
	condition = condition.AND(Accounts.FavColor.IN(Strings(filters.FavColors)...))
}

// nice and clean
query := SELECT(
	Accounts.AllColumns,
).FROM(
	Accounts,
).WHERE(
	condition,
)

Yes, I realize an ugly workaround would be to build it separately, then pass it into JET when finished, but besides being ugly, it defeats a lot of the purpose of a query builder.

Couldn't disagree more. When you are calling a go function, do you first call a function, and then modify it parameters, or you first calculate parameters and then call a function. It seems to me you spend to much time using GORM.

@veqryn
Copy link
Author

veqryn commented Sep 1, 2024

I have never used GORM, and I dislike ORM's quite a bit for many reasons.

I would consider an SQL builder to be something that builds an SQL statement.

Here, lets take github.com/Masterminds/squirrel for a try. It is a SQL builder, and that is all it does:

func (d DAO) SelectAllAccountsByFilter(ctx context.Context, filters models.Filters) ([]models.Account, error) {
	query := sq.
		Select(
			"id",
			"name",
			"email",
			"active",
			"fav_color",
			"fav_numbers",
			"properties",
			"created_at").
		From("accounts").
		OrderBy("id")

	if len(filters.Names) > 0 {
		query = query.Where(sq.Eq{"name": filters.Names})
	}
	if filters.Active != nil {
		query = query.Where(sq.Eq{"active": *filters.Active})
	}
	if len(filters.FavColors) > 0 {
		query = query.Where(sq.Eq{"fav_color": filters.FavColors})
	}

	sqlStr, args, err := query.PlaceholderFormat(sq.Dollar).ToSql()
	if err != nil {
		return nil, err
	}

The result is as expected, if there are multiple filters, each of them will be added onto the WHERE statement.

This is one of the main purposes of an SQL builder, to let you build out an SQL statement following business logic that often requires you to add to the SQL based on the results of if statements.

If I knew exactly what the SQL should look like, without any if statements involved, then I would just write straight SQL myself without any need for involvement by an SQL builder. (I often already do that everywhere there isn't any logic involved.)

Having to "build" the SQL statement myself manually, before passing it into Jet's "sql builder", seems like a big miss.

But speaking practically, maybe there is an opportunity for something like a WHERE_AND function.

@go-jet
Copy link
Owner

go-jet commented Sep 1, 2024

If you want to quote the SQL builder library, at least quote the real SQL builder library. For example, this is JOOQ for Java:

public Condition condition(HttpServletRequest request) {
    Condition result = noCondition();

    if (request.getParameter("title") != null)
        result = result.and(BOOK.TITLE.like("%"   request.getParameter("title")   "%"));

    if (request.getParameter("author") != null)
        result = result.and(BOOK.AUTHOR_ID.in(
            select(AUTHOR.ID).from(AUTHOR).where(
                    AUTHOR.FIRST_NAME.like("%"   request.getParameter("author")   "%")
                .or(AUTHOR.LAST_NAME .like("%"   request.getParameter("author")   "%"))
            )
        ));

    return result;
}

// And then:
create.select()
      .from(BOOK)
      .where(condition(httpRequest))
      .fetch();

By the looks of it, squirrel is more of an ORM than a SQL builder.

The result is as expected, if there are multiple filters, each of them will be added onto the WHERE statement.

Probably for the ORM users.

But speaking practically, maybe there is an opportunity for something like a WHERE_AND function.

For start WHERE_AND is not an SQL keyword. In some cases, it is worth adding a new statement method if there is a clear benefit. For example, the MODEL method of insert statement for type safety benefits. But, adding a new method just to support SQL builder anti-pattern programming is a big NO.

@veqryn
Copy link
Author

veqryn commented Sep 2, 2024

Squirrel is probably the most popular Golang SQL Builder library. It is not an ORM at all.

@houten11
Copy link

houten11 commented Sep 3, 2024

I don't think I would call squirrel a complete SQL Builder. Except for some basic sql keywords(like select, where, order by...), everything else is still basically a raw string. Meaning the risk of sql injection attacks is only slightly reduced compared to raw string.
Regarding WHERE_AND you can get something similar using global AND function:

conditions := []BoolExpression{Bool(true)}

if len(filters.Names) > 0 {
	conditions = append(conditions, Accounts.Name.IN(Strings(filters.Names)...))  
}
if filters.Active != nil {
	conditions = append(conditions, Accounts.Active.EQ(Bool(*filters.Active)))
}
if len(filters.FavColors) > 0 {
	conditions = append(conditions, Accounts.FavColor.IN(Strings(filters.FavColors)...))
}

query := SELECT(
	Accounts.AllColumns,
).FROM(
	Accounts,
).WHERE(AND(
	conditions...,
    ),
)

@trevlt
Copy link

trevlt commented Sep 4, 2024

If squirrel always adds AND after each Where, it means it is impossible to completely replace the where condition during SQL building phase. That feels like a limiting factor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants