-
-
Notifications
You must be signed in to change notification settings - Fork 526
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
Let last_insert_id
optional and flexible.
#2393
base: master
Are you sure you want to change the base?
Conversation
8e19e10
to
8ed9b89
Compare
8ed9b89
to
2495b40
Compare
cc @billy1624 @tyt2y3 |
last_insert_id
optional.last_insert_id
optional and flexible.
This reverts commit 3356023.
e5a7246
to
fb8ea9d
Compare
b1d27f2
to
60d0964
Compare
Some attempts were rolled back because gluesql/gluesql#1579
Temporarily circumvent the problem of not being able to correctly query the newly inserted elements caused by the upstream warehouse. gluesql/gluesql#1579
hi, thank you for your contribution. this is such a big breaking change. may I ask what's the usecase for it? can you provide an example? we have a TryInsert struct, may be it is more suitable for what you're trying to do? |
The example revision is already included in this PR. By the way, I took a look at the implementation of I am considering whether I need to make further improvements to the question I just asked.. |
PR Info
I'm sorry for submitting such a PR so presumptuously. I found that the type of
last_insert_id
is flawed.last_insert_id
will not be returned anyway, because Postgres may directly use a non-integer primary key.last_insert_id
is entirely up to the user. If the user cannot get a valid ID from the actual database transferred (for example, for databases such assurrealdb
, their IDs are UUIDs rather than integers), it is unreasonable to return a 0 directly.This change is likely to affect a considerable number of downstream libraries, so it is a breaking change. Whether to make such a change requires discussion and decision.
Breaking Changes
last_insert_id
optional.last_insert_id
follows the primary key's type.I am not in a hurry to merge now. I am trying to make more radical changes to synchronize
last_insert_id
directly with the type corresponding to PrimaryKey instead of hard-coded u64 type. It may adapt to more complex table structures.In addition to the fact that this field should be optional, I also consider the default generation of PrimaryKey. As mentioned in another discussion I cited, what we actually need is not necessarily a query logic that "may not return
last_insert_id
", but that we can generate the primary key by SeaORM instead of the database engine if necessary.Combining these two requirements, we get a
last_insert_id
that can be generated in advance and returned normally without hard-coding it asu64
. Not only meet the needs of primary keys that may not necessarily return u64 type, but also specify the "default value" of this primary key which is generated by us in advance rather than provided by the database engine itself.Finally, let's review the scenarios where this is applicable. One type is to use such as UUID, ULID, GUID as the primary key of the database table. The other type is to use the creation time as the index of the database table. Both of these types belong to the categories that the database engine cannot guarantee to have their corresponding implementations. We should indirectly provide auxiliary implementations through ORM.