-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
DRAFT PR: Feature batch update #4916
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
base: main
Are you sure you want to change the base?
Conversation
…ple of Assign structs. Update first working BatchUpdate test named_struct_batch.
weiznich
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening this draft PR and sorry for taking that long to response. There was just too much stuff going on in the last weeks to take the time to really look into this.
Hopefully the comments I made help to understand how to finish this.
Otherwise I think it might also be a good idea to:
- Add at least one compile test to show that this won't compile with Sqlite/MysqlConnection (it should already be the case, I just want to have a test for this for the future)
- Add a changelog entry as this is a relevant new feature
| impl<'a, T> AsChangeset for &'a [T] | ||
| where | ||
| T: AsChangeset, | ||
| // TODO: [UndecoratedUpdateRecord] could be used to enforce setting a where_clause maybe. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the where clause needs to be generated automatically
|
|
||
| // TODO: Create proper struct for alias that implements QueryFragment. | ||
| // Maybe there is already something? | ||
| const ALIAS: &str = "\"tmp\""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine to use a hard coded name for this, but maybe something with less collision potential like __diesel_internal_temp_values or similar
| fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, DB>) -> QueryResult<()> { | ||
| if let None = self.values.first() { | ||
| // TODO: more meaningful return maybe? | ||
| return Ok(()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the "normal" changesets we return an error if it's empty
| out.push_sql(")"); | ||
| } | ||
|
|
||
| // id = primary key -> therefore not available in update batch! See Note below. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should get the primary key names via Table::primary_key()
We then also need to ensure that
- we don't list these keys in the assign in the actual update (that is likely already the case?)
- the update struct contains primary key values. Requiring
Identifiableon the struct should give us the right information there. We can list these values as first/last values in theVALUESclause
No description provided.