Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Array expansion produces SQL with inconsistent parenthesis between RDBMS #1871

Open
JoshuaRogers opened this issue Jan 9, 2023 · 6 comments · May be fixed by #1874
Open

Array expansion produces SQL with inconsistent parenthesis between RDBMS #1871

JoshuaRogers opened this issue Jan 9, 2023 · 6 comments · May be fixed by #1874
Assignees

Comments

@JoshuaRogers
Copy link

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.

Dapper/Dapper/SqlMapper.cs

Lines 1988 to 2002 in 6ec3804

public static void PackListParameters(IDbCommand command, string namePrefix, object value)
{
// initially we tried TVP, however it performs quite poorly.
// keep in mind SQL support up to 2000 params easily in sp_executesql, needing more is rare
if (FeatureSupport.Get(command.Connection).Arrays)
{
var arrayParm = command.CreateParameter();
arrayParm.Value = SanitizeParameterValue(value);
arrayParm.ParameterName = namePrefix;
command.Parameters.Add(arrayParm);
}
else
{
bool byPosition = ShouldPassByPosition(command.CommandText);

... skipping further into the else ...

Dapper/Dapper/SqlMapper.cs

Lines 2121 to 2128 in 6ec3804

var sb = GetStringBuilder().Append('(').Append(variableName);
if (!byPosition) sb.Append(1); else sb.Append(namePrefix).Append(1).Append(variableName);
for (int i = 2; i <= count; i++)
{
sb.Append(',').Append(variableName);
if (!byPosition) sb.Append(i); else sb.Append(namePrefix).Append(i).Append(variableName);
}
return sb.Append(')').ToStringRecycle();

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
Copy link
Member

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

@JoshuaRogers
Copy link
Author

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

JoshuaRogers pushed a commit to JoshuaRogers/Dapper that referenced this issue Jan 10, 2023
…nt handling of list parameters regardless of RDBMS (DapperLib#1871)
JoshuaRogers pushed a commit to JoshuaRogers/Dapper that referenced this issue Jan 10, 2023
…nt handling of list parameters regardless of RDBMS (DapperLib#1871)
@JoshuaRogers
Copy link
Author

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

@southernprogrammer
Copy link

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

@mgravell mgravell self-assigned this Feb 19, 2024
@southernprogrammer
Copy link

@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?

@JoshuaRogers
Copy link
Author

JoshuaRogers commented Dec 12, 2024

Is there anything I can do to help escalate this issue? I've provided a pull request that adds an option that corrects this behavior but that does not change the default behavior. That has the effect of not breaking existing users. I've also updated the documentation in that pull request to explain how to use it.

I produce an app which has to support Postgres, SQL Server, and Oracle. At present, in order to work around this issue, I have to manually build SQL. (Thankfully I've only had the need to do this with inputs that I had control of, so no risk of user provided values, but it certainly puts me into a difficult position.)

Edit: Typos and clarification around default value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants