Programmabsturz trotz Exception handling

BlackJack · Dienstag 30. Dezember 2014, 17:43

@hwm: Wieso wäre das Overkill? Es vereinfacht den Umgang mit verschiedenen DBMS. Man kann keine syntaktischen Fehler beim SQL mehr machen oder zumindest nicht mehr so einfach. Man hat keine Probleme mit SQL-Injections mehr, wie Dein Code momentan. Die Probleme die Du mit den ``SELECT *`` verursachst gibt es nicht mehr. Man kann bei DBMS die es unterstützen ”reflection” verwenden, braucht also keine Tabellenobjekte definieren. Andererseits *kann* man das machen, und sogar ORM-Klassen. Bei SQLAlchemy werden auch mehr Datentypen automatisch umgewandelt als beim SQLite-Modul. Zum Beispiel weiss das Modul aus der Standardbibliothek das man `datetime.datetime` auf ``TIMESTAMP`` abbilden kann, aber bei ``DATETIME`` wird das unverständlicherweise nicht gemacht.

Und wenn man aus den '?' '%s' macht dann funktioniert es ja nicht mehr mit SQLite.

hwm · Dienstag 30. Dezember 2014, 19:05

Overkill aus folgenden Gründen:
1. zu hoher Einarbeitungsaufwand im Verhältnis zu solch einem Miniprogramm.
2. Zur SQL Injections Problematik: klar, das ist schon ein Argument, aber doch nicht in einer kleinen Privatanwendung. Hier bestünde natürlich das Problem, dass vom Webserver, von dem ich die Daten abhole, "verseuchte" Inhalte kämen, die ich ungeprüft in meine lokale DB überführe. Sollte das der Fall sein, hätte ich (und der Admin des Webservers) ein anderes und größeres Problem. Trotzdem habe ich das Coding geändert, auch weil es einfacher und eleganter ist.
3. SELECT * sollte man sowieso vermeiden wenn es irgendwie geht, bei den beiden in meinem Coding ist es aber so, dass ich tatsächlich jeweils den kompletten Datenbanksatz benötige (wobei der eine ohnehin nur eine Spalte hat).
Und 4. (hat nichts mit Overkill zu tun): ich hätte Angst, dass meine SQL Kenntnisse schwinden.

SQLAlchemy werde ich mir aber auf jeden Fall anschauen.

Und jetzt habe ich mir auch noch eine passende GUI zur Visualisierung der Messwerte mit PyQT gebastelt:

http://postimg.org/image/hw2eazh59/

BlackJack · Dienstag 30. Dezember 2014, 20:36

Ad 1.: Zu hoher Einarbeitungsaufwand? Da arbeitet man einmal ein Tutorial durch, wovon man dann bei jedem folgenden Programm etwas hat das eine relationale Datenbank verwendet. Und Programme haben auch die Tendenz mit der Zeit zu wachsen. Was heute als Miniprogramm anfängt, kann in zwei Jahren eine ausgewachsene Anwendung sein.

Ad 2.: SQL-Injektion ist auch bei Programmen ein Problem die nie angegriffen werden. Denn das ist ein Problem mit der geistigen Haltung des Programmierers. Die Lösung ist total einfach, da kann man nicht einmal von Mehraufwand sprechen. Wer das Problem kennt und *trotzdem* selber die Werte ins SQL formatiert sollte nicht programmieren dürfen.

Ad 3.: Noch hat die Tabelle nur eine Spalte. Gerade wenn sich das ändern sollte rächen sich solche '*'. Wobei ich mich gerade Frage wozu man die Tabelle mit *einer* Spalte benötigt? Wenn man die IDs (?) der vorhandenen Geräte wissen möchte kann man ja auch die `daten`-Tabelle abfragen. Andererseits sollte die `geraete`-Tabelle vielleicht nicht nur eine Spalte enthalten: Verwendest Du etwa eine Zeichenkette Geräte-Bezeichnung und speicherst die redundant in mehr als einer Tabelle? Und ich frage mich gerade was der Primärschlüssel von der `daten`-Tabelle ist?

Ad 4.: Wenn man nur die SQL-Abstraktion benutzt, dann ist relativ eindeutig wie das SQL aussehen wird, und man kann sich das ja auch jederzeit ausgeben lassen. Beim ORM auch, und da muss man das spätestens beim optimieren mal in das SQL schauen was da generiert wird.

snafu · Dienstag 30. Dezember 2014, 22:02

hwm hat geschrieben:Und 4. (hat nichts mit Overkill zu tun): ich hätte Angst, dass meine SQL Kenntnisse schwinden.

Um hier noch BlackJack zu ergänzen: Bei der SQL-Abstraktion werden dieselben Bezeichnungen verwendet wie im SQL, was ja auch Sinn macht, damit man sich schnell dort zurechtfindet. Es ist halt nur objektorientiert, d.h. du übermittelst die Befehle via Python-Syntax, anstatt einen reinen String für das SQL-Statement zu nutzen. Dein Einwand deutet IMHO stark darauf hin, dass du dir diesen Punkt schlicht noch nicht angeguckt hast.

Moderne datenbankbasierte Anwendungen nutzen häufig ein ORM - sei es in Python, in Java oder in einer anderen Sprache. Man macht definitiv nichts falsch, wenn man sich mal mit sowas näher befasst.

BlackJack · Mittwoch 31. Dezember 2014, 00:55

Dein Skript mal grob überarbeitet, ungetestet und nicht ganz Python 3 kompatibel, und mit leicht anderen Annahmen bei den Datenbanktabellen (englische Namen und künstliche IDs als Primärschlüssel):

Code: Alles auswählen

#!/usr/bin/env python
# coding: utf8
from __future__ import absolute_import, division, print_function

import sqlite3
from datetime import datetime as DateTime
from time import sleep
from urllib2 import URLError

import lnetatmo


def main():
    connection = sqlite3.connect('netatmo.db')
    cursor = connection.cursor()

    while True:
        try:
            devices = lnetatmo.DeviceList(lnetatmo.ClientAuth())
        except URLError:
            print('Problem with Netatmo server, will retry in 60 seconds')
            sleep(60)
        else:
            for name, values in devices.lastData().items():
                cursor.execute(
                    'SELECT id FROM device WHERE name=?', (name,)
                )
                result = cursor.fetchone()
                if result:
                    device_id = result[0]
                else:
                    cursor.execute(
                        'INSERT INTO device(name) VALUES (?)', (name,)
                    )
                    device_id = cursor.lastrowid

                timestamp = DateTime.now()
                cursor.execute(
                    'INSERT INTO daten(device_id, `timestamp`, temperature,'
                    ' pressure, noise, co_2, humidity, battery_vp)'
                    ' VALUES (?, ?, ?, ?, ?, ?, ?, ?)',
                    (
                        device_id,
                        timestamp,
                        values['Temperature'],
                        values.get('Pressure', 0),
                        values.get('Noise', 0),
                        values.get('CO2', 0),
                        values.get('Humidity', 0),
                        values.get('battery_vp', 0),
                    )
                )

                date = format(timestamp, '%Y-%m-%d')
                cursor.execute(
                    'INSERT OR REPLACE INTO extrema(device_id, `date`,'
                    '   min_value, max_value)'
                    'VALUES (?, ?, ?, ?)'
                    'WHERE device_id=? AND `date`=?',
                    (
                        device_id,
                        date,
                        values.get('min_temp', 0),
                        values.get('max_temp', 0),
                        device_id,
                        date,
                    )
                )
                while True:
                    try:
                        connection.commit()
                        break
                    except sqlite3.Error:
                        print(
                            'Database seems to be locked,'
                            ' will retry in 5 seconds'
                        )
                        sleep(5)

            sleep(600)


if __name__ == '__main__':
    main()

hwm · Mittwoch 31. Dezember 2014, 10:48

@BlackJack: nahezu genauso sah mein Script nach den vorgeschlagenen Änderungen aus, mit der einen Ausnahme, Vermeidung von 'INSERT OR REPLACE'

@Snafu: In meinem Job habe ich leider mit 'Moderne datenbankbasierte Anwendungen' nichts zu tun, ich entwickle auf SAP Systemen.

Euch allen einen guten Rutsch nach 2015.

BlackJack · Mittwoch 31. Dezember 2014, 16:23

@hwm: Ich weiss das Programm sieht inzwischen etwas anders aus aber im ersten gezeigten Programm ist übrigens ein Fehler: Du verwendest `sys.exit()` ohne `sys` importiert zu haben.

Die Fehlermeldung wenn eine Verbindung zur Datenbank fehlschlägt ist irreführend. Wenn die Datenbankdatei nicht existiert, dann wird sie automatisch angelegt, also kann der Grund für eine Ausnahme nicht sein das die Datei nicht gefunden wurde.

Falls die Datei nicht existierte und angelegt werden musste, dann ist die Datenank leer aber das Programm läuft weiter und dann natürlich bei der ersten Datenankanfrage in eine Ausnahme weil die Tabellen nicht existieren. Dieser Fall wird vom Programm aber nirgends behandelt. Ich würde da einfach auch die ”Fehlerbehandlung” beim `connect()` weglassen. Das Programm bricht sowieso ab und man hat mehr Informationen, zum Beispiel den Traceback und die genaue Ausnahme plus potentiell weitere Informationen.

