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

SQL Mapper Extensions GetFormatter returns incorrect adapter when using Transacted Connection

Open ATECoder opened this issue 5 years ago • 4 comments

In the code below, the 'name' is set to TransactedConnection, which, naturally, does not have an entry in the AdapterDictionary, returning the default SQL Server adapter. Most likely, an exception is needed for a Transacted Connection getting the connection name from the Conn property of the Transacted Connection

` private static ISqlAdapter GetFormatter(IDbConnection connection) { var name = GetDatabaseType?.Invoke(connection).ToLower() ?? connection.GetType().Name.ToLower();

        return AdapterDictionary.TryGetValue(name, out var adapter)
            ? adapter
            : DefaultAdapter;
    }

` This modified code seems to address this issue:

` private static ISqlAdapter GetFormatter(IDbConnection connection) { if (connection is TransactedConnection) { connection = ((TransactedConnection)(connection)).Connection; }

        var name = GetDatabaseType?.Invoke(connection).ToLower()
                   ?? connection.GetType().Name.ToLower();

        return AdapterDictionary.TryGetValue(name, out var adapter)
            ? adapter
            : DefaultAdapter;
    }

`

ATECoder avatar Jun 03 '20 18:06 ATECoder

The problem there is: TransactedConnection is something that only exists in the tests, not the library. There is no well-known "I'm a decorator" pattern, for DbConnection, or in general. Are you using TransactedConnection somewhere?

mgravell avatar Jun 04 '20 10:06 mgravell

Yes, I am using the Transacted Connection thinking it is a super clever way to implement a transaction on existing code.


From: Marc Gravell [email protected] Sent: Thursday, June 4, 2020 3:49:29 AM To: StackExchange/Dapper [email protected] Cc: David Hary [email protected]; Author [email protected] Subject: Re: [StackExchange/Dapper] SQL Mapper Extensions GetFormatter returns incorrect adapter when using Transacted Connection (#1478)

The problem there is: TransactedConnection is something that only exists in the tests, not the library. There is no well-known "I'm a decorator" pattern, for DbConnection, or in general. Are you using TransactedConnection somewhere?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FStackExchange%2FDapper%2Fissues%2F1478%23issuecomment-638773752&data=02%7C01%7C%7Cecc8f2cfbd914b2ba47608d80874f61e%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637268645718068993&sdata=uKyeOZT9k%2FwM27rId4aEJmxn7yv23wvIrYrB8waV8TU%3D&reserved=0, or unsubscribehttps://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAAGBGSQHB7GNDMH5TIXW2ILRU533TANCNFSM4NR5KN4A&data=02%7C01%7C%7Cecc8f2cfbd914b2ba47608d80874f61e%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637268645718078985&sdata=TrRSAFG5oAo4UVPuZaLjngYFVugASq2Whe8YODgSd7M%3D&reserved=0.

ATECoder avatar Jun 04 '20 13:06 ATECoder

To be explicit: the test code is not part of "the library" that we support. If there's a needed feature here, we can look into it, but: I'm not sure we should focus too much on TransactedConnection itself.

mgravell avatar Jun 04 '20 16:06 mgravell

Thank u for the quick response and for the great work u guys are putting into Dapper. It is much appreciated!

Seeing how Transacted Connection allowed me to use transactions on existing code, Transacted Connections seem like a feature many can use.

Meanwhile, it has become part of my fork of the Library.

Also in that fork are:

  • removing the 's suffix to table names;
  • support for table inserts with 'default' values as with a table of an auto increment key and default timestamp.

Another issue I had to deal with is the Library defaulting to assume a field named 'id' is a key field where in fact I used it as a foreign key to another table. Renaming the field solved that issue. Perhaps it might be helpful to used an attribute called 'field' for designating a simple field.

Also, I was wondering if the Library could benefit from a set of settings such as 'require attribute' for keys and optional appending of 's to table names. In fact, supporting plurality for table names requires language-specific dictionaries. Ergo my preference for dropping plurality altogether.

ATECoder avatar Jun 04 '20 18:06 ATECoder