[plaso] Added database schema matching to sqlite plugins #1093

[plaso] Added database schema matching to sqlite plugins #1093

[Joachim Metz, 2017-04-08 18:45:40]

for review this WIP to migrate https://codereview.appspot.com/319750043/

[Joachim Metz, 2017-04-08 18:52:44]

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

https://codereview.appspot.com/316460043/diff/1/plaso/parsers/mediator.py#new...
plaso/parsers/mediator.py:501: def RemoveEventAttribute(self, attribute_name):
note to self, what is the advantage of having this method?

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

https://codereview.appspot.com/316460043/diff/1/plaso/parsers/sqlite.py#newco...
plaso/parsers/sqlite.py:106: self.schema = {}
alpha ordering, why is this public?

https://codereview.appspot.com/316460043/diff/1/plaso/parsers/sqlite_plugins/...
File plaso/parsers/sqlite_plugins/android_calls.py (right):

https://codereview.appspot.com/316460043/diff/1/plaso/parsers/sqlite_plugins/...
plaso/parsers/sqlite_plugins/android_calls.py:43: REQUIRED_TABLES =
frozenset([u'calls'])
Is this still needed?

https://codereview.appspot.com/316460043/diff/1/plaso/parsers/sqlite_plugins/...
plaso/parsers/sqlite_plugins/android_calls.py:50: SCHEMAS = [
Move to data files?

https://codereview.appspot.com/316460043/diff/1/plaso/parsers/sqlite_plugins/...
File plaso/parsers/sqlite_plugins/interface.py (right):

https://codereview.appspot.com/316460043/diff/1/plaso/parsers/sqlite_plugins/...
plaso/parsers/sqlite_plugins/interface.py:99: for row in sql_results:
move repetitive code into a separate method

[Joachim Metz, 2017-04-09 12:54:47]

Code updated.

[Joachim Metz, 2017-04-09 12:59:04]

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

https://codereview.appspot.com/316460043/diff/1/plaso/parsers/mediator.py#new...
plaso/parsers/mediator.py:501: def RemoveEventAttribute(self, attribute_name):
Added reminder to https://github.com/log2timeline/plaso/issues/771 to see if
this interface can be deprecated.

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

https://codereview.appspot.com/316460043/diff/1/plaso/parsers/sqlite.py#newco...
plaso/parsers/sqlite.py:106: self.schema = {}
On 2017/04/08 18:52:44, Joachim Metz wrote:
> alpha ordering, why is this public?

Done.

https://codereview.appspot.com/316460043/diff/1/plaso/parsers/sqlite_plugins/...
File plaso/parsers/sqlite_plugins/android_calls.py (right):

https://codereview.appspot.com/316460043/diff/1/plaso/parsers/sqlite_plugins/...
plaso/parsers/sqlite_plugins/android_calls.py:43: REQUIRED_TABLES =
frozenset([u'calls'])
still used in sqlite.py parser

https://codereview.appspot.com/316460043/diff/1/plaso/parsers/sqlite_plugins/...
plaso/parsers/sqlite_plugins/android_calls.py:50: SCHEMAS = [
leave as-is for now.

https://codereview.appspot.com/316460043/diff/1/plaso/parsers/sqlite_plugins/...
File plaso/parsers/sqlite_plugins/interface.py (right):

https://codereview.appspot.com/316460043/diff/1/plaso/parsers/sqlite_plugins/...
plaso/parsers/sqlite_plugins/interface.py:99: for row in sql_results:
On 2017/04/08 18:52:44, Joachim Metz wrote:
> move repetitive code into a separate method

Done.

[dc3.plaso, 2017-04-10 17:32:55]

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

https://codereview.appspot.com/316460043/diff/1/plaso/parsers/sqlite.py#newco...
plaso/parsers/sqlite.py:106: self.schema = {}
On 2017/04/08 18:52:44, Joachim Metz wrote:
> alpha ordering, why is this public?

The schema gets accessed by the SQLitePlugin class and it's not good practice to
hide it behind a getter/setter.
(https://docs.quantifiedcode.com/python-anti-patterns/correctness/implementing...)
 

Therefore, it should be public.

https://codereview.appspot.com/316460043/diff/1/plaso/parsers/sqlite_plugins/...
File plaso/parsers/sqlite_plugins/android_calls.py (right):

https://codereview.appspot.com/316460043/diff/1/plaso/parsers/sqlite_plugins/...
plaso/parsers/sqlite_plugins/android_calls.py:43: REQUIRED_TABLES =
frozenset([u'calls'])
On 2017/04/08 18:52:44, Joachim Metz wrote:
> Is this still needed?

Yes, it's needed for determining correct plugins for a given database.

https://codereview.appspot.com/316460043/diff/20001/plaso/parsers/sqlite_plug...
File plaso/parsers/sqlite_plugins/interface.py (right):

https://codereview.appspot.com/316460043/diff/20001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/interface.py:234: schema defined in the SCHEMA
class constant.
We should still be matching plugins by REQUIRED_TABLES. We wouldn't want a
sqlite plugin not to run just because the database schema has only slightly
changed.

https://codereview.appspot.com/316460043/diff/20001/utils/schema_extractor.py
File utils/schema_extractor.py (right):

https://codereview.appspot.com/316460043/diff/20001/utils/schema_extractor.py...
utils/schema_extractor.py:22: """SQLite database file schema extractor."""
Why are these in a class? It seems unnecessary for two simple functions.

[Joachim Metz, 2017-04-10 18:57:23]

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

https://codereview.appspot.com/316460043/diff/1/plaso/parsers/sqlite.py#newco...
plaso/parsers/sqlite.py:106: self.schema = {}
Thx, agree Python does not need setters/getters (unless in the C bindings ;)

I did not realize schema was used elsewhere

https://codereview.appspot.com/316460043/diff/1/plaso/parsers/sqlite_plugins/...
File plaso/parsers/sqlite_plugins/android_calls.py (right):

https://codereview.appspot.com/316460043/diff/1/plaso/parsers/sqlite_plugins/...
plaso/parsers/sqlite_plugins/android_calls.py:43: REQUIRED_TABLES =
frozenset([u'calls'])
thx for confirming

https://codereview.appspot.com/316460043/diff/20001/plaso/parsers/sqlite_plug...
File plaso/parsers/sqlite_plugins/interface.py (right):

https://codereview.appspot.com/316460043/diff/20001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/interface.py:234: schema defined in the SCHEMA
class constant.
good catch, I realize this later and should undo this docstring change.

https://codereview.appspot.com/316460043/diff/20001/utils/schema_extractor.py
File utils/schema_extractor.py (right):

https://codereview.appspot.com/316460043/diff/20001/utils/schema_extractor.py...
utils/schema_extractor.py:22: """SQLite database file schema extractor."""
Is more of a grouping for potential reuse and for future tests.

[Joachim Metz, 2017-04-11 04:56:53]

Code updated.

[onager, 2017-04-12 19:42:02]

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/mediator.py
File plaso/parsers/mediator.py (left):

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/mediator.py...
plaso/parsers/mediator.py:207: KeyError: If an attribute with the given name is
already set.
Why is this being removed? Now we silently swallow any duplicate calls to this
method, which is going to make it harder to debug.

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

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/mediator.py...
plaso/parsers/mediator.py:503: 
This should raise if the attribute isn't set - the caller should catch the
exception if they don't know if the attribute is set or not.

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

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite.py#n...
plaso/parsers/sqlite.py:88: schema (dict[str, str]): schema as an SQL query per
table name.
I don't understand what this means, please rewrite or add an example.

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite.py#n...
plaso/parsers/sqlite.py:119: return self.schema.keys()
Raise exception if the database isn't open.

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
File plaso/parsers/sqlite_plugins/android_calls.py (right):

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/android_calls.py:51: {u'_sync_state':
This very hard to read - please clean up. Perhaps indent the queries?

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/android_calls.py:102: u'last_time_used INTERGER NOT
NULL DEFAULT 0, FOREIGN KEY(data_id) '
INTERGER?

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
File plaso/parsers/sqlite_plugins/interface.py (right):

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/interface.py:56: """Parses the results of a query
on a database.
This doesn't just parse the results of a query, it executes teh query on the
database. Please fix the docstring here.

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/interface.py:66: schema_match = any(schema ==
database.schema for schema in self.SCHEMAS)
Why is this being tested on every query? Surely we only want to check if the
schema matches once for each plugin, in Process() or GetEntries()?

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/interface.py:86: u'unable to parser query: {0:s}
result on database with error: '
"unable to parse results of query"

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/interface.py:106: cache (Optional[SQLiteCache]):
cache.
Given that this is being called in a try: above, I assume this raises
sqlite3.DatabaseError under some conditions. Please add to docstring.

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/interface.py:122: 
+rows already present in the row cache will be skipped.

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/interface.py:147: """Parses the results of a query
on a database with WAL.
+rows already present in the row cache will be skipped.

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/interface.py:147: """Parses the results of a query
on a database with WAL.
This actually executes the query, not just parsing the results. Please fix
docstring.

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/interface.py:156: wal_file_entry
(Optional[dfvfs.FileEntry]): file entry for the database
The snippet below in setfilentry() seems to suggest this is actually the file
entry for the WAL file, not the database.

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/interface.py:159: wal_schema_match = any(
Again, shouldn't this be done once per database-with-WAL?

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/interface.py:183: u'unable to parser query: {0:s}
result on database and WAL '
"unable to parse results of query"

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/interface.py:194: """Extracts event from a SQLite
database.
events

[Joachim Metz, 2017-04-19 19:18:25]

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/mediator.py
File plaso/parsers/mediator.py (left):

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/mediator.py...
plaso/parsers/mediator.py:207: KeyError: If an attribute with the given name is
already set.
Ack, I'm fine either way, I think we should remove this function and use event
data for this. Added a reminder to
https://github.com/log2timeline/plaso/issues/771

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

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/mediator.py...
plaso/parsers/mediator.py:503: 
On 2017/04/12 19:42:01, onager wrote:
> This should raise if the attribute isn't set - the caller should catch the
> exception if they don't know if the attribute is set or not.

Done.

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

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite.py#n...
plaso/parsers/sqlite.py:88: schema (dict[str, str]): schema as an SQL query per
table name.
On 2017/04/12 19:42:02, onager wrote:
> I don't understand what this means, please rewrite or add an example. 

Done.

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite.py#n...
plaso/parsers/sqlite.py:119: return self.schema.keys()
what about returning none instead?

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
File plaso/parsers/sqlite_plugins/android_calls.py (right):

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/android_calls.py:51: {u'_sync_state':
built a text formatter for this, see if you agree and I'll apply it to the rest
of the SCHEMAS definitions.

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/android_calls.py:102: u'last_time_used INTERGER NOT
NULL DEFAULT 0, FOREIGN KEY(data_id) '
On 2017/04/12 19:42:02, onager wrote:
> INTERGER?

I'll double check but likely.

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
File plaso/parsers/sqlite_plugins/interface.py (right):

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/interface.py:56: """Parses the results of a query
on a database.
On 2017/04/12 19:42:02, onager wrote:
> This doesn't just parse the results of a query, it executes teh query on the
> database. Please fix the docstring here.

Done.

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/interface.py:66: schema_match = any(schema ==
database.schema for schema in self.SCHEMAS)
good catch

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/interface.py:86: u'unable to parser query: {0:s}
result on database with error: '
On 2017/04/12 19:42:02, onager wrote:
> "unable to parse results of query"

Done.

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/interface.py:106: cache (Optional[SQLiteCache]):
cache.
On 2017/04/12 19:42:02, onager wrote:
> Given that this is being called in a try: above, I assume this raises
> sqlite3.DatabaseError under some conditions. Please add to docstring.

Done.

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/interface.py:122: 
On 2017/04/12 19:42:02, onager wrote:
> +rows already present in the row cache will be skipped.

Done.

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/interface.py:147: """Parses the results of a query
on a database with WAL.
On 2017/04/12 19:42:02, onager wrote:
> +rows already present in the row cache will be skipped.

Done.

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/interface.py:147: """Parses the results of a query
on a database with WAL.
On 2017/04/12 19:42:02, onager wrote:
> This actually executes the query, not just parsing the results. Please fix
> docstring.

Done.

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/interface.py:156: wal_file_entry
(Optional[dfvfs.FileEntry]): file entry for the database
changed

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/interface.py:159: wal_schema_match = any(
On 2017/04/12 19:42:02, onager wrote:
> Again, shouldn't this be done once per database-with-WAL?

Done.

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/interface.py:183: u'unable to parser query: {0:s}
result on database and WAL '
On 2017/04/12 19:42:02, onager wrote:
> "unable to parse results of query"

Done.

https://codereview.appspot.com/316460043/diff/40001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/interface.py:194: """Extracts event from a SQLite
database.
On 2017/04/12 19:42:02, onager wrote:
> events

Done.

[Joachim Metz, 2017-04-19 20:52:41]

Code updated.

[Joachim Metz, 2017-04-19 21:16:29]

Code updated.

[Joachim Metz, 2017-04-20 05:36:25]

https://codereview.appspot.com/316460043/diff/80001/utils/schema_extractor.py
File utils/schema_extractor.py (right):

https://codereview.appspot.com/316460043/diff/80001/utils/schema_extractor.py...
utils/schema_extractor.py:121: #pyperclip.copy(database_schema)
fix this in final touch up

[onager, 2017-04-24 06:36:25]

https://codereview.appspot.com/316460043/diff/80001/plaso/parsers/mediator.py
File plaso/parsers/mediator.py (right):

https://codereview.appspot.com/316460043/diff/80001/plaso/parsers/mediator.py...
plaso/parsers/mediator.py:207: KeyError: if the event attribute with is already
set.
-with

https://codereview.appspot.com/316460043/diff/80001/plaso/parsers/mediator.py...
plaso/parsers/mediator.py:515: KeyError: if the event attribute with is not set.
-with

https://codereview.appspot.com/316460043/diff/80001/plaso/parsers/sqlite_plug...
File plaso/parsers/sqlite_plugins/android_calls.py (right):

https://codereview.appspot.com/316460043/diff/80001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/android_calls.py:51: u'_sync_state': (
This is much more readable - please go ahead and apply it to the other parsers.

[Joachim Metz, 2017-05-07 14:27:11]

https://codereview.appspot.com/316460043/diff/80001/plaso/parsers/mediator.py
File plaso/parsers/mediator.py (right):

https://codereview.appspot.com/316460043/diff/80001/plaso/parsers/mediator.py...
plaso/parsers/mediator.py:207: KeyError: if the event attribute with is already
set.
On 2017/04/24 06:36:24, onager wrote:
> -with

Done.

https://codereview.appspot.com/316460043/diff/80001/plaso/parsers/mediator.py...
plaso/parsers/mediator.py:515: KeyError: if the event attribute with is not set.
On 2017/04/24 06:36:24, onager wrote:
> -with

Done.

https://codereview.appspot.com/316460043/diff/80001/plaso/parsers/sqlite_plug...
File plaso/parsers/sqlite_plugins/android_calls.py (right):

https://codereview.appspot.com/316460043/diff/80001/plaso/parsers/sqlite_plug...
plaso/parsers/sqlite_plugins/android_calls.py:51: u'_sync_state': (
On 2017/04/24 06:36:24, onager wrote:
> This is much more readable - please go ahead and apply it to the other
parsers.

Done.

https://codereview.appspot.com/316460043/diff/80001/utils/schema_extractor.py
File utils/schema_extractor.py (right):

https://codereview.appspot.com/316460043/diff/80001/utils/schema_extractor.py...
utils/schema_extractor.py:121: #pyperclip.copy(database_schema)
On 2017/04/20 05:36:25, Joachim Metz wrote:
> fix this in final touch up

Done.

[Joachim Metz, 2017-05-07 14:46:26]

Code updated.

[onager, 2017-05-09 02:55:27]

LGTM

[Joachim Metz, 2017-05-09 05:13:01]

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 sqlschema

[dc3.plaso, 2017-05-09 18:52:44]

https://codereview.appspot.com/316460043/diff/100001/utils/schema_extractor.py
File utils/schema_extractor.py (right):

https://codereview.appspot.com/316460043/diff/100001/utils/schema_extractor.p...
utils/schema_extractor.py:74: for table_name, query in schema.items():
The original Jinja2 template was using dictsort to keep tables in alphabetical
order. I suggest adding sorted() function.

Also, I suggest using the enumerate() function for the index:

---
for index, (table_name, query) in enumerate(sorted(schema.items())):
  # stuff
---

[Joachim Metz, 2017-05-28 12:02:46]

Added reminder to: https://github.com/log2timeline/plaso/issues

https://codereview.appspot.com/316460043/diff/100001/utils/schema_extractor.py
File utils/schema_extractor.py (right):

https://codereview.appspot.com/316460043/diff/100001/utils/schema_extractor.p...
utils/schema_extractor.py:74: for table_name, query in schema.items():
On 2017/05/09 18:52:44, dc3.plaso wrote:
> The original Jinja2 template was using dictsort to keep tables in alphabetical
> order. I suggest adding sorted() function.
> 
> Also, I suggest using the enumerate() function for the index:
> 
> ---
> for index, (table_name, query) in enumerate(sorted(schema.items())):
>   # stuff
> ---

Acknowledged.

[Joachim Metz, 2017-05-28 12:03:06]

meant: https://github.com/log2timeline/plaso/issues/1093