Bug: Text PK can have collisions #115
Comments
|
That sounds good. Would you also know how to upgrade the databases with alembic? |
|
Probably in the wrong issue here (ref to #114), but would a database merger (into one DB) necessitate the users reclone the repo, or could this be cleanly done with the current upgrade system? |
|
I think alembic can make that smoother. I'm a bit out of practice. I was holding off on the db branch until I get that figured out. You've had more practice with alembic lately than I have so I'll have to pick your brains when things get closer. Sent from my iPhone
|
|
I've been going through and making a lot of small changes (which add up to thousands of changes overall) related to PEP8, logging, etc., and closing old issues to clear the way to work on some of these you started a while ago. I came to this issue and have been trying to wrap my head around how to go about upgrading the databases with alembic, and I'm so far stumped as to how to accomplish this. Essentially, will we replace the 8-character ID with a uuid, then go through and replace all occurrences of any 8-char string with the new uuid, so all references remain intact? I'm no alembic wizard, so I'm having difficulty understanding how to do this. Also, what do you mean by using an integer for a primary key? Just a very long random integer to prevent potential collisions? |
|
Getting better at alembic is still on my list. So I can't speak much about that part. So taking an example from the Method model: class Method(Base):
__tablename__ = "method"
id = Column(TEXT, unique=True, primary_key=True)
name = Column(TEXT)
method_id = Column(TEXT)
# ... etcIf I recall the method_id is where the random string was assigned right? If not just correct me. The UUID is just a string so you don't need to convert it, but it would be a great chance to change the type to from uuid import uuid4
# ... other imports
class Method(Base):
__tablename__ = "method"
id = Column(Integer, primary_key=True)
name = Column(TEXT)
method_id = Column(String, nullable=False, default=str(uuid4()))
# ... etc |
|
The next step after that is to move some of the common model routines into the models. The model knows the most about what it holds and how to use it so it's the best place for certain responsibilities. The easiest example I can think of right now is your class Users(Base):
__tablename__ = "users"
user_id = Column(INTEGER, primary_key=True)
user_name = Column(VARCHAR(64), unique=True, index=True)
user_password_hash = Column(VARCHAR(255))
user_email = Column(VARCHAR(64), unique=True, index=True)
user_restriction = Column(VARCHAR(64))
user_theme = Column(VARCHAR(64))
def __repr__(self):
output = "<User: <name='{name}', email='{email}' is_admin='{isadmin}'>"
return output.format(name=self.user_name,
email=self.user_email,
isadmin=bool(self.user_restriction == 'admin'))
def set_password(self, new_password):
self.user_password_hash = bcrypt.hashpw(new_password.encode('utf-8'), bcrypt.gensalt())
@staticmethod
def check_password(password, hashed_password):
hashes_match = bcrypt.hashpw(password.encode('utf-8'), hashed_password.encode('utf-8'))
return hashes_matchYou gave it those |
|
Methods are handled a little differently than the sensors, PIDs, relays, timers, graphs, and LCDs. For all but methods, the 'id' is the where the unique id is stored to distinguish different controllers. For methods, the 'id' is still used to distinguish between, modify, and delete entries, but the 'method_id' is used to associate entries. So, if a method is created, an entry will be made with a random 'id' and random 'method_id' and will contain basic info about the method (name, type, etc.). Then, when a time period/point is added to the method, an entry is created with a random 'id' but the 'method_id' will match that of the previous parent entry's 'method_id'. So, 'id' should be unique, but 'method_id' should not be. |
|
Is method_id a foreign key then? |
|
Yes, it will reference the 'method_id' of the only entry with the method_order of 0. Here's some data to illustrate.
|
|
I've been thinking about what would be necessary for a successful database upgrade. The current IDs from each table will need to be searched for and replaced throughout the database to preserve references. Then the column type needs to be changed, then assigned the new unique integer. However, I found sqlite3 column types can't be altered. So, unless there's a new method I haven't discovered, I'm coming to the conclusion that having users create a new database will probably be necessary. I believe this should be a major version number change, to prevent users from automatically upgrading to it from the lower major version number. I envision the last version release of the lower major version (e.g. 4.1.20) would present a new option on the upgrade screen, allowing a major version upgrade (5.0.0) but warning them that their database will be created anew and no settings will be transferred over. I'm trying to think of any other database structural changes that would be needed or would be useful, but I'm just not that familiar with database design. Are there any other changes you can think of that should be included in this update? |
|
I would take the time to rethink how your database objects are related to each other and add foreign key relationships. If you are considering having users setup new settings it gives you the freedom to design the database that you want. Look for areas in the code where relationships between database models is being resolved in software instead of in the database. I may be mistaken but it looks like there is a relationship between the Relay model and the RelayConditional models. I see in controller_relay.py the following: def check_conditionals(self, relay_id, on_duration):
conditionals = db_retrieve_table(MYCODO_DB_PATH, RelayConditional)
conditionals = conditionals.filter(RelayConditional.if_relay_id == relay_id)
conditionals = conditionals.filter(RelayConditional.activated == True)
if self.is_on(relay_id):
conditionals = conditionals.filter(RelayConditional.if_action == 'on')
conditionals = conditionals.filter(RelayConditional.if_duration == on_duration)
else:
conditionals = conditionals.filter(RelayConditional.if_action == 'off')
for each_conditional in conditionals.all():
message = None
...I might be wrong about this specific object because I don't quite understand what the relay conditional class is doing, but it looks like it could be a one-to-many relationship between Relay and Conditionals. This relationship can be created in the database with a foreign key in the Conditionals table: class RelayConditional(Base):
__tablename__ = "relayconditional"
id = Column(Integer, primary_key=True)
relay_id = Column(Integer, ForeignKey(relays.id), nullable=False)
name = Column(TEXT)
activated = Column(BOOLEAN)
#... more conditionsNow you can do this instead: def check_conditionals(self, relay_id, on_duration):
my_relay = Relay.query.filter_by(id=relay_id).first()
for condition in my_relay.conditionals:
# ... do stuffIt would be a large change but it could streamline the code, which makes it easier to manage and work with. It's worth daydreaming about. Does it sound right that Relays could have assigned PIDControllers that have their own set of conditional values? Well that's a relationship that you can make in the database. It makes the objects more useful and it prevents things like database rows containing conditional values that are not referenced or attached to anything. I'm sure there are tons of relationships in there that could be useful to implement. |
|
The conditionals are a bit complex. The relay conditionals, in particular, have an "if relay" (if this relay acts) and a "do relay" (then act on this relay). The conditional can also either be active or inactive. The current code is finding only activated conditionals for the particular "if relay" that just changed its state (either turning on or off), then further filtering conditionals with the "if action" (whether the "if relay" turned on or off). This will ultimately yield only relay conditionals that match the state (such as "relay X turned off", or "relay Y turned on for Z seconds"). There may be multiple conditionals that match these criteria, so it's essential to filter for all of them instead of returning the first. The action of these (which can be a number of things that happen) are then executed. I thought it would be easiest to perform the filter on the database query instead of in a loop. I'm intrigued by database relationships, primary/foreign keys, and how to effectively use them. I'm still trying to wrap my head around how they can be best used in this scenario. I don't believe your example is using the relay conditional table correctly, but I'm also not experienced enough to know what should be done. I'll have to spend a while experimenting with it to understand. |
|
Also, when the databases are eventually merged into one, and flask is using flask-sqlalchemy, how should the daemon access the database? With the sqlalchemy module that's currently being used? Would using the flask-sqlalchemy models cause issues with that? Again, excuse my limited understanding of databases. |
|
I just pushed a database model I've been working on. It includes the settings and user tables. This is what I'd like to release with Mycodo 5.0, since a new database will need to be created to use this. This model should permit moving over to flask-sqlalchemy, to resolve the threading errors that plague the HTTP log, which I think will bring performance improvements. Also, if I'm understanding foreign keys correctly, we can also refactor a lot of the code to be more efficient at referencing IDs and filtering data in the database to obtain the correct entries/rows. |
|
I have a new branch I've been working on, using flask_sqlalchemy. I've finally figured it out and am going through the entire software, refactoring with sql_alchemy. I'll push it a little later once I have it all working. I just got it working up to the login page (after creating database, populating default tables with default entries, and crating an admin user), and am currently on a roll with a new cup of coffee. |
|
Nice work I cant wait to see it. I think flask sqlalchemy is the way to go. It's so much easier to manage and it just plays well. |
|
Wow, literally 8 seconds after you said that I pushed the new branch ;) Not all pages are working (I noticed Methods errors for example), but the frontend and backend are working and using the same model, with Flask using flask_sqlalchemy. |
|
I really like the Relay class. It's got everything you think a relay should do and it has state because it's a database object |
|
I could see the Cameras being a single 'Camera' class with a 'camera_type' attribute. That could even be a foreign key that points to a 'CameraType' table that has a 'name', maybe a recommendation like brand or product number. When you are selecting your camera type it tells you which things are supported. Just thinking out loud here so you don't really need the extra table but the idea struck me. |
@Cabalist is the one who actually made the Relay class functions, if I recall correctly. I'm sure I'm doing some things improperly, but it's at least a start. And it's actually working! I had a difficult time at first getting both the flask app and the daemon to use the same database model. I attempted to use this method to be able to use the same query syntax (e.g. 'Class.query.first()') in the daemon (outside the flask app), but I wasn't understanding it enough to get it working.
Ah, yes! That's a great idea. I'll play around with it when I'm more awake. |
|
That is a great article. I'll have to play with it but that's great. for the camera class combination I was thinking: 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('CameraType.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) # 'Still', 'Timelapse', 'Infrared', etc
# for extra ease you might want to add a CRUDMixin:
class CRUDMixin(object):
"""
Basic Create, Read, Update and Delete methods
Models that inherit from this class automatically get these CRUD methods
"""
def save(self, session):
""" creates the model in the database """
try:
session.add(self)
session.commit()
return self
except Exception as error:
session.rollback()
logging.error("Unable to save {model} due to error: {err}".format(model=self, err=error))
raise error
def delete(self, session):
""" deletes the record from the database """
try:
session.delete(self)
session.commit()
except Exception as error:
# many things can go wrong during the commit() so we have a broad except clause
logging.error("Failed to delete '{record}' due to error: '{err}'".format(record=self, err=error))The CRUDMixin allows your database models to have """ To make things easy lets assume that the camera_type
comes from a constants file that just has a dictionary of types """
# namedtuple would be good here too
CAMTYPE = dict(still='Still Camera', time_lapse='Time-lapse', stream='Stream')
# ----- Normal SQLAlchemy Style -VS- CRUDMixin ------
from flask import current_app
# without a CRUDMixin:
new_camera = Camera(camera_type=CAMTYPE['time_lapse'])
try:
db.session.add(new_camera)
db.session.commit()
except Exception as err:
msg = "Error occurred when saving {cls} model: {err}"
current_app.logger.error(msg.format(err=err, cls="Camera")
session.roll_back()
# do that for each model you want to save. What if you wanted to make sure you logged this?
# How many copy & paste logging statements would you need?
# -- VS --
new_camera = Camera(camera_type=CAMTYPE['time_lapse']).save()
|
|
That's nice! It will save a lot of time and code. I'll work on restructuring the database a little more once I have the majority of things working again. Interestingly, when testing the use of the uuid4 in some of the default values of the database tables, I found something odd: class PID(db.Model):
__tablename__ = "pid"
id = db.Column(db.Integer, unique=True, primary_key=True)
method_id = Column(String, nullable=False, default=str(uuid.uuid4()))
...If table entries were created (committed) inside a loop (when adding multiple sensors, relays, etc.), all entries would have the same uuid. It appeared that uuid4() was only being called once and that value was being applied to every new entry. I found this solution that works nicely to create a unique uuid: class UUID(types.TypeDecorator):
impl = MSBinary
def __init__(self):
self.impl.length = 16
types.TypeDecorator.__init__(self,length=self.impl.length)
def process_bind_param(self,value,dialect=None):
if value and isinstance(value,uuid.UUID):
return value.bytes
elif value and not isinstance(value,uuid.UUID):
raise ValueError('value {val} is not a valid uuid.UUID'.format(val=value))
else:
return None
def process_result_value(self, value, dialect=None):
if value:
return uuid.UUID(bytes=value)
else:
return None
@staticmethod
def is_mutable():
return Falseclass PID(db.Model):
__tablename__ = "pid"
id = db.Column(db.Integer, unique=True, primary_key=True)
unique_id = db.Column(UUID, nullable=False, unique=True, default=uuid.uuid4)
...The main reason a unique UUID is necessary is for influxdb. Entries (sensor measurements, relay durations, pid setpoints) are associated an ID tag when added to the influxdb database. If the For instance:
If |
|
I should have remembered that for a default field to work in a sqlalchemy column you need to assign it to a function or else the value doesn't always change. So for the def set_uuid():
return str(uuid4())
# ... then in your model...
some_unique_text =db.Column(db.String, default=set_uuid, unique=True) |
|
It's just like setting a datetime.now() call |
|
So this is what I'm now unsure where to go... Should And I suppose I should cut out the UUID class type in favor of just using a function to call uuid4? |
|
There is definitely a debate between people who feel that the primary key of a table should be an int and left alone vs those who feel that a field that is known to be unique could be the primary key. I argue for the former. The primary key field should be an an int and if you need a serial number like identifier then create a field that holds that random string. Here are some random thoughts to why integer based primary keys feel better to me:
The whole function call being passed to default might also work for your original code if you wrap your random.choice() call in a function and assign the function (Not the call) to the default. That suggestion you found looks like the person didn't understand what was happening and built way more code than what was needed to generate random uuids. I would still test what I suggested to make sure they are unique. Toss it in a for loop and make a million records. If it does't throw and error I'd say that is good proof that you are safe using it. |
|
That makes sense. Since I only need a uuid for 3 tables, there's also no need for unnecessary overhead in the other tables. One thing I can see potentially causing problems is if all rows are deleted in a table, the Is there a way to prevent this? Also, interesting is the time difference from entering entries different ways. Here's something I made to time and test making entries: try:
num_entries = 1000
first_run = True
for x in xrange(2):
logger.error("Starting SQL uuid generation test {num}: "
"{n} entries...".format(num=x,
n=num_entries))
before_count = PID.query.count()
a = datetime.datetime.now()
if first_run:
for _ in xrange(num_entries):
db.session.add(PID())
db.session.commit()
else:
for _ in xrange(num_entries):
db.session.add(PID())
db.session.commit()
after_count = PID.query.count()
b = datetime.datetime.now()
delta = b - a
logger.error("Completed: {time}, " \
"New entries: {new}, " \
"Total entries: {tot}".format(
time=delta,
new=after_count - before_count,
tot=after_count))
first_run = False
except Exception as msg:
logger.error("Error creating entries: {err}".format(err=msg))Here's the relavant part: if first_run:
for _ in xrange(num_entries):
db.session.add(PID())
db.session.commit()
else:
for _ in xrange(num_entries):
db.session.add(PID())
db.session.commit()And the output: One is clearly more efficient. |
|
yeah closing the session all of the time with the commit is Soooo costly. It's much faster to add a ton of stuff to the session and then commit at the end. That's a great example and a good observation. It's the kind of test that will change the way you code. The difference is huge. |
|
Hmm. I seem to be hitting a wall with the test: At this point I went to sleep and woke to find the flask app had crashed. The database had reached ~40 MB. Here's the code that was running: def test_sql():
try:
num_entries = 1000000
factor_info = 25000
PID.query.delete()
db.session.commit()
logger.error("Starting SQL uuid generation test: "
"{n} entries...".format(n=num_entries))
before_count = PID.query.count()
run_times = []
a = datetime.now()
for x in xrange(1, num_entries + 1):
db.session.add(PID())
if x % factor_info == 0:
db.session.commit()
after_count = PID.query.count()
b = datetime.now()
run_times.append(float((b - a).total_seconds()))
logger.error("Run Time: {time:.2f} sec, "
"New entries: {new}, "
"Total entries: {tot}".format(
time=run_times[-1],
new=after_count - before_count,
tot=PID.query.count()))
before_count = PID.query.count()
a = datetime.now()
avg_run_time = sum(run_times) / float(len(run_times))
logger.error("Finished. Total: {tot} entries. "
"Averages: {avg:.2f} sec, "
"{epm:.2f} entries/min".format(
tot=PID.query.count(),
avg=avg_run_time,
epm=(factor_info / avg_run_time) * 60.0))
except Exception as msg:
logger.error("Error creating entries: {err}".format(err=msg))I'm not sure what's wrong, why it began taking longer to create entries. python ram use was between 40 and 60 MB last I saw it. Here's an earlier screenshot with top running in the middle.
It's not something I'm going to invest a lot of time into figuring out, but thought it was interesting enough to share. |
I'll fix this but let me describe the problem and get some info about how the PK is used.
There are two primary key strategies happening in the code base:
During the tests that I've ran I have found that there is a random
UNIQUE constraint failederror that occurs when creating models that use the text based primary key. This makes sense and it would cause issues.At first I thought it would be simple to normalize the primary key strategy and change everything over an integer since this is part of best practices for keys. However I found that the primary key is actually used in flashing messages to the user.
I would suggest that the models move to using an integer base primary key and we add a uuid to handle the user facing identifier. The is very little chance of getting a collision and it could be created when the model is created. We could then move all of the models over to being consistent with their primary keys. UUIDs look like this:
Let me know what your thoughts are on this. Like I said I'll take care of implementing it when I push the tests I'm writing for the models since I can see where they are failing.
The text was updated successfully, but these errors were encountered: