feat: Add dashboard search filter support #112

feng-tao · 2020-06-09T19:57:48Z

Summary of Changes

Include search filter for dashboard
Refactor filter API to be generic with base API
rename search table filter API from search_table to search_table_filter to make it consistent across all entities.

Tests

Add API test and change related backend tests.

Documentation

What documentation did you add or modify and why? Add any relevant links then remove this line

CheckList

Make sure you have checked all steps below to ensure a timely review.

PR title addresses the issue accurately and concisely. Example: "Updates the version of Flask to v1.0.2"
- In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.
PR includes a summary of changes.
PR adds unit tests, updates existing unit tests, OR documents why no test additions or modifications are needed.
In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain docstrings that explain what it does
PR passes make test

codecov-commenter · 2020-06-09T20:07:50Z

Codecov Report

Merging #112 into master will decrease coverage by 0.51%.
The diff coverage is 72.22%.

@@            Coverage Diff             @@
##           master     #112      +/-   ##
==========================================
- Coverage   75.28%   74.76%   -0.52%     
==========================================
  Files          17       18       +1     
  Lines         615      650      +35     
  Branches       77       85       +8     
==========================================
+ Hits          463      486      +23     
- Misses        127      135       +8     
- Partials       25       29       +4

Impacted Files	Coverage Δ
search_service/proxy/elasticsearch.py	`73.50% <65.76%> (-3.08%)`	⬇️
search_service/api/base.py	`92.30% <92.30%> (ø)`
search_service/api/dashboard.py	`100.00% <100.00%> (ø)`
search_service/api/table.py	`100.00% <100.00%> (ø)`
search_service/proxy/atlas.py	`49.03% <100.00%> (ø)`
search_service/proxy/base.py	`68.18% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a56bea6...e28c2f8. Read the comment docs.

ttannis

Some comments and questions.

search_service/__init__.py

search_service/api/filter.py

jinhyukchang

Good refactoring! Left some comments.

jinhyukchang · 2020-06-09T21:44:08Z

search_service/api/filter.py

+        """
+        Fetch search results based on the page_index, query_term, and
+        search_request dictionary posted in the request JSON.
+        :return: list of table results. List can be empty if query


How about updating to: return json payload of schema? Also, from the structure of this generic class, I don't think we can assume it's a list.

jinhyukchang · 2020-06-09T21:45:08Z

search_service/api/filter.py

+            return {'message': msg}, HTTPStatus.BAD_REQUEST
+
+        query_term = args.get('query_term')  # type: str
+        if ':' in query_term:


Didn't know this. Why we don't allow this?

mostly due to colon is not allowed or needs to escape in doing query string filter.

jinhyukchang · 2020-06-09T21:46:58Z

search_service/api/filter.py

+from search_service.proxy import get_proxy_client
+
+
+class BaseFilterAPI(Resource):


Suggestion: how about separate this class into separate file? It just because this file would grow as we introduce more filterable resource.

jinhyukchang · 2020-06-09T21:48:28Z

search_service/api/swagger_doc/dashboard/search_dashboard_filter.yml

+                  schema:
+                    type: integer
+                    default: 0


Mmm... Not sure what schema with default value 0 is for.

copy paste error, will clean up.

jinhyukchang · 2020-06-09T21:55:11Z

search_service/proxy/base.py

-                                               index: str = '') -> SearchTableResult:
+    def fetch_search_results_with_filter(self, *,
+                                         query_term: str,
+                                         search_request: dict,


Do we validate if the key is filterable key? For example, if client sends some unknown key, how search service would react?

example: { 'bogus': ['mode'] }

currently we validate the search filter value inside the proxy https://github.com/lyft/amundsensearchlibrary/blob/master/search_service/proxy/elasticsearch.py#L365

search_service/proxy/elasticsearch.py

feng-tao · 2020-06-16T05:38:00Z

@jinhyukchang @ttannis update the pr and test it on staging
cc @dikshathakur3119 @Golodhros

jinhyukchang

Looks good. Thanks for the update!

ttannis

Previous comments resolved

feat: Add search filter support dashboard

382b0fe

feng-tao requested review from ttannis and jinhyukchang June 9, 2020 19:57

fix mypy

bccef72

feng-tao changed the title ~~feat: Add search filter support dashboard~~ feat: Add dashboard search filter support Jun 9, 2020

keep table filter endpoint the same for now

b59eb76

ttannis reviewed Jun 9, 2020

View reviewed changes

search_service/__init__.py Show resolved Hide resolved

search_service/__init__.py Show resolved Hide resolved

search_service/api/filter.py Outdated Show resolved Hide resolved

jinhyukchang reviewed Jun 9, 2020

View reviewed changes

feng-tao added 3 commits June 9, 2020 22:03

fix issue

20eb829

pass index by default

f802daf

remove index and keep page_index

168d8ee

ttannis reviewed Jun 11, 2020

View reviewed changes

search_service/proxy/elasticsearch.py Show resolved Hide resolved

feng-tao commented Jun 11, 2020

View reviewed changes

search_service/proxy/elasticsearch.py Outdated Show resolved Hide resolved

feng-tao added 3 commits June 11, 2020 14:01

update tag name

681bf88

update per feedback

aff3b1b

update version

e28c2f8

jinhyukchang approved these changes Jun 16, 2020

View reviewed changes

ttannis approved these changes Jun 16, 2020

View reviewed changes

feng-tao merged commit 17c6739 into master Jun 16, 2020

feng-tao deleted the tfeng_search_filter branch June 16, 2020 18:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add dashboard search filter support #112

feat: Add dashboard search filter support #112

feng-tao commented Jun 9, 2020

codecov-commenter commented Jun 9, 2020 •

edited

Loading

ttannis left a comment

jinhyukchang left a comment

jinhyukchang Jun 9, 2020

jinhyukchang Jun 9, 2020

feng-tao Jun 10, 2020

jinhyukchang Jun 9, 2020

jinhyukchang Jun 9, 2020

feng-tao Jun 10, 2020

jinhyukchang Jun 9, 2020

feng-tao Jun 16, 2020

feng-tao commented Jun 16, 2020

jinhyukchang left a comment

ttannis left a comment

		from search_service.proxy import get_proxy_client


		class BaseFilterAPI(Resource):

feat: Add dashboard search filter support #112

feat: Add dashboard search filter support #112

Conversation

feng-tao commented Jun 9, 2020

Summary of Changes

Tests

Documentation

CheckList

codecov-commenter commented Jun 9, 2020 • edited Loading

Codecov Report

ttannis left a comment

Choose a reason for hiding this comment

jinhyukchang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feng-tao commented Jun 16, 2020

jinhyukchang left a comment

Choose a reason for hiding this comment

ttannis left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 9, 2020 •

edited

Loading