🏦 Database Change: convert commentflag's created_utc to created_timestampz

[VietThan]

created alembic migration like #597

Partially fix #601

[zorbathut]

It looks like something went wrong in the migration, though I'm not sure what yet. Also, please use the new field and remember to remove the old one from the flags.py!

[VietThan]

@zorbathut , fixed. It was a problem with how table classes inherit from CreatedBase instead of Base = declarative_base() directly. This is ready for a rerun.

[zorbathut]

Hrm. I hate to say it, but I'm not a big fan of this approach. I don't think changing the base class is the right approach; I'd rather see the change in the base class. Splitting it out just increases complexity, and if there's one thing we don't need, it's more complexity. Any way to just make the relevant change in CreatedBase, along with anything that references that field name?

(also, I think there may be a bug in the update; in my experience you can't create a column that's nullable=False, you have to create a nullable=True column, update it with the data you want, and then alter it to be nullable=False, which is a sort of annoying dance that I wish sqlalchemy made easier. this is admittedly sort of irritating to test because you have to switch to the old version, add stuff of that type, and then switch to the new version to see if your update works)

[VietThan]

@zorbathut. I was going to present the argument that CreatedBase represent another level of abstraction that hides complexity. It seems to be a layer purely to make sure tables inheriting from it will have a created_utc column. But I can see that CreatedBase's properties do seem to be quite convenient and standardize how time/date should be represented/formatted. Single-use properties now (age_timedelta for example) might be of use in the future, as such the possible value is worth the cost of abstraction.

With 13 classes inheriting from CreatedBase, I don't feel confident about doing a massive migration on all 13 tables at once. As a proposal to maintain the same general structure, we can create a CreatedTimestampzBase to replace CreatedBase. This PR will also mark CreatedBase as deprecated. One PR at a time, we will be moving classes to CreatedTimestampzBase. With the last PR we will then also delete CreatedBase. What do you think?

Will take note regarding the migration steps.

[justcool393]

so a bit of history here: on the old rdrama codebase, each model had its own created_utc property that was attached to it. the problem is that a lot of tables never got this data attached or had subtlety different semantics to represent what was ultimately the same thing, the date/time something was created.

i unified it initially because it is incredibly useful to have some of the semantics that we have, and also because we want to know when something happened in most cases, and also because all of the same semantics made many things useful (like for example age_timedelta as you observe). the complexity of knowing when something exists is still there, but by splitting out some of the parts that are common to everything, we gain maintainability.

the problem I see with this approach is that this kinda regresses back to "some things are behaving in subtlety different ways" so I wonder why not just make the modification to all of the tables? it seems like this would have better semantics and would be easier to use. i'd love to see created_utc be a property that's calculated, rather than the baseline property (as since we have a date/time datatype, we can describe the database schema using those semantics :)), don't get me wrong, but i do think it should probably be done all at once, so we don't get into a weird state where half of the things are done but half aren't.

[justcool393]

another quick note: btw we should be using the database-agnostic DateTime rather than the specific TIMESTAMP. in SQLA, the uppercase values are transmitted literally, whereas using the DateTime one gets us semantics that knows that we're using postgres and plans accordingly. in general, unless absolutely necessary, the generic should be used.

[zorbathut]

With 13 classes inheriting from CreatedBase, I don't feel confident about doing a massive migration on all 13 tables at once. As a proposal to maintain the same general structure, we can create a CreatedTimestampzBase to replace CreatedBase. This PR will also mark CreatedBase as deprecated. One PR at a time, we will be moving classes to CreatedTimestampzBase. With the last PR we will then also delete CreatedBase. What do you think?

This sounds fine to me, with one addendum: with the last PR, rename CreatedTimestampzBase back to CreatedBase :)

[VietThan]

@zorbathut , @justcool393 , notes taken. I moved this PR to draft until ready to merge again.

[VietThan]

@zorbathut , this PR is ready for review.

Only two side notes:

1. Was implementing CreatedDateTimeBase.created_date but notice logic seems to be not doing what the signature suggests.
2. datetimez vs timestampz (naming things is hard).

[VietThan]

@justcool393 , I think there are some benefits of doing one table at a time here:

1. The PRs are easier to review as they're smaller.
2. There is reduced blast radius on rollback. Admittedly converting from utc int to timestampz and vice versa is not hard , but upgrade/downgrade 13 tables is going to be different from only 1.
3. I don't know the data size but performance is a consideration too.

At worst, I can line up all 13 PRs (with each one inheriting from the previous). So the last PR would be all 13 tables at once, but merge can be one at a time on any arbitrary schedule.

Will resolve the other comments

[zorbathut]

Looks good - let me know if there's other tweaks you want to do or if you want to call it good :)

[zorbathut]

Alright, looks like you're not actively developing right now, pushing it in :V