- Download Diff

Summary:

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

Review Request #410 — Created Feb. 5, 2016 and submitted Feb. 19, 2016, 4:38 p.m.

Information
Owner:	rubendgr
Repository:	Lunr
Branch:	rubendgr:natural_join_fix
Bugs:
Depends On:
Reviewers
Groups:	lunr
People:

Description

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

Testing Done

Unit test.

Issues

0
0
12
0
All Issues: 12

Description	From	Last Updated
Ok, so the logic here looks fine in general, but there are several issues with this solution: 1) $this->is_natural_join is ...	pprkut	Feb. 12, 2016, 1:22 p.m.
Kind of defeats the purpose, no? I mean, a natural join never has an ON clause, so testing with one ...	pprkut	Feb. 12, 2016, 1:23 p.m.
I think substr would be better here. We know exactly what we're looking for	pprkut	Feb. 12, 2016, 1:23 p.m.
why are two flags better than one?	pprkut	Feb. 12, 2016, 1:24 p.m.
AFAICS this should be "if the join is not finished"	tardypad	Feb. 12, 2016, 1:24 p.m.
if this is really enough to fix the problem, I would rename this variable to something more clear	tardypad	Feb. 12, 2016, 1:25 p.m.
Please make sure your IDE is not autoformatting to his own coding style	tardypad	Feb. 12, 2016, 1:25 p.m.
This test is not coherent between what the text says it tests, its name and its content	tardypad	Feb. 12, 2016, 1:25 p.m.
boolean variables/methods should start with "is" if possible, to convey that they contain a boolean value. In this case I ...	pprkut	Feb. 12, 2016, 1:26 p.m.
I don't think we need to explicitely set it to FALSE.	pprkut	Feb. 12, 2016, 1:27 p.m.
better simply use != it's much easier to read	tardypad	Feb. 15, 2016, 2:25 p.m.
"DontSet"	tardypad	Feb. 15, 2016, 2:25 p.m.

Description:

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

src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php (Diff revision 1)

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

tardypad Feb. 5, 2016, 3:11 p.m.

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

rubendgr Feb. 12, 2016, 1:27 p.m.

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

src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsTest.php (Diff revision 1)

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.

rubendgr Feb. 12, 2016, 1:27 p.m.
```
Changed to WHERE.
```

Change Summary:

made an extra test to test if there is a natural join and improved my other test with natural join

Diff:

Revision 2 (+60 -3)

Show changes

	src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsJoinTest.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsTest.php

src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php (Diff revision 2)
Show all issues
```
I think substr would be better here. We know exactly what we're looking for
```

src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php (Diff revision 2)

Show all issues

why are two flags better than one?

Change Summary:

replaced explode by substr and made the methods more simple

Diff:

Revision 3 (+44 -10)

Show changes

	src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsJoinTest.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsTest.php

src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php (Diff revision 3)
Show all issues
```
AFAICS this should be "if the join is not finished"
```
1. rubendgr Feb. 15, 2016, 2:25 p.m.
  Improved the comment.

src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php (Diff revision 3)

Show all issues

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

rubendgr Feb. 15, 2016, 2:25 p.m.

The is_join is renamed to is_unfinished_join.

src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php (Diff revision 3)

Show all issues

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

rubendgr Feb. 15, 2016, 2:25 p.m.
```
Fixed.
```

src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsTest.php (Diff revision 3)

Show all issues

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

rubendgr Feb. 15, 2016, 2:25 p.m.
```
Changed the comments of this test.
```

Branch:

-	rubendgr:review
+	rubendgr:natural_join_fix

Branch:

-	rubendgr:natural_join_fix
+	rubendgr:review

Branch:

-	rubendgr:review
+	rubendgr:natural_join_fix

Change Summary:

Renamed the variable is_join and made some better comments.

Diff:

Revision 4 (+23 -20)

Show changes

	src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsJoinTest.php

src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php (Diff revision 4)

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".

src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php (Diff revision 4)

Show all issues

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

Change Summary:

Replaced the is_join in a couple of classes for is_unfinished_join.

Diff:

Revision 5 (+29 -30)

Show changes

	src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsConditionTest.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsJoinTest.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsTest.php

Change Summary:

Corrected the diff.

Diff:

Revision 6 (+61 -28)

Show changes

	src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsConditionTest.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsJoinTest.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsTest.php

Ship it!

src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php (Diff revision 6)
Show all issues
```
better simply use !=

it's much easier to read
```
1. rubendgr Feb. 15, 2016, 2:25 p.m.
  I agree and changed it to !=
src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsJoinTest.php (Diff revision 6)
Show all issues
```
"DontSet"
```
1. rubendgr Feb. 15, 2016, 2:25 p.m.
  Changed.

Change Summary:

Applied the M2mobi coding standards and made a small fix with a method naming and logical expression.

Diff:

Revision 7 (+79 -45)

Show changes

	src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsConditionTest.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsJoinTest.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsTest.php

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Merged into master

You have a pending review.

Review Board 3.0.24

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

Description:

Change Summary:

Diff:

Change Summary:

Diff:

Branch:

Branch:

Branch:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Status: Closed (submitted)

Change Summary: