Skip to content
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

DB API2 support in SmartHome.py used in SQLite plugin #131

Merged
merged 153 commits into from
Nov 27, 2016

Conversation

ohinckel
Copy link
Member

@ohinckel ohinckel commented Oct 8, 2016

(copied from mknx/smarthome#69)

This pull request implements three things:

  • make it possible to use different database backends in core or plugins
  • make use of database backend accessor in SQLite plugin
  • implement new plugin dblog which logs item changes to a database and use different implementation (currently only tested with SQLite, but should also work with any MySQL DB API2 implementation)

I implemented this in one pull request, since the database plugin depends on the DB API enhancement. The documentation is also updated and can be provided in a new pull request when this is merged.

Using database backends

I thought about how to use different database implementation / backends with SmartHome.py and came to this implementation.

It adds a new configuration setting to smarthome.conf called db. This configures the database backend implementation (need to implement the DB API 2 specifications to have a common interface available) and defaults to sqlite:sqlite3, which means the SQLite API is available using the alias sqlite and the Python module sqlite3.

Since SQLite is bundled with Python directly you do not need to explicitely configure this - it's always added as default if not registered explicitely. It's also possible to register multiple database backends using |, e.g.
db = sqlite:sqlite3 | mysql:mysqldb.

Additionally to this configuration the SmartHome.py core have a new function called dbapi(alias) which returns the module for the given alias (e.g. in the case above sh.dbapi('sqlite') will return the sqlite3 module and you can do something like this:

db = sh.dbapi('sqlite')
db.connect(...)
db.execute(...)

For convenience a new class lib.db.Database was added which can be constructed by using the following code

 lib.db.Database("NameIdentifier", sh.dbapi("sqlite"), {"database" : "/path/to/sqlite.db"})

The first identifier argument is just a name which is used when something is written to the log and one want to identifier which database connection is related to a log message. Additionally to this, it is also used internally (e.g. to name the version table) - so just us characters like [a-zA-Z0-9_] and do not start with numbers.

The second DB API parameter is the module of the database backend implementation. In this example it will use the SQLite backend implementation which is registered as default.

The third connect parameter is a dict containing all connection parameters required to invoke the module's connect() function. Alternatively this can also be a simple string using a map-like syntax of parameters, e.g. host:localhost | port:1234 | ... - see also below.

Use DB API in SQLite plugin

This pull request will make use of this new feature in the SQLite plugin already to be able to use other database backends later (not part of this pull request and may be implemented in a new pull request and maybe a new plugin which could obsolete the SQLite plugin; suggestions are welcome!).

Since the SQLite module already implements the DB API2 specification it was not really hard to change it. The plugin implementation was already using the specification and the patch is very simple (just use the sh.dbapi('sqlite') - everything else seems to be working well.

The 'sqlite' backend is always automatically registered and available.

New Plugin database

The new plugin is logging item changes to the database. It was tested using a SQLite database to log changes to. It will not do any pack mechanism which is the SQLite plugin doing. It's something like a log and stores all changes to the database.

The amount of data could be large when SmartHome.py is running long time. That's why it also support other database backends (e.g. MySQL or PostgreSQL).

The configuration of this plugin is simple:

  • db - defines which database backend should be used (e.g. sqlite - which is registered as default)
  • connect - the list of parameters to pass to database backend's connect() function (e.g. for SQLite something like connect = database:/path/to/log.db | check_same_thread:0) - the parameters depends on the database implementation
[database]
  class_name = Database
  class_path = plugins.database
  db = sqlite
  connect = database:/home/service/apps/smarthome/var/db/log.db | check_same_thread:0

This plugin is already using the new lib.db.Database class to do something with the database backend.

Since the name dblog does not fit well, here are some suggestions for the plugin name:

  • dblog - could confuse users assuming it has something to do with a message log written to the database
  • history - maybe a to much generic name
  • database - conflicts with InfluxDB plugin #71 <-- this name was choosen
  • rdbms
  • itemlog
  • dbconnector
  • ... add further suggestions here!

Open issues before merge:

  • Rename plugin: dblog -> database (see suggestions above)
  • Fix logging: initialize logger in plugin's __init__() method (see also other plugins)
  • Create migration script for SQLite to database migration (somethink like @Foxi352 did)
  • Improve documentation (suggestions/feedback welcome)
  • Support dblog = init & dblog = yes: cinfigure explicitly when the plugin should initialize item values (f.e. KNX items will get their init value from the bus but will also be logged to database)

Additional notes

Further I start testing the dblog plugin with another DB API: PyMySQL. It working well with it and using this I was able to log the data into a MySQL database.

Added a new class lib.db.Database which can be used to talk to different database backend implementations using DB API2. It have the following functions:

  • __init__(): will get a identifier name, backend implementation module and connect parameter (see introduction)
  • connect(): will connect to the database
  • close(): will close the connection
  • setup(): can be used to run multiple database setup queries (e.g. create tables / indexes)
  • execute(): execute the given query with given parameters (use correct quoting style of backend module)
  • fetchone(): execute given query and return one row
  • fetchall(): execute given query and return all rows
  • cursor(): create a cursor, which can be passed to the execute() and fetch*() functions
  • lock(): create a lock
  • release(): release the lock
  • verify(): verify if the connection is still available and try to reconnect if required

It can be used to execute simple queries, query for data and return one or all of them, make use of cursors explicitely by using cursor() function and create/release locks - if required. It also checks the database backend module's parameter style in SQL queries and replaces the placeholders accordingly. The queries passed to the query function of the Database class should always use ? (qmark style)!

I just commited a solution for:

The dblog plugin tries to create tables / indexes and log errors but continue with the plugin. I think this behaviour is not very clean, but works at the moment. Maybe one of you have another good idea how to solve this (maybe trying to select data from a table to check if it exist).

The setup() method will not accept not only a list of queries to setup a database, instead it will accept a dict where the keys are just incrementing version numbers and the values are the queries to execute. The lib.db.Database class will automatically create a (internal) table called <name>_version to track the current status of the database (where name is the name given in the Database constructor). This table is used to find out which queries need to be executed to reach the newest version of the database.

F.e. using the setup() method with the following dictionary

  db.setup({
    '1' : 'CREATE TABLE item(name varchar)',
    '2' : 'CREATE INDEX item_name on test (name)',
    '3' : 'CREATE TABLE log(name varchar, date datetime, data varchar)',

    // These lines were added in a later release
    '4' : 'ALTER TABLE item add id integer autoincrement',
    '5' : 'ALTER TABLE log add item_id integer',
    '6' : 'UPDATE log set item_id=id FROM item, log where item.name= log.name'
  });

... will automatically migrate the table structure and data to the new layout (don't know if all of the queries are working, it should only show what should be possible with this). The layout was changed to introduce a new ID column (in item and log table) and the data was migrated (based on the name in log table the ID in item table will be updated).

- always support sqlite3
- provide function sh.dbapi(<name>) to retrieve the given database API module
…dbapi-support

Conflicts:
	bin/smarthome.py
# which is not part of DB API2 specification
- check DB API paramstyle and format SQL query accordingly
# with generic statements we have a warning if we try to re-create
# the table / index - there no common way to check if a table / index
# exists
# queries used will use the qmark style and will be converted accordingly
  database backends providing DB API 2 implementation
- always register SQLite with "sqlite" alias since it is used with this alias
  in plugins already
  version numbers and the value is the query to update
# the setup() method will create "version" table automatically to store the
# current version of the database and upgrade accordingly when setup() is
# called
…sing

  lock handle in close() function
- add optional timeout parameter to lock() function
…other RDBMS

- change index log_item_id to include time too
# other RDBMS have the BIGINT type, which is simply mapped in SQLite to
# INTEGER - but other RDBMS will handle it differently
# and when including the time as the second field in index speeds up selections
# which always will have a item and possibly a time
…the Database class

  for different plugins by using different names in constructor
# most other time columns are BIGINT, doing same here to keep consistency
…only create

  new cursor if no cursor was given
# represents the current status and the duration can not be calculated
- change "log" table to contain history entries and not the current status
- put current status only into "item" table
- fix duration calculation for "log" table
- when stopping the plugin dump all current values into "log" and "item"
  table
… status instead

- before dumping items check if db connectivity is present and
  try to reconnect if required
- fix current item dump in finalize dump
# same implementation like SQLite plugin does, but can not be used while
# SQLite or RRD plugin enabled due to the added items methods
- completely do not take buffered values into account, instead only use values
  stored in database
@psilo909
Copy link
Contributor

psilo909 commented Nov 21, 2016

@ohinckel @onkelandy kann es sein, dass dem dblog in der init ein
self.logger = logging.getLogger(__name__)
fehlt?

@ohinckel
Copy link
Member Author

Genau deswegen hatte ich auch Namensvorschlaege gehofft. Wenn man es konsequent machen wuerde, pass database am besten, da das SQLite auch nur sqlite heisst und nicht den Namen hat abgeleitet von dem was es macht (koennte auch Logs in eine SQLite DB schreiben). Fuer weitere Vorschlaegewaere och dankbar!

Zum Logger: das sollte bereits im SmartPlugin initialisiert sein.

@onkelandy
Copy link
Member

@Foxi352 @ohinckel Aus meiner Sicht kommt nur ein Name für das Plugin in Frage: database. Es wurde ja auch schon im anderen Thread (Issue) darüber diskutiert, dass es keinen Sinn hat, zwei Datenbank Plugins parallel zu integrieren. Da aktuell wohl eh nur sehr wenige das database Plugin nutzen, wäre ein Wechsel sicher noch in Ordnung. Die paar wenigen können ja über das Forum informiert werden, dass die Konfiguration nun anders auszusehen hat.

Ich denke, dass früher oder später auch das sqlite Plugin aus dem Programm genommen werden bzw. in den Hintergrund rücken sollte. Also es ist zwar integriert aus Kompatibilitätsgründen, aber beim Start und in der Doku wird darauf hingewiesen, dass es nicht mehr entwickelt ist und auf database umgestellt werden soll. Denn sonst kommt ein Neuling einfach überhaupt nicht mit, warum es mehrere Optionen für das Gleiche gibt und was nun die Unterschiede sind.

@psilo909 hatte übrigens Recht! Mit dem self.logger Eintrag im Init funktioniert nun das Logging auch so wie es soll! Kannst das bitte noch updaten? Wie gesagt wäre auch noch die init bzw. yes Funktion super ;)

@psilo909
Copy link
Contributor

ich habe schon an dbconnector oder sowas gedacht, optimal ist das aber auch nicht. kreativität braucht zeit, muss ja nicht bis morgen definiert sein

@psilo909
Copy link
Contributor

ich würde den PR fast reinmergen. was denkt ihr? optimieren kann man ja immer noch. speziell die doku. es wäre evtl noch cool, ein migrationstool zu mysql zu haben. @Foxi352 hatte das im alten Database plugin gemacht. sonst werden viele nicht wechseln denke ich

von @cstrassburg - der kurzzeitig wieder aufgetaucht ist - habe ich leider keine aussage bekommen, ob er größere PRs im kern von sh nochmal reviewen möchte. der antwort von ihm entnehme ich aber, dass es in ordnung geht. wenn nicht, bitte ich hiermit im öffentlichen raum nochmals um mehr präsenz im projekt...

@Foxi352
Copy link
Member

Foxi352 commented Nov 26, 2016

Ich wurde es in database umbenennen, das orm plugin löschen und dann das ganze mergen. Es ist eine devel version. Ausser den dev's hier sollten eigentlich nicht viele Endnutzer die devel nutzen, und schon ganz nicht ein plugin welches noch nicht offiziell fertig ist. Wer das trotzdem gemacht hat muss halt damit rechnen dass nach einem pull das System umkonfiguriert werden muss. Ich denke das nimmt matn automatisch mit in Kauf wenn man ein bleeding-edge nutzt ... IMHO...

@psilo909
Copy link
Contributor

@Foxi352 hast du auch schon mal reingeschaut?

@Foxi352
Copy link
Member

Foxi352 commented Nov 26, 2016

Noch nicht, hatte ein paar stressige Wochen. Bin eine neue Website am aufbauen. Wollte aber dieses Wochenende umschalten zum testen. Ist natürlich einfacher wenn es schon im devel gemerged ist :-)

@psilo909
Copy link
Contributor

ich nehme mal das alte database plugin raus. die umbenennung könnte dann @ohinckel machen

@onkelandy
Copy link
Member

Klingt gut.. schön wäre noch, wenn die Logger-Problematik gelöst werden könnte. Ist ja nur eine Zeile.

@psilo909
Copy link
Contributor

psilo909 commented Nov 26, 2016

@onkelandy stimmt.. :) sorry ich war auch einige tage im "halbschatten" wegen ner krankheit aus dem kindergarten... ich teste das mit dem logging auch nochmal. im smartplugin ist die zeile wirklich, ich vermute nur, dass das trotzdem nicht den gewuenschten effekt hat
da würden wir mit @cstrassburg weniger im trüben fischen ;)

@ohinckel
Copy link
Member Author

ohinckel commented Nov 26, 2016

Danke fuer euer Feedback! Ich habe die offenen Punkte in die Beschreibung aufgenommen.

@psilo909
Copy link
Contributor

@ohinckel warte noch wegen doku, da bin ich dann gerne dabei. habe mich ja auch durchgekämpft und bin dir SEHR dankbar für den beitrag zu shNG.. nicht dass das zu kurz kommt!!!

@psilo909
Copy link
Contributor

psilo909 commented Nov 26, 2016

ich hoffe auch, dass @msinn und @bmxp dann mal testweise ihre sqlite anbindung auf das neue plugin umstellen. sqlite haben wir damit bisher leider kaum getestet
vor einem release sollte der beta test doch mehr als 3 leute umfassen :)

@bmxp
Copy link
Member

bmxp commented Nov 27, 2016

Ich habe in den nächsten Wochen bis Weihnachten kaum Zeit aber wenn ihr kurz einen Pointer gebt auf das nötige Vorgehen, dann werde ich gerne versuchen das einzuschieben und mit SQLite zu testen...

@psilo909
Copy link
Contributor

psilo909 commented Nov 27, 2016

@ohinckel ich lasse nochmal 1-2h laufen und würde den PR dann mergen, auch wenn es sicher noch finetuning bedarf gibt.

