Skip to content
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

Add initial support for composite primary keys #417

Merged
merged 2 commits into from
Aug 29, 2016

Conversation

sgrif
Copy link
Member

@sgrif sgrif commented Aug 25, 2016

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

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
@sgrif
Copy link
Member Author

sgrif commented Aug 25, 2016

Review? @diesel-rs/contributors

@killercup
Copy link
Member

LGTM. Do you maybe want to add some docs on how to use this? (Mentioning that the table! macro allows a comma separated list of columns for composite primary keys should suffice.)

@sgrif sgrif merged commit 0c7693c into master Aug 29, 2016
@sgrif sgrif deleted the sg-composite-primary-keys branch August 29, 2016 14:00
@sgrif
Copy link
Member Author

sgrif commented Aug 29, 2016

(I added docs before merging)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants