WHERE expressions sometimes need parens and don't get them

[hdgarrood]

For example:

q :: SqlQuery (Value Int)
q = do
  where_ (subSelectUnsafe (pure (val True)))
  pure (val 0)

renders as

SELECT ?
WHERE SELECT ?

which is a syntax error, at least under Postgres. I commented on #79 already thinking it was related, but after a second look I'm not sure.

If we generated this instead, I think we'd be okay.

SELECT ?
WHERE (SELECT ?)

I think this might be due to the parens field of ERaw being ignored in makeWhere:

makeWhere :: IdentInfo -> WhereClause -> (TLB.Builder, [PersistValue])
makeWhere _    NoWhere                       = mempty
makeWhere info (Where v) = first ("\nWHERE " <>) $ x info
  where
    x =
        case v of
            ERaw _ f             -> f
            EAliasedValue i _    -> aliasedValueIdentToRawSql i
            EValueReference i i' -> valueReferenceToRawSql i i'
            ECompositeKey _      -> throw (CompositeKeyError -> EsqueletoError
CompositeKeyErr MakeWhereError)

[belevy]

Is that even legal syntax in the SQL standard. I mean I'm fine with adding the parens in as long as it doesn't cause a regression for other backends but what the hell is that query doing.

[parsonsmatt]

postgres accepts it:

matt=# select 1 where (select true);
 ?column? 
----------
        1
(1 row)

It's a minimal repro, not a real query lol

[hdgarrood]

I've no idea what the SQL standard says as I've never been able to get hold of it, sadly. (Do you know how?)

It's just a minimal reproducing example. The real case where I'm running into this is along the lines of

select $ do
  p <- from $ Table @Password
  where_ (subSelectForeign p PasswordUserId (exists $ do ...))
  pure p

and while it should be possible to refactor to avoid sub-selects, it's awkward in this case because of how my code is factored.

[hdgarrood]

I had a go at this in https://github.com/bitemyapp/esqueleto/commit/3b9469b059e913dbc3d6f7acc88b0d40400835e9 but I'm now getting this, which I think is unlikely to break anything but it does seem less than ideal:

esqueleto         >   test/Common/Test.hs:1845:7: 
esqueleto         >   1) Tests that are common to all backends, testRenderSql, works
esqueleto         >        expected: "SELECT Person.name, Person.age\nFROM Person\nWHERE Person.name = ?\n"
esqueleto         >         but got: "SELECT Person.name, Person.age\nFROM Person\nWHERE (Person.name = ?)\n"

(but I don't think I can justify spending much more time on this right now unfortunately)

[belevy]

So I think it should be perfectly straightforward to refactor to an InnerJoin on your subquery or a lateral cross join if you need access to p inside of the subquery. Esqueleto 3.4 let's you just InnerJoin do

[parsonsmatt]

@belevy I think we need to make sure this works, even if it's possible to rewrite the query - esqueleto's ability to decompose and recombine query fragments means that it's not always feasible to alter the structure of a query

[hdgarrood]

Yes, it should be possible to refactor what I've written above, but there's more to it than that still: in my real code, the argument to where_ is a type class member whose implementation might have no sub selects, or might have many sub selects, depending on the instance in question. I just inlined it above so that it was easier to understand.

It seems reasonable to expect that queries which type check in Haskell ought not to produce syntax errors at least, doesn't it?

[hdgarrood]

I've worked around this issue in my code by adding a val True &&. to the argument to where_, which forces parens to be added.

[belevy]

Oh no doubt this ought to work. Just proposing workarounds that avoids sub selects