/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

Support composite primary keys #42

Closed
sgrif opened this Issue Dec 2, 2015 · 1 comment

Comments

Assignees
No one assigned
Labels
Projects
None yet
Milestone
  0.9
1 participant
@sgrif
@sgrif
Member

sgrif commented Dec 2, 2015

I believe this should be straightforward. There's a couple of things to consider

Edit: The thoughts in this comment are super outdated, and likely no longer relevant.

  • Right now Table::PrimaryKey has the constraint Column<Table=Self>. Unsure if we want to relax that to SelectableExpression, or if we want to introduce the Columns trait. We could even implement Column for tuples now if we really want to, as there's no longer any automatically added behaviors.
  • With the way our type system is set up, conn.find(&posts_tags, &(1, 1)) would be more likely to generate SELECT * FROM posts_tags WHERE (post_id, tag_id) = $1, instead of SELECT * FROM posts_tags WHERE post_id = $1 AND tag_id = $2. Might be fine for an initial implementation, but we do eventually need to solve this to support other databases.

@sgrif sgrif added the enhancement label Dec 20, 2015

@sgrif sgrif added this to the 0.6 milestone Feb 5, 2016

@sgrif

This comment has been minimized.

Sign in to view
Member

sgrif commented Feb 5, 2016

This will likely be much easier once we implement the Identifiable trait that we're looking to add to support associations as part of 0.6

@sgrif sgrif modified the milestones: 1.0, 0.7 Jun 27, 2016

@sgrif sgrif modified the milestones: 0.8, 1.0 Aug 11, 2016

sgrif added a commit that referenced this issue Aug 25, 2016

@sgrif
Add initial support for composite primary keys 
This pull request allows the `table!` macro and codegen to handle tables
which use more than one column for their primary key. At the moment they
are fairly useless, but they will compile.

The methods in question are primarily used for `FindDsl` and
associations. Things like update are defined in terms of `FindDsl` and
`Identifiable`. Since the generators for those traits don't handle
composite primary keys, none of that can be used (yet).

Working on this also made me realize a bit of a hole in our type system.
We implement `Expression` for a tuple of expressions as a comma
delimited list. However, a comma delimited list isn't always valid. In
particular a composite table *does* actually implement `FindDsl` for an
expression that matches it's primary key -- and would generate invalid
SQL like `pk1, pk2 = $1, $2`. Luckily, we're saved by the fact that
tuples don't implement `AsExpression`, so trying to do `.find((1, 2))`
won't compile.

I'm not sure if we should fix this or not, but the fix would basically
mean separating out "arbitrary expression" from "expression valid as a
select or returning clause", and potentially other places where tuples
appear such as set clauses and in expressions. For now though, as long
as the inproper code doesn't compile for the reasonable cases, I'm not
super worried. We should fix this eventually though, as I don't like
something that we can statically determine is invalid SQL compiling.

Ref #42
bc1a22f

@sgrif sgrif referenced this issue Aug 25, 2016

Merged

Add initial support for composite primary keys #417

@jimmycuadra jimmycuadra referenced this issue Sep 25, 2016

Closed

Composite primary keys #450

sgrif added a commit that referenced this issue Dec 8, 2016

@sgrif
Allow `#[derive(Identifiable)]` to work with composite primary keys 
This is a tad bit magic in the macro piece. We're expecting the primary
keys *not to* have a trailing comma, so we can rely on the fact that
`(T)` is equivalent to `T`, but if there's more than one element it
would be a tuple.

Beyond that everything was quite straightforward with the ground work
that we've laid.

Fixes #42.
7df5fdb

@sgrif sgrif referenced this issue Dec 8, 2016

Merged

Allow `#[derive(Identifiable)]` to work with composite primary keys #533

sgrif added a commit that referenced this issue Dec 8, 2016

@sgrif
Allow `#[derive(Identifiable)]` to work with composite primary keys 
This is a tad bit magic in the macro piece. We're expecting the primary
keys *not to* have a trailing comma, so we can rely on the fact that
`(T)` is equivalent to `T`, but if there's more than one element it
would be a tuple.

Beyond that everything was quite straightforward with the ground work
that we've laid.

Fixes #42.
ec99769

sgrif added a commit that referenced this issue Dec 8, 2016

@sgrif
Allow `#[derive(Identifiable)]` to work with composite primary keys 
This is a tad bit magic in the macro piece. We're expecting the primary
keys *not to* have a trailing comma, so we can rely on the fact that
`(T)` is equivalent to `T`, but if there's more than one element it
would be a tuple.

Beyond that everything was quite straightforward with the ground work
that we've laid.

Fixes #42.
Loading status checks…
edd145e

@sgrif sgrif closed this in #533 Dec 8, 2016

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