-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: Add embed field attribute for AsChangeset #2529
base: master
Are you sure you want to change the base?
Conversation
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 working on this. This requires at least the following changes:
- Add the corresponding documentation on top of the
derive_as_changeset
method - Add at least one test case showing that this works
EDIT: I try to find some time to have a look at the CI errors. It seems like a regression in rustc 😞
EDIT2: Opened #2531 to fix the CI issues.
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 the update. Left 1 minor style comments + CI should pass.
diesel_tests/tests/update.rs
Outdated
name: None, | ||
hair_color: HairColor::Black, | ||
}) | ||
.get_result(&connection) |
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.
Better use a separate query here to load the results, as both Sqlite
and Mysql
do not support queries containing RETURNING
clauses. (Or use the UpdateAndFetchResult
trait impls.)
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 the hint! I was wandering about the error messages here.
[skip ci] Co-authored-by: Georg Semmler <[email protected]>
Well, I'm a little confused about the |
@heyrict I would probably try to dump the generate SQL via |
Thanks for the hint! I didn't touch the implementation of UPDATE "users" SET ("users"."hair_color" = $1) WHERE ("users"."id" = $2)
-- instead of
UPDATE "users" SET "hair_color" = $1 WHERE ("users"."id" = $2) |
let field_ty = &field.ty; | ||
parse_quote!(diesel::dsl::Eq<#table_name::#column_name, #lifetime #field_ty>) | ||
parse_quote!(#lifetime #field_ty) |
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.
From looking again at this here I think we must call <#lifetime #field_ty as AsChangeSet>::Changeset
here`
if lifetime.is_some() { | ||
parse_quote!(self#field_access.as_ref().map(|x| #table_name::#column_name.eq(x))) | ||
if field.has_flag("embed") { | ||
parse_quote!(#lifetime self#field_access) |
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.
That should then be <#lifetime #field_ty as AsChangeset>::as_changeset(self#field_access)
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! I'll try that this weekend.
Closes #2527