@tgoeg Thanks for reporting this.
I have added a draft MR that could be tested. The idea behind the solution/workaround is before indexing items, go through all the fields and for any numeric types add a duplicated field to the index with __facet__ prefix and a value converted to string. Then when generating facets we go through each one and check if any of them are numeric type - if they are run the facet query on meilisearch with prefixed __facet__ instead of the original one. At the end move those __facet__ results to the proper/original facet.
Patch can be tested but it is nowhere ready for merging or production, it is in mvp state and there are issues with filterableAttributes re-setting sometimes, losing the __facet__ prefixed attributes etc.
Can be tested by applying the patch, having a numeric field in the index and reindexing the entire index.
At the moment there is no item boosting implemented. It should be possible to add a new "boost" attribute or similar to each document and have a custom ranking rule "boost:desc" as per https://www.meilisearch.com/docs/learn/core_concepts/relevancy#custom-rules.
This will require code changes so I'm updating the issue.
I went ahead and merged this. If the same problem impacts sortable attributes we will fix them in another issue. Thanks.
@ravi kant
There is an update hook that sets the default value via drush updb. I don't see any style breaking on my side.
Hi @tobiasb
Thanks for the report. IIRC we added a sort function to compare old and new searchable attributes so that we don't do unnecessary setting changes to Meilisearch index (which causes reindexing of every item). But given the fact that the order of attributes impacts relevancy - we should not sort them which the PR fixes.
I wonder if the same issue also impacts sortable attributes because we also use sort() on them...
@ravi kant
Yes, I managed to add settings field and overrided the toasts template to load the proper attribute from config.
I added a PR that fixes the template and adds a new setting field. I don't know if this will be merged but it might be helpful for others on how to modify their subtheme if needed.
If this issue at hand is available as a submodule, I think I can offer testing. It's just that I am currently stuck on 1.x, as the upgrade to 2.x breaks my facetted searches, even when I add the new facetting submodule. No errors whatsoever, I just don't get any results, but that's a different story.
Feel free to open an issue with as much info as possible, there might be some hidden issues we are not aware of.
I'm not really sure what to make of this feature as it is now. I think this goes way above what the base module should provide as I explained in #2. If this is moved to a submodule as perhaps a separate backend provider (so user can choose standard connector or advanced connector with keys management, auto generate keys for specific indexes etc.) then I'm ok with it. But in the current form of this PR I do not want to merge it. For this reason I'm setting the status to back to "Needs work".
What I do agree here is that current connector should not use master key, but API and Search key fields should be configurable and used instead. But this is a breaking change as current installations would need to update configuration manually and a separate issue should be opened on how to best deal with this.
Nice thanks, although I use custom docker-compose.yml, other contributors here use a DDEV setup with custom meiiisearch service added, this comes in handy to get the environment up and running easily.
I'll update the readme when I get the chance, it is missing a section for development (setting up, running automated tests. etc), and include the link to the addon.
Ready for review.
Branch rebased and added DI for event dispatcher.
Ported to 2.x as well, thanks.
Commited to 1.x, needs porting to 2.x
Merged, thanks.
I think @admirlju explained everything, Meilisearch does not have a concept of cores like Solr so I am closing this issue.
@admirlju Do we still need to do something here? This issue as you mentioned would only occur if reindexing happened while on the dev branch in between the issues we were fixing regarding the document id and @tgoeg probably did the reindexing between the issues. If I remember correctly we added a BC which had to be reverted.
I'm closing this issue for now, feel free to reopen if I missed something.
Merged, thanks.
I think merging this right now is too soon. Yes D9 is EOL but I'd like to keep the support for D9 for now. If it's the external dependency the issue, we could write our own adapter seeing that the dependency has a few lines of code anyway.
Merged, thanks.
Merged, thanks. Any issues regarding facets can use the "Facets" option from "Component" dropdown.
I've added an "Autocomplete" option to the "Component" dropdown so any issues regarding autocomplete submodule can use that option.
Branch rebased.
Merged, thanks.
I have removed the condition to skip filtering if "zxx" and "und" language are in filter. I also fixed the test as it was under incorrect namespace and was missing @group tag.
Needs a test for when autocomplete settings has a min/max limit set to make sure this works properly.
Rebased, cleaned up the code and used query limit from settings.
Merged to 2.x as well.
Thanks, merged but needs to be applied to 2.x as well.
Merged, thanks.
This works but we should use hidden and always enabled processor plugin https://www.drupal.org/docs/8/modules/search-api/getting-started/processors → .
Search method is already bloated with code and we will need to do some cleanup in the future but we can start here :)
Ready for review.
Merged, thanks.
Rebased and merged, thanks.
Merged, thanks.
Used EXISTS operator for null value operator parser and uncommented tests
Can be reviewed
Ready to move forward.
Ready to move forward.
Merged, thanks.
The new version of meili changed their naming from MeiliSearch to Meilisearch and the namespaces were not updated to match that, so changed that.
Good catch, thanks.
For whatever reason, locally the QueryFilterTest the testFiltersWithSpecialCharacters is failing for me, but here it seems it passes.
Did you update the meilisearch docker image to v1.3.3? The only reason I see tests failing locally is with v0.28.1 as the parser was updated to escape backslashes properly.
Merged, thanks.
Merged, thanks.
I created a 1.1.0-rc release → which still uses v0.28 so the users are not stuck on the ancient and mostly broken 1.0.0 release while we work on creating a 2.x release supporting the latest meilisearch server.
Suprisingly there were very few things needed to fix, so I updated all links as well and changed MeiliSearch to Meilisearch wording wherever I could find. Ready for test.
Unblocked and ready to be pushed forward
Created a new 2.x branch
I think we got everything covered. Thanks everyone.
Rebased and merged, thanks.
Merged, thanks.
Added presave hook to throw an exception if id field is defined on an index and the index uses Meilisearch. Also reworded the error messages to be more user friendly.
Tests revealed more issues connected that I had to fix:
- Special fields were not added to filterable and sortable attributes
- Setting backend configuration programatically would not update url or master key in the meilisearch api service
I have a feeling we need to drop and recreate the index when upgrading from 1.0.0 to dev-1.0.x anyway. Not 100% sure about it, but that's what I did whenever I got strange results (as e.g. in #3385410: No results from file_extractor extracted data in Drupal 10.1 and some other scenarios not documented in the issue queue) and it mostly magically solved it. I could be wrong and the fixed issue #3384347: Remove entity id mapping config (and my multi-version setup) was also to blame and things work out with the current codebase.
Yes the fixed issue was the culprit as we changed the primary key there, but Meilisearch does not allow that when documents already exist. Hence this issue was opened.
It might even be easier for end users to automatically have them being recreated, as otherwise they would need to migrate the data over when switching Meilisearch versions (<1.0 to >1.0, if that even works?) but I have to look into this sometime.
The above restrictions sound like too much of a hassle only for making the upgrade process easier for the few people that might use it in production already.
Deleting the index and re-importing the config with drush is a matter of 2 minutes. I had upgrade paths that were a lot harder than this!
This would be fine if Meilisearch indexes contain only data from Drupal and is most likely the case for 99% of the users, but if some custom documents were added from other sources (not in Drupal) that data would be lost and that is why I'm hesitant to go this route. As for migrating the data when upgrading Meilisearch - this is completely on Meilisearch side, there are upgrade guides on their official page but our module should work just fine. When I was testing 🐛 Invalid characters in filter values returns an error Fixed I had to switch versions and did data upgrades, this module still worked just fine with it.
I mean for me as a developer the easiest approach would be to do nothing and just say "Hey users just delete everything and recreate" but that sets a bad precedence (imo) for the quality of the code, product etc.
I'd consider 1.0.0 to only be something for the daring, so anyone really using it in production (is this anyone else but me?) should be OK to have a more elaborate update to what I'd probably call 2.0.0, if this eases future development and user friendliness.
My thinking here is that once we fix all the critical issues (without upgrading meilisearch php and support for never versions) I would make a 1.1.0 release supporting the Meilisearch v0.28.1 then create a new 2.x branch where we start upgrading meilisearch version support and make a 2.0.0 release for the Meilisearch v1.3 version.
I do have a few issues to add (from the top of my head I know at least one major issue exists with synonyms processor) that are blocking a new stable release, I will get to them when I find the time.
Updated the UpdateHook9002Test.php test that reproduces the issue.
An upgrade to a new version implementing #3384347: Remove entity id mapping config means not only do the indexes have to be rebuilt, they have to be deleted and created anew (easiest by just drush cim after deleting the index as all views etc. also get lost when deleting the index; having a current config exported beforehand should be self-explanatory but could be added as well).
As mentioned in the other issue this needs to be looked at and fixed. I'll see if I can reproduce this and open a new issue.
Merged, thanks.
Hmm thanks for reporting this, this should be a new issue, we should handle this somehow via update hooks, the users should not be required to delete indexes.
Fixed in 🐛 AJAX HTTP error occurred when adding search server Fixed . Giving credits here as well.
Merged, thanks.
Merged, thanks.
The "read only" feature should work as it was designed by the search_api module. Theres no point at the moment making some custom logic for this.
So the data in meilisearch must not be altered in any way if the index is set to read-only. That also means that if you are setting an index as read-only, you expect data to exist in meilisearch and is ready for use. The thing here is that the fields meilisearch documents must match with how we generate them by indexing items. So at least the attribute "search_api_id" must exist in documents to match documents with drupal entities. In short, connecting to an existing meilisearch index will work only if the index and documents were generated by some other drupal instance using this module.
If we want to support connecting to indexes that were generated by 3rd party, then a custom datasource would need to be implemented. Solr does this by implementing solr_document and solr_multisite_document datasource plugins that allow you to configure field mapping ie. drupal field <-> solr field. But that is another feature that can be implemented in the future.
As for the different api keys I already mentioned possible solution in ✨ Allow usage of API with dropped privileges (not only master key) Needs work . Feel free to contribute there.
I believe the issue is only with one \ character at the end, having 2 or more works fine and the other characters should not present any issues.
This is not entirely correct, using any odd number of backslashes will produce an error (1, 3, 5 etc. backslashes)
Rebased the branch and added a fix. The api service was not being removed from the plugin before serialization which caused the error.
Rebased, added minor test fixes and merged. Thanks everyone.
Rebased and merged, thanks everyone.
(I'd even trade in more inconvenience to get rid of that cumbersome configuration files! It makes rolling out dev config to production a pretty dangerous step if you don't exclude these files. And export production config before. And then merge the new dev config with the old entity id mapping of prod. This is content-dependent configuration which always makes my life hard.)
Ouch, I forgot about this completely. That is definitely a big problem on deployments. If you have any other hassles like this, open an issue! While we are trying to get the module into a stable production ready state we are limited in thinking of issues that potentially exist. We are aware of issues in the code or functionality, but stuff like fiddling with configs across deployments didn't cross my mind as I am running the module on local environment only with a clean drupal installation.