-
Notifications
You must be signed in to change notification settings - Fork 31
Refactoring saved search reporting APIs #73
Refactoring saved search reporting APIs #73
Conversation
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.
Should we delete dataReport.ts
and dateReportMetadata.ts
, it seems like they are no longer being used
Thanks for reminding! Removed these two and their test file. |
@akbhatta @davidcui-amzn @zhongnansu Addressed all your comments. I found that the current json-2-csv lib doesn't provide option for Excel sanitize. So I made a little more changes to avoid CSV injection by following same approach in SQL: opendistro-for-elasticsearch/sql#447. |
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.
Ship it!
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.
LGTM - just one minor question left in comments
Issue #, if available:
Description of changes:
Functional
nbRows
andscrollSize
argument in generating report API: Because no such option is provided for customer, fetching data by search or scroll is determined internally by data size and max result window setting.Example: Here is a sample request in which only
report_name
,report_format
,time_from
,time_to
andsaved_search_id
are interested:test report table order_2020-09-16T18:53:26.909Z_e78bcad0-f84d-11ea-9b2e-8572650d15ab.csv
Non-functional
excel
boolean option is added to avoid CSV injection for Excel.limit
param is added tocore_params
section. It's optional with default value 10k and will truncate result set to the limit given to avoid OOM.Testing: A new UT is added with mock ES client. Basic export by search/scroll code path is covered.
Not implemented yet:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.