Nochmal völlig ungetestet wie das dann mit SQLAlchemy ohne SQL in Zeichenketten aussehen könnte:

Code: Alles auswählen

#!/usr/bin/env python
# coding: utf8
from __future__ import absolute_import, division, print_function

from datetime import datetime as DateTime
from time import sleep
from urllib2 import URLError

import lnetatmo
from sqlalchemy import (
    and_, Column, create_engine, Date as SADate, Float, ForeignKey, Integer,
    MetaData, select, Table, Text, DateTime as SADateTime, UniqueConstraint
)

metadata = MetaData()


Device = Table('device', metadata,
    Column('id', Integer, primary_key=True, autoincrement=True),
    Column('name', Text, unique=True),
)


Measurement = Table('measurement', metadata,
    Column('id', Integer, primary_key=True, autoincrement=True),
    Column('device_id', ForeignKey(Device.c.id), nullable=False),
    Column('timestamp', SADateTime, nullable=False),
    Column('temperature', Float),
    Column('pressure', Float),
    Column('noise', Float),
    Column('co_2', Float),
    Column('humidity', Float),
    Column('battery_vp', Float),
)


Extrema = Table('extrema', metadata,
    Column('id', Integer, primary_key=True, autoincrement=True),
    Column('device_id', ForeignKey(Device.c.id), nullable=False),
    Column('date', SADate, nullable=False),
    Column('min_value', Float, nullable=False),
    Column('max_value', Float, nullable=False),
    UniqueConstraint('device_id', 'date'),
)


def main():
    engine = create_engine('sqlite:///netatmo.db3', echo=True)
    connection = engine.connect()
    metadata.create_all(engine)

    while True:
        try:
            devices = lnetatmo.DeviceList(lnetatmo.ClientAuth())
        except URLError:
            print('Problem with Netatmo server, will retry in 60 seconds')
            sleep(60)
        else:
            for name, values in devices.lastData().items():
                device_id = connection.execute(
                    select([Device.c.id]).where(Device.c.name == name)
                ).fetchone()
                if device_id is None:
                    device_id = connection.execute(
                        Device.insert().values(name=name)
                    ).inserted_primary_key

                timestamp = DateTime.now()
                connection.execute(
                    Measurement.insert().values(
                        device_id=device_id,
                        timestamp=timestamp,
                        temperature=values['Temperature'],
                        pressure=values.get('Pressure', 0),
                        noise=values.get('Noise', 0),
                        co_2=values.get('CO2', 0),
                        humidity=values.get('Humidity', 0),
                        battery_vp=values.get('battery_vp', 0)
                    )
                )

                date = timestamp.date()
                extrema = connection.execute(
                    Extrema.select().where(
                        and_(
                            Extrema.c.device_id == device_id,
                            Extrema.c.date == date
                        )
                    )
                ).fetchone()
                min_value = values.get('min_temp', 0)
                max_value = values.get('max_temp', 0)
                if extrema is None:
                    connection.execute(
                        Extrema.insert().values(
                            device_id=device_id,
                            date=date,
                            min_value=min_value,
                            max_value=min_value,
                        )
                    )
                else:
                    connection.execute(
                        Extrema.update().values(
                            min_value=min_value, max_value=max_value
                        ).where(Extrema.c.id == extrema[Extrema.c.id])
                    )

                connection.commit()

            sleep(600)


if __name__ == '__main__':
    main()

Man hat hier zwar jetzt die zusätzliche Arbeit die Tabellenstruktur als Objekte zu definieren (obwohl man sich die auch von der Datenbank abfragen lassen könnte!), aber das ermöglicht es dann auch mit der Zeile nach der `connection` die Tabellen anzulegen sofern sie noch nicht existieren. Und bei den `values()`-Methodenaufrufen sieht man viel leichter welcher Wert welcher Spaltezugeordnet wird im Gegensatz zum SQL wo man Spaltennamen und Werte getrennt angibt, und erst die Positionen abzählen muss wenn man wissen will welcher Wert zu einer bestimmten Spalte gehört oder zu welcher Spalte ein bestimmter Wert gehört.

Der `create_all()`-Aufruf setzt übrigens folgendes SQL ab:

Code: Alles auswählen

CREATE TABLE device (
        id INTEGER NOT NULL, 
        name TEXT, 
        PRIMARY KEY (id), 
        UNIQUE (name)
)

CREATE TABLE extrema (
        id INTEGER NOT NULL, 
        device_id INTEGER NOT NULL, 
        date DATE NOT NULL, 
        min_value FLOAT NOT NULL, 
        max_value FLOAT NOT NULL, 
        PRIMARY KEY (id), 
        UNIQUE (device_id, date), 
        FOREIGN KEY(device_id) REFERENCES device (id)
)

CREATE TABLE measurement (
        id INTEGER NOT NULL, 
        device_id INTEGER NOT NULL, 
        timestamp DATETIME NOT NULL, 
        temperature FLOAT, 
        pressure FLOAT, 
        noise FLOAT, 
        co_2 FLOAT, 
        humidity FLOAT, 
        battery_vp FLOAT, 
        PRIMARY KEY (id), 
        FOREIGN KEY(device_id) REFERENCES device (id)
)

hwm · Mittwoch 31. Dezember 2014, 18:05

@BlackJack: stimmt, fehlgeschlagene Verbindungen zur DB gibt es bei Sqlite nicht, das Coding hatte ich aus meinen Postgres Anwendungen kopiert. Das ist das erste mal, dass ich Sqlite verwende, ist doch ziemlich anders als z. B.Postgres., mit Vor- und eben auch Nachteilen. Die Exception habe ich entfernt, somit erübrigt sich auch das fehlende import sys (was in diesem Fall nicht tragisch wäre, da die Exception ja sowieso nie auftreten kann; bei Postgres würde das Programm einfach abstürzen, was auch nicht tragisch wäre, denn ohne Datenbankverbindung muss sich das Programm sowieso verabschieden).

SQLAlchemy muss ich mir dann doch mal näher ansehen.

BlackJack · Samstag 3. Januar 2015, 15:59

Und noch mal etwas ungetestetes — diesmal mit dem ORM:

Code: Alles auswählen

#!/usr/bin/env python
# coding: utf8
from __future__ import absolute_import, division, print_function

from datetime import datetime as DateTime
from time import sleep
from urllib2 import URLError

#import lnetatmo
from sqlalchemy import (
    and_, Column, create_engine, Date as SADate, Float, ForeignKey, Integer,
    select, Text, DateTime as SADateTime, UniqueConstraint
)
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy.orm import sessionmaker

Base = declarative_base()


class Device(Base):

    __tablename__ = 'device'

    id = Column(Integer, primary_key=True, autoincrement=True)
    name = Column(Text, unique=True)

    @classmethod
    def get_or_create_by_name(cls, session, name):
        result = session.query(cls).filter_by(name=name).scalar()
        if result is None:
            result = cls(name=name)
            session.add(result)
        return result


class Measurement(Base):

    __tablename__ = 'measurement'
    
    id = Column(Integer, primary_key=True, autoincrement=True)
    device_id = Column(ForeignKey(Device.id), nullable=False)
    timestamp = Column(SADateTime, nullable=False)
    temperature = Column(Float)
    pressure = Column(Float)
    noise = Column(Float)
    co_2 = Column(Float)
    humidity = Column(Float)
    battery_vp = Column(Float)


class Extrema(Base):

    __tablename__ = 'extrema'
    
    id = Column(Integer, primary_key=True, autoincrement=True)
    device_id = Column(ForeignKey(Device.id), nullable=False)
    date = Column(SADate, nullable=False)
    min_value = Column(Float, nullable=False)
    max_value = Column(Float, nullable=False)

    __table_args__ = (UniqueConstraint(device_id, date),)

    @classmethod
    def update_values(cls, session, device, date, min_value, max_value):
        extrema = (
            session.query(cls)
                .filter_by(device_id=device.id, date=date)
                .scalar()
        )
        if extrema is None:
            session.add(
                Extrema(
                    device_id=device.id,
                    date=date,
                    min_value=min_value,
                    max_value=max_value
                )
            )
        else:
            extrema.min_value = min_value
            extrema.max_value = max_value


def main():
    engine = create_engine('sqlite:///netatmo.db3', echo=True)
    Base.metadata.create_all(engine)
    session = sessionmaker(engine)()

    while True:
        try:
            devices = lnetatmo.DeviceList(lnetatmo.ClientAuth())
        except URLError:
            print('Problem with Netatmo server, will retry in 60 seconds')
            sleep(60)
        else:
            for name, values in devices.lastData().items():
                device = Device.get_or_create_by_name(session, name)
                timestamp = DateTime.now()
                session.add(
                    Measurement(
                        device_id=device.id,
                        timestamp=timestamp,
                        temperature=values['Temperature'],
                        pressure=values.get('Pressure', 0),
                        noise=values.get('Noise', 0),
                        co_2=values.get('CO2', 0),
                        humidity=values.get('Humidity', 0),
                        battery_vp=values.get('battery_vp', 0)
                    )
                )
                Extrema.update_values(
                    session,
                    device,
                    timestamp.date(),
                    values.get('min_temp', 0),
                    values.get('max_temp', 0)
                )
                session.commit()

            sleep(600)


if __name__ == '__main__':
    main()