[plaso] Merged JSON serializers into single attribute container serializer #633

[plaso] Merged JSON serializers into single attribute container serializer #633

[onager, 2016-04-28 12:09:46]

Initial pass. I want to think a little more about the overall design here too...

https://codereview.appspot.com/299840043/diff/1/plaso/containers/interface.py
File plaso/containers/interface.py (right):

https://codereview.appspot.com/299840043/diff/1/plaso/containers/interface.py...
plaso/containers/interface.py:11: CONTAINER_TYPE = None
What does this constant contain? What does it mean?

https://codereview.appspot.com/299840043/diff/1/plaso/containers/reports.py
File plaso/containers/reports.py (right):

https://codereview.appspot.com/299840043/diff/1/plaso/containers/reports.py#n...
plaso/containers/reports.py:22: CONTAINER_TYPE = u'report'
What if we have other things that are reports, that aren't analysis reports?

https://codereview.appspot.com/299840043/diff/1/plaso/containers/reports.py#n...
plaso/containers/reports.py:28: plugin_name: optional string containing the name
of the analysis plugin
Why is this optional now?

https://codereview.appspot.com/299840043/diff/1/plaso/serializer/json_seriali...
File plaso/serializer/json_serializer.py (right):

https://codereview.appspot.com/299840043/diff/1/plaso/serializer/json_seriali...
plaso/serializer/json_serializer.py:285: u'event': events.EventObject,
Why is this needed, given the CONTAINER_TYPE constant exists in the containers
now?

https://codereview.appspot.com/299840043/diff/1/plaso/serializer/json_seriali...
plaso/serializer/json_serializer.py:322: raise ValueError(u'Unuspported
attribute container type: {0:s}.'.format(
Unsupported. Also, this error message seems overly broad - the issue is that the
CONTAINER_TYPE is not present.

https://codereview.appspot.com/299840043/diff/1/plaso/serializer/json_seriali...
plaso/serializer/json_serializer.py:383: The dictionary of the JSON serialized
objects consists of:
of a JSON serialized object?

https://codereview.appspot.com/299840043/diff/1/plaso/serializer/json_seriali...
plaso/serializer/json_serializer.py:444: container_class =
cls._CONTAINER_CLASS_PER_TYPE.get(container_type, None)
This should be done with a manager like for everything else.

https://codereview.appspot.com/299840043/diff/1/plaso/serializer/json_seriali...
plaso/serializer/json_serializer.py:605: def WriteSerialized(cls,
attribute_container):
This doesn't write anything, it just creates a thing.

[Joachim Metz, 2016-04-29 04:49:00]

https://codereview.appspot.com/299840043/diff/1/plaso/containers/interface.py
File plaso/containers/interface.py (right):

https://codereview.appspot.com/299840043/diff/1/plaso/containers/interface.py...
plaso/containers/interface.py:11: CONTAINER_TYPE = None
Added some explanation to the doc string.

https://codereview.appspot.com/299840043/diff/1/plaso/containers/reports.py
File plaso/containers/reports.py (right):

https://codereview.appspot.com/299840043/diff/1/plaso/containers/reports.py#n...
plaso/containers/reports.py:22: CONTAINER_TYPE = u'report'
I'll change this to "analysis_report"

https://codereview.appspot.com/299840043/diff/1/plaso/containers/reports.py#n...
plaso/containers/reports.py:28: plugin_name: optional string containing the name
of the analysis plugin
Mainly to make serialization the same for all attribute containers.

https://codereview.appspot.com/299840043/diff/1/plaso/serializer/json_seriali...
File plaso/serializer/json_serializer.py (right):

https://codereview.appspot.com/299840043/diff/1/plaso/serializer/json_seriali...
plaso/serializer/json_serializer.py:285: u'event': events.EventObject,
There is a need to map the type to the class, we could add registration of the
attribute containers similar to the plugins to do this for us. Seeing that we
only have 3 for now, I opted to just hard code it.

https://codereview.appspot.com/299840043/diff/1/plaso/serializer/json_seriali...
plaso/serializer/json_serializer.py:322: raise ValueError(u'Unuspported
attribute container type: {0:s}.'.format(
A missing container type would mean the attribute container is not well formed
and therefore unsupported. What do you propose?

https://codereview.appspot.com/299840043/diff/1/plaso/serializer/json_seriali...
plaso/serializer/json_serializer.py:383: The dictionary of the JSON serialized
objects consists of:
The dict can contain more that 1 object.

https://codereview.appspot.com/299840043/diff/1/plaso/serializer/json_seriali...
plaso/serializer/json_serializer.py:444: container_class =
cls._CONTAINER_CLASS_PER_TYPE.get(container_type, None)
Maybe, for now I thought it would be overkill.

https://codereview.appspot.com/299840043/diff/1/plaso/serializer/json_seriali...
plaso/serializer/json_serializer.py:605: def WriteSerialized(cls,
attribute_container):
On 2016/04/28 12:09:46, onager wrote:
> This doesn't write anything, it just creates a thing.

That is what the current interface is. What do you propose?

[Joachim Metz, 2016-04-29 04:51:47]

Code updated.

[Joachim Metz, 2016-04-30 10:30:28]

Code updated.

[Joachim Metz, 2016-05-05 06:24:12]

Code updated.

[Joachim Metz, 2016-05-05 06:27:01]

Code updated.

[Joachim Metz, 2016-05-08 06:26:00]

Code updated.

[onager, 2016-05-11 05:20:16]

Docstring nits, LGTM

https://codereview.appspot.com/299840043/diff/100001/plaso/serializer/json_se...
File plaso/serializer/json_serializer.py (right):

https://codereview.appspot.com/299840043/diff/100001/plaso/serializer/json_se...
plaso/serializer/json_serializer.py:303: make up the preprocessing object
attributes.
preprocessing?

https://codereview.appspot.com/299840043/diff/100001/plaso/serializer/json_se...
plaso/serializer/json_serializer.py:310: A dictionary of the JSON serialized
objects.
"A JSON dictionary representing the AttributeContainer"?

https://codereview.appspot.com/299840043/diff/100001/plaso/serializer/json_se...
plaso/serializer/json_serializer.py:383: The dictionary of the JSON serialized
objects consists of:
"The JSON serialized dictionary must consist of..."

https://codereview.appspot.com/299840043/diff/100001/plaso/serializer/json_se...
plaso/serializer/json_serializer.py:398: json_dict: a dictionary of the JSON
serialized objects.
In general, I'm not sure what "the JSON serialized objects" means.

[onager, 2016-05-11 05:20:17]

Docstring nits, LGTM

https://codereview.appspot.com/299840043/diff/100001/plaso/serializer/json_se...
File plaso/serializer/json_serializer.py (right):

https://codereview.appspot.com/299840043/diff/100001/plaso/serializer/json_se...
plaso/serializer/json_serializer.py:303: make up the preprocessing object
attributes.
preprocessing?

https://codereview.appspot.com/299840043/diff/100001/plaso/serializer/json_se...
plaso/serializer/json_serializer.py:310: A dictionary of the JSON serialized
objects.
"A JSON dictionary representing the AttributeContainer"?

https://codereview.appspot.com/299840043/diff/100001/plaso/serializer/json_se...
plaso/serializer/json_serializer.py:383: The dictionary of the JSON serialized
objects consists of:
"The JSON serialized dictionary must consist of..."

https://codereview.appspot.com/299840043/diff/100001/plaso/serializer/json_se...
plaso/serializer/json_serializer.py:398: json_dict: a dictionary of the JSON
serialized objects.
In general, I'm not sure what "the JSON serialized objects" means.

[Joachim Metz, 2016-05-11 06:38:39]

https://codereview.appspot.com/299840043/diff/100001/plaso/serializer/json_se...
File plaso/serializer/json_serializer.py (right):

https://codereview.appspot.com/299840043/diff/100001/plaso/serializer/json_se...
plaso/serializer/json_serializer.py:303: make up the preprocessing object
attributes.
On 2016/05/11 05:20:16, onager wrote:
> preprocessing?

Done.

https://codereview.appspot.com/299840043/diff/100001/plaso/serializer/json_se...
plaso/serializer/json_serializer.py:310: A dictionary of the JSON serialized
objects.
JSON dictionary would imply to me a string containing a dict represented in JSON

this is a dict containing JSON intermediate (not strings) serialized objects.

https://codereview.appspot.com/299840043/diff/100001/plaso/serializer/json_se...
plaso/serializer/json_serializer.py:383: The dictionary of the JSON serialized
objects consists of:
Alters the meaning.

https://codereview.appspot.com/299840043/diff/100001/plaso/serializer/json_se...
plaso/serializer/json_serializer.py:398: json_dict: a dictionary of the JSON
serialized objects.
These are intermediate object representations of the serialized objects.

[Joachim Metz, 2016-05-11 06:41:11]

Let's go over this wording some later point in time I've added a note to
https://github.com/log2timeline/plaso/issues/338

[Joachim Metz, 2016-05-11 06:43:24]

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 phased1