Dapper.Contrib icon indicating copy to clipboard operation
Dapper.Contrib copied to clipboard

Dapper.Contrib cannot handle GUID primary keys & SqlMapperExtensions.Insert<T> has wrong usage int & long

Open SwissMaWi opened this issue 3 years ago • 9 comments

SqlMapperExtensions.Insert<T> will fail when numeric db keys reach the Int32 limit (yes, this happens on high traffic dbs) and also fails completely since it assumes primary keys are integers. If primary keys are not integers (i.e. GUIDs or something else) the library fails completely. Is there any interest for an improvement? I have some proposition in mind.

`        /// <summary>
        /// Inserts an entity into table "Ts" and returns identity id or number of inserted rows if inserting a list.
        /// </summary>
        /// <typeparam name="T">The type to insert.</typeparam>
        /// <param name="connection">Open SqlConnection</param>
        /// <param name="entityToInsert">Entity to insert, can be list of entities</param>
        /// <param name="transaction">The transaction to run under, null (the default) if none</param>
        /// <param name="commandTimeout">Number of seconds before command execution timeout</param>
        /// **<returns>Identity of inserted entity, or number of inserted rows if inserting a list</returns>**
        public static **long** Insert<T>(this IDbConnection connection, T entityToInsert, IDbTransaction transaction = null, int? commandTimeout = null) where T : class
`

SwissMaWi avatar Mar 22 '21 09:03 SwissMaWi

@SwissMaWi , You are right it indeed has a limitation. Could you please format the code?

@mgravell or @SamSaffron , your thoughts?

maulik-modi avatar Mar 31 '21 08:03 maulik-modi

My proposition is to separate insertion of single rows and lists. Then we will be able to remove the limitation if primary key types to int and make it generic or dynamic. Primary keys can be anything of int, (n)varchar of any kind and of course GUIDs. The latter is important for security to remove the possibility of key guessing. The list insertion could then return a list of inserted primary keys.

SwissMaWi avatar Mar 31 '21 09:03 SwissMaWi

@SwissMaWi , Do you mean something like https://github.com/tmsmith/Dapper-Extensions/blob/master/DapperExtensions/DapperImplementor.cs

I had to use DapperExtensions because it allows Fluent mapping in separate class as opposed to Attributes decorated on top of entities.

maulik-modi avatar Mar 31 '21 09:03 maulik-modi

This was my reason for not using this lib and creating my own. Check out dapper.database https://github.com/dallasbeek/Dapper.Database

dallasbeek avatar Jun 17 '21 03:06 dallasbeek

@dallasbeek let's improve dapper... this is sustainable. @NickCraver should I work out an improvement here?

SwissMaWi avatar Jun 17 '21 05:06 SwissMaWi

Just tried to implement something with minimal changes, however it is not possible since if you do not use DB autoincrement INT keys, the pattern of primary key creation is entirely different. I think we have to separate the concern of auto incrementing PK from the concern of PK type first, before this can be implemented. No matter how, it will cause major changes in the INSERT workflow I believe. Regarding GUID keys: should the application create them or should Dapper offer to "autocreate" them?`I would need some opinions here...

SwissMaWi avatar Jun 17 '21 06:06 SwissMaWi

@SwissMaWi , EF Core has sequential GUID generator https://github.com/dotnet/efcore/blob/main/src/EFCore/ValueGeneration/SequentialGuidValueGenerator.cs

Also EF Core has a method or attribute for ValueGeneratedNever https://www.learnentityframeworkcore.com/configuration/fluent-api/valuegeneratednever-method

maulik-modi avatar Jun 17 '21 06:06 maulik-modi

@maulik-modi so in your opinion generating GUID keys should be the concern of Dapper.Contrib? I am aware that .Net GUIDS are affine to SQL Server GUIDS but what about other DB systems?

SwissMaWi avatar Jun 17 '21 06:06 SwissMaWi

@SwissMaWi ,

  1. Ability to opt-out from auto incremented Id/PK using explicit option ValueGeneratedNever or DatabaseGenerated:False
  2. Allow to declare TId type, find record by TId
  3. For inserting multiple, I doubt there is native support in ADO.NET to return back list of generated Id e.g. https://github.com/dotnet/runtime/issues/28633 is still open
  4. GUID generation should not be kept in Dapper.contrib

maulik-modi avatar Jun 17 '21 07:06 maulik-modi