[plaso] CSV parser: moved sources, changes for Unicode issue #1468, added tests

[plaso] CSV parser: moved sources, changes for Unicode issue #1468, added tests

[onager, 2017-10-19 21:42:34]

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/csv_parser.py
File plaso/parsers/csv_parser.py (right):

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/csv_parser.py#n...
plaso/parsers/csv_parser.py:15: # The Python 2 version of the csv module does
not support Unicode input
I don't think this class should be in this file. In fact, I think this is
probably best in dfvfs.

If not, move it to lib/, since that where the other specialized file reader is
(binary.py).

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/csv_parser.py#n...
plaso/parsers/csv_parser.py:72: the file-like object, where None represents all
remaining data.
Doesn't None represent no limit, not all data?

Also, mention that this can't be bigger than the overall size limit, per below
code.

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/csv_parser.py#n...
plaso/parsers/csv_parser.py:74: Raises:
+Returns

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/csv_parser.py#n...
plaso/parsers/csv_parser.py:228: # Pythonesque alias for get_offet().
offset

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/csv_parser.py#n...
plaso/parsers/csv_parser.py:239: """Comma seperated values (CSV) parser
interface."""
Please rename this, given this is a parser for all character-separated text
files (as per the value separator comment below).

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/csv_parser.py#n...
plaso/parsers/csv_parser.py:239: """Comma seperated values (CSV) parser
interface."""
Please add extra notes there that subclasses will at least need to override the
COLUMNS constant, and implement Parse and VerifyRow.

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/csv_parser.py#n...
plaso/parsers/csv_parser.py:327: except (StopIteration, csv.Error):
Please add exception information to the error message

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/csv_parser.py#n...
plaso/parsers/csv_parser.py:370: """Parses a line of the log file and extract
events.
-extracts +produces

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/csv_parser.py#n...
plaso/parsers/csv_parser.py:381: """Verifies if a line of the file corresponds
with the expected format.
-corresponds +matches or "is in"

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/mactime.py
File plaso/parsers/mactime.py (right):

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/mactime.py#newc...
plaso/parsers/mactime.py:143: """Verifies if a line of the file corresponds with
the expected format.
-corresponds +matches or "is in"

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/mactime.py#newc...
plaso/parsers/mactime.py:157: # Check if the md5 a valid MD5 or '0' if not set.
+is

Or drop this comment, I find it more confusing than helpful. It'd also be easier
to make the capitalization consistent.

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/mcafeeav.py
File plaso/parsers/mcafeeav.py (right):

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/mcafeeav.py#new...
plaso/parsers/mcafeeav.py:97: row (dict[str, str]): fields of a single row, as
denoted in COLUMNS.
-denoted +specified

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/mcafeeav.py#new...
plaso/parsers/mcafeeav.py:125: """Verifies if a line of the file corresponds
with the expected format.
See previous comment about this docstring

https://codereview.appspot.com/332950043/diff/1/test_data/password.txt
File test_data/password.txt (right):

https://codereview.appspot.com/332950043/diff/1/test_data/password.txt#newcode1
test_data/password.txt:1: place,user,password
rename to password.csv?

https://codereview.appspot.com/332950043/diff/1/tests/parsers/csv_parser.py
File tests/parsers/csv_parser.py (right):

https://codereview.appspot.com/332950043/diff/1/tests/parsers/csv_parser.py#n...
tests/parsers/csv_parser.py:159: """Tests the comma seperated values (CSV)
parser."""
separated.

https://codereview.appspot.com/332950043/diff/1/tests/parsers/mactime.py
File tests/parsers/mactime.py (right):

https://codereview.appspot.com/332950043/diff/1/tests/parsers/mactime.py#newc...
tests/parsers/mactime.py:33: # The order in which CSVParser generates events is
nondeterministic
Is this comment needed any more? All lists of events are unsorted, AFAIK, unless
GetSorted.. is used.

https://codereview.appspot.com/332950043/diff/1/tests/parsers/mcafeeav.py
File tests/parsers/mcafeeav.py (right):

https://codereview.appspot.com/332950043/diff/1/tests/parsers/mcafeeav.py#new...
tests/parsers/mcafeeav.py:30: events = list(storage_writer.GetEvents())
GetSortedEvents

https://codereview.appspot.com/332950043/diff/1/tests/parsers/symantec.py
File tests/parsers/symantec.py (right):

https://codereview.appspot.com/332950043/diff/1/tests/parsers/symantec.py#new...
tests/parsers/symantec.py:48: events = list(storage_writer.GetEvents())
GetSorted

[Joachim Metz, 2017-10-20 05:46:42]

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/csv_parser.py
File plaso/parsers/csv_parser.py (right):

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/csv_parser.py#n...
plaso/parsers/csv_parser.py:15: # The Python 2 version of the csv module does
not support Unicode input
it should not be in dfvfs, seeing a readline interface for a non-text file is
not that common. I'll move it to lib.

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/csv_parser.py#n...
plaso/parsers/csv_parser.py:72: the file-like object, where None represents all
remaining data.
not sure what you mean

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/csv_parser.py#n...
plaso/parsers/csv_parser.py:74: Raises:
On 2017/10/19 21:42:34, onager wrote:
> +Returns

Done.

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/csv_parser.py#n...
plaso/parsers/csv_parser.py:228: # Pythonesque alias for get_offet().
On 2017/10/19 21:42:34, onager wrote:
> offset

Done.

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/csv_parser.py#n...
plaso/parsers/csv_parser.py:239: """Comma seperated values (CSV) parser
interface."""
rename to what, VSVParser or ValueSeparatedRecordParser? CSV, though technically
incorrect, is the most commonly used term for this?

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/csv_parser.py#n...
plaso/parsers/csv_parser.py:239: """Comma seperated values (CSV) parser
interface."""
On 2017/10/19 21:42:34, onager wrote:
> Please add extra notes there that subclasses will at least need to override
the
> COLUMNS constant, and implement Parse and VerifyRow.

Done.

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/csv_parser.py#n...
plaso/parsers/csv_parser.py:327: except (StopIteration, csv.Error):
On 2017/10/19 21:42:34, onager wrote:
> Please add exception information to the error message

Done.

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/csv_parser.py#n...
plaso/parsers/csv_parser.py:370: """Parses a line of the log file and extract
events.
On 2017/10/19 21:42:34, onager wrote:
> -extracts +produces

Done.

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/csv_parser.py#n...
plaso/parsers/csv_parser.py:381: """Verifies if a line of the file corresponds
with the expected format.
On 2017/10/19 21:42:34, onager wrote:
> -corresponds +matches or "is in"

Done.

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/mactime.py
File plaso/parsers/mactime.py (right):

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/mactime.py#newc...
plaso/parsers/mactime.py:143: """Verifies if a line of the file corresponds with
the expected format.
On 2017/10/19 21:42:34, onager wrote:
> -corresponds +matches or "is in"

Done.

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/mactime.py#newc...
plaso/parsers/mactime.py:157: # Check if the md5 a valid MD5 or '0' if not set.
On 2017/10/19 21:42:34, onager wrote:
> +is
> 
> Or drop this comment, I find it more confusing than helpful. It'd also be
easier
> to make the capitalization consistent.

Done.

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/mcafeeav.py
File plaso/parsers/mcafeeav.py (right):

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/mcafeeav.py#new...
plaso/parsers/mcafeeav.py:97: row (dict[str, str]): fields of a single row, as
denoted in COLUMNS.
On 2017/10/19 21:42:34, onager wrote:
> -denoted +specified

Done.

https://codereview.appspot.com/332950043/diff/1/plaso/parsers/mcafeeav.py#new...
plaso/parsers/mcafeeav.py:125: """Verifies if a line of the file corresponds
with the expected format.
On 2017/10/19 21:42:34, onager wrote:
> See previous comment about this docstring

Done.

https://codereview.appspot.com/332950043/diff/1/test_data/password.txt
File test_data/password.txt (right):

