-
Notifications
You must be signed in to change notification settings - Fork 66
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
fix: fix table post/put api bug #172
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,10 +2,11 @@ | |
# SPDX-License-Identifier: Apache-2.0 | ||
|
||
import logging | ||
import json | ||
|
||
from http import HTTPStatus | ||
from typing import Tuple, Any | ||
|
||
from ast import literal_eval | ||
from flasgger import swag_from | ||
from flask_restful import Resource, reqparse | ||
from search_service.proxy import get_proxy_client | ||
|
@@ -58,7 +59,7 @@ def post(self) -> Tuple[Any, int]: | |
:param data: list of data objects to be indexed in Elasticsearch | ||
:return: name of new index | ||
""" | ||
self.parser.add_argument('data', required=True) | ||
self.parser.add_argument('data', required=True, action='append') | ||
args = self.parser.parse_args() | ||
|
||
try: | ||
|
@@ -78,11 +79,15 @@ def put(self) -> Tuple[Any, int]: | |
:param data: list of data objects to be indexed in Elasticsearch | ||
:return: name of index | ||
""" | ||
self.parser.add_argument('data', required=True) | ||
self.parser.add_argument('data', required=True, action='append') | ||
args = self.parser.parse_args() | ||
|
||
try: | ||
data = self.schema(many=True, strict=False).loads(args.get('data')).data | ||
table_dict_list = [] | ||
for table_str in args.get('data'): | ||
table_dict_list.append(literal_eval(table_str)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you could also simplify the above as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does the endpoint work for user,table,dashboard as well? would be good to add a todo for things that are missing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For user, we already have a different endpoint, for dashboard, we need to think about it, not sure whether the fields are the same as table, if not, we can also create a new one. |
||
table_list_json = json.dumps(table_dict_list) | ||
data = self.schema(many=True, strict=False).loads(table_list_json).data | ||
results = self.proxy.update_document(data=data, index=args.get('index')) | ||
return results, HTTPStatus.OK | ||
except RuntimeError as e: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,38 +8,46 @@ | |
|
||
from .base import Base | ||
from search_service.models.tag import Tag | ||
import time | ||
|
||
|
||
@attr.s(auto_attribs=True, kw_only=True) | ||
class Table(Base): | ||
""" | ||
This represents the part of a table stored in the search proxy | ||
""" | ||
database: str | ||
cluster: str | ||
schema: str | ||
name: str | ||
key: str | ||
id: str | ||
database: Optional[str] = None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are we changing them to optional str? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. some fields may be |
||
cluster: Optional[str] = None | ||
schema: Optional[str] = None | ||
name: Optional[str] = None | ||
key: Optional[str] = None | ||
display_name: Optional[str] = None | ||
tags: List[Tag] | ||
badges: List[Tag] | ||
tags: List[Tag] = [] | ||
badges: List[Tag] = [] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typically we put None for list default value (e.g https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds good, I just wanted to be consistent with original code |
||
description: Optional[str] = None | ||
last_updated_timestamp: int | ||
last_updated_timestamp: int = int(time.time()) | ||
# The following properties are lightly-transformed properties from the normal table object: | ||
column_names: List[str] | ||
column_names: List[str] = [] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same for other list |
||
column_descriptions: List[str] = [] | ||
programmatic_descriptions: List[str] = [] | ||
# The following are search-only properties: | ||
total_usage: int = 0 | ||
schema_description: Optional[str] = attr.ib(default=None) | ||
|
||
def get_id(self) -> str: | ||
# uses the table key as the document id in ES | ||
return self.key | ||
return self.id | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this id is same as table key or the ES document id? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ES document id is always root of truth, but we can also use key as the ES document id (assuming key is unique) |
||
|
||
def get_attrs_dict(self) -> dict: | ||
attrs_dict = self.__dict__.copy() | ||
attrs_dict['tags'] = [str(tag) for tag in self.tags] | ||
attrs_dict['badges'] = [str(tag) for tag in self.badges] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [str(badge) for badge in self.badges] ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds good. |
||
return attrs_dict | ||
|
||
@classmethod | ||
def get_attrs(cls) -> Set: | ||
return { | ||
'id', | ||
'name', | ||
'key', | ||
'description', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -116,6 +116,8 @@ def _get_search_result(self, page_index: int, | |
|
||
for hit in response: | ||
try: | ||
es_metadata = hit.__dict__.get('meta', {}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it has been a while, could you add an example on the return value of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure, will do. |
||
|
||
# ES hit: {'_d_': {'key': xxx...} | ||
es_payload = hit.__dict__.get('_d_', {}) | ||
if not es_payload: | ||
|
@@ -124,6 +126,7 @@ def _get_search_result(self, page_index: int, | |
for attr, val in es_payload.items(): | ||
if attr in model.get_attrs(): | ||
result[attr] = self._get_instance(attr=attr, val=val) | ||
result['id'] = self._get_instance(attr='id', val=es_metadata['id']) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the id is ES document id? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes |
||
|
||
results.append(model(**result)) | ||
except Exception: | ||
|
@@ -590,7 +593,7 @@ def _build_update_actions(self, data: List[Table], index_key: str) -> List[Dict[ | |
|
||
for item in data: | ||
actions.append({'update': {'_index': index_key, '_type': item.get_type(), '_id': item.get_id()}}) | ||
actions.append({'doc': item.__dict__}) | ||
actions.append({'doc': item.get_attrs_dict()}) | ||
return actions | ||
|
||
def _build_delete_actions(self, data: List[str], index_key: str, type: str) -> List[Dict[str, Any]]: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,7 +33,7 @@ def test_post(self, get_proxy: MagicMock, RequestParser: MagicMock) -> None: | |
@patch('search_service.api.document.get_proxy_client') | ||
def test_put(self, get_proxy: MagicMock, RequestParser: MagicMock) -> None: | ||
mock_proxy = get_proxy.return_value = Mock() | ||
RequestParser().parse_args.return_value = dict(data='{}', index='fake_index') | ||
RequestParser().parse_args.return_value = dict(data=[], index='fake_index') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could you add a unit test to update multiple values as it is broken before? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good, will do. |
||
|
||
response = DocumentTablesAPI().put() | ||
self.assertEqual(list(response)[1], HTTPStatus.OK) | ||
|
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.
is table_str actually a tablefields object? the naming is a bit confusing. also we typically don't put type behind the variable for naming.
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.
what are we using
literal_eval
here for?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.
table_str is a string representation of dict, like '{"key1": val1, "key2": val2}'
literal_eval
will concert the table_str into a dict first, orjson.dumps
will dump a list ofstring
instead of a list ofdict