Implement where clauses on upsert for pg#2479
Conversation
|
If nobody from @diesel-rs/reviewers has time I will do a review after I'm back from holiday in sepember. |
|
@weiznich I should be able to leave a review on this in the next day or two. |
|
This was a little bit complex for me. I don't know the diesel code that much yet. |
|
@ELD is there any chance you can give me feedback on my PR in the next 1 or 2 days? |
|
@ropottnik Yes, absolutely. It's on my todo list for today. Sorry for taking so long to get back to this. |
ELD
left a comment
ELD
left a comment
There was a problem hiding this comment.
I took a quick look at this tonight. Again, sorry for taking so long to get to this. I’ll follow up in the morning and consider it a bit more closely. On the face of it, it looks good, though.
I should have answered your question regarding the prelude export for the DecoratableTarget trait. Hopefully that helps!
I’ll also give some thought to other test cases we may want for this.
Overall, it looks good! :)
diesel/src/query_builder/mod.rs
Outdated
| mod sql_query; | ||
| mod update_statement; | ||
| pub(crate) mod upsert; | ||
| pub use upsert::on_conflict_target_decorations::DecoratableTarget; |
There was a problem hiding this comment.
Minor nit:
It would be good to match the style of other pub use statements and do it like this:
pub use self::upset::on_conflict_target_decorations::DecoratableTarget;Also would be good to be moved down to the other use statements, as well.
diesel_tests/tests/debug/mod.rs
Outdated
| @@ -1,4 +1,5 @@ | |||
| use crate::schema::TestBackend; | |||
| use diesel::upsert::DecoratableTarget; | |||
There was a problem hiding this comment.
To resolve this to make it so use diesel::* also pulls in the DecoratableTarget, you should add the trait as a public reexport in lib.rs of Diesel in the inline prelude module.
See this for more details: https://github.com/diesel-rs/diesel/blob/master/diesel/src/lib.rs#L308
There was a problem hiding this comment.
aaah, makes sense, thanks!
| { | ||
| fn walk_ast(&self, mut out: AstPass<DB>) -> QueryResult<()> { | ||
| self.target.walk_ast(out.reborrow())?; | ||
| // out.push_sql(" "); |
There was a problem hiding this comment.
Is this a debug statement left in that is no longer effectual?
There was a problem hiding this comment.
oh yeah, that's just a remnant. I'll remove it.
|
@ELD I think I just figured out myself why I would get this nullable error. I need to have the same bounds for the Could it be that something there changed rather recently? Because when I submitted the PR, my test passed. Now all of a sudden I get these |
|
@ropottnik That's likely caused by #2182 |
|
@ELD @weiznich thanks for your comments. I could fix the test that I had added previously. Also I was able to simplify the I have also added one more test that checks repeated Could you have a look again? |
|
@ropottnik I'll give this another look this afternoon! |
|
Yes, going to carve out time this afternoon. I’m sorry I keep promising to look at it and then taking several extra days. Trying to get better about not getting distracted!
…
|
|
Thanks! |
|
|
||
| /// Interface to add information to conflict targets. | ||
| /// Designed to be open for further additions to conflict targets like constraints | ||
| pub trait DecoratableTarget<P> { |
There was a problem hiding this comment.
Is there any reason why this needs to be a separate trait and we cannot reuse FilterDsl here?
There was a problem hiding this comment.
Thanks for the review @weiznich!
My initial thought also was to use FilterDsl. However, I created a new trait for 2 reasons
- For me it is semantically different to add a
WHEREclause on aSELECTorUPDATEstatement which filters the statement as a whole. In contrast theWHEREclause added to a conflict target specifies the on conflict policy. These statements are usually chained together so to me it made sense to indicate that theWHEREclause groups with the conflict target. - I designed the
DecoratableTargetsuch that it easily allows for further specification of the on conflict policy such as adding constraint names as indicated in the PostgreSQL docs on insert.
does that make sense to you?
|
@ropottnik Can we backport this into 1.4.x? Do you mind if I use this PR as a base to open a new PR? @weiznich is this something you would be interested in backporting into 1.4.x? Were there any blockers preventing this to go back into 1.4.x release? |
|
@sumeetattree We do not accept backports of new features to old releases. In addition we also do not accept any backports to the 1.4.x release anymore as we do not have the capacity to maintain two major diesel versions at the same time. Please update to the 2.0 release to get access to the latest features. |
5 participants
closes #2430
Hey all! Since this here is my first ever PR written in Rust I'm especially keen on getting feedback for things that someone better at Rust would do totally different.
There are to issues with which I would need expert help anyway:
DecoratableTargettrait insidediesel_tests/tests/debug/mod.rs because I failed exporting it properly to be part of ause diesel::**statement. You can see my failed attempts indiesel/src/query_builder/mod.rsand others. If you could help me get these exports right I could remove that explicituse diesel::upsert::DecoratableTarget;indiesel_tests/tests/debug/mod.rs`"ON CONFLICT (...) WHERE ...". What other test scenarios would you like to have me cover in order to make sure it works.