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

Feature/rework new search #1455

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Feature/rework new search #1455

wants to merge 2 commits into from

Conversation

kwahlin
Copy link
Contributor

@kwahlin kwahlin commented Jul 12, 2024

I've worked for some time now with decomposing the "Libris Search" code into smaller and more appropriate classes. This is how far I've come and hopefully the classes make much more sense now. Extensive tests are still lacking; untangling the whole thing required more effort than I expected. Some unit tests are the only thing I've had time to add so far.

The functionality should be more or less the same, however many minor flaws and hard coding have been addressed along the way. For example we now load filters from definitions instead of declaring them in the code. See libris/definitions#490. (Note: Required!)

Except for more tests, TODOs in the code, naming etc. there are a couple of other things that probably should be addressed:

  • Where should the code "live"? Now I've bundled up all the classes in one package search2 in whelk-core just to keep them together. Exactly what parts should belong to whelk-core and what should be in the rest module I'm not sure but it's something that needs to be figured out.
  • There are now two "utility" classes: Disambiguate and QueryUtil, where the former is supposed be used for operations requiring vocab resources while the latter is more for general methods that are either used by several classes or require a Whelk instance. There is a bit of an overlap between these two classes however and I'm not sure that they make total sense.

Even though it's far from perfect we should probably merge this ASAP before adding new features. There are of course no guarantees that it doesn't introduce new bugs but if that happens it should at least be a lot easier now to address them and to add tests preventing them from appearing again. I've tested fairly thoroughly by hand without finding any obvious (new) flaws. Adding new features in a sustainable way should hopefully also be a lot easier going forward.

This PR is huge, I know, sorry! But since it was so tangly to begin with it was really difficult to do this methodically piece by piece. Hence why there are only two commits; there were so many of the intermediate commits that didn't make sense in isolation so I figured it was better to just squash them all together.

Copy link
Contributor

@jannistsiroyannis jannistsiroyannis left a comment

Choose a reason for hiding this comment

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

I don't understand what half of this stuff is doing, so take anything I say with a pinch of salt. There seems to be a lot more to this than I imagined. :)

I thought this was supposed to do just two things:

  1. Find a mapping from all "wierd" code forms to their corresponding correct form (as used in elastic), like for example:
    förf -> kbv:author (or whatever it is).

  2. Build a must/should/whatever (elastic form query) out of the parsed query tree. This I know you had working very early, with a lot less code than this!

But there seems to be a lot of other stuff going on here that I neither understand how it works, or what it's for.

That of course doesn't mean that it's wrong! Just that I don't get it! :)

The one concrete thing I found (maybe) was at QueryTree:34, where it seems a static variable is being assigned with every query (and then used later on), which dosen't look thread safe (two concurrent queries could perhaps mess with each others results).

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