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

Parameter binding change seems to have moved from nchar to ntext #39

Open
robx opened this issue Sep 13, 2021 · 7 comments
Open

Parameter binding change seems to have moved from nchar to ntext #39

robx opened this issue Sep 13, 2021 · 7 comments

Comments

@robx
Copy link

robx commented Sep 13, 2021

Not quite sure who/what's wrong here, but thought I'd log this one anyway since others might run into similar issues.

I tried updating hasura/graphql-engine to build against odbc-0.2.5 (currently it builds against 7c0cea45d0b779419eb16177407c4ee9e7ba4c6f which is between 0.2.3 and 0.2.4), and I see test failures with

[Microsoft][ODBC Driver 17 for SQL Server][SQL Server]The data types nvarchar and ntext are incompatible in the equal to operator.

Reading through the recent changes to this package, I believe this is likely to be related to how commit 555182e changes text values to be passed via withBindParameter as a TextParam t, where it was previously encoded directly via renderValue.

Now it may well be that the tests are just wrong, but if so, it seems like the change in behaviour might be worth mentioning in the changelog?

@spencerjanssen
Copy link
Contributor

I also encountered this #34 (comment). We're still using the approach described in the comment of casting to nvarchar(max), which is working well for us. At the time I think I looked into whether it was possible to avoid ntext for at least short strings, but it didn't work out for reasons that escape me at the moment.

@chrisdone
Copy link
Contributor

Thanks chaps. I don't have time right now to look at this in detail, but I understand that explicit binding vs the more implicit text rendering causes comparison issues.

@chrisdone
Copy link
Contributor

@spencerjanssen Do you have the commit for your patch? I'm curious what approach you took.

@spencerjanssen
Copy link
Contributor

Sorry, the workaround isn't in any public repository.

In our codebase we use a ToQuery class which for most types is the same as Database.ODBC.SQLServer.ToSql. For Text we expand to cast(? as nvarchar(max)).

class ToQuery a where
    toQuery :: a -> Query
    default toQuery :: ToSql a => a -> Query
    toQuery = toSql

-- odbc treats Text as ntext, which has all sorts of problems including equality comparison
-- and use in AT TIME ZONE
instance ToQuery Text where
    toQuery x = "cast(" <> toSql x <> " as nvarchar(max))"

@chrisdone
Copy link
Contributor

Ah, thanks. That wouldn't cover the parametrized case that we're discussing (https://github.com/hasura/graphql-engine-mono/issues/2913#issuecomment-1010162201), but good to know. 👌

@chrisdone
Copy link
Contributor

I could wrap up the ? with a cast to nvarchar(max), though. So that tells me something helpful.

@chrisdone
Copy link
Contributor

Related attempt: #44

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

3 participants