Change is_downloaded column to download_status

[sssoleileraaa]

Description

The files messages and replies tables have a nullable boolean column called is_downloaded. For code clarity, let's make this a download_status non-nullable integer column instead.

download_status

• NOT_ATTEMPTED
• FAILED
• SUCCEEDED

Same goes for is_decrypted which I'm not creating a separate issue for in case @redshiftzero addresses this in the open PR #262

[redshiftzero]

So there are multiple ways of doing this:

0. Store the status strings directly in the rows for each file, message, and reply (as Strings) - denormalized and not space efficient but then again we are not particularly space constrained. This is also clear to the developer.
1. Store the statuses as integer status codes which are foreign keyed to new tables that explain the status codes.
2. We keep storing the statues as Null, True, False which is effectively 0, 1, 2 (prefer not as this is also not clear and it's not extensible in case we want to add statuses e.g. if we want to break up FAILED into FAILED_REASON_X, FAILED_REASON_Y in the future).
3. We hardcode the meaning of the status codes somewhere in the code (this isn't a bad idea either but it's slightly less clear to the developer than 1 imho).

I am in favor of 1 above - as we add new statuses, we can just add a row to the table. If we want to rename a status code i.e. FAILED -> FAILED_BAD_SUBMISSION_KEY, it's pretty easy to do so.

Thoughts? We should use the same logic for decrypt status.

[heartsucker]

We should do something like 0 (see example at bottom). First, we shouldn't do 1 because it's cumbersome.

class DecryptionStatus(Base):

     code = Column(Integer, primary_key=True, autoincrement=False)
     value = Column(String, nullable=False)

class Reply(Base):

    ... # the usual stuff
    decryption_status_id = Column(Integer, ForeignKey('decryption_status_code.id'), nullable=False)
    decryption_status = relationship('decryption_status_code')


# usage

reply = session.query.(Reply).filter_by(uuid=uuid).one()
if reply.status_code.value == WHAT_GOES_HERE: # <-- :(
    # do things

We end up having to either use constants or magic strings to pull the values anyway. We'd also have to pre-load that table during the app start up to ensure we have the expected values there or we'd have to load it during the alembic migration.

For 3, we'd have to add constraints to ensure we're not loading bad values, and this feels like a lot of work, so what we should do is use enums. Here's a working example.

#!/usr/bin/env python
import enum
import os
from sqlalchemy import Column, Integer, String, MetaData, create_engine, Enum
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy.orm import sessionmaker

DATABASE = os.path.join(os.path.dirname(__file__), 'db.sqlite')
metadata = MetaData()
Base = declarative_base(metadata=metadata)


class MyEnum(enum.Enum):

    WAT = 1
    LOL = 2
    OMG = 3


class User(Base):

    __tablename__ = 'users'

    id = Column(Integer, primary_key=True)
    username = Column(String(255), nullable=False)
    typ = Column(Enum(MyEnum), nullable=False)

    def __repr__(self):
        return '<User {} {}>'.format(self.username, self.typ)


if os.path.exists(DATABASE):
    os.remove(DATABASE)

engine = create_engine('sqlite:///{}'.format(DATABASE))
Session = sessionmaker(bind=engine)
session = Session()

Base.metadata.create_all(engine)

new_user = User(username='Bob', typ=MyEnum.WAT)
session.add(new_user)
session.commit()

wats = session.query(User).filter(User.typ == MyEnum.WAT).all()
print(wats)

This gives us a bit of type checking and also a bit of autogenerated checking at the DB level. We're going to have to have constants in the code somehow (no matter what option we pick) in order to be able to reference stuff, we might as well make it as unconvoluted as possible.

Adding new values would require a migration to update the constraint, but we'd need that anyway if we did 1 in order to be able to load the new values into the table.

There is a small chance this does something weird with SQLAlchemy / Alembic with the naming conventions but I suspect that's easily work-around-able if so.

[sssoleileraaa]

Ah, yes, I think we should use sqlalchemy's enum type!

[eloquence]

From 2022-08-15 review with @gonzalo-bulnes @eloquence:

• Still seems worthy of continued discussion, keeping open.