db: make value.*_values TEXT instead of String(255)

[RhinosF1]

Description

Tin. Fixes #2291

Checklist

• I have read CONTRIBUTING.md
• I can and do license this contribution under the EFLv2
• No issues are reported by make qa (runs make quality and make test)
• I have tested the functionality of the things this change touches

[RhinosF1]

This is as deployed on the beta bot I run.

[dgw]

This makes perfect sense, and I'm surprised none of us thought about the potential issues in #1446 or follow-ups.

Since there's no mechanism yet for DB schema migrations, how do others want to handle this? There is precedent for a "migrate database" script that bot owners are instructed to run before starting a new version of the bot, and that's the simplest option.

@RustyBower Does SQLAlchemy offer some simple automatic migration feature we could use, since VARCHAR to TEXT is quite possibly the simplest schema change ever? Alembic seems a bit heavy for this, not that I know how to use it anyway.

[RustyBower]

I'll test the migration this evening. Otherwise we can make some "prerun" script that runs that validates the state of the database.

[RustyBower]

Also IIRC, the reason everything is set for String(255) was either compatibility with the old database or because of SQLite weirdness with SQLAlchemy...

[RustyBower]

So testing this, it appears there's no weirdness with MariaDB or SQLite in terms of the schema.

@dgw I'm happy to follow this up with some Alembic work, since that is probably the most scalable solution going forward?

[RustyBower]

Digging into this further. This is super breaking, because SQLite doesn't support the alter column functionality that other SQL engines support (https://www.sqlite.org/omitted.html)

Although it appears you can hack around it with Alembic's batch_alter_table functionality (https://alembic.sqlalchemy.org/en/latest/ops.html#alembic.operations.Operations.batch_alter_table)

[dgw]

SQLite hasn't seemed to exhibit the originally described problem with column value length, though. It doesn't appear to have any concept of constrained text length, unless I'm just missing that part of the datatype documentation.

But that Alembic feature looks purpose designed to work around such issues if I am missing something in the SQLite docs.

[RustyBower]

I think you're correct.

Although trying to implement this dynamically to leverage db.get_uri() function from the bot is not as straightforward as I would have hoped. You can leverage some python magic in the env.py file for Alembic to dynamically generate the SQLAlchemy URI to use, but I need an instantiating SopelDB to call that...

[dgw]

About getting a SopelDB, all you need is the config filename. Pass the filename to a new Config instance, then pass the Config instance to a new SopelDB instance. You can probably steal a lot of Config setup logic from the CLI code (e.g. for sopel-config), which has a utility function for finding the config.

(An aside: I see we don't actually compile the docstrings for sopel.cli stuff anywhere, so I can't link Sphinx docs of those. @Exirel is working on continued doc improvements, so that might get added to the list.)

[Exirel]

I'll add a mental note to look further into Alembic. I already used it on a previous project with custom migrations and custom commands, so that shouldn't be too much of a trouble, at least for Sopel's migrations. For plugins, that'll be another thing, yet I already have some ideas to allow a plugin author to hooks their migrations to Sopel's future db migration command.

[dgw]

Can we reasonably expect this not to break existing SQLite instances—the default and therefore majority?

If this has to be migrated only for non-default RDBMS backends, I'm a lot less concerned by the idea of merging before the migration is in place. 😅

[RhinosF1]

Existing installations can simply choose to apply needed. I'm fairly confident it can't cause any damage to them on either schema simply by merging this without an explicit migration.

[dgw]

@RhinosF1 Except for SQLite users (again, the default), there is no ALTER TABLE. That's why I'm asking if this will break them in any way. Someone who has already set up Postgres or Maria has the knowledge to read release notes and update schemas, of course.

[RustyBower]

This should be handled via #2317

[dgw]

Note: This is still blocked on #2317 or an equivalent, so we can guarantee as much as possible that existing DBs won't break.

@Exirel Were you still planning to implement a seamless migration?

[Exirel]

Yes. Other things took priority, while this is at the back of my head.

[dgw]

I think we should leave this for 9.0, given that there's been no progress on database migrations in 9 months. Sopel 7.x is getting pretty long in the tooth, and it's really time we tied up our loose ends and started on a release.

Migration requirements:

• it needs to be a sopel command line, allowing to select a different configuration file
• it needs to allow us to generate new migrations that can be packaged
• it needs to allow Sopel's owner to run migrations that was packaged
• it needs to allow plugin's author to generate new migrations for their plugin (in that case, maybe only packaged version?)
• it needs to collect plugin's migrations to run them alongside Sopel's own migrations

Basically, we don't need the default behavior of Alembic, which is to run for a project you 100% control, we need a system closer to what Django's ORM support: generate migrations for your project, package it, then collect all migrations from the project and its installed plugin to run them. And trust me, I know it's not easy, I had to done it once a few years ago and it wasn't fun at all.

As a side note, the migration needs to have a complete downgrade handler, this one is incomplete.

Originally posted by @Exirel in #2317 (comment)