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

feat(search): The aliases are now searchable. #313

Merged

Conversation

panwarabhishek345
Copy link
Contributor

Problem

#BB-310 Search for entity by alias added and was not possible earlier.

Solution

Added multi_match query and indexed entity aliases in elastic search.

Areas of Impact

src/server/helpers/search.js

@coveralls
Copy link

coveralls commented Nov 2, 2019

Coverage Status

Coverage remained the same at 43.014% when pulling 87e50b4 on panwarabhishek345:elasticsearch-alias-search into 762ffc6 on bookbrainz:master.

@MonkeyDo
Copy link
Contributor

MonkeyDo commented Nov 5, 2019

🎉
Good choice of issue, that's a great improvement for an easy solution !

I'm not going to merge this right now, even though the search page part works perfectly, because there are other areas that use the search, and I need to determine if including aliases is what we want everywhere.

For example, when creating a new entity, we do a search based on the name to see if the new entity would be a possible duplicate. I am not 100% sure yet that searching for aliases here would be okay, and it might depend on the entity type.

Capture d’écran 2019-11-05 à 13 17 27

It does say "with exactly the same name *or alias* " there, but I need to ensure that won't be a problem for certain cases.

@panwarabhishek345
Copy link
Contributor Author

panwarabhishek345 commented Nov 5, 2019

That's totally fine!

@MonkeyDo
Copy link
Contributor

MonkeyDo commented Nov 6, 2019

I thought about this a bit more, and did some testing.

First of all, my concerns over what other components it could affect were unfounded, so that's good !

Second, I think you can actually revert to a single match query where you used a multi_match, but change the indexing so that 'aliasSet.aliases' is indexed instead of defaultAlias.
The reason for that is that the defaultAlias is already on of the aliases contained in aliasSet.aliases.

@panwarabhishek345
Copy link
Contributor Author

@MonkeyDo That's great. If it is then yes, we can use single match query. And what about the "creating a new entity" page?

@MonkeyDo
Copy link
Contributor

MonkeyDo commented Nov 6, 2019

That uses another mechanism to do the search on the database, and is unaffected by the changes in this PR.

I did find an important bug however, when testing the search component in the relationship editor.
Since you're working on search functionality, I figured you might be interested in having a go at it: https://tickets.metabrainz.org/browse/BB-361
It's another component entirely, but uses the results of the work you did in this PR
P.S: There's absolutely no rush, I'm just putting this here not to forget about it :)

@panwarabhishek345
Copy link
Contributor Author

@MonkeyDo Made the appropriate changes to the commit. Now only aliasSet.aliases is indexed and queried.

@MonkeyDo
Copy link
Contributor

MonkeyDo commented Nov 7, 2019

OK, I've done more testing, and I am satisfied this PR works nicely with no undesirable side-effects !

Thank you !

@MonkeyDo MonkeyDo merged commit 9eae9e4 into metabrainz:master Nov 7, 2019
@panwarabhishek345
Copy link
Contributor Author

panwarabhishek345 commented Nov 7, 2019 via email

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