tomjaguarpaw / haskell-opaleye

Other
602 stars 115 forks source link

Using the Opaleye Json operator (.->>) in a restrict (within a runQuery) causes a fatal SQL error in runtime #350

Closed kahlil29 closed 6 years ago

kahlil29 commented 6 years ago

Note : This issue occurs with PostgreSQL 9.4.4

Using the .->> operator to extract out a part of a PGJsonb column in a restrict gives the following error:

SqlError {sqlState = "42883", sqlExecStatus = FatalError, sqlErrorMsg = "operator does not exist: jsonb ->> boolean", sqlErrorDetail = "", sqlErrorHint = "No operator matches the given name and argument type(s). You might need to add explicit type casts."}

The following is the code with which I encountered the error :

runSearchQuery :: (HasDatabase m) => String -> m[FilteredJob]
runSearchQuery searchParam = runQuery $ proc() -> do
  result <- leftJoinQueryJobUserClient -< ()  
  restrict -< ((fromNullable (result^._5) (toNullable(result^._1).->>(pgStrictText "tag"))) .== (pgStrictText $ toS searchParam)) 
  returnA -< result
tomjaguarpaw commented 6 years ago

I suspect some kind of parenthesis precedence error. Could you send me the SQL it generates? You'll have to run Opaleye.Sql.showSql on your Query.

kahlil29 commented 6 years ago

Adjusted the query so that the SQL is as short as possible :

showSQL :: Maybe String
showSQL = Opaleye.showSql $ proc() -> do
  result <- queryTable Job.tableForJob -< ()  
  restrict -< ((fromNullable (pgStrictText "Default") (toNullable(result^. payload).->>(pgStrictText "tag"))) .== (pgStrictText "QueuedMail")) 
  returnA -< result

The SQL generated is :

SELECT "id0_1" as "result1_2",
       "created_at1_1" as "result2_2",
       "updated_at2_1" as "result3_2",
       "client_id3_1" as "result4_2",
       "user_id4_1" as "result5_2",
       "status5_1" as "result6_2",
       "payload6_1" as "result7_2",
       "last_error7_1" as "result8_2",
       "attempts8_1" as "result9_2",
       "run_at9_1" as "result10_2",
       "locked_at10_1" as "result11_2",
       "locked_by11_1" as "result12_2"
FROM (SELECT *
      FROM (SELECT "id" as "id0_1",
                   "created_at" as "created_at1_1",
                   "updated_at" as "updated_at2_1",
                   "client_id" as "client_id3_1",
                   "user_id" as "user_id4_1",
                   "status" as "status5_1",
                   "payload" as "payload6_1",
                   "last_error" as "last_error7_1",
                   "attempts" as "attempts8_1",
                   "run_at" as "run_at9_1",
                   "locked_at" as "locked_at10_1",
                   "locked_by" as "locked_by11_1"
            FROM "jobs" as "T1") as "T1"
      WHERE ((CASE WHEN ("payload6_1") ->> E'tag' IS NULL THEN E'Default' ELSE ("payload6_1") ->> E'tag' END) = E'QueuedMail')) as "T1"

On Postgres 9.4.4 it returns :

ERROR:  operator does not exist: jsonb ->> boolean
LINE 27:       WHERE ((CASE WHEN ("payload6_1") ->> E'tag' IS NULL TH...
                                                ^
HINT:  No operator matches the given name and argument type(s). You might need to add explicit type casts.

On Postgres 9.5.9 it runs without an issue and returns the appropriate rows.

tomjaguarpaw commented 6 years ago

Can you run the same query through psql or postgresql-simple or something? Try changing the CASE to

CASE WHEN (("payload6_1") ->> E'tag') IS NULL THEN E'Default' ELSE ("payload6_1") ->> E'tag' END) = E'QueuedMail'))

I suspect there's an ambiguity in operator precedence.

kahlil29 commented 6 years ago

Yes that seems to have done the trick @tomjaguarpaw
It did not give me the error after replacing the CASE with the one you've given.

tomjaguarpaw commented 6 years ago

I suspect the fix is to use ParensSqlExpr on this line, like the line above. Do you want to try it?

https://github.com/tomjaguarpaw/haskell-opaleye/blob/bdeb800242738dcbd5f03de0307e94f89700f295/src/Opaleye/Internal/HaskellDB/Sql/Default.hs#L118

kahlil29 commented 6 years ago

Yeah will do tomorrow! Thanks

saurabhnanda commented 6 years ago

@kahlil29 we already have a fork at https://github.com/vacationlabs/haskell-opaleye to which you have access. Create a branch there and if the fix works, raise a PR.

tomjaguarpaw commented 6 years ago

Note Table 4-2 in version 9.4 and version 9.5. They changed the relative precedence of IS NULL and "other native and user-defined operators"!

saurabhnanda commented 6 years ago

Why does toNullable need to do anything at the SQL level? I believe in SQL, any identifier/ column can be NULL. Non-nullability is checked only while storing data into a column - I don't believe it exists at the type level.

tomjaguarpaw commented 6 years ago

toNullable doesn't do anything at the SQL level. fromNullable does.

saurabhnanda commented 6 years ago

Ah okay - my bad. Can we replace that CASE ... WHEN with COALESCE?

tomjaguarpaw commented 6 years ago

Oh I see, in this use case you have to redudantly cast to nullable in order to use .->>. Yes, this is a design flaw addressed in https://github.com/tomjaguarpaw/haskell-opaleye/issues/97

A hacky workaround could be to add

instance PGIsJson (Nullable T.PGJson)
instance PGIsJson (Nullable T.PGJsonb)
tomjaguarpaw commented 6 years ago

Yes, I think we probably could. What would be the benefit of that?

tomjaguarpaw commented 6 years ago

See https://github.com/tomjaguarpaw/haskell-opaleye/issues/353

tomjaguarpaw commented 6 years ago

I explained that fromNullable is similar to COALESCE: https://github.com/tomjaguarpaw/haskell-opaleye/commit/e62cd7190981d4d1f179061e241603394cb821b5

We should still support COALESCE though.

kahlil29 commented 6 years ago

Just finished writing a test for this issue (while testing the fix).

testRestrictWithJsonOp :: (O.PGIsJson a) => Query (Column a) -> Test
testRestrictWithJsonOp dataQuery = it "restricts the rows returned by checking equality with a value extracted using JSON operator" $ testH query (`shouldBe` table8data)
  where query = dataQuery >>> proc col1 -> do
          t <- table8Q -< ()
          O.restrict -< (O.fromNullable (O.pgStrictText "Default") (O.toNullable col1 O..->> O.pgStrictText "c") ) .== (O.pgStrictText "21")
          Arr.returnA -< t

The above test function reproduces the error and it is fixed with your suggested fix in Default.hs @tomjaguarpaw

However, one thing that I noted is that if I change the restrict clause and use the following :

testRestrictWithJsonOp :: (O.PGIsJson a) => Query (Column a) -> Test
testRestrictWithJsonOp dataQuery = it "restricts the rows returned by checking equality with a value extracted using JSON operator" $ testH query (`shouldBe` table8data)
  where query = dataQuery >>> proc col1 -> do
          t <- table8Q -< ()
          O.restrict -< (O.toNullable col1 O..->> O.pgStrictText "c") .== O.toNullable (O.pgStrictText "21")
          Arr.returnA -< t

The error does not occur.

Is this expected? (Based on your understanding of why exactly this error occurs.) Just thought I'd point it out before raising the PR.

kahlil29 commented 6 years ago

@tomjaguarpaw Did you take a look at my findings above? Anything peculiar about this or is it alright?

tomjaguarpaw commented 6 years ago

Your change to the restrict clause won't trigger the bug because it doesn't use fromNullable and therefore it doesn't use IS NULL.

tomjaguarpaw commented 6 years ago

So it's totally expected.