/diesel

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

Feature request: Boxed where clauses for delete statements #1128

Closed
quadrupleslap opened this Issue Aug 25, 2017 · 6 comments

Comments

Assignees
No one assigned
Labels
Projects
None yet
Milestone
  1.2
4 participants
@quadrupleslap @Eijebong @sgrif @ivanovaleksey
@quadrupleslap

quadrupleslap commented Aug 25, 2017

A query that has been created dynamically with into_boxed() can't be passed to diesel::delete. Is there an alternative for dynamically adding filters to deletes or is there no workaround for this feature that looks like it's missing?
@Eijebong

This comment has been minimized.

Sign in to view
Member

Eijebong commented Aug 25, 2017

I think this is probably a bug as I don't see why boxed queries shouldn't be used with a delete statement.

@Eijebong Eijebong added the bug label Aug 25, 2017

@sgrif

This comment has been minimized.

Sign in to view
Member

sgrif commented Sep 30, 2017

The reason the impl isn't there is that not all boxed queries are valid delete statements. If any clause is present other than the where clause, it would result in an error. However, now that we have filter on Update and DeleteStatement directly, I'd be fine with a separate BoxedDeleteStatement and BoxedUpdateStatement (or even just boxing the where clause on those by default -- there's not a huge drawback to doing so)

@sgrif sgrif changed the title from Cannot use BoxedDsl with delete. to Feature request: Boxed where clauses for delete statements Sep 30, 2017

@sgrif sgrif removed the bug label Jan 17, 2018

@sgrif

This comment has been minimized.

Sign in to view
Member

sgrif commented Jan 17, 2018

I actually think boxing the where clauses by default would have been the right way to go. Unfortunately, we can't change that now that we're post-1.0. I'm definitely interested in having BoxedUpdateStatement and BoxedDeleteStatement though. The API surface on those is relatively small. I don't think we need to support upsert for boxed update statements (unless it's easy to add support). Boxed update statements should box the values as well as the where clause.

I'm happy to mentor someone interested in working on this. It'll be a decent sized patch, but it's pretty straightforward once you understand the basics of our codebase, and BoxedSelectStatement is a perfect reference for this, as the implementation will be quite similar.

@sgrif sgrif added the mentoring available label Jan 17, 2018

@sgrif sgrif added this to the 1.2 milestone Jan 17, 2018

sgrif added a commit that referenced this issue Feb 3, 2018

@sgrif
Add `DeleteStatement::into_boxed`. 
This is one half of #1128. This method (and the added type) correspond
to `SelectStatement::into_boxed` and `BoxedSelecStatement`. I had
originally stated that I thought we could just box the where clause by
default, but we could no longer do that due to backwards compatibility.
Neither of those statements are actually true.

While we can't remove the type parameter for the where clause, it is
documented that you are not allowed to rely on the types that parameter
can hold, or make any assumptions about them. So we could either change
it to be an unused parameter, or enforce that it is always a boxed where
clause.

However, we can't just change it to be unused, as we need to capture a
lifetime somewhere. So if we wanted to re-use `DeleteStatement` for
this, we'd have to go the second route of having that parameter always
be a boxed where clause. I realize now that we don't actually want to do
that though, as it would require `delete` to take the database the query
would be run against.

There's more duplicated code than I'd like (basically all of this code
was copied from either `DeleteStatement` or `BoxedSelectStatement`, but
I'm not sure that there's a ton we can do about this. It's unfortunate
that we basically have to remember to update `BoxedDeleteStatement` any
time we do anything to `DeleteStatement`, but that's no different than
the situation with selects.

While conditionally modifying the where clause of a delete statement is
definitely going to be less common than doing it for a select, I do
think that this is a reasonable feature, and one worth having.
20610b2

@sgrif sgrif referenced this issue Feb 3, 2018

Merged

Add `DeleteStatement::into_boxed`. #1534

@ivanovaleksey

This comment has been minimized.

Sign in to view
Contributor

ivanovaleksey commented Mar 4, 2018

I would like to give it a try.
However, I am not very fluent in Rust and I totally don't understand how Diesel works.
Do you have extra-patience for this? 😄
@sgrif

This comment has been minimized.

Sign in to view
Member

sgrif commented Mar 4, 2018

@ivanovaleksey I appreciate the offer, but this has already been mostly implemented (support for delete statements is merged, I have a branch with support for update statements that I need to clean up one last thing on before pushing up)
@ivanovaleksey

This comment has been minimized.

Sign in to view
Contributor

ivanovaleksey commented Mar 4, 2018

Yeah, I see merged #1534 and I wonder doesn't it close this issue.
Thank you for your work!

sgrif added a commit that referenced this issue Apr 5, 2018

@sgrif
Implement boxed update statements 
Just like delete statements, the only thing we actually want to box here
is the where clause. I considered boxing the values clause, but I
couldn't come up with any use case to do so that isn't already handled
by `Option`.

I felt silly re-implementing every method on the statement just to box
the where clause *again* so I factored out that handling, and got rid of
the explicit `BoxedDeleteStatement`.

Fixes #1128.
fdb4085

@sgrif sgrif referenced this issue Apr 5, 2018

Merged

Implement boxed update statements #1619

sgrif added a commit that referenced this issue Apr 5, 2018

@sgrif
Implement boxed update statements 
Just like delete statements, the only thing we actually want to box here
is the where clause. I considered boxing the values clause, but I
couldn't come up with any use case to do so that isn't already handled
by `Option`.

I felt silly re-implementing every method on the statement just to box
the where clause *again* so I factored out that handling, and got rid of
the explicit `BoxedDeleteStatement`.

Fixes #1128.
af25cd9

@sgrif sgrif closed this in #1619 Apr 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment