Bugfix: autocomplete suggests nonsearchable terms

kuafucode commented 2 months ago

Description (*)

Since 2.4.7 we noticed search autocomplete would suggest search terms with no result.

here are some more cases of bad search suggestions with sample data package : Intending to search for "Didi Sports Watch" "didi" -> suggestion "duti" with no matches

Intending to search for "Bottle" "bot" -> suggestion for "but" and "boi", the latter bringing no search results

Intending to serach for "Summit "summ" -> suggestion "seam" and "sued" which bring results but not the watch

Manual testing scenarios (*)

install sample data package
type in following terms in searchbox on store front and verify it don't suggest autocomplete with strange result:
- "didi" -> does not suggestion "duti"
- "bot" -> does not suggest "boi"
- "summ" -> does notsuggestion "seam" or "sued"

Contribution checklist (*)

[x ] Pull request has a meaningful description of its purpose
[x] All commits are accompanied by meaningful commit messages
[ ] All new or changed code is covered with unit/integration tests (if applicable)
[ ] README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
[ ] All automated tests passed successfully (all builds are green)

Vinai commented 2 months ago

Thanks very much for the PR! I'll test it out today :)

kuafucode commented 2 months ago

updated unitest, please check again

rhoerr commented 2 months ago

To check my understanding of the situation:

The issue is that the autocomplete suggestions were including fields that aren't actually searchable.
There isn't an existing field or flag in the context (app/code/Magento/Elasticsearch/Model/DataProvider/Base/Suggestions.php) that would allow us to limit it to searchable fields -- or maybe there is a searchable field but it doesn't give the data we need.
To address that, you added a new suggestible flag to the field mapping, and set that to the value of is_searchable from the underlying attribute.
Then you check that new suggestible flag when filtering fields for search suggestions, which limits it to values that will actually produce searchable matches.

Is that correct?

kuafucode commented 2 months ago

@rhoerr that's correct

There isn't an existing field or flag in the context (app/code/Magento/Elasticsearch/Model/DataProvider/Base/Suggestions.php) that would allow us to limit it to searchable fields -- or maybe there is a searchable field but it doesn't give the data we need.

There might be a way to retrieve the searchable flag by adding a dependency, but adding this flag to the field mapping is more natural and reliable to me.

Vinai commented 2 months ago

I just ran bin/magento setup:upgrade while still on the branch, and I now get an error that seems related:

Running data recurring...{"error":{"root_cause":[{"type":"mapper_parsing_exception","reason":"unknown parameter [suggestible] on mapper [custom_layout_update_file] of type [keyword]"}],"type":"mapper_parsing_exception","reason":"unknown parameter [suggestible] on mapper [custom_layout_update_file] of type [keyword]"},"status":400}

Commenting out

$fieldMapping[$fieldName]['suggestible'] = $attributeAdapter->isSuggestible();

and

$fieldMapping[$childFieldName]['suggestible'] = $attributeAdapter->isSuggestible();

in StaticField.php makes the error go away. Seems like that array is used in more ways maybe?

Do you have an idea?

kuafucode commented 2 months ago

Yes I was able to replicate the error. I thought the FieldProvider class I modified is a provider for in-memory mapping, turns out it is also used to build mapping for index, and we can not introduce a random property 'isSuggestible' just like that, opensearch won't take it.

If there is no way to get searchable flag from existing index, I might have to limit the modification within the Suggestions class, and retrieve searchable attributes from EAV.

My initial thought was to change isSearchable definition in vendor/magento/module-elasticsearch/Model/Adapter/FieldMapper/Product/AttributeAdapter.php to just

  /**
     * Check if attribute is searchable.
     *
     * @return bool
     */
    public function isSearchable(): bool
    {
        return $this->getAttribute()->getIsSearchable();
    }

as it seems to me developers mixed up 'searchable' and 'indexible' overtime, but that might be a backward incompatible change.

Vinai commented 2 months ago

Oh boy, mixing concerns indeed. I'm sorry I'm not familiar enough with the code to provide ideas for sensible workarounds. That said, I trust your technical prowess :)

kuafucode commented 2 months ago

PR updated, please review

rhoerr commented 1 month ago

I requested small constructor changes for backwards compatibility, but otherwise it looks good to me. It's much less likely to cause problems with your refactor, thanks for doing that.

rhoerr commented 1 month ago

I believe the unit test failures are unrelated and a result of the in-progress upstream transition to PHPUnit 10.

Since Vinai tested and approved other than the constructor change that is now done, I'm going to go ahead and merge this.

kuafucode commented 1 month ago

@rhoerr I think it is still related to unit test for this change. I thought Suggestions is not a unitest covered class as I don't see unit test of this class in elasticsearch module, but it is actually in the elasticsearch7/opensearch module, we will have to look into those tests

mage-os / mageos-magento2