Fixes for issue #351 and #336

[johandanforth]

Affects Dapper.Contrib only. Added support for application supplied keys, introduced a new [ExplicitKey] attribute for this. Added new tests for this as well as a couple of test projects for MsSql and SQLite.

[NickCraver]

I'm not following how this would work, e.g. if the ID column is a GUID a lot of these chains still assume integer behavior. We'd only really be supporting int and "less than int"...or am I missing something?

[johandanforth]

Yes you're right, it's really an attempt to prevent breaking the interface.

[NickCraver]

I think a lot of this has good changes, but I don't agree with the direction of Insert, because it doesn't account for non-interger keys and IMO introduces more problems inadvertently. While an object's property would be set right, for non-interger or integer convertible types we're returning 0. IMO that's very misleading and can easily lead to breaks.

I'd propose we have T Insert<T> and Task<T> InsertAsync<T> for other key types where the return value is actually useful. This can be a non-breaking change with existing methods calling into their int versions (which would be JIT inlined). This would offer a wider range of support and cover issues such as #351 more properly.

[NickCraver]

Side note: I'm not sure about the ISqlAdapter changes - I kind of agree on having all of this on the base, but I think it needs to be an abstract class at this point (rather than an unversioned interface) given the number of things we're seeing diverge on platforms down-the-road style. It's a wider issue than this PR because there are more things elsewhere like array handling, TVPs, etc. Thoughts? /cc @mgravell

What are your thoughts on this? /cc @mgravell as well

[johandanforth]

Thanks for the review @NickCraver ! Good thoughts on the Insert() parts, and I agree with you on going with T Insert<T> and . Task<T> InsertAsync<T>. As for ISqlAdapter - it's clear I need to read up on effects of changing interfaces or having abstract classes instead and what is best for projects like this. Looking forward to what you and @mgravell says. I'll be happy to do the necessary work on the code :)

[NickCraver]

@johandanforth We met up and concur - can you implement the Insert underneath as-is today? The int Insert() can just call into Insert<int> underneath without breaking the API. There are much better ways to handle the outgoing bits for GUID, etc. via OUTPUT and RETURNING than are done today - I can take care of that.

If you can fix the Insert methods up (or I can) and rename the parameters, we're ready to do a lot of PR merging and code splitting to make life much easier for anyone working on Dapper. Also, the plan is to make ISqlAdapter into an abstract class (which isn't practically exposed today since the dictionary is private), but I'd much rather do that post pull request since PR merging is the bottleneck.

Also, can you make sure you've enabled 2-factor auth? Removing from the org was due to not detecting it - if you enable 2-factor auth on GitHub I can re-add you to the project :)

Thanks!

[NickCraver]

I merged this in manually with the fixes so we can keep rolling /cc @mgravell - I figure we can do the split today then I can work the Insert. As-is @johandanforth's work fixes longs and such so it's net-win already. Making it work for all key types next can just be another commit.

Thanks @johandanforth! and please ping me when the 2-factor is ready to go - if I add you before that's in place, our security scripts will just keep reverting it.

[johandanforth]

Been really busy, but I'll take some time to both read through all comments from your massive work on the repo and fix the 2-factor thingy soon. You guys have sure been busy lately ;)

[johandanforth]

Ping @NickCraver I've got 2FA setup now!

[johandanforth]

I've now given a lot of love to the Insert methods, added a T Insert as discussed above. Will create a PR when I've tested it a bit more, but it looks great and I'm pretty pleased with what I'm looking at 😊 /cc @NickCraver @mgravell

[AndrewSmart]

I got the time to try this out, thank you again for your work! Though looks like I'm late it was already merged into master.

You got the last_insert_id() right for MySQL, instead of last_insert_rowid()
I ran Dapper.Contrib.Tests, and the ShortIdentity() test failed here:

public void ShortIdentity()
        {
            using (var connection = GetOpenConnection())
            {
                var id = connection.Insert(new Stuff() { Name = "First item" });
                id.IsEqualTo(1);

id was 0, so it was not equal to 1 as expected.

I just followed the lead of the SQLiteAdapter.Insert method in changing the above to:

var cmd = String.Format("insert into {0} ({1}) values ({2}); select last_insert_id() id", tableName, columnList, parameterList);
var r = connection.QueryMultiple(cmd, entityToInsert, transaction, commandTimeout);
var id = (int)r.Read().First().id; //(int) cast necessary as MySQL returns last_insert_id() as ulong.
//if (id == null) return 0; //this line no longer necessary.

With that change all tests passed when using MySQL.

[johandanforth]

nice @AndrewSmart I'll change the code and make sure it comes along in the end (If I ever get through all the "under the cover" changes I'm deep in... 😕 I'm so thinking of starting on a fresh Dapper.Contrib.2 :D

[neeasade]

Hi, I'm looking to insert with Guid generated ID and I'm having a bit of trouble flowing through related issues -- the conclusion here was a delay/Dapper.Contrib.2, and currently we are without support for non-database-generated GUID as PK currently, right?