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

Allowing different entities indexed into same index #92

Merged
merged 4 commits into from
Jun 22, 2021

Conversation

codedge
Copy link
Contributor

@codedge codedge commented Jun 20, 2021

While this PR got out of control πŸ˜„ and poses a bigger rewrite of the tests, it addresses multiple issues.

Passing index names via command line

Passing index name via command line now takes care if the name of the is already prefixed or not. Running

  • bin/console meili:import --indices=prefix_indexName
  • bin/console meili:import --indices=indexName

leads to the same index creations which is prefix_indexName - given the prefix hold a value.

Indexing different models into the same index

See #90

Although MS supports importing different models into the same index, this was not easiliy possible using this package.
The configuration in meili_search.yml would not allowe to set the same index name twice.

Now you can do the following

meili_search:
    # Other...
    indices:
        -   name: tags
            class: 'MeiliSearch\Bundle\Test\Entity\Tag'
            index_if: isPublic
        # Yes, we want to have links in the same index as tags
        # We just set the same index name 'tags'
        -   name: tags
            class: 'MeiliSearch\Bundle\Test\Entity\Link'
            index_if: isSponsored

This would lead to the result of having models of Tag and Link inside the very same index tags.

⚠️ Make sure your identity key is unique across both models. Given you use a field id, the value of this needs to be unique across both models.

@curquiza When the PR is approved and merged this should be documented inside a the wiki.

Using aggregators

See #90

You can now create an aggregator, register your models and will get an aggregated index created in MS. As the identity field of each model is used as identity field in the aggregated index...

Make sure your identity key is unique across both models. Given you use a field id, the value of this needs to be unique across both models.

@curquiza When the PR is approved and merged this should be documented inside a the wiki.

Fixing inconsistency

See #89

Fixes some left of overs from refatoring indexName -> Γ¬ndexUid`. #89 is outdated with this PR here.

Further changes

  • No separate folder to hold the test database in tests/blog.sqlite. Just use the var/test.sqlite folder which already holds cached/runtime data.
  • Reset the database and MS instance for each test via the setUp method. No need to have a dedicated refreshDb and clearUp call spread across the test. Each test should be atomic and not relying on former data. So πŸ—‘οΈ everything πŸ˜ƒ
  • Update the phpunit.xml to the more recent format using the --migrate-configuration option.
  • Updating the test entities to make it easier with having properties inside the database and imported into MS.
  • Converting the config to a Collection object, see illuminate/collection. Makes it a lot easier to deal with, if you ask me πŸ™ˆ

@curquiza
Copy link
Member

Thank you so much @codedge for taking care of this repository ❀️

It seems there are merge conflicts, I will review once the conflicts are done!

And sorry for the delay!

@codedge codedge force-pushed the same-index-multiple-entities branch from 78b6b42 to f5aa67a Compare June 21, 2021 18:22
@codedge
Copy link
Contributor Author

codedge commented Jun 21, 2021

Fixed

Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

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

LGTM! Awesome addition!

bors merge

@curquiza
Copy link
Member

@codedge, if I understand well, this PR is not breaking for the user?

@bors
Copy link
Contributor

bors bot commented Jun 22, 2021

@bors bors bot merged commit f259799 into main Jun 22, 2021
@bors bors bot deleted the same-index-multiple-entities branch June 22, 2021 13:36
@codedge
Copy link
Contributor Author

codedge commented Jun 22, 2021

Yes, it is not breaking. It even allows more πŸ˜€

@norkunas
Copy link
Collaborator

norkunas commented Jul 2, 2021

do you really needed to introduce laravel collections (in a Symfony world :D) dependency which pulls up another 2 dependencies only for this? :)

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

Successfully merging this pull request may close these issues.

None yet

3 participants