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

Support composite foreign keys #956

Closed
YetAnotherMinion opened this Issue Jun 18, 2017 · 1 comment

Comments

Projects
None yet
2 participants
@YetAnotherMinion
Contributor

YetAnotherMinion commented Jun 18, 2017

What do you want to do and how do you expect Diesel to support you with that?

I would like to use a composite foreign key on the child table when I am using composite primary keys on the primary table. I would be happy with the ability to implement the BelongsTo trait for the child struct and the respective JoinTo trait for the parent struct while continuing to derive Insertable, Queryable, and Identifiable for both child and parent.

How do you think this can be added to Diesel?

Modify the signature of BelongsTo::foreign_key(&self) from a type reference to just a type: change return type from Option<&Self::ForeignKey> to Option<Self::ForeignKey>. Then update the BelongsTo! macro to set BelongsTo::ForeignKey to &$foreign_key_ty instead: line 151. Also change the BelongsTo! macro on line 154 to return the Option<Self::ForeignKey> instead of Option<&$foreign_key_ty>.

I have no idea. I tried above modification and I do not yet understand how to fix the GroupBy trait.


The end result would be the user can manually implement BelongsTo trait for a composite foreign key like so

pub struct FakeForeignKey((child_table::id_1, child_table::id_2));

impl diesel::Column for FakeForeignKey {                                        
    type Table = table_b::table;                                                
                                                                                
    fn name() -> &'static str { "my_foreign_key" }                              
}                                                                               
                                                                                
impl diesel::Expression for FakeForeignKey {                                    
    type SqlType = (diesel::types::Int, diesel::types::Int);                  
}

impl diesel::associations::BelongsTo<parent_table> for Child {
    type ForeignKey = (&i32, &i32);
    type ForeignKeyColumn = FakeForeignKey;

    fn foreign_key(&self) -> Option<Self::ForeignKey> {
         Some(&self.id_1, &self.id_2)
    }
    fn foreign_key_column() -> Self::ForeignKeyColumn {
        FakeForeignKey((child_table::id_1, child_table::id_2))
    }
}

For real bonus points the derive macro could be changed to support specifying a composite foreign key so that composite foreign keys could be derived just like Identifiable is right now.

Possibly ForeignKeyColumn: Column could be replaced with ForeignKeyExpression: Expression (because it looks like it is only used in one place and it only uses the eq_any function. I don't understand the design intention of ForeignKeyColumn type, but perhaps the trait bound could be relaxed here? The presence of the extra trait type ForeignKeyColumn appears to be the primary difference from the Identifiable trait.

A similar change to support composite primary keys was implemented in #510, and it looks like there will be a similar level of effort to support composite foreign keys.

What are possible alternatives?

Don't change anything and keep using separate structs for Insertable and Queryable.

Are there any disadvantages?

It is a public API change so a major version bump will be required.
Those using derive should not notice, but everyone who has a custom implementation of BelongsTo trait will have to update their code. However the lifetimes of everything will stay the same, so looks like it would be a easy change for users.

@sgrif

This comment has been minimized.

Member

sgrif commented Dec 16, 2017

Unfortunately, this isn't going to happen in the near future. If we can add this backwards compatibly once generic associated types land, we will revisit it then. Right now though, I'm clearing out any issues that require breaking changes or new language features, as they're not actionable at this point in time.

@sgrif sgrif closed this Dec 16, 2017

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