fix rendering of joins

[Philonous]

Esqueleto exhibits strange behaviour with respect to joins. All joins seem to be left-associated even though the join Constructors are right associative. Explicitly adding parenthesis to left-associate the constructors adds seemingly superfluous parenthesis to the generated SQL. On the other hand there is no way to generate right-associated joins in the SQL. This pullrequest takes a stab at fixing this. While the tests run though fine, I'm a bit unsure about this. I don't understand SQL well enough to be certain that this is correct in all cases.

This is related to #8

The downside of this patch is that the default associativity of the join operators now leads to different SQL code, e.g.

foo = from $ \((p1 :: SqlExpr (Entity DB.Product))
              `InnerJoin` (p2 :: SqlExpr (Entity DB.Product))
              `InnerJoin` (p3 :: SqlExpr (Entity DB.Product))) -> do
    on (p3 ^. DB.ProductName ==. p1 ^. DB.ProductName)
    on (p2 ^. DB.ProductName ==. p1 ^. DB.ProductName)
    return ( p1 ^. DB.ProductName
           , p2 ^. DB.ProductName
           , p3 ^. DB.ProductName
           )

used to compile into

SELECT "product"."name", "product2"."name", "product3"."name"
FROM "product" INNER JOIN "product" AS "product2" ON "product2"."name" = "product"."name" INNER JOIN "product" AS "product3" ON "product3"."name" = "product"."name"

But after this patch it becomes

SELECT "product"."name", "product2"."name", "product3"."name"
FROM "product" INNER JOIN ("product" AS "product2" INNER JOIN "product" AS "product3" ON "product3"."name" = "product"."name") ON "product2"."name" = "product"."name"

However, this is arguably the correct behaviour, as long as InnerJoin etc. are right-associative. Changing the associativity to infixl 2 restores the old behaviour and seems more intuitive to begin with, but can change the type of existing code.

[meteficha]

Thanks for looking into this, @Philonous!

I'm also wary of changing this behaviour, especially since it's possible to change the meaning of a query while shuffling parenthesis (source).

I have a few questions I'd like to be answered before we land this:

• Disregarding backwards compatibility, is it better to associate constructors to the left or to the right?
• Can you write test cases showing current esqueleto failing and your new version working? BTW, would the example I linked above be enough?
• Are we going to make any valid queries in current esqueleto invalid? If yes, we need at least a major version bump.
• Are we going to change the meaning of any valid queries in current esqueleto? If yes, we'll need to be extra careful, make a blog post and some mailing list announcements.

[meteficha]

Note to self: regardless of the answers to the questions above, it's probably going to be a good idea to ask people on the mailing lists to test their code against new esqueleto just to be sure we didn't miss anything.

[meteficha]

@Philonous, after staring at your commits for some time, I think you got it right. But I don't feel too confident on my own judgement. Actually, I'm the one who introduced this bug in the first place ;-). So, I'd still appreciate if you could take a stab at the questions I've stated above.

[Philonous]

1) I feel that left-associating would be the more natural choice. It avoids parens in the SQL and generally seems to be the intuitive thing to do (Also, the old behaviour was left-associative, even though the constructors right-associated)

2) An example for the old behaviour failing would be visible for (p LeftOuterJoin (q InnerJoin r)). The expected behaviour is to have a row for every p even when the inner join is empty. In the old version, this join would be rendered as ((p LeftOuterJoinq) InnerJoin r), where the inner join might drop rows from p. (In this particular case the order of the joins could be changed to work around the problem, but it becomes impossible when both sides of the outer join are inner joins)

Edit: See 451beb9a559bcfc0c02e13bacbb647511f00b9a3 for a test. The test fails without this patch and succeeds with it.

3 and 4) I think, but am not entirely certain, that the semantics of existing queries should be preserved as long as the constructors are changed to be left-associative. However, the type of expressions and patterns like (p InnerJoin q InnerJoin r) would change, leading to possible breakage, so a major version bump would be necesary.

[jonkri]

+1

[meteficha]

@Philonous, instead of editing older comments, in the future please write a new comment. I just saw that you wrote the test case due to @jonkri's comment. I don't get e-mail notifications for new commits nor comment edits.

I'll merge and ask for feedback later, but thanks in advance!

[Philonous]

Oops sorry

[jonkri]

Great, thanks! Any idea when this could be merged and released onto Hackage?

[meteficha]

@Philonous, thank you for your PR, it's merged! I've bumped the version to 2.2.

@jonkri, I'm going to send a few e-mails asking people to test this new version. If everything goes well, I'll release it to Hackage in the next couple of weeks. Please let me know if it works for your codebase and if you had to make any adjustements for it to work.

[meteficha]

Tying the knot: https://groups.google.com/forum/#!topic/yesodweb/duwc5jcxgDY .

[jonkri]

Thanks! The sooner this can be released, the better it would be for us; we have some features lined up that would need this patch. :) For us, only a few type signatures changed, and so it was just a matter of deleting them and having GHC infer them. Take care!

[erikd]

All fine for my biggest Esqueleto using project.

[meteficha]

Thank you for your reports, @jonkri and @erikd, I'm getting more confident with these changes.

[jonkri]

Well, then I should probably mention that me and @Philonous are working on the same code base, so... :)

[meteficha]

I've released esqueleto-2.2. Thanks, everyone!

[jonkri]

Great! Thanks! :)