Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Second part of the DboSource cleanup. #2415

Closed
wants to merge 15 commits into from

3 participants

@bar
bar commented

Related to #2289 and #2268 (comment)

bar added some commits
@bar bar Make getConstraint() a tiny bit more readable. fd14d8e
@bar bar Test for a self join only when needed. a34955e
@bar bar Model objects should be in CamelCase.
Add a Model type hint.
fba33af
@bar bar Remove is_object() calls when the test subject is already type hinted…
… as a Model.
f64bbf4
@bar bar Update DboSource::group():
* Update documentation.
* Add Model type hint.
* Bail early when fields are empty.
5f85bd1
@bar bar Add more Model type hints, and ease the model testing replacing
is_object() for  '!== null'.
72f96f1
@bar bar Build the same cache key when $fields:
* is a string containing fields separated by comma.
* is an array with one element being a string containing fields
separated by comma.
* is an array containing fields.

In every case, the cache key will be the same when:
 * fields (which can contain expressions) are duplicated.
 * fiels are empty.
b4197df
@bar bar Add documentation to the quite obscure fields(), and make some additi…
…ons:

* Avoid duplicating virtual fields when trying to remove them from the fields array.
* _constructVirtualFields() should handle field (full) qualification.
* Remove unnecessary vars.
* Handle the case where $fields is an SQL function with a fully qualified or a numeric field inside.
aa1aac9
lib/Cake/Model/Datasource/DboSource.php
((18 lines not shown))
foreach ($fields as $field) {
+ if (strpos($field, '.') !== false) {
+ $field = str_replace($Model->alias . '.', '', $field);
+ }
@markstory Owner

Why did this get added?

@bar
bar added a note

Because I remove it from fields() to make it easier to avoid duplicating virtual fields when trying to remove them from the fields array.

That was the only meaning of the array_unique() here

@bar
bar added a note

The new iteration does not need to test for Model->alias anymore :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/Cake/Model/Datasource/DboSource.php
((48 lines not shown))
if (empty($alias)) {
- $alias = $model->alias;
+ $alias = $Model->alias;
+ }
+
+ $virtualFields = $Model->getVirtualField();
+
+ $allFields = empty($fields);
+
+ if (!$allFields) {
+ if (is_array($fields)) {
+ if (count($fields) === 1) {
+ $field = reset($fields);
+ if (is_string($field) && strpos($field, ',') !== false) {
@markstory Owner

4 ifs in a row. Is there a way we can make this require less nesting?

@bar
bar added a note

That piece is a little bitchy, it tries to accomplish most of the magic inside this very commit.

Take a look at the tests.

I'm trying to make 'title' and 'Article.title' build the same cache key when quoted. For that, it is necessary to practically completely process $fields before building the key.

If I can do it in a smooth way, those lines will be better handled inside a function.

@AD7six Collaborator
AD7six added a note

these two ifs can be stuck together, but this whole chunk would be better off as a seperate function IMO i.e.

if (!$allFields) {
    $fields = $this->_appropriateName($fields);
}

Probably with a good dose of return-early.

@bar
bar added a note

Yes, it can be done but I didn't want the elseif (is_string($fields)) to start with the failed (negated) previous test !(is_array($fields) && count($fields) === 1) && is_string($fields), because It would lead to a not so self explanatory code and difficult to maintain later, instead it is !is_array($fields) && is_string($fields).

I feel the same way, it should be better contained inside a function. But as I stated earlier, most of the $quote block code should be there to avoid writing to the cache when the fields end up being the same.

@bar
bar added a note

@AD7six made a few improvements concerning virtual fields.

For instance, they can be used with expressions now :)

More in the last commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/Cake/Model/Model.php
((17 lines not shown))
+
+ if (empty($this->virtualFields)) {
+ return array();
+ }
+
+ $modelVirtualFields = $this->virtualFields;
+ $keys = array_keys($modelVirtualFields);
+
+ $virtualFields = array_combine($keys, $keys);
+ foreach ($virtualFields as $field) {
+ $virtualFields["{$field} "] = $this->alias . '.' . $field;
+ }
+
+ $_virtualFields = array_map('serialize', $virtualFields);
+ $_fields = array_map('serialize', (array)$fields);
+ $_validVirtualFields = array_intersect($_virtualFields, (array)$_fields);
@markstory Owner

Why not just have proper names for these variables?

@bar
bar added a note

Something like $serializedValidVirtualFields?

@markstory Owner

Sure, using _ in variable names generally feels wrong to me.

@bar
bar added a note

Yep, I use them when I try to express that the variable itself is somewhat a hack to achieve a bigger purpose... but then again every variables does that...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
bar added some commits
@bar bar Improve virtual fields generation:
* Delegate model virtual fields extraction to the Model at issue.
* Virtual fields can be extracted from a list of fields which can contain expressions.
* Extracting virtual fields from different fields list, resulting in the same virtual fields generates the same cache key.

When building the cache key, it will be the same when:

* fields (which can contain expressions) are duplicated.
* fields are empty.
* virtual fields are qualified or unqualified.
a95da63
@bar bar Test that fields() method builds different cache keys when using virt…
…ual fields with the same key but different expressions.
aed9bd9
@markstory markstory commented on the diff
lib/Cake/Model/Datasource/DboSource.php
@@ -2339,137 +2364,197 @@ protected function _scrubQueryData($data) {
}
/**
- * Converts model virtual fields into sql expressions to be fetched later
+ * Normalizes the fields list of an SQL query.
+ *
+ * It also takes care of removing duplicates and empty fields.
+ *
+ * Note: it does not deal with quoting or fully qualifying.
+ *
+ * @param mixed $fields String or array containing fields, expressions are also allowed.
+ * @return array
+ */
+ protected function _normalizeFields($fields = array()) {
@markstory Owner

Why do we need to start doing this? Virtual fields have been around for a few years, and I don't really understand why this is needed now.

@bar
bar added a note

You mean _normalizeFields() or the whole "lets fix something virtual fieldy" thing?

@markstory Owner

Both, the pull request doesn't really explain what was wrong with virtualFields. I understand they don't work in string fields for example, but is that worth fixing?

@bar
bar added a note

The PR was just a batch of commits related to DboSource. I tried to explain each one in their respective log.

  • The cache is being polluted with different cache keys containing the same values (for fields and virtual fields); e.g. each line of this tests and this tests misses the cache query and ends up doing the same process again and creates a new cache entry.

Most of the idea was to use the constructed fields/virtual to create the cache key, that way it will be the same. So the idea was to move the code which was below

if ($return = $this->cacheMethod(__FUNCTION__, $cacheKey)) {
  return $return;
}

above it.

When $fields are used, they should be normalized before, not after querying the cache. _normalizeFields() does that for $fields, it is the same logic used before but with some alterations (taking care of more cases).

@bar
bar added a note
  • You can't use virtual fields with expressions.

The way virtual fields identification and later extraction from $fields was broken if $fields has an expression (a class instance) inside, because you can't array_diff() or array_intersect() anymore.

I solved this inside Model::extractVirtualFields(). It does what it did before taking care of expressions, exposing a public Api for convenience, and made it easier to document and add a bit of order to fields(), making it easier for developers to make contributions later.

@bar
bar added a note
  • Fields and virtual fields generation had very few tests.
@bar
bar added a note

I believe this was most of it involving fields/virtual fields.

I know that all the calculations done before actually building the cache key and using it are done every time incurring in higher processing time, so building the fields can be done later if you think it is a bad call. Maybe it was done like this to avoid String::tokenize(), it's a trade of.

But that leads to constructing/extracting virtual fields from $fields after building the key, resulting in a lot of keys having the same values once again

And if the virtual fields expressions (not the key) are not used to calculate the cache key, then when you query fields() and later change the expression of a virtual field, the wrong cache will be brought.

@bar
bar added a note

Finally, virtual fields now working in string fields is just a collateral, it just happened because the steps are built in cascade blocks.

  1. Fields
    a. empty $fields > all fields from Model->schema() already normalized
    b. !empty $fields > normalize fields _normalizeFields($fields)

  2. Virtual fields
    a. empty $fields > virtual fields from Model->getVirtualFields() already normalized
    b. !empty $fields > extract virtual fields Model->extractVirtualFields($fields)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bar

This has become a mess, I'm sorry for that.

I'm closing it and will open a PR with some proposals.

@bar bar closed this
@markstory
Owner

Ok.

@dereuromark dereuromark referenced this pull request in CakeDC/search
Closed

Dev builds fail #119

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 3, 2013
  1. @bar
  2. @bar
  3. @bar

    Model objects should be in CamelCase.

    bar authored
    Add a Model type hint.
  4. @bar
  5. @bar

    Update DboSource::group():

    bar authored
    * Update documentation.
    * Add Model type hint.
    * Bail early when fields are empty.
  6. @bar

    Add more Model type hints, and ease the model testing replacing

    bar authored
    is_object() for  '!== null'.
  7. @bar

    Build the same cache key when $fields:

    bar authored
    * is a string containing fields separated by comma.
    * is an array with one element being a string containing fields
    separated by comma.
    * is an array containing fields.
    
    In every case, the cache key will be the same when:
     * fields (which can contain expressions) are duplicated.
     * fiels are empty.
  8. @bar

    Add documentation to the quite obscure fields(), and make some additi…

    bar authored
    …ons:
    
    * Avoid duplicating virtual fields when trying to remove them from the fields array.
    * _constructVirtualFields() should handle field (full) qualification.
    * Remove unnecessary vars.
    * Handle the case where $fields is an SQL function with a fully qualified or a numeric field inside.
  9. @bar

    Cache fields even when unquoted.

    bar authored
    Add missing tests.
  10. @bar
  11. @bar
  12. @bar
Commits on Dec 4, 2013
  1. @bar
Commits on Dec 5, 2013
  1. @bar

    Improve virtual fields generation:

    bar authored
    * Delegate model virtual fields extraction to the Model at issue.
    * Virtual fields can be extracted from a list of fields which can contain expressions.
    * Extracting virtual fields from different fields list, resulting in the same virtual fields generates the same cache key.
    
    When building the cache key, it will be the same when:
    
    * fields (which can contain expressions) are duplicated.
    * fields are empty.
    * virtual fields are qualified or unqualified.
  2. @bar

    Test that fields() method builds different cache keys when using virt…

    bar authored
    …ual fields with the same key but different expressions.
Something went wrong with that request. Please try again.