Enclosing column names in square brackets breaks Dapper.Contrib in PostgresSQL

[oliverw]

Enclosing column names in square brackets completely breaks Dapper.Contrib in PostgresSQL.

insert into products([Title], [ManufacturerId],) values((('Vibe P1')::text), ((1)::int4)) RETURNING Id

[johandanforth]

@oliverw Could you please explain a bit more what you are trying to do. The dapper.contrib insert extension has been tested quite a lot I believe.

[oliverw]

Well, the queries above, and the one below have been generated by Dapper.Contrib (latest version from NuGet). When running the query below through NpgSql 3.0 (though happens on 2.x as well) it results in the following error (sorry for the German error message, I have yet to figure out how to force the PGSQL error log to be in English):

2015-09-16 12:24:17 CEST ANWEISUNG update rss_product_feeds set [Url] = $1, [Region] = $2, [Marketplace] = $3, [Name] = $4, [Created] = $5, [LastProcessed] = $6, [TimesProcessed] = $7 where [Id] = $8 
2015-09-16 12:24:17 CEST FEHLER Syntaxfehler bei '[' bei Zeichen 30

If I patch SqlMapperExtensions.cs to omit generating the square brackets, everything works as expected.

[NickCraver]

@oliverw sanity check here: I see an extra comma in your column list - you sure that's not a contributing factor here...or is that a mis-paste?

[AndrewSmart]

See mrjoltcola's explanation for a detailed understanding of this issue.

This square bracket issue causes Dapper.Contrib to not work with MySQL, and as oliverw noted, PostgresSQL.

As a solution, I suggest augmenting the ISqlAdapter interface with a columnList creation method, such that the column names are quoted with appropriate delimiters for each SQL provider where appropriate.

[tms]

Just for history, #60 and #298 introduced the non-universal escaping. Something similar almost happened with Dapper.Rainbow in #167 too, but @mgravell reverted after I pointed out it broke compatibility. Whatever solution is decided on here should probably be applied to that code as well, since it currently doesn't allow escaping.

[johandanforth]

Thanks for the pointers @tms So double quote seems to be the right way of escaping column names on most database platforms but I'll spend some time today and dig into it. This must be solved.

[johandanforth]

Seems that the SQL-99 standard specifies that double quote (") is used to delimit identifiers, but at least MySQL has to be in ANSI mode and SQL Server only supports it when QUOTED_IDENTIFIER is ON. The contrib extensions for Insert, Update and Delete all use brackets, not just the Insert. So the question is, should we replace the brackets with double quotes everywhere and refer to the standard (and risk breaking working code out there)? Or perhaps add some other way for devs to supply a escape character, with a delegate or other method?

[Tewr]

Introducing delimiter of a specific dialect was probably a mistake, so forcing another, even if standard, would probably be a mistake as well. Specifying delimiters in the ISqlAdapter implementation as suggested by @AndrewSmart could be the way to go as brackets may be used in sql server regardless of the QUOTED_IDENTIFIER setting. Don't know about MySql ANSI mode though. I guess the ansi mode setting could be read (SELECT @@SESSION.sql_mode; ?) in a specific MySql ISqlAdapter adapter and eventually be cached on the connection instance. The impact of breaking working code may be somewhat limited as the current state of affairs seem to have broken some code already. Brackets would be the default as it is now (as SqlServerAdapter is default) and the other adapters could use double quote (except the specific MySql adapter, then?).

[AndrewSmart]

@Tewr In MySQL backticks as the identifier delimiter will always work regardless of its ANSI_QUOTES setting.

[johandanforth]

Yes @Tewr and @AndrewSmart , specifying delimiters in ISqlAdapter implementations feels right to me too. The sql server adapter could keep using brackets as you wrote. Not sure at all about the proper way to handle mysql, but from what I can google, http://dev.mysql.com/doc/refman/5.6/en/identifiers.html says a "backtick" should be used. And for sqllite and postgressql use double quotes. How bout that?

I'll take a look at how to implement this in all parts needed, Insert(), Update() and Delete() and their corresponding async methods.

[tms]

@johandanforth Agreed, that seems like the right approach. I was going to go ahead and implement something yesterday, but I wanted to confirm first with you, @NickCraver, and @mgravell there was no concerns with changing that interface. If you have the free time though I'll defer to you :)

[AndrewSmart]

@johandanforth Sounds good to me. Though thinking back to what @tms said in #167, I think it would be optimal to not escape every identifier in pursuit of Dapper's maximal performance goal. I'd suggest to have users either not use reserved keywords, or use another data annotation such as [Table("Tablename", EscapeIdentifiers=true)]. That data annotation could drive the escaping as implemented in ISqlAdapter implementations.

[johandanforth]

@AndrewSmart hmmm, yeah, it will impact performance. Not much, but still. I tend to agree with you, but if the default would be to not escape names, that would affect some existing code out there which depend on the existing brackets around column names.