Skip to content

Commit

Permalink
Refactoring the database layer so that query values are bound using the
Browse files Browse the repository at this point in the history 
query object instead to each individual expression.

This make unit testing easier for both the core and in userland, also
simplifies some routines by making them more explicit and profits from
a small performance gain.
  • Loading branch information
@lorenzo
lorenzo committed Aug 5, 2013
1 parent 8f82ff5 commit fb3b540
Show file tree
Hide file tree
Showing 12 changed files with 256 additions and 265 deletions.
42 changes: 34 additions & 8 deletions lib/Cake/Database/Expression/Comparison.php
View file
Expand Up @@ -18,6 +18,7 @@
namespace Cake\Database\Expression;

use Cake\Database\ExpressionInterface;
use Cake\Database\ValueBinder;

class Comparison extends QueryExpression {

Expand All @@ -39,7 +40,6 @@ public function __construct($field, $value, $type, $conjuntion) {
$this->_type = current($types);
}

$this->_identifier = substr(spl_object_hash($this), 7, 9);
$this->_conditions[$field] = $value;
}

Expand All @@ -59,20 +59,46 @@ public function getValue() {
return $this->_value;
}

public function placeholder($token) {
return sprintf(':c%s', $this->_identifier);
}

public function sql() {
/**
* Convert the expression into a SQL fragment.
*
* @param Cake\Database\ValueBinder $generator Placeholder generator object
* @return string
*/
public function sql(ValueBinder $generator) {
$value = $this->_value;
$template = '%s %s %s';
$template = '%s %s (%s)';

if (!($this->_value instanceof ExpressionInterface)) {
$value = $this->_bindValue($this->_field, $value, $this->_type);
$type = $this->_type;
if (strpos($this->_type, '[]') !== false) {
$value = $this->_flattenValue($generator);
} else {
$template = '%s %s %s';
$value = $this->_bindValue($generator, $value, $this->_type);
}
} else {
$value = $value->sql($generator);
}

return sprintf($template, $this->_field, $this->_conjunction, $value);
}

protected function _bindValue($generator, $value, $type) {
$placeholder = $generator->placeholder($this->_field);
$generator->bind($placeholder, $value, $type);
return $placeholder;
}

protected function _flattenValue($generator) {
$parts = [];
$type = str_replace('[]', '', $this->_type);
foreach ($this->_value as $value) {
$parts[] = $this->_bindValue($generator, $value, $type);
}
return implode(',', $parts);
}

public function count() {
return 1;
}
Expand Down
20 changes: 17 additions & 3 deletions lib/Cake/Database/Expression/FunctionExpression.php
View file
Expand Up @@ -18,6 +18,7 @@
namespace Cake\Database\Expression;

use Cake\Database\ExpressionInterface;
use Cake\Database\ValueBinder;

/**
* This class represents a function call string in a SQL statement. Calls can be
Expand Down Expand Up @@ -95,7 +96,7 @@ public function add($params, $types = []) {
}

$type = isset($types[$k]) ? $types[$k] : null;
$this->_conditions[] = $this->_bindValue('param', $p, $type);
$this->_conditions[] = ['value' => $p, 'type' => $type];
}

return $this;
Expand All @@ -107,12 +108,25 @@ public function add($params, $types = []) {
* in their place placeholders are put and can be replaced by the quoted values
* accordingly.
*
* @param Cake\Database\ValueBinder $generator Placeholder generator object
* @return string
*/
public function sql() {
public function sql(ValueBinder $generator) {

$parts = [];
foreach ($this->_conditions as $condition) {
if ($condition instanceof ExpressionInterface) {
$condition = $condition->sql($generator);
} else if (is_array($condition)){
$p = $generator->placeholder('param');
$generator->bind($p, $condition['value'], $condition['type']);
$condition = $p;
}
$parts[] = $condition;
}
return $this->_name . sprintf('(%s)', implode(
$this->_conjunction . ' ',
$this->_conditions
$parts
));
}

Expand Down
9 changes: 8 additions & 1 deletion lib/Cake/Database/Expression/OrderByExpression.php
View file
Expand Up @@ -17,6 +17,9 @@
*/
namespace Cake\Database\Expression;

use Cake\Database\ExpressionInterface;
use Cake\Database\ValueBinder;

class OrderByExpression extends QueryExpression {

/**
Expand All @@ -33,11 +36,15 @@ public function __construct($conditions = [], $types = [], $conjunction = '') {
/**
* Convert the expression into a SQL fragment.
*
* @param Cake\Database\ValueBinder $generator Placeholder generator object
* @return string
*/
public function sql() {
public function sql(ValueBinder $generator) {
$order = [];
foreach ($this->_conditions as $k => $direction) {
if ($direction instanceof ExpressionInterface) {
$direction = $direction->sql($generator);
}
$order[] = is_numeric($k) ? $direction : sprintf('%s %s', $k, $direction);

This comment has been minimized.

Sign in to view

Copy link
@thaJeztah

thaJeztah Aug 9, 2013

Wondering; would is_int() be a better approach here? is_numeric() Also accepts, floats, hexadecimal etc, so including string starting with 0x or 0b

This comment has been minimized.

Sign in to view

Copy link
@lorenzo

lorenzo Aug 9, 2013

Author Member

It is hardly a possibility here, but you are right. Want toake a pull request and have a commit in the core? :)

This comment has been minimized.

Sign in to view

Copy link
@thaJeztah

thaJeztah Aug 10, 2013

@lorenzo I'm currently trying to follow the development of Cake 3 with interest and making some notes with that may need/can be improved (all my 2c of course!). I am thinking of (attempting to) assist in making those ideas reality, but cannot guarantee I have consistently time to do that. Haven't worked with GIT before also (LOL), so that may be an issue :)

This comment has been minimized.

Sign in to view

Copy link
@lorenzo

lorenzo Aug 10, 2013

Author Member

Learning git took me a couple days, I'm pretty sure you will master it rather quickly, you should try this page: http://try.github.io/levels/1/challenges/1

Looking forward to your contributions!

This comment has been minimized.

Sign in to view

Copy link
@thaJeztah

thaJeztah Aug 10, 2013

Thanks for that link! Was already trying to figure out how to get my fork up-to-date (Aparently I forked Cake a year ago) coming from SVN, I need to get used to the Git-specific terminology and workflow. Will start playing around and see whatever I can contribute. Thanks for all the great work!
}
return sprintf('ORDER BY %s', implode(', ', $order));
Expand Down
162 changes: 12 additions & 150 deletions lib/Cake/Database/Expression/QueryExpression.php
View file
Expand Up @@ -19,18 +19,14 @@

use Cake\Database\ExpressionInterface;
use Cake\Database\Query;
use Cake\Database\ValueBinder;
use \Countable;

/**
* Represents a SQL Query expression. Internally it stores a tree of
* expressions that can be compiled by converting this object to string
* and will contain a correctly parenthesized and nested expression.
*
* This class also deals with internally binding values to parts of the expression,
* used for condition comparisons. When a string representation of an instance
* of this class is built any value bound will be expressed as a placeholder,
* thus this class exposes methods for getting the actual bound values for each of
* them so they can be used in statements or replaced directly.
*/
class QueryExpression implements ExpressionInterface, Countable {

Expand All @@ -50,40 +46,6 @@ class QueryExpression implements ExpressionInterface, Countable {
*/
protected $_conditions = [];

/**
* Array containing a list of bound values to the conditions on this
* object. Each array entry is another array structure containing the actual
* bound value, its type and the placeholder it is bound to.
*
* @var array
*/
protected $_bindings = [];

/**
* An unique string that identifies this object. It is used to create unique
* placeholders.
*
* @car string
*/
protected $_identifier;

/**
* A counter of the number of parameters bound in this expression object
*
* @var integer
*/
protected $_bindingsCount = 0;

/**
* Whether to process placeholders that are meant to bind multiple other
* placeholders out of an array of values. This value is automatically
* set to true when an "IN" condition is used or when a value is bound
* with an array type.
*
* @var boolean
*/
protected $_replaceArrayParams = false;

/**
* Constructor. A new expression object can be created without any params and
* be built dynamically. Otherwise it is possible to pass an array of conditions
Expand All @@ -102,7 +64,6 @@ class QueryExpression implements ExpressionInterface, Countable {
*/
public function __construct($conditions = [], $types = [], $conjunction = 'AND') {
$this->type(strtoupper($conjunction));
$this->id(substr(spl_object_hash($this), 7, 9));
if (!empty($conditions)) {
$this->add($conditions, $types);
}
Expand Down Expand Up @@ -335,63 +296,6 @@ public function not($conditions, $types = []) {
return $this->add(['NOT' => $conditions], $types);
}

/**
* Associates a query placeholder to a value and a type for this level of the
* expressions tree.
*
* If type is expressed as "atype[]" (note braces) then it will cause the
* placeholder to be re-written dynamically so if the value is an array, it
* will create as many placeholders as values are in it. For example "string[]"
* will create several placeholders of type string.
*
* @param string|integer $token placeholder to be replaced with quoted version
* of $value
* @param mixed $value the value to be bound
* @param string|integer $type the mapped type name, used for casting when sending
* to database
* @return string placeholder name or question mark to be used in the query string
*/
public function bind($param, $value, $type) {
$number = $this->_bindingsCount;
$this->_bindings[$number] = compact('value', 'type') + [
'placeholder' => substr($param, 1)
];
if (strpos($type, '[]') !== false) {
$this->_replaceArrayParams = true;
}
return $this;
}

/**
* Creates a unique placeholder name if the token provided does not start with ":"
* otherwise, it will return the same string and internally increment the number
* of placeholders generated by this object.
*
* @param string $token string from which the placeholder will be derived from,
* if it starts with a colon, then the same string is returned
* @return string to be used as a placeholder in a query expression
*/
public function placeholder($token) {
$param = $token;
$number = $this->_bindingsCount++;

if ($param[0] !== ':') {
$param = sprintf(':c%s%s', $this->_identifier, $number);
}

return $param;
}

/**
* Returns all values bound to this expression object at this nesting level.
* Subexpression bound values will not be returned with this function.
*
* @return array
*/
public function bindings() {
return $this->_bindings;
}

/**
* Returns the number of internal conditions that are stored in this expression.
* Useful to determine if this expression object is void or it will generate
Expand All @@ -409,15 +313,20 @@ public function count() {
* in their place placeholders are put and can be replaced by the quoted values
* accordingly.
*
* @param Cake\Database\ValueBinder $generator Placeholder generator object
* @return string
*/
public function sql() {
if ($this->_replaceArrayParams) {
$this->_replaceArrays();
}
public function sql(ValueBinder $generator) {
$conjunction = $this->_conjunction;
$template = ($this->count() === 1) ? '%s' : '(%s)';
return sprintf($template, implode(" $conjunction ", $this->_conditions));
$parts = [];
foreach ($this->_conditions as $part) {
if ($part instanceof ExpressionInterface) {
$part = $part->sql($generator);
}
$parts[] = $part;
}
return sprintf($template, implode(" $conjunction ", $parts));
}

/**
Expand Down Expand Up @@ -462,23 +371,6 @@ public function iterateParts(callable $callable) {
return $this;
}

/**
* Sets the unique identifier string for this object, which is used for generating
* placeholders. If called with no arguments it will return the currently defined
* identifier.
*
* @param string $identifier the string to be used as prefix for generating
* placeholders. If null current identifier is returned
* @return string|QueryExpression
*/
public function id($identifier = null) {
if ($identifier === null) {
return $this->_identifier;
}
$this->_identifier = $identifier;
return $this;
}

/**
* Auxiliary function used for decomposing a nested array of conditions and build
* a tree structure inside this object to represent the full SQL expression.
Expand Down Expand Up @@ -557,30 +449,9 @@ protected function _parseCondition($field, $value, $types) {
$type .= $typeMultiple ? null : '[]';
$operator = $operator == '=' ? 'IN' : $operator;
$operator = $operator == '!=' ? 'NOT IN' : $operator;
$multi = true;
}

if ($value instanceof ExpressionInterface || $multi === false) {
return new Comparison($expression, $value, $type, $operator);
}

$placeholder = $this->_bindValue($field, $value, $type);
return sprintf('%s %s (%s)', $expression, $operator, $placeholder);
}

/**
* Helper function used to bind a value to a field and return the placeholder
* generated for it.
*
* @param string $field field to generate placeholder for
* @param mixed $value the value to be bound to the field
* @param string $type the type that will be associated to the value
* @return string generated placeholder
*/
protected function _bindValue($field, $value, $type) {
$param = $this->placeholder($field);
$this->bind($param, $value, $type);
return $param;
return new Comparison($expression, $value, $type, $operator);
}

/**
Expand Down Expand Up @@ -632,13 +503,4 @@ protected function _bindMultiplePlaceholders($field, $values, $type) {
return implode(', ', $params);
}

/**
* Returns a string representation of this object
*
* @return string
*/
public function __toString() {
return $this->sql();
}

}
20 changes: 17 additions & 3 deletions lib/Cake/Database/Expression/UnaryExpression.php
View file
Expand Up @@ -17,11 +17,25 @@
*/
namespace Cake\Database\Expression;

use Cake\Database\ExpressionInterface;
use Cake\Database\ValueBinder;

class UnaryExpression extends QueryExpression {

public function sql() {
reset($this->_conditions);
return $this->_conjunction . ' (' . ((string)current($this->_conditions)) . ')';
/*
* Converts the expression to its string representation
*
* @param Cake\Database\ValueBinder $generator Placeholder generator object
* @return string
*/
public function sql(ValueBinder $generator) {
foreach ($this->_conditions as $condition) {
if ($condition instanceof ExpressionInterface) {
$condition = $condition->sql($generator);
}
// We only use the first (and only) condition
return $this->_conjunction . ' (' . ((string)$condition) . ')';
}
}

}

4 comments on commit fb3b540

@thaJeztah
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without reading all code and considerations, why drop the __toString() interface??

IMO

(string)$someObject;

Is a LOT cleaner than calling $someObject->sql(ValueBinder) every time a string representation is required.

The additional if (InstanceOf ExpressionInterface) checks in various parts makes the code a lot more messy

@lorenzo
Copy link
Member Author

@lorenzo lorenzo commented on fb3b540 Aug 9, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because there is no need to stringify an expression directly. Casting a query to an expression is still possible without Using a bumser. Having more type checks made the code a lot more explicit in what it is doing, but I think the other approach involved a lot of magic that was hard to understand.

@thaJeztah
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to trickle down to the not-so-strict approach in CakePHP in 'what' type of arguments and return-types are used throughout the framework (e.g. params can either be an 'array', 'string', 'bool' or 'null'). To be honest, I'm kinda hoping CakePHP 3 (being a major overhaul), will become more strict in this regard. This will save a lot of type-checking/casting during development and (hopefully) leads to cleaner, more readable code. In this example, to only accept ExpressionInterface inside '_conditions' etc.

I've been developing with CakePHP since version 1.1(? .. first non-beta release), so taking CakePHP to 'the next level' is something I hope will be realised.

I do want to help/assist/brain-storm if time is on my hand (busy job), but don't want to be like a "bull in a china shop", because I don't know what discussions/principles/considerations have been made before some code was written. What is the best location to get 'in the loop' and/or discuss things? (Google+ group, GitHub, Lighthouse?)

@lorenzo
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately php does not support method overloading, so accepting multiple types in the same function implementation is the only way of providing any sort of "pattern matching". Pattern matching is good since it allows developers to be both expressive and relaxed in the way they code which is an aspect of CakePHP that I would like to keep.

Quite the contrary happens when you only accept one type of argument, you would need to cast, wrap or alter your values before they are passed to the method. One example of this is the Hash class, in the past Set used to accept whatever you passed to it, hence it was possible to iterate over any traversable. With the change to hash, which is stricter in its typing, is just impossible.

A very elegant way of solving such problem is to have a Maybe type, but since functional programming is so foreign to the way we do php, I'd rather not go that way :P

In particular _conditions cannot accept ExpressionInterface as its only param type because people can write string expressions and wrapping everything inside another expression (I mean, everything!) it's in our minds a bit overkill, but we might be wrong about it.

The best place to discuss is this group: https://groups.google.com/forum/#!forum/cakephp-core

Please sign in to comment.