https://codereview.appspot.com/332950043/diff/1/test_data/password.txt#newcode1
test_data/password.txt:1: place,user,password
On 2017/10/19 21:42:34, onager wrote:
> rename to password.csv?

Done.

https://codereview.appspot.com/332950043/diff/1/tests/parsers/csv_parser.py
File tests/parsers/csv_parser.py (right):

https://codereview.appspot.com/332950043/diff/1/tests/parsers/csv_parser.py#n...
tests/parsers/csv_parser.py:159: """Tests the comma seperated values (CSV)
parser."""
On 2017/10/19 21:42:34, onager wrote:
> separated.

Done.

https://codereview.appspot.com/332950043/diff/1/tests/parsers/mactime.py
File tests/parsers/mactime.py (right):

https://codereview.appspot.com/332950043/diff/1/tests/parsers/mactime.py#newc...
tests/parsers/mactime.py:33: # The order in which CSVParser generates events is
nondeterministic
the comment refers to that CSVParser uses a dict, hence the order is
nondeterministic, hence the usage of GetSortedEvents. If it was using a list the
events would be in-order of extraction (deterministic).

https://codereview.appspot.com/332950043/diff/1/tests/parsers/mcafeeav.py
File tests/parsers/mcafeeav.py (right):

https://codereview.appspot.com/332950043/diff/1/tests/parsers/mcafeeav.py#new...
tests/parsers/mcafeeav.py:30: events = list(storage_writer.GetEvents())
On 2017/10/19 21:42:34, onager wrote:
> GetSortedEvents

Done.

https://codereview.appspot.com/332950043/diff/1/tests/parsers/symantec.py
File tests/parsers/symantec.py (right):

https://codereview.appspot.com/332950043/diff/1/tests/parsers/symantec.py#new...
tests/parsers/symantec.py:48: events = list(storage_writer.GetEvents())
On 2017/10/19 21:42:34, onager wrote:
> GetSorted

Done.

[Joachim Metz, 2017-10-20 06:00:31]

Code updated.

[onager, 2017-10-20 16:57:30]

https://codereview.appspot.com/332950043/diff/20001/plaso/lib/line_reader_fil...
File plaso/lib/line_reader_file.py (right):

https://codereview.appspot.com/332950043/diff/20001/plaso/lib/line_reader_fil...
plaso/lib/line_reader_file.py:60: the file-like object, where None represents
all remaining data.
None doesn't represent the reading of all remaining data, a maximum of
self._MAXIMUM_READ_BUFFER_SIZE will be read, per line 82. Please fix the
docstring.

https://codereview.appspot.com/332950043/diff/20001/plaso/lib/line_reader_fil...
plaso/lib/line_reader_file.py:138: size (Optional[int]): maximum string size to
read.
Please move the description of the "size" argument to the args section from the
description above.

https://codereview.appspot.com/332950043/diff/20001/plaso/lib/line_reader_fil...
plaso/lib/line_reader_file.py:159: result, separator, lines_data =
lines_data.partition(self._end_of_line)
Given the size of the data buffer, please use splitlines here instead, to avoid
excessive memory usage. Using partition like this leads to a ~16MB memory copy
on each call.

https://codereview.appspot.com/332950043/diff/20001/plaso/lib/line_reader_fil...
plaso/lib/line_reader_file.py:166: result, separator, self._lines_buffer =
self._lines_buffer.partition(
See above regarding use of "partition".

https://codereview.appspot.com/332950043/diff/20001/plaso/lib/line_reader_fil...
plaso/lib/line_reader_file.py:185: sizehint (Optional[int]): maximum byte size
to read.
Please clarify what this argument does, as above you say "approximately" this
many bytes are read.

https://codereview.appspot.com/332950043/diff/20001/plaso/parsers/csv_parser.py
File plaso/parsers/csv_parser.py (right):

https://codereview.appspot.com/332950043/diff/20001/plaso/parsers/csv_parser....
plaso/parsers/csv_parser.py:21: class CSVParser(interface.FileObjectParser):
Regarding naming, the ever-canonical wikipedia says separated values:
"https://en.wikipedia.org/wiki/Delimiter-separated_values

[Joachim Metz, 2017-10-20 17:34:59]

https://codereview.appspot.com/332950043/diff/20001/plaso/lib/line_reader_fil...
File plaso/lib/line_reader_file.py (right):

https://codereview.appspot.com/332950043/diff/20001/plaso/lib/line_reader_fil...
plaso/lib/line_reader_file.py:138: size (Optional[int]): maximum string size to
read.
no idea what you mean. what line number?

[onager, 2017-10-20 17:36:39]

https://codereview.appspot.com/332950043/diff/20001/plaso/lib/line_reader_fil...
File plaso/lib/line_reader_file.py (right):

https://codereview.appspot.com/332950043/diff/20001/plaso/lib/line_reader_fil...
plaso/lib/line_reader_file.py:138: size (Optional[int]): maximum string size to
read.
On 2017/10/20 17:34:59, Joachim Metz wrote:
> no idea what you mean. what line number?

Lines 132-134

[Joachim Metz, 2017-10-20 17:39:03]

https://codereview.appspot.com/332950043/diff/20001/plaso/lib/line_reader_fil...
File plaso/lib/line_reader_file.py (right):

https://codereview.appspot.com/332950043/diff/20001/plaso/lib/line_reader_fil...
plaso/lib/line_reader_file.py:138: size (Optional[int]): maximum string size to
read.
I parse this correctly, you are trying to tell me is that you would like part of
the description not to be above the Args section but to be part of it.

https://codereview.appspot.com/332950043/diff/20001/plaso/parsers/csv_parser.py
File plaso/parsers/csv_parser.py (right):

https://codereview.appspot.com/332950043/diff/20001/plaso/parsers/csv_parser....
plaso/parsers/csv_parser.py:21: class CSVParser(interface.FileObjectParser):
On 2017/10/20 16:57:30, onager wrote:
> Regarding naming, the ever-canonical wikipedia says separated values:
> "https://en.wikipedia.org/wiki/Delimiter-separated_values

Renamed to dsv

[onager, 2017-10-20 17:40:13]

https://codereview.appspot.com/332950043/diff/20001/plaso/lib/line_reader_fil...
File plaso/lib/line_reader_file.py (right):

https://codereview.appspot.com/332950043/diff/20001/plaso/lib/line_reader_fil...
plaso/lib/line_reader_file.py:138: size (Optional[int]): maximum string size to
read.
On 2017/10/20 17:39:03, Joachim Metz wrote:
> I parse this correctly, you are trying to tell me is that you would like part
of
> the description not to be above the Args section but to be part of it.

Yes

[Joachim Metz, 2017-10-20 18:10:37]

https://codereview.appspot.com/332950043/diff/20001/plaso/lib/line_reader_fil...
File plaso/lib/line_reader_file.py (right):

https://codereview.appspot.com/332950043/diff/20001/plaso/lib/line_reader_fil...
plaso/lib/line_reader_file.py:60: the file-like object, where None represents
all remaining data.
On 2017/10/20 16:57:30, onager wrote:
> None doesn't represent the reading of all remaining data, a maximum of
> self._MAXIMUM_READ_BUFFER_SIZE will be read, per line 82. Please fix the
> docstring.

Done.

https://codereview.appspot.com/332950043/diff/20001/plaso/lib/line_reader_fil...
plaso/lib/line_reader_file.py:138: size (Optional[int]): maximum string size to
read.
On 2017/10/20 17:40:13, onager wrote:
> On 2017/10/20 17:39:03, Joachim Metz wrote:
> > I parse this correctly, you are trying to tell me is that you would like
part
> of
> > the description not to be above the Args section but to be part of it.
> 
> Yes

Done.

https://codereview.appspot.com/332950043/diff/20001/plaso/lib/line_reader_fil...
plaso/lib/line_reader_file.py:159: result, separator, lines_data =
lines_data.partition(self._end_of_line)
On 2017/10/20 16:57:30, onager wrote:
> Given the size of the data buffer, please use splitlines here instead, to
avoid
> excessive memory usage. Using partition like this leads to a ~16MB memory copy
> on each call.

Done.

https://codereview.appspot.com/332950043/diff/20001/plaso/lib/line_reader_fil...
plaso/lib/line_reader_file.py:166: result, separator, self._lines_buffer =
self._lines_buffer.partition(
On 2017/10/20 16:57:30, onager wrote:
> See above regarding use of "partition".

Done.

https://codereview.appspot.com/332950043/diff/20001/plaso/lib/line_reader_fil...
plaso/lib/line_reader_file.py:185: sizehint (Optional[int]): maximum byte size
to read.
On 2017/10/20 16:57:30, onager wrote:
> Please clarify what this argument does, as above you say "approximately" this
> many bytes are read.

Done.

[Joachim Metz, 2017-10-20 18:49:19]

Code updated.

[onager, 2017-10-20 19:18:44]

https://codereview.appspot.com/332950043/diff/40001/plaso/lib/line_reader_fil...
File plaso/lib/line_reader_file.py (right):

https://codereview.appspot.com/332950043/diff/40001/plaso/lib/line_reader_fil...
plaso/lib/line_reader_file.py:137: non-negative, it is a maximum byte count
(including the trailing
See https://codereview.appspot.com/331050043/ for similar comments about
indentation and use of splitlines()

https://codereview.appspot.com/332950043/diff/40001/plaso/parsers/dsv_parser.py
File plaso/parsers/dsv_parser.py (right):

https://codereview.appspot.com/332950043/diff/40001/plaso/parsers/dsv_parser....
plaso/parsers/dsv_parser.py:28: # A DSV file is delimiter separated, but this
can be overwritten to include
Please fix docstring, to read, for example "The default delimiter is a comma,
but..."

https://codereview.appspot.com/332950043/diff/40001/plaso/parsers/dsv_parser....
plaso/parsers/dsv_parser.py:31: VALUE_SEPARATOR = b','
Rename to DELIMITER

[Joachim Metz, 2017-10-21 05:23:33]

Code updated.

[Joachim Metz, 2017-10-22 04:54:57]

Code updated.

[Joachim Metz, 2017-10-23 18:00:08]

https://codereview.appspot.com/332950043/diff/40001/plaso/lib/line_reader_fil...
File plaso/lib/line_reader_file.py (right):

https://codereview.appspot.com/332950043/diff/40001/plaso/lib/line_reader_fil...
plaso/lib/line_reader_file.py:137: non-negative, it is a maximum byte count
(including the trailing
On 2017/10/20 19:18:44, onager wrote:
> See https://codereview.appspot.com/331050043/ for similar comments about
> indentation and use of splitlines()

Done.

https://codereview.appspot.com/332950043/diff/40001/plaso/parsers/dsv_parser.py
File plaso/parsers/dsv_parser.py (right):

https://codereview.appspot.com/332950043/diff/40001/plaso/parsers/dsv_parser....
plaso/parsers/dsv_parser.py:28: # A DSV file is delimiter separated, but this
can be overwritten to include
On 2017/10/20 19:18:44, onager wrote:
> Please fix docstring, to read, for example "The default delimiter is a comma,
> but..."

Done.

https://codereview.appspot.com/332950043/diff/40001/plaso/parsers/dsv_parser....
plaso/parsers/dsv_parser.py:31: VALUE_SEPARATOR = b','
On 2017/10/20 19:18:44, onager wrote:
> Rename to DELIMITER

Done.

[Joachim Metz, 2017-10-23 18:05:01]

Code updated.

[onager, 2017-10-23 18:33:34]

LGTM

[Joachim Metz, 2017-10-23 18:39:33]

Changes have been merged with master branch. To close the review and clean up
the feature branch you can run: python ./utils/review.py close dfvfsuni