ps: leider hatte ich den effekt, dass nach der umstellung auf den neuen namen ein "table log already exists" kam. habe jetzt nochmal einen datenreset durchgefuehrt

@onkelandy
Copy link
Member

onkelandy commented Nov 27, 2016

Habe jetzt nochmals frisch alles gezogen, aber die Tabellen leben lassen. Musste in der Datenbank dblog_version in database_version umbenennen. Ansonsten scheint es nach Umbennen aller Einträge auf database gut zu klappen.

Beim Readme wäre die Frage, ob man nicht besser auf das SmarthomeNG Wiki verweisen sollte statt mknx

@psilo909
Copy link
Contributor

@onkelandy ups das habe ich zu spät gelesen.. nunja bei mir also ein clean start.. dafür kann ich wieder nach zickzackkurven suchen jetzt

@psilo909 psilo909 merged commit 56cd106 into smarthomeNG:develop Nov 27, 2016
cstrassburg pushed a commit to smarthomeNG/plugins that referenced this pull request Dec 10, 2016
cstrassburg pushed a commit that referenced this pull request Dec 10, 2016
cstrassburg pushed a commit that referenced this pull request Dec 10, 2016
DB API2 support in SmartHome.py used in SQLite plugin

Hiermit gemerged um den PR einer breiteren Masse zum Testen zu geben.
@ohinckel ohinckel deleted the dev-dbapi-support branch December 23, 2016 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants