Array expansion produces SQL with inconsistent parenthesis between RDBMS

[JoshuaRogers]

First, much to Dapper's credit, amongst the other wonderful functionality it providers, it provides a way for all of its supported databases to be able to use arrays as parameters, regardless of whether the underlying driver provides such support itself or not. This allows for easy and safe passing of an arbitrary number of parameters without requiring consumers to fall back on string concatenation. This is definitely an awesome feature, but due to an under-the-hood detail in how this expansion is performed, writing SQL that uses arrays and is intended to work with multiple database providers is problematic.

The crux of the issue is that, when performing expansion, Dapper conditionally adds a set of parenthesis around the section being expanded. Taking the example from Readme.md:

Dapper allows you to pass in IEnumerable<int> and will automatically parameterize your query.

For example:

connection.Query<int>("select * from (select 1 as Id union all select 2 union all select 3) as X where Id in @Ids", new { Ids = new int[] { 1, 2, 3 } }); 

Will be translated to:

select * from (select 1 as Id union all select 2 union all select 3) as X where Id in (@Ids1, @Ids2, @Ids3)" // @Ids1 = 1 , @Ids2 = 2 , @Ids2 = 3 

However, Dapper does not unconditionally attempt to perform expansion, but only does so if the underlying driver lacks support for arrays. In the event that the driver does support arrays, no expansion is performed, and the original SQL is passed to the database driver as-is.

https://github.com/DapperLib/Dapper/blob/6ec3804f2c44f2bf6b757dc3522bf009cc64b27d/Dapper/SqlMapper.cs#L1988-L2002 ... skipping further into the else ... https://github.com/DapperLib/Dapper/blob/6ec3804f2c44f2bf6b757dc3522bf009cc64b27d/Dapper/SqlMapper.cs#L2121-L2128

The practical effect of this is that Dapper adds parenthesis around arrays for all databases except Postgres and ClickHouse, since those are the only two that are listed as supporting arrays (based on the values set in https://github.com/DapperLib/Dapper/blob/a31dfd3dd4d7f3f2580bd33c877199d7ef3e3ef9/Dapper/FeatureSupport.cs).

This means that, given the original SQL from the Readme.md, Postgres would attempt to execute where Id in @Ids, whereas another database, such as Oracle, would attempt to run where Id in (@Ids1, @Ids2, @Ids3). Without the addition of parenthesis, the above is considered invalid. If, however, I add parenthesis to the original SQL so that it becomes valid for Postgres then the expanded form, as would run on Oracle, would read Id in ((@Ids1, @Ids2, @Ids3)) and would fail, due to the extra parenthesis.

In order to allow for a consistent interface for array handling that could be used by multiple database providers, would you be willing to consider the addition of a new SqlMapper.Setting, possible ForceArrayExpansion that could be enabled parameter expansion even if they driver already has array support? This would allow a path for consistent handling without breaking backwards-compatibility by changing default behavior.

If this would be acceptable, I'd be more than willing to implement it and provide a MR for critique.

[mgravell]

Yes, I think a PR for that would be fine and reasonable

[JoshuaRogers]

Thank you. I'll start on that right away. :)

[JoshuaRogers]

I've implemented the change in the above pull request. What should I do next?

[southernprogrammer]

@mgravell Do you feel anything else need to be done here? Just seeing that this is still not merged.

[southernprogrammer]

@mgravell Thank you for changing the status to needs-review to get the ball rolling a bit more on this. Any idea on a timeline?