colinmollenhour / magento-mongo

MongoDb abstraction layer and atomic job queue for Magento
Other
124 stars 50 forks source link

Fix processing 'hash' type in 'dehydrate' method. #5

Closed sergeykalenyuk closed 11 years ago

sergeykalenyuk commented 11 years ago

...itional index check.

Fix processing 'hash' type in 'dehydrate' method.

colinmollenhour commented 11 years ago

I think doing a recursive diff could cause problems. That is, the "hash" type should only be expected to update one level of values. If a value is a nested hash then the top-level key should get overwritten if anything inside it has changed rather than attempting to update only one value within that nested hash. I think this will be more predictable and if the user wants to update only a specific value then an embedded object should be used instead. Let me know what you think.

sergeykalenyuk commented 11 years ago

I assume possibility to update single value in a multilevel hash might be useful. An embedded object might be used for one level hash. An embeddedSet collection might be used for two levels hash. But they don't cover situation when three or more levels hash is used. In this case recursive diff should work. Also for a specific hash with a single usage place adding embedded/embeddedSet data types will inflate mongo.xml declaration.

colinmollenhour commented 11 years ago

For multi-level using embedded.embedded.embedded would allow update of deeply nested values. Using a hash in my mind means you want a simple set of key->value pairs and if value changes it should be overwritten. If this is not the case then there is no easy way to remove a key->value from a nested hash.

sergeykalenyuk commented 11 years ago

Thanks for the explanation. I agree that a hash should be a set of key->value pairs. As array_diff_assoc function only checks one dimension of a n-dimensional array, I believe we can replace

$value = array_diff_assoc($converter->$type($mapping, $rawValue), $object->getOrigData($field));

with:

$value = array();
$data = $converter->$type($mapping, $rawValue);
$origData = $object->getOrigData($field);
foreach ($data as $k => $v) {
  if (!isset($origData[$k]) || $v !== $origData[$k]) {
    $value[$k] = $v;
  }
}
colinmollenhour commented 11 years ago

I believe that is a good solution for now. Thanks!