Model design improvements

[etiology]

There are a few areas where the models can be improved to prevent bugs. Keep in mind that this product works, and it works really well. That's an awesome accomplishment. I just wanted to list them as I see them so that there is some visibility. We can fix these as we go along with updates:

This is kind of related to #114 for DB improvements but this can be done now instead of waiting.

1. Use of str types instead of a table relationship or Enum:
   The use of a string as a type property can create issues. The string is case sensitive and one can make typos. This should be changed to something more static.
   • The string property for type (and order) is an example of this model object knowing too much about other things. Why does it need to know about what types there are? Instead we should make a MethodTypes object and create a foreign key relationship to it. This encapsulates the property and protects the Method class from knowing to much about types. Adding new types is easy since we just add it to the method_type table, and nothing has to change in the methods table.
   • Also look at the other models for areas that fall into this category. Sensor has a few of these properties that should be tables, and I'm certain that there are other models that could be subject to this.
2. Foreign Keys constraints should be used to create relationships instead of performing a software evaluation later. Look at RelayConditional's use of the if_relay_id property. In software later a comparison is made to evaluate the a potential relationship. This should be a constraint instead of an evaluation. The constraint makes the behavior of the relay and its controller much more robust in a changing environment.
3. All models need to define repr. This is considered part of best practices. It really helps later on for debugging and instance outputs.
4. Standard Model Behavior . All models should have certain things that are common behaviors. Similar to the issue with string base PKs in Bug: Text PK can have collisions #115. We should move all primary keys to integers and if we need a unique constraint for printing to the user then we should make a property that is set to unique and defaults to something like a generated UUID.
5. The user_restriction in the user model should either link to a role table or, if it is just used for defining admin vs normal user then it should be changed to a boolean property called is_admin

[kizniche]

All great suggestions. I'm not sure what you mean with a couple statements, but I trust your judgement. I'll have to brush up on the terminology you're using to fully understand what you're talking about ;)

[etiology]

Plenty of time. Enjoy your trip

[etiology]

Related to #131

I'm putting this in here since it pops up in the installer and is DB structure related. I get errors when running setup.sh at the initial DB setup part

Here is the error I get on a fresh install:

#### Creating SQLite databases
Creating/verifying  at sqlite:////home/pi/Mycodo/mycodo/../databases/mycodo.db ...
(sqlite3.IntegrityError) UNIQUE constraint failed: alembic_version.version_num [SQL: u'INSERT INTO alembic_version (version_num) VALUES (?)'] [parameters: ('52aeaa701dfb',)]
(sqlite3.IntegrityError) UNIQUE constraint failed: displayorder.id [SQL: u'INSERT INTO displayorder (id, graph, lcd, log, pid, relay, remote_host, sensor, timer) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)'] [parameters: ('0', '', None, '', '', '', None, '', None)]
(sqlite3.IntegrityError) UNIQUE constraint failed: cameratimelapse.id [SQL: u'INSERT INTO cameratimelapse (id, relay_id, path, prefix, file_timestamp, display_last, cmd_pre_camera, cmd_post_camera, extra_parameters) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)'] [parameters: ('0', '', '/var/www/mycodo/camera-timelapse', 'Timelapse', 1, 1, '', '', '--nopreview --contrast 20 --sharpness 60 --awb auto --quality 20 --vflip --hflip --width 800 --height 600')]
(sqlite3.IntegrityError) UNIQUE constraint failed: camerastill.id [SQL: u'INSERT INTO camerastill (id, hflip, vflip, rotation, relay_id, timestamp, display_last, cmd_pre_camera, cmd_post_camera, extra_parameters) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)'] [parameters: ('0', 0, 0, 0, '', 1, 1, '', '', '--vflip --hflip --width 800 --height 600')]
(sqlite3.IntegrityError) UNIQUE constraint failed: camerastream.id [SQL: u'INSERT INTO camerastream (id, relay_id, cmd_pre_camera, cmd_post_camera, extra_parameters) VALUES (?, ?, ?, ?, ?)'] [parameters: ('0', '', '', '', '--contrast 20 --sharpness 60 --awb auto --quality 20 --vflip --hflip --nopreview --width 800 --height 600')]
(sqlite3.IntegrityError) UNIQUE constraint failed: misc.id [SQL: u'INSERT INTO misc (id, force_https, dismiss_notification, hide_alert_success, hide_alert_info, hide_alert_warning, stats_opt_out, login_message, relay_stats_volts, relay_stats_cost, relay_stats_currency, relay_stats_dayofmonth) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)'] [parameters: ('0', 1, 0, 0, 0, 0, 0, '', 120, 0.05, '$', 15)]
(sqlite3.IntegrityError) UNIQUE constraint failed: smtp.id [SQL: u'INSERT INTO smtp (id, host, ssl, port, user, passw, email_from, hourly_max) VALUES (?, ?, ?, ?, ?, ?, ?, ?)'] [parameters: ('0', 

The unique constraints that are failing are the primary keys:

Field	Type
displayorder.id	TEXT
cameratimelapse.id	TEXT
camerastill.id	TEXT
camerastream.id	TEXT
misc.id	TEXT
smtp.id	TEXT

We need to refactor the model design but this is a very large ask. To do this safely we need to make very large progress on #128 before we can do this without breaking everything. I have a branch that is suggesting a direction for the models to move towards but it will need to be updated with proper relationships. This branch is trying to make the different models behave in a consistent way. Changes in this area will provide significant improvements to the stability of the app and to the installer.

We should also examine the relationships that are implemented in software and move them to database constraints. I will need help with understanding some of these.

[kizniche]

Hmm. So, currently new users will be unable to fully install the system because they'll be prevented from creating the database?

This error isn't being caused by mycodo.db already being present?

[etiology]

It may be. I've ran the script a few times due to some issues so perhaps it got past the db creation and made some users. I'll delete the databases and see if it goes away.

[kizniche]

I would venture to guess that it can't create those entries because they already exist. I don't believe there is a check to see if the database already exists.

[etiology]

Ok good call - It had already created the data in the databases. I jumped the gun there

[kizniche]

Whew. That gave me some momentary anxiety, especially since there's been a good amount of traffic today, after being featured on an email that went out to all HackadayIO members, featuring Hackaday TV's Show & Tell Saturday (that I was a guest of) and featuring Mycodo as a finalist in the Hackaday Prize.

[etiology]

@kizniche Good luck on your upcoming PhD work (I know you have some big stuff this month)

I had some ideas of how to use the database models but I didn't want to do a big refactor until it was solid. So I created a repo that focuses on the core functionality - sensors, actuators, and daemons. I wanted to put this on your radar but don't stress too much about it.

Right now I think it has some neat features. The biggest one is that anytime you subclass AbstractSensor the database becomes aware of it through some introspection magic and your daemons can just ask the database to create that type of sensor and run it.

[kizniche]

I remodeled the User table to reference the Role table, which stores specific restrictions for each role:

class Role(CRUDMixin, db.Model):
    __tablename__ = "roles"

    id = db.Column(db.Integer, unique=True, primary_key=True)
    name = db.Column(db.String, nullable=False, unique=True)
    edit_camera = db.Column(db.Boolean, default=False)
    edit_controllers = db.Column(db.Boolean, default=False)
    edit_users = db.Column(db.Boolean, default=False)
    view_settings = db.Column(db.Boolean, default=False)
    view_camera = db.Column(db.Boolean, default=False)
    view_stats = db.Column(db.Boolean, default=False)
    view_logs = db.Column(db.Boolean, default=False)

    user = db.relationship("User", back_populates="role")

I will eventually add a way to create more role types with different specific restrictions, but for now there are 4 roles initially set in the config:

USER_ROLES = [
    dict(id=1, name='Admin',
         edit_camera=True, edit_controllers=True, edit_users=True,
         view_settings=True, view_camera=True, view_stats=True,
         view_logs=True),
    dict(id=2, name='Editor',
         edit_camera=True, edit_controllers=True, edit_users=False,
         view_settings=True, view_camera=True, view_stats=True,
         view_logs=True),
    dict(id=3, name='Monitor',
         edit_camera=False, edit_controllers=False, edit_users=False,
         view_settings=True, view_camera=True, view_stats=True,
         view_logs=True),
    dict(id=4, name='Guest',
         edit_camera=False, edit_controllers=False, edit_users=False,
         view_settings=False, view_camera=False, view_stats=False,
         view_logs=False)
]

I also modeled the new Camera table after your idea:

class Camera(CRUDMixin, db.Model):
    __tablename__ = "camera"

    id = db.Column(db.Integer, unique=True, primary_key=True)
    relay_id = db.Column(db.Integer, db.ForeignKey('relay.id'), default=None)  # Relay to turn on while capturing
    hflip = db.Column(db.Boolean, default=False)  # Horizontal flip image
    vflip = db.Column(db.Boolean, default=False)  # Vertical flip image
    rotation = db.Column(db.Integer, default=0)  # Rotation degree (0-360)
    cmd_pre_camera = db.Column(db.Text, default='')  # Command to execute before capture
    cmd_post_camera = db.Column(db.Text, default='') # Command to execute after capture
    camera_type = db.Column(db.Integer, db.ForeignKey('camera_type.id'), nullable=False)


class CameraType(CRUDMixin, db.Model):
    """ Holds camera description or kind: 'still', 'timelapse', 'stream', etc """
    __tablename__ = 'camera_type'

    id = db.Column(db.Integer, primary_key=True)
    type_name = db.Column(db.String, nullable=False, unique=True)

With 3 default entries created initially:

CAM_TYPES = (
    'Still',
    'Time-lapse',
    'Stream'
)

[etiology]

Is the USER_ROLES list used to populate the database initially? Like you might have a for loop consume it during your first boot?

# during your first boot on a clean system...
for config in USER_ROLES:
    Roles(**config).save()

Or is it used for something else? Also you don't need to declare the id field. A primary key that is an integer will autoincrement.

[kizniche]

Yes, in the mycodo_db model __init__.py:

def populate_db(db_path):
    """
    Populates the db tables with default values.  This will likely
    be replaced in the future by just setting the default values in the
    db fields
    """
    engine = create_engine(db_path)
    Session = sessionmaker(bind=engine)
    session = Session()
    try:
        for index, each_camera_type in enumerate(CAM_TYPES, 1):
            insert_or_ignore(Camera(id=index, camera_type=each_camera_type), session)
        for each_role in USER_ROLES:
            insert_or_ignore(Role(**each_role), session)
        insert_or_ignore(AlembicVersion(), session)
        insert_or_ignore(DisplayOrder(id=1), session)
        insert_or_ignore(Misc(id=1), session)
        insert_or_ignore(SMTP(id=1), session)
    finally:
session.close()

I included the id because otherwise it will add those entries every time the app is started. If the id is defiled and already in the table, it will not be able to add the entry.

[kizniche]

I left populate_db() largely as it was, using insert_or_ignore() because I was trying to make many small changes everywhere that would preserve functionality. I'm not sure if there's a more efficient way to accomplish what's in populate_db(), but at least it's all working.

[kizniche]

Well, I modified the Camera table again, now consolidated into one table. This is in preparation for enabling the use of multiple cameras (different types, #193). I'm really just learning about database structures as I go, so hopefully this will evolve into something that's logical and efficient.

class Camera(CRUDMixin, db.Model):
    __tablename__ = "camera"

    id = db.Column(db.Integer, unique=True, primary_key=True)
    name = db.Column(db.Text, unique=True, nullable=False)
    camera_type = db.Column(db.Text, nullable=False)
    library = db.Column(db.Text, nullable=False)
    hflip = db.Column(db.Boolean, default=False)  # Horizontal flip image
    vflip = db.Column(db.Boolean, default=False)  # Vertical flip image
    rotation = db.Column(db.Integer, default=0)  # Rotation degree (0-360)
    height = db.Column(db.Integer, default=640)
    width = db.Column(db.Integer, default=480)
    contrast = db.Column(db.Float, default=0.0)
    exposure = db.Column(db.Float, default=0.0)
    gain = db.Column(db.Float, default=0.0)
    hue = db.Column(db.Float, default=0.0)
    saturation = db.Column(db.Float, default=0.0)
    white_balance = db.Column(db.Float, default=0.0)
    relay_id = db.Column(db.Integer, db.ForeignKey('relay.id'), default=None)  # Turn relay on during capture
    cmd_pre_camera = db.Column(db.Text, default='')  # Command to execute before capture
    cmd_post_camera = db.Column(db.Text, default='') # Command to execute after capture

[kizniche]

I finished adding support for multiple cameras. Essentially, you can now add as many cameras as you want. More specifically, you can add as many configurations as you want, which means you can have multiple configurations per camera. Besides that awesomeness, there's one minor issue holding back the timelapse feature from fully working... I can't save to the SQLite database from the mycodo daemon (specifically, outside of the flask app).

I didn't notice this bug until now because none of the code tried to save to the database outside the flask app. Since the daemon does the timekeeping and captures timelapses, it needs to modify the database in order to modify the next timelapse capture time, the increment number for the filename, and reset the settings when the time surpasses the timalapse end time.

I'm going to test a few ways to use the current DB model to save to the database outside flask and see how much progress I get.

[kizniche]

It seems like sqlservice might allow the best of both worlds, in and out of the flask app: This issue discusses the method for linking it to a flask app.

[etiology]

What's with the saving issue? I'd like to hear more about that because in my experience daemons can save to the database.

[kizniche]

Essentially, I need to modify a table here:

with session_scope(MYCODO_DB_PATH) as new_session:
    mod_camera = new_session.query(Camera).filter(
        Camera.id == each_camera.id)
    mod_camera.timelapse_started = False
    mod_camera.timelapse_paused = False
    mod_camera.timelapse_start_time = None
    mod_camera.timelapse_end_time = None
    mod_camera.timelapse_interval = None
    mod_camera.timelapse_next_capture = None
    mod_camera.timelapse_capture_number = None
    new_session.commit()

However, it doesn't appear to be saving.

[etiology]

do you need to add it to the session?

new_session.add(mod_camera)
new_session.commit()

[kizniche]

Testing!...

[kizniche]

No bueno. Isn't add() only used when creating a table entry? I'm only wanting to edit a current table.

2017-02-12 00:42:14,301 - mycodo - ERROR - Timelapse ERROR                                              [45/1969]
Traceback (most recent call last):
  File "/var/www/mycodo/mycodo/mycodo_daemon.py", line 286, in run
    new_session.add(mod_camera)
  File "/usr/local/lib/python2.7/dist-packages/sqlalchemy/orm/session.py", line 1586, in add
    raise exc.UnmappedInstanceError(instance)
UnmappedInstanceError: Class 'sqlalchemy.orm.query.Query' is not mapped
2017-02-12 00:42:14,618 - mycodo.databases.utils - ERROR - Error raised in session_scope.  Session will be rolled
 back: db_uri='sqlite:////home/kiz/Mycodo/mycodo/../databases/mycodo.db', error='Class 'sqlalchemy.orm.query.Quer
y' is not mapped'

[kizniche]

Oh.. perhapse because I don't have .first() applied:

    mod_camera = new_session.query(Camera).filter(
        Camera.id == each_camera.id)

to

    mod_camera = new_session.query(Camera).filter(
        Camera.id == each_camera.id).first()

[kizniche]

It works!

[etiology]

Nice. I'm glad it was quick. When you first mentioned the issue I thought it might have been something more complicated.

[kizniche]

Yeah, sorry, I sometimes jump the gun with issues. I thought it was more complicated, but it was just a simple error after a long few hours of coding the new camera system.

Thanks for the help! You saved me from going down a long rabbit hole with a different branch attempting to use sqlservice.

Now the new timelapse functionality with multiple cameras is working!

[kizniche]

The latest 5.0-dev commit includes the move to Flask-Login. It was fairly straightforward but I'm wondering if my method for creating a unique but persistent flask secret_key (to preserve remembered logins after flask app restart) is how it should be done. Without saving (and reusing) the secret_key, the remember_token from Flask-Login would become invalid if the app generated a new secret_key. This overcomes the issue of the secret_key not persisting without sacrificing a strong random key for each Mycodo install.

if not os.path.isfile(SECRET_KEY_PATH):
    with open(SECRET_KEY_PATH, 'w') as file:
        file.write(os.urandom(24))
SECRET_KEY = open(SECRET_KEY_PATH, 'rb').read()