-
Notifications
You must be signed in to change notification settings - Fork 76
[SVCS-675] Add file size limit to tabular renderer. #325
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
[SVCS-675] Add file size limit to tabular renderer. #325
Conversation
* Add more documentation for the `sheets` data structure. Rename or create some derived variables to make code clearer. * Log the number of columns and rows when one or the other exceeds the maximum allowed number.
* Tabular data has to be slurped into memory to be displayed, meaning large files can cause resource shortages or oom errors. Throw an informative error if the user tries to render a file larger than `MAX_FILE_SIZE` bytes. * Fix order of imports, removed old commented-out code.
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.
@felliott LGTM 🎆
Two minor and non-blocking issues:
- Import order and style
- Missing tests
I suggest we can make a ticket to do it after the release.
| import os | ||
| import json | ||
| import logging | ||
| import os |
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.
Fix import order which should be length based.
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.
I thought for MFR we were doing alphabetical order, or am I misremembering that?
|
|
||
| from mfr.core import extension | ||
| from mfr.extensions.tabular import settings | ||
| from mfr.extensions.tabular import exceptions |
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.
How about from mfr.extensions.tabular import settings, exceptions?
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.
👍
| from mfr.extensions.tabular import settings | ||
| from mfr.extensions.tabular import exceptions | ||
|
|
||
| logger = logging.getLogger(__name__) |
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.
👍
| import logging | ||
| import os | ||
|
|
||
| from humanfriendly import format_size |
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.
👍
A nice package that I was trying to find a while ago.
| self.attr_stack.append([self.__TYPE, {'formatting_function': self.formatting_function}]) | ||
|
|
||
|
|
||
| class FileTooLargeError(TabularRendererError): |
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.
I remember you mentioned in WB but I am not sure about MFR: when an exception is added, should we update the tests somewhere to make sure that serialization works?
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.
Not needed for MFR. That test is to make sure the error survives the pickling/unpickling done by WB's celery worker.
Ticket
Not created yet
Purpose
Refuse to render tabular files if they exceed a certain size.
Changes
Add a configurable setting for tabular file size limits. If files is above this size, refuse to render it.
Side effects
QA Notes
Deployment Notes