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

InsertResult to return the primary key's type #103

Closed
tyt2y3 opened this issue Aug 23, 2021 · 8 comments · Fixed by #117
Closed

InsertResult to return the primary key's type #103

tyt2y3 opened this issue Aug 23, 2021 · 8 comments · Fixed by #117
Assignees

Comments

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 23, 2021

Currently, the last_insert_id from InsertResult simply uses the ExecResult's last_insert_id.
However, at least for Postgres with returning, we should be able to retrieve the primary key column for the Entity and return the value in its native type. For example, a uuid instead of an i64.

@tyt2y3 tyt2y3 changed the title InsertResult return the column's type, not i64 InsertResult to return the column's type, not i64 Aug 23, 2021
@billy1624
Copy link
Member

How we handle composite primary keys?

@tyt2y3
Copy link
Member Author

tyt2y3 commented Aug 24, 2021

Composite primary keys can't be auto generated, and so it's less important. But I think the type could be a tuple, if we really try to support it. Let's leave it and return 0 for now.

@billy1624
Copy link
Member

I think this issue can be related to #101 if we have an attribute primary_key. Then, we can derive a struct to represent value of returning columns (primary keys).

@billy1624
Copy link
Member

billy1624 commented Aug 24, 2021

And btw... returning statement in Postgres can return multiple columns at once.
https://www.postgresql.org/docs/current/dml-returning.html

@tyt2y3
Copy link
Member Author

tyt2y3 commented Aug 24, 2021

I think for the average case where there is only one column as primary key, adding one container layer is counter intuitive.
But perhaps, we can make an associated type def in the PrimaryKeyTrait

impl PrimaryKeyTrait for PrimaryKey {
    type ValueType = i32;
}

@billy1624
Copy link
Member

Adding type def is a good suggestion

@tqwewe
Copy link
Contributor

tqwewe commented Aug 26, 2021

I've had a dive into the code for this.. it seems like adding ValueType on the PrimaryKeyTrait would work great!
Then perhaps InsertResult can be given a generic:

pub struct InsertResult<T> {
    pub last_insert_id: T,
}

And exec on Insert could return the value type as the generic type:

impl Future<
  Output = Result<InsertResult<<A::Entity as EntityTrait>::PrimaryKey::ValueType>, DbErr>
>

This what I'm thinking would be the best way..

@tyt2y3 tyt2y3 added this to the 0.2.0 milestone Aug 26, 2021
@tyt2y3
Copy link
Member Author

tyt2y3 commented Aug 26, 2021

Agreed.

@tyt2y3 tyt2y3 changed the title InsertResult to return the column's type, not i64 InsertResult to return the primary key's type, not i64 Aug 26, 2021
@tyt2y3 tyt2y3 changed the title InsertResult to return the primary key's type, not i64 InsertResult to return the primary key's type Aug 26, 2021
@billy1624 billy1624 mentioned this issue Sep 3, 2021
11 tasks
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 a pull request may close this issue.

3 participants