fadion / Bouncy

Map Elasticsearch results to Eloquent models
MIT License
71 stars 26 forks source link

BouncyTrait@updateIndex uses BouncyTrait@documentFields for construct… #19

Open tpalts opened 9 years ago

tpalts commented 9 years ago

…ing the indexed fields

previously, the documentFields() function was completely ignored when updating index.

fadion commented 9 years ago

Actually, I believe that the best approach would be to find the documentFields() that are dirty, so only those values are updated. Your approach would work, but would incur a performance penalty by updating all the documentFields().

I think the best way would be:

if ($fields) {
    $body = $fields;
}
// Find the dirty documents fields
elseif ($dirty = array_intersect_key($this->getDirty(), $this->documentFields())) {
    $body = $dirty;
}
else {
    return true
}

What do you think? If you want to update the PR, you're welcome to do so.

Thanks for your input.

tpalts commented 9 years ago

That does look more efficient than my solution. A problem still not addressed: if you use documentFields() to set fields that are not available on the Eloquent model (I often do), they will always be returned by array_intersect_key(..), even if their value in ES will not change. If these custom fields make up a big portion of all of the indexed fields, there is a performance loss. I don't have a good idea what to do about this, though.

Edit: I guess a solution would be to only update/index the document, if there are any fields returned from documentFields(). That way the user could ensure inside their own documentFields() that only changed fields will be returned.

BouncyTrait@documentFields would perhaps look like this:

public function documentFields()
    {
        return (empty($this->documentFields)) ? $this->getDirty() : $this->documentFields;
    }

BouncyTrait@updateIndex

if ($fields) {
    $body = $fields;
}
// Find the dirty documents fields
elseif ($dirty = $this->documentFields()) {
    $body = $dirty;
}
else {
    return true
}
...
fadion commented 9 years ago

Didn't think about such a use case. Actually, the intersection will discard the custom keys that are not present on the model, as getDirty() has no information about them.

Let's give it a few days to investigate any good approach. Otherwise, I believe we'll go for your original method of updating all the documentFields().

tpalts commented 9 years ago

I believe my above-mentioned solution makes more sense. I would guess the average dev will use Bouncy to index the whole document. Therefor the default solution I provided in my previous comment for BouncyTrait@documentFields would be sufficient for most ppl. If more customization is sought, the dev customizing should also pay attention to the fact that the customised fields are only returned if they need to be (re)indexed. If you decide in favour of this solution, the documentation should probably also be updated.

Thoughts?