Use SQLite for Logger #34
Conversation
bootstrap/lib/logger.py
Outdated
| @@ -176,7 +177,7 @@ def print_subitem(prefix, subdictionary, stack_displacement=3): | |||
| def _execute(self, statement, parameters=None, commit=True): | |||
| parameters = parameters or () | |||
| if not isinstance(parameters, tuple): | |||
| parameters = (parameters) | |||
| parameters = (parameters,) | |||
There was a problem hiding this comment.
Good catch ;)
bootstrap/lib/logger.py
Outdated
| @@ -207,39 +208,43 @@ def _list_columns(self, table_name): | |||
| table_name = self._get_internal_table_name(table_name) | |||
| query = "SELECT name FROM PRAGMA_TABLE_INFO(?)" | |||
| qry_cur = self._run_query(query, (table_name,)) | |||
| columns = [res[0] for res in qry_cur] | |||
| columns = (res[0] for res in qry_cur) | |||
There was a problem hiding this comment.
Any particular reason for this change?
There was a problem hiding this comment.
there's no need to have 2 lists, since its just an intermediate variable, a generator is just fine imo ;-)
| statement = "ALTER TABLE ? ADD ? {}".format(self._get_data_type(value_sample)) | ||
| return self._execute(statement, (table_name, column_name)) | ||
| value_type = self._get_data_type(value_sample) | ||
| statement = f'ALTER TABLE {table_name} ADD COLUMN "{column_name}" {value_type}' |
There was a problem hiding this comment.
This is not SQL safe; any reason for the change?
There was a problem hiding this comment.
yes, you can't use parameters substitution for table and columns names in sqlite, only for values
bootstrap/lib/logger.py
Outdated
| if isinstance(value, dict): | ||
| self._flatten_dict(value, flatten_dict, prefix=local_prefix) | ||
| elif not isinstance(value, (float, int)): |
There was a problem hiding this comment.
We have to be careful about double standards here: numbers.Number vs float, int
There was a problem hiding this comment.
good point, i'd prefer to have numbers.Number everywhere but I can change with (float, int) if you prefer ;-)
| table_name = self._get_internal_table_name(table_name) | ||
| column_string = ', '.join(columns) | ||
| value_placeholder = ', '.join(['?'] * len(columns)) | ||
| statement = f'INSERT INTO ?({column_string}) VALUES({value_placeholder})' | ||
| statement = f'INSERT INTO {table_name} ({column_string}) VALUES({value_placeholder})' |
There was a problem hiding this comment.
This is also not SQL safe. Is this a requirement for table names?
There was a problem hiding this comment.
yes, you can't use parameters substitution for table and columns names in sqlite, only for values
| for c in columns: | ||
| if c not in table_columns: | ||
| self.log_message(f'Unknown column "{c}"', log_level=self.ERROR) | ||
| column_string = ', '.join([f'"{c}"' for c in columns]) |
There was a problem hiding this comment.
Also not SQL safe. But I guess we'll have to accept columns and table names are passive to injection...
There was a problem hiding this comment.
yes, you can't use parameters substitution for table and columns names in sqlite, only for values
There was a problem hiding this comment.
maybe we could maintain a state with the current valid tables in the database...
Cadene
force-pushed
the
MicaelCarvalho/sqlite
branch
from
da363ec to
7782b4c
Compare
Work in progress