Bugfix: autocomplete suggests nonsearchable terms

[kuafucode]

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 (*)

1. install sample data package
2. 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
• 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]

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

[kuafucode]

updated unitest, please check again

[rhoerr]

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]

@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]

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]

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]

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]

PR updated, please review

[rhoerr]

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.

[Vinai]

Besides the change request from @rhoerr to the constructor parameters, I have tested it and can confirm it works as intended! Also, setup:upgrade works again, too.

Thank you very much!

[rhoerr]

Looks good to me! Thank you

[rhoerr]

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]

@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