-
Notifications
You must be signed in to change notification settings - Fork 189
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
added advanced search query for elasticsearch #2617
added advanced search query for elasticsearch #2617
Conversation
This needs a proper description of what it is, what it changes, how to test it, etc. |
This was not meant to be compared to Seneca Master... I am still cherry picking commits - sorry about this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs tests.
f24b390
to
3925230
Compare
Where can I find the tests to write for this? I know you've mentioned it a bunch of times... do we want to just test the routes? |
They should be in https://github.com/Seneca-CDOT/telescope/tree/master/src/api/search, but apparently we have no tests for this service? Please file an issue for us to add them, since they don't exist, and we'll do it all at once. |
I've filed this: #2621 |
OK, so let's get my other comments addressed in updates and try to land this ASAP. |
e4e4f25
to
ad0209e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting close, a few more things
src/api/search/src/search.js
Outdated
}, | ||
}; | ||
|
||
const must = query.query.bool.must; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const { must } = query.query.bool;
src/api/search/src/search.js
Outdated
* Match field queries: https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-ppmatch-query.html#query-dsl-match-query-zero | ||
*/ | ||
const advancedSearch = async (options) => { | ||
const query = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets' name this result
so we don't have query.query
5d6fd1f
to
e772fb6
Compare
e772fb6
to
6c9c50b
Compare
6561ae8
to
41cf077
Compare
41cf077
to
30a0d26
Compare
Let us know when this is ready for review. |
added advanced route with params separated query validation based on route pathname cleaned up the files added validation cleaned up validator added oneof in declarations fixed validation changed query fields and added if statements for options passed changed description for advanced search fixed date format to correct format
e51118f
to
ff02418
Compare
it is ready now! Thank you @JerryHue! |
I updated all the instructions if you want to build and try it out. :) But also for my own memory... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where can I find the build instructions?
It's in the PR Description. @JerryHue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested, works good. My only concern is the lack of tests but the search service doesn't have any tests at all. Might want to file a follow-up to get those added in unless that's been filed already
It's been filed and assigned! :) |
Just noticed #2621 so once you get another approval, you can rebase and merge this in @AmasiaNalbandian |
Issue This PR Addresses
This PR closes #2110
Type of Change
Description
This PR Is aimed to help make search queries more flexible. What I have done here is I looked into Elasticsearch so that we can further query different fields we have indexed in Posts. I looked at the data presented back by Elasticsearch and realized we have more fields we can use to search:
This PR allows for us to query all of this on the backend, with search and Elasticsearch.
Additionally, this query seems to be more promising than the original query for the post/author search. We should eventually use the query provided here, as the
zero_terms_query: 'all',
means that if that field is not provided, to search without it. This means we can use this query to search for one, two, three... or all fields, acting the same way as the previous implementation but doing more. The query will also work if we want to search for all posts published on X date, or from x to y dates.Steps to test:
cp config/env.development .env
docker-compose --env-file ./config/env.development up --build search elasticsearch redis posts
docker-compose --env-file ./config/env.development up --build -d search posts
(NOT Necessary - Skip! just FYI)pnpm start
to start the feed parser. Wait a bit for some posts to populate.http://localhost/v1/search/advanced/?post=elasticsearch&author=amasia
feel free to add more filters such as author, title, from, to and post
After you are done ensure your containers are down by running
docker-compose --env-file ./config/env.development down
Checklist
Example of how the search works:
The screenshot is taken from elasticsearch on localhost:9200 for reference of the post: