-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Comments
Yes, I think a PR for that would be fine and reasonable |
Thank you. I'll start on that right away. :) |
…nt handling of list parameters regardless of RDBMS (DapperLib#1871)
…nt handling of list parameters regardless of RDBMS (DapperLib#1871)
I've implemented the change in the above pull request. What should I do next? |
@mgravell Do you feel anything else need to be done here? Just seeing that this is still not merged. |
@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? |
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. |
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:
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
... skipping further into the else ...
Dapper/Dapper/SqlMapper.cs
Lines 2121 to 2128 in 6ec3804
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 runwhere 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 readId 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
, possibleForceArrayExpansion
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.
The text was updated successfully, but these errors were encountered: