-
Notifications
You must be signed in to change notification settings - Fork 114
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
Comments
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 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,
)
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. |
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 I knew exactly what the SQL should look like, without any 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 |
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,
Probably for the ORM users.
For start |
Squirrel is probably the most popular Golang SQL Builder library. It is not an ORM at all. |
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. 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...,
),
) |
If squirrel always adds |
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):
Code snippet
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.
The text was updated successfully, but these errors were encountered: