- Download Diff

Summary:

[Gravity] Implemented grouping for sql conditions

Review Request #157 — Created Sept. 26, 2013 and submitted Oct. 11, 2013, 3:58 p.m.

Information
Owner:	andrea
Repository:	Lunr
Branch:	master
Bugs:
Depends On:
Reviewers
Groups:	lunr
People:

Description

This change introduces functionality to support logical grouping of SQL conditional statements (WHERE, HAVING, ON).
What was possible so far where statements like

WHERE col1=1 AND col2=2 OR col3=3

With this change we are now able to do

WHERE col1=1 AND (col2=2 OR col3=3)

Testing Done

Checked if calling sql_conditions('a','b','=') after setting bracket to open results in "WHERE ( a = b"
Checked if calling sql_bracket_close() adds a ' )' to the condition.

Issues

0
0
32
6
All Issues: 38

Description	From	Last Updated
be careful with whitespaces :) there are plenty, I am not going to mark them all	dinos	Sept. 26, 2013, 3:33 p.m.
There are still whitespaces in the code	dinos	Sept. 26, 2013, 5:48 p.m.
isn't it missing the closing "bracket" here? Do you add it differently?	dinos	Sept. 26, 2013, 4:19 p.m.
why do you have to set the closing "bracket" via a function and after using it to invalidate it (empty ...	dinos	Oct. 1, 2013, 12:24 p.m.
I think you are missing the tests of sql_opne_bracket() and sql_bracket()	dinos	Oct. 1, 2013, 12:25 p.m.
You haven't written unit tests for this function.	dinos	Sept. 27, 2013, 10:06 a.m.
Your parentheses open that by the way could be name as start_group() HINT is overwriting your property. You should think ...	dinos	Sept. 27, 2013, 2:58 p.m.
sql_parentheses_close that could be named sql_group_end() or so HINT. It seems that it is doing more than it should. Also ...	dinos	Sept. 27, 2013, 2:58 p.m.
This should be tested in a separate test function.	dinos	Sept. 27, 2013, 2:58 p.m.
where is hardcoded. You could use the appropriate properties.	dinos	Sept. 27, 2013, 2:59 p.m.
You are testing only WHERE. What about HAVING and ON?	dinos	Sept. 27, 2013, 2:59 p.m.
honestly I think this is overcomplicating things. You already know which property you need to write into, so why not ...	pprkut	Oct. 1, 2013, 12:25 p.m.
the point being?	pprkut	Oct. 1, 2013, 12:25 p.m.
style	pprkut	Oct. 2, 2013, 10:01 a.m.
if ($is_join) { $this->$condition .= 'ON ('; $this->is_join = FALSE; } else { $this->$condition .= '('; } Saves you the ...	pprkut	Oct. 2, 2013, 11:05 a.m.
style	pprkut	Oct. 2, 2013, 10:01 a.m.
style	pprkut	Oct. 2, 2013, 10:01 a.m.
yeah, no	pprkut	Oct. 2, 2013, 10:03 a.m.
that doesn't make any sense. What are you trying to solve here?	pprkut	Oct. 2, 2013, 11:39 a.m.
please use the conditionalKeywordProvider	pprkut	Oct. 8, 2013, 4:24 p.m.
nitpick, we don't do the strtoupper in sql_conditional so we don't really need it here either. Just assume everything is ...	pprkut	Oct. 8, 2013, 4:26 p.m.
I don't think this is unit tested.	pprkut	Oct. 8, 2013, 5:31 p.m.
nitpick, we don't do the strtoupper in sql_conditional so we don't really need it here either. Just assume everything is ...	pprkut	Oct. 8, 2013, 4:26 p.m.
SQLite3DMLQueryBuilder	pprkut	Oct. 8, 2013, 4:27 p.m.
ditto	pprkut	Oct. 8, 2013, 4:29 p.m.
ditto	pprkut	Oct. 8, 2013, 4:29 p.m.
ditto	pprkut	Oct. 8, 2013, 4:29 p.m.
ditto	pprkut	Oct. 8, 2013, 4:29 p.m.
ditto	pprkut	Oct. 8, 2013, 4:29 p.m.
wrong class in @covers, also a few others in this file	pprkut	Oct. 9, 2013, 10:09 a.m.
assertPropertyEquals	pprkut	Oct. 8, 2013, 4:54 p.m.
assertPropertyEquals (more down in this file)	pprkut	Oct. 8, 2013, 4:54 p.m.
This test belongs in a different file	pprkut	Oct. 8, 2013, 5:18 p.m.
What does this test that is not already tested?	pprkut	Oct. 8, 2013, 5:25 p.m.
wrong file	pprkut	Oct. 8, 2013, 5:20 p.m.
what does this test that is not already tested?	pprkut	Oct. 8, 2013, 5:30 p.m.
wrong file	pprkut	Oct. 9, 2013, 10:22 a.m.
wrong file	pprkut	Oct. 9, 2013, 10:26 a.m.

Summary:

-	Implemented grouping for sql conditions
+	[Gravity] Implemented grouping for sql conditions

Branch:

+	master

I would appreciate a better description. I don't really understand what have you worked on, without checking the code.

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

Show all issues

be careful with whitespaces :)
there are plenty, I am not going to mark them all

Change Summary:

Removed white spaces in empty lines.

Diff:

Revision 2 (+90)

Show changes

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

Change Summary:

Adjusted description to make the nature of the change more clear

Description:

~		Is now possible to use sql_bracket_open() and sql_bracket_close() to group sql conditions correctly
	~	This change introduces functionality to support logical grouping of SQL conditional statements (WHERE, HAVING, ON).
	+	What was possible so far where statements like
	+
	+	WHERE col1=1 AND col2=2 OR col3=3
	+
	+	With this change we are now able to do
	+
	+	WHERE col1=1 AND (col2=2 OR col3=3)

src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php (Diff revision 2)
Show all issues
```
There are still whitespaces in the code
```

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

This is not a blocking comment that why it is not marked as an issue, but I believe that the particular symbol is called parentheses.

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

Show all issues

isn't it missing the closing "bracket" here?
Do you add it differently?

andrea Sept. 26, 2013, 5:48 p.m.

It's not missing. The closing bracket is added directly from the function sql_bracket_close().
The opening one instead, is added here because it needs to be added after WHERE|HAVING (also added by this function) but before the condition.

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

Show all issues

why do you have to set the closing "bracket" via a function and after using it to invalidate it (empty string) ? IMHO this function could be 2 lines

andrea Sept. 26, 2013, 5:48 p.m.

You're definitely right. It could be shortened.
I did it this way because the bracket_open function is updating the bracket variable so I made also this one do the same for consistency.
Will change it.

src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderBaseTest.php (Diff revision 2)
Show all issues
```
I think you are missing the tests of sql_opne_bracket() and sql_bracket()
```

Also you can run the following command to find the codestyle issues
phpcs --standard=Lunr src/Lunr

Diff:

Revision 3 (+76)

Show changes

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

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

Show all issues

You haven't written unit tests for this function.

andrea Sept. 27, 2013, 10:07 a.m.

This function is being called in the first unit test I wrote. (the one that covers sql_condition)

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

Show all issues

Your parentheses open that by the way could be name as start_group() **HINT**
is overwriting your property. You should think more use cases of grouping than the simple one where (a & b)

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

Show all issues

sql_parentheses_close that could be named sql_group_end() or so **HINT**. It seems that it is doing more than it should. 
Also check line 609 and you'll understand why this won't work with a HAVING condition.

src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderBaseTest.php (Diff revision 3)
Show all issues
```
This should be tested in a separate test function. 
```
src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderBaseTest.php (Diff revision 3)
Show all issues
```
where is hardcoded. You could use the appropriate properties.
```
src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderBaseTest.php (Diff revision 3)
Show all issues
```
You are testing only WHERE. What about HAVING and ON?
```

Change Summary:

Changed the implementation, now sql_group_start($condition) raises a flag "group" that indicates for which condition (WHERE|HAVING|ON) we should start the grouping
The same way, sql_group_end($condition), closes the group for the right condition

Diff:

Revision 4 (+218)

Show changes

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

Diff:

Revision 5 (+514)

Show changes

	src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php
	src/Lunr/Gravity/Database/MySQL/MySQLDMLQueryBuilder.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLDMLQueryBuilderConditionTest.php
	src/Lunr/Gravity/Database/SQLite3/SQLite3DMLQueryBuilder.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3DMLQueryBuilderConditionTest.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderBaseTest.php

It's already pretty nice. However, the use case I mentioned "WHERE ((a=1 AND b=2) OR c=3) OR d=4" still is not possible.
Also, beware of those whitespaces.

Change Summary:

Fixed implementation to consider nested groups.
An array of counters (one for each of WHERE|HAVING|ON) is used to keep track of how many open parentheses the condition should be prefixed with.
Now queries like "WHERE ((a=1 AND b=2) OR c=3) OR d=4" should be possible.
The input is assumed right from the developers (there is no error checking, a bracket could be opened and never closed or viceversa)

Diff:

Revision 6 (+641 -23)

Show changes

	src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php
	src/Lunr/Gravity/Database/MySQL/MySQLDMLQueryBuilder.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLDMLQueryBuilderConditionTest.php
	src/Lunr/Gravity/Database/SQLite3/SQLite3DMLQueryBuilder.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3DMLQueryBuilderConditionTest.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderBaseTest.php

src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php (Diff revisions 5 - 6)

Show all issues

honestly I think this is overcomplicating things. You already know which property you need to write into, so why not go ahead and just do it rather than delay it?

andrea Oct. 1, 2013, 10:09 a.m.

I implemented it this way because you once showed me that the method calls can happen not in order. You showed there could be a call to where, then to having, and then where again. In this case, I think my implementation is legit.

pprkut Oct. 1, 2013, 10:26 a.m.

Sure, I'm not saying it's not working. I'm just saying it's implemented a bit more complicated than it needs to be :)

src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php (Diff revisions 5 - 6)

Show all issues

the point being?

andrea Oct. 1, 2013, 10:09 a.m.

The point is that I need a variable containing just the keyword (where|having|join) to use it as an index for the array, but $condition string could be more than that. On a second iteration its value could be for example "WHERE a = b AND "

pprkut Oct. 1, 2013, 10:26 a.m.

transitive law: If a=b and b=c then a=c

$condition is exactly the same as $condition_keyword

Change Summary:

Changed the implementation of the grouping. I no longer use the variable 'group' and the array 'group_count' to keep track of the number of the opened groups for each conditional statement (where|having|on).
Instead, 'sql_group_start' and 'sql_group_end' attach the parentheses to the right string right away. The function 'sql_conditions' handles the special cases.

Diff:

Revision 7 (+589 -27)

Show changes

	src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php
	src/Lunr/Gravity/Database/MySQL/MySQLDMLQueryBuilder.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLDMLQueryBuilderConditionTest.php
	src/Lunr/Gravity/Database/SQLite3/SQLite3DMLQueryBuilder.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3DMLQueryBuilderConditionTest.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderBaseTest.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsTest.php

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

Show all issues

style

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

Show all issues

if ($is_join)
{
     $this->$condition .= 'ON (';
     $this->is_join = FALSE;
}
else
{
     $this->$condition .= '(';
}

Saves you the trouble of the looping in sql_conditional

src/Lunr/Gravity/Database/MySQL/MySQLDMLQueryBuilder.php (Diff revision 7)
Show all issues
```
style
```
src/Lunr/Gravity/Database/MySQL/MySQLDMLQueryBuilder.php (Diff revision 7)
Show all issues
```
style
```

src/Lunr/Gravity/Database/MySQL/Tests/MySQLDMLQueryBuilderConditionTest.php (Diff revision 7)

Show all issues

yeah, no

Diff:

Revision 8 (+542 -6)

Show changes

	src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php
	src/Lunr/Gravity/Database/MySQL/MySQLDMLQueryBuilder.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLDMLQueryBuilderConditionTest.php
	src/Lunr/Gravity/Database/SQLite3/SQLite3DMLQueryBuilder.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3DMLQueryBuilderConditionTest.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderBaseTest.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsTest.php

please rebase against http://reviews.lunr.nl/r/152/

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

Show all issues

that doesn't make any sense. What are you trying to solve here?

andrea Oct. 2, 2013, 11:40 a.m.

Conditional is needed, ltrim are indeed no use.

src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderBaseTest.php (Diff revision 8)

Show all issues

please use the conditionalKeywordProvider

Change Summary:

Removed unnecessary 'ltrim' as suggested by Heinz
Collapsed some unit tests by using conditionalKeywordProvider

Diff:

Revision 9 (+442 -8)

Show changes

	src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php
	src/Lunr/Gravity/Database/MySQL/MySQLDMLQueryBuilder.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLDMLQueryBuilderConditionTest.php
	src/Lunr/Gravity/Database/SQLite3/SQLite3DMLQueryBuilder.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3DMLQueryBuilderConditionTest.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderBaseTest.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsTest.php

```
Please rebase against master
```

Change Summary:

Re-based on master. While solving conflict in 2 unit test files, reflection methods have been updated to the new ones to keep the file consistent (a change on master was to update them, but I have new unit tests that weren't considered)

Diff:

Revision 10 (+487 -8)

Show changes

	src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php
	src/Lunr/Gravity/Database/MySQL/MySQLDMLQueryBuilder.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLDMLQueryBuilderConditionTest.php
	src/Lunr/Gravity/Database/SQLite3/SQLite3DMLQueryBuilder.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3DMLQueryBuilderConditionTest.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderBaseTest.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsTest.php

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

Show all issues

nitpick, we don't do the strtoupper in sql_conditional so we don't really need it here either. Just assume everything is passed upper case

src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php (Diff revision 10)
Show all issues
```
I don't think this is unit tested.
```

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

Show all issues

nitpick, we don't do the strtoupper in sql_conditional so we don't really need it here either. Just assume everything is passed upper case

src/Lunr/Gravity/Database/SQLite3/SQLite3DMLQueryBuilder.php (Diff revision 10)
Show all issues
```
SQLite3DMLQueryBuilder
```
src/Lunr/Gravity/Database/SQLite3/SQLite3DMLQueryBuilder.php (Diff revision 10)
Show all issues
```
ditto
```
src/Lunr/Gravity/Database/SQLite3/SQLite3DMLQueryBuilder.php (Diff revision 10)
Show all issues
```
ditto
```
src/Lunr/Gravity/Database/SQLite3/SQLite3DMLQueryBuilder.php (Diff revision 10)
Show all issues
```
ditto
```
src/Lunr/Gravity/Database/SQLite3/SQLite3DMLQueryBuilder.php (Diff revision 10)
Show all issues
```
ditto
```
src/Lunr/Gravity/Database/SQLite3/SQLite3DMLQueryBuilder.php (Diff revision 10)
Show all issues
```
ditto
```
src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3DMLQueryBuilderConditionTest.php (Diff revision 10)
Show all issues
```
assertPropertyEquals
```
src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3DMLQueryBuilderConditionTest.php (Diff revision 10)
Show all issues
```
assertPropertyEquals (more down in this file)
```

src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderBaseTest.php (Diff revision 10)

Show all issues

This test belongs in a different file

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

Show all issues

What does this test that is not already tested?

andrea Oct. 8, 2013, 5:31 p.m.

As far as I remember this test is the result of grouping similar tests by using ConditionalKeywordProvider (previous comment on this review).
You are probably thinking of the same tests with hardcoded 'where|having|on' that are not supposed to be present anymore.
(checked quickly with 'git grep')
Tell me if I'm wrong

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

Show all issues

wrong file

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

Show all issues

what does this test that is not already tested?

andrea Oct. 8, 2013, 5:31 p.m.
```
Same as above.
```

Change Summary:

Amend previous comments by Heinz. Mainly fixed unit tests.

Diff:

Revision 11 (+534 -106)

Show changes

	src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php
	src/Lunr/Gravity/Database/MySQL/MySQLDMLQueryBuilder.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLDMLQueryBuilderConditionTest.php
	src/Lunr/Gravity/Database/SQLite3/SQLite3DMLQueryBuilder.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3DMLQueryBuilderConditionTest.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderBaseTest.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsConditionTest.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsJoinTest.php
	1 more

src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3DMLQueryBuilderConditionTest.php (Diff revisions 10 - 11)
Show all issues
```
wrong class in @covers, also a few others in this file
```

src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3DMLQueryBuilderConditionTest.php (Diff revisions 10 - 11)

while I appreciate the effort, this is essentially an unrelated change. Please refrain from putting them in the same review in the future. For this time it is fine.

src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsJoinTest.php (Diff revision 11)

Show all issues

wrong file

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

Show all issues

wrong file

Diff:

Revision 12 (+474 -106)

Show changes

	src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php
	src/Lunr/Gravity/Database/MySQL/MySQLDMLQueryBuilder.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLDMLQueryBuilderConditionTest.php
	src/Lunr/Gravity/Database/SQLite3/SQLite3DMLQueryBuilder.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3DMLQueryBuilderConditionTest.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderBaseTest.php
	src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsConditionTest.php

Status: Closed (submitted)

Change Summary:

Merged slightly modified version into master

You have a pending review.

Review Board 3.0.24

[Gravity] Implemented grouping for sql conditions

Summary:

Branch:

Change Summary:

Diff:

Change Summary:

Description:

Diff:

Change Summary:

Diff:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Diff:

Status: Closed (submitted)

Change Summary: