Gravity: fixed a bug where natural join was not correctly processed by sql_group_start

Review Request #410 — Created Feb. 5, 2016 and submitted

rubendgr
Lunr
rubendgr:natural_join_fix
lunr

[Gravity] fixed a bug where natural join was not correctly processed by sql_group_start

Unit test.

  • 0
  • 0
  • 12
  • 0
  • 12
Description From Last Updated
There are no open issues
rubendgr
pprkut
  1. 
      
  2. src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    Ok, so the logic here looks fine in general, but there are several issues with this solution:

    1) $this->is_natural_join is not set to TRUE anywhere
    2) I'm not entirely sure using two flags is simpler than just not setting $this->join to TRUE in case of a natural join

    1. I agree with all those points.
      I think it's not easy to make sure everything works fine with only the unit tests.
      Especially on that part as we don't have other kind of tests to check the construction of full queries.
      You would have spotted the first issue if you would have tried to actually create the query for real
      I would recommend you try those queries on a small project

    2. The is_natural_join is removed and is_join is changed to is_unfinished_join. Now there is only one flag.

  3. The issue has been resolved. Show all issues

    Kind of defeats the purpose, no?
    I mean, a natural join never has an ON clause, so testing with one is kind of pointless. Would make more sense to test with WHERE or HAVING here.

    1. Changed to WHERE.

  4. 
      
rubendgr
pprkut
  1. 
      
  2. The issue has been resolved. Show all issues

    I think substr would be better here. We know exactly what we're looking for

  3. src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php (Diff revision 2)
     
     
     
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    why are two flags better than one?

  4. 
      
rubendgr
tardypad
  1. 
      
  2. The issue has been resolved. Show all issues

    AFAICS this should be "if the join is not finished"

    1. Improved the comment.

  3. src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    if this is really enough to fix the problem, I would rename this variable to something more clear

    1. The is_join is renamed to is_unfinished_join.

  4. src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php (Diff revision 3)
     
     
     
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    Please make sure your IDE is not autoformatting to his own coding style

  5. src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsTest.php (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    This test is not coherent between what the text says it tests, its name and its content

    1. Changed the comments of this test.

  6. 
      
rubendgr
rubendgr
rubendgr
rubendgr
pprkut
  1. 
      
  2. The issue has been resolved. Show all issues

    boolean variables/methods should start with "is" if possible, to convey that they contain a boolean value. In this case I guess I would prefer "is_join_in_progress", "is_unfinished_join" or "is_incomplete_join".

  3. src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php (Diff revision 4)
     
     
     
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    I don't think we need to explicitely set it to FALSE.

  4. 
      
rubendgr
rubendgr
tardypad
  1. 
      
  2. The issue has been resolved. Show all issues

    better simply use !=
    it's much easier to read

    1. I agree and changed it to !=

  3. The issue has been resolved. Show all issues

    "DontSet"

  4. 
      
rubendgr
pprkut
  1. Ship It!
  2. 
      
rubendgr
Review request changed

Status: Closed (submitted)

Change Summary:

Merged into master

Loading...