Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Aggregate is casting numbers? #158

Open
hailwood opened this issue Feb 27, 2019 · 7 comments
Open

Aggregate is casting numbers? #158

hailwood opened this issue Feb 27, 2019 · 7 comments
Labels

Comments

@hailwood
Copy link

I have a model where we have a field, "line_id" the db type is string..
This looks like a number but it's not as we have e.g. "831", but we also have "062", the leading zero is important.

Our models toSearchableArray looks like this (Model is DDI):

public function toSearchableArray()
{
  $data = $this->toArray();
  $data['display_name'] = $data['name'];
  $data['model'] = 'ddi';
  $this->transform($data);

  return array_only($data, [
    'id',
    'line_id',
    'number',
    'name',
    'display_name',
    'model',
    'created_at',
    'updated_at'
  ]);
}

scout:import against the model works fine and in the index we see
image

We also have an aggregate defined which looks like this

class GlobalEntities extends Aggregator
{

  protected $models = [
    Client::class,
    DDI::class,
    Contact::class,
  ];
}

And that DDI that's imported looks like this in the search index
image

So why would the aggregate be casting line_id to a number hence dropping the leading zero and how can I fix it?

@nunomaduro
Copy link
Contributor

That happens because you are using the method $this->transforms. Please read this documentation: https://www.algolia.com/doc/framework-integration/laravel/indexing/configure-searchable-data/?language=php#transformers.

@nunomaduro nunomaduro added the support Help label Feb 28, 2019
@hailwood
Copy link
Author

Interesting, That makes sense,

I'm curious then why would it not be casting the single model algolia import to a number given the toSearchableArray is on the model?

@nunomaduro
Copy link
Contributor

Can you re-phrase your question?

@hailwood
Copy link
Author

Well,

As shown above, we have the toSearchableArray method on the model (Eloquent) which uses the transform method.

Next we have an Aggregate defined.

We're sending both these things up to Algolia, yet only the aggregate has the casting happening.
Surely if it's the transform method doing the casting, it should affect both the single-model-type index, and the aggregate index?

@nunomaduro
Copy link
Contributor

nunomaduro commented Mar 11, 2019

This is the toSearchableArray method of aggregators. It calls the base model toSearchableArray behind the scenes.

    /**
     * Get the searchable array of the searchable.
     *
     * @return array
     */
    public function toSearchableArray(): array
    {
        if ($this->model === null) {
            throw new ModelNotDefinedInAggregatorException();
        }

        return method_exists($this->model, 'toSearchableArray') ? $this->model->toSearchableArray() :
            $this->model->toArray();
    }

So yes, it should affect both the single-model-type index, and the aggregate index.

@hailwood
Copy link
Author

@nunomaduro So I figured I'd disable the ConvertNumericStringsToNumbers transformer and just use the dates by updating my toSearchableArray method on my models to call

$this->transform($data, [ConvertDatesToTimestamps::class]);

But found something strange, In the aggregator the ConvertNumericStringsToNumbers was still getting called.

It's because this check is failing
https://github.com/algolia/scout-extended/blob/master/src/Jobs/UpdateJob.php#L113

Specifically because the toSearchableArray method is on the base Aggregator.
Meaning that the default transform gets called on the models for the aggregator.

Easy fix, just adding the following to my own Aggregator.

  public function toSearchableArray(): array
  {
    return parent::toSearchableArray();
  }

Could explain why I'm getting different transforms depending on whether it's a different model, regardless, the hasToSearchableArray method should probably be updated to check if we're working with an Aggregator and if so rerun the check against each of the models?

@nunomaduro
Copy link
Contributor

@hailwood Not sure if I understood. But if you are sure that there is a big here, do you mind of submit a PR with tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants