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

ToSql on Text hits SQL Server stack limit #32

Open
spencerjanssen opened this issue Mar 16, 2021 · 6 comments
Open

ToSql on Text hits SQL Server stack limit #32

spencerjanssen opened this issue Mar 16, 2021 · 6 comments

Comments

@spencerjanssen
Copy link
Contributor

The ToSql instance for Text can generate very large expressions of + which throws the following error on my installation of SQL Server.

Internal error: Server stack limit has been reached. Please look for potentially deep nesting in your query, and try to simplify it.

The use-case that caused us to hit this was a modestly-sized HTML document, but here's a more minimal example:

blowup cn = query cn ("select " <> toSql (Data.Text.replicate 2000 ">"))
@spencerjanssen
Copy link
Contributor Author

Is the character escaper overly defensive? As a workaround I'm using a simpler scheme which only replaces ' with '' and passes all other characters through. It does appear to work on [0 .. 255] and the few Unicode strings I've tried.

@chrisdone
Copy link
Contributor

chrisdone commented Mar 16, 2021

It's overly defensive, yeah.

Does your approach pass the property test suite? That might be a fine fix.

The Right(tm) fix for this is to support binding parameters via the odbc API. That doesn't have to be an API change for the odbc package, either. As the Query type is abstract, values could be separated from select "xyz" into select ? and [StringValue "xyz"] underneath automatically (and thus benefiting the user who doesn't have to make sure their ?s and parameter list match up). The Part type has ValuePart so it's easy to do this separation.

The harder part is probably doing the necessary FFI call.

I don't think it would be necessary for most of the types, but for text and binary it could be a significant security and performance boost.

@spencerjanssen
Copy link
Contributor Author

Tests for nvarchar pass with my approach but varchar has some problems. PR coming in a bit.

  test/Main.hs:343:15: 
  4) Database.ODBC.SQLServer, Conversion to SQL, QuickCheck roundtrip: HS=Maybe ByteString, SQL=varchar(1024)
       Assertion failed (after 20 tests):
         "\SUB`t\153\&3"
         Expected: Just "\SUB`t\153\&3"
         Actual: Just "\SUB`t?3"
         Query was: INSERT INTO test VALUES (('`t3'))

spencerjanssen added a commit to spencerjanssen/odbc that referenced this issue Mar 16, 2021
spencerjanssen added a commit to spencerjanssen/odbc that referenced this issue Mar 16, 2021
@chrisdone
Copy link
Contributor

I'm working on proper query parametrization (using the odbc API) this week. It'll be automatic and won't introduce an API change.

@spencerjanssen
Copy link
Contributor Author

I did write a little code for binding parameters after I reported this issue, feel free to use or not: spencerjanssen/odbc@e836897

I wasn't able to resolve round-trip issues with higher characters. The ODBC driver insists on treating C char strings as UTF-8 which means we can't just send a byte like \143, it gets mapped to ?. UTF-8 encoding the ByteString as if each byte were a code point might work but I didn't get that far.

@chrisdone
Copy link
Contributor

@spencerjanssen Your code is very similar to mine, so that's good. We can't both be wrong. 😂

I'll keep an eye on the encoding issue, I'm using slightly different SQL types. I have to test some more, but got side-tracked on another task temporarily.

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

No branches or pull requests

2 participants