db: adding multi database support #1446

RustyBower · 2019-01-13T07:01:27Z

This should fix #736.

This PR migrates from direct SQLite SQL statements to using the SQLAlchemy ORM. This will allow users to choose whether they want to stick with SQLite (Default), or leverage another database engine.

I've added a few more variables to core to handle non-SQLite databases: db_type, db_user, db_pass, db_host, and db_name. If db_type is not specified, or is specified with sqlite, the other values do not need to be specified. If db_type is specified with MySQL, users will need to specify the other parameters.

We could probably move the Models into their own model.py to clean things up a bit, but I will leave that to your discretion @dgw

cerealcable

Thanks for the work @RustyBower, just a few comments ;)

sopel/db.py

cerealcable · 2019-01-13T15:19:09Z

sopel/config/core_section.py

@@ -96,8 +96,23 @@ class CoreSection(StaticSection):
    channels = ListAttribute('channels')
    """List of channels for the bot to join when it connects"""

+    db_type = ValidatedAttribute('db_type')
+    """The type of database to use for Sopel's database. (SQLite or MySQL)"""


Can we simply refer to sqlalchemy docs instead of limiting choices?

I'd love for this to be a ChoiceAttribute, for the automatic error message Sopel would spit out if configured to an unknown value, but that would certainly wind up biting us at some point when SQLAlchemy adds support for some newfangled database type that Sopel doesn't know about…

That's probably going to be infrequent though and a minor PR to address, that seems like a reasonable choice. Downside to this is that it limits usability in non-standard databases. There are some that ship supported in SQLAlchemy and others that are through external packages.

The docs for what Dialects (databases) SQLAlchemy supports (as in listed on their website): https://docs.sqlalchemy.org/en/latest/dialects/index.html

After thinking about it, given that SQLAlchemy could have any database plugin written and then consumed since anyone could write a new Dialect for SQLAlchemy, it'd probably be best to not limit this by using a ChoiceAttribute. I think warning when a connection couldn't be made instead is best.

I've made this a ChoiceAttribute for now. It looks like there aren't that many dialects to deal with (7), and honestly people only probably use 4 of those (sqlite, mysql, postgres, and mssql).

The biggest question is how we are going to handle dependencies for these, since mysql requires pymysql, etc. Is that something that should be documented or should the PyMySQL dependency be included in requirements (which would also need to include the dependencies for all other database dialects, and that feels bad)

Dependencies for the various dialects should be left to documentation, IMO. Adding them all to requirements would partially defeat the purpose of making the backend a choice, because the point is to use SQLite (from Python's standard library) unless the user specifically sets it up otherwise. And to do that, they should need to read some documentation. 😸

I like it. I've added some of the basic stuff into coretasks, but I will definitely update README with some more of the different db backend requirements.

RustyBower · 2019-01-14T07:55:10Z

Just finished testing. MSSQL, MySQL, Postgres, and SQLite all work.

I am unable to test Oracle because apparently you need to pay for a license. I haven't tested the last 2 (firebird or sybase).

I don't love having String(255) everywhere, but I'm unable to just have String, since it causes MySQL to explode. I'm still looking at other options.

dgw

Gave this a preliminary once-over. Nothing major, but a few things did jump out.

(Some stuff was already fixed after @cerealcable's review, so 👍 for that!)

sopel/db.py

sopel/config/core_section.py

sopel/db.py

All addressed; need to re-review after testing for real.

dgw · 2019-03-21T10:19:41Z

@RustyBower I think you messed up this PR the other day, like you did with the reddit plugin one. Review conversations that were marked as resolved are missing their resolutions in the current PR state.

Looks like 2c0b884 was the last good state.

dgw · 2019-03-22T00:58:58Z

@RustyBower Still broken compared to before. A lot of issues that were fixed as of 2c0b884 are present again in the current state, because some commits are missing.

Merging upstream into a feature branch is a bad idea. I suggest doing a git reset --hard 2c0b884 and rebasing that onto master, fixing the conflicts manually. (Something I can obviously do myself, but if you learn about rebasing yourself it's a helpful skill to have. 😸)

RustyBower · 2019-03-22T15:59:37Z

This should be all set. I just triple checked with various databases, and it all works 👍

Exirel

Damn, naming things is really hard.

sopel/db.py

sopel/config/core_section.py

sopel/db.py

Exirel · 2019-03-25T22:39:32Z

@RustyBower almost done! There is this generic Exception raised that I would like to see replaced by a more-specific one.

Exirel

In sopel.db, it appears that you did not import sopel.config.

While you are at it, could you replace from sopel.tools import Identifier by this:

from sopel import config, tools

and then use tools.Identifier and config.ConfigurationError when needed?

sopel/db.py

RustyBower · 2019-03-27T22:39:32Z

Squashed all the commits, should be ready for the big show now

dgw · 2019-03-28T20:14:49Z

Well, I approuve, as long as everything's green. 😛

Too bad everything's not green. There's an undefined name, that DatabaseConfigurationError you wanted so much… (Not that I disagree about making the exception specific!)

dgw

The good news is, my comments fall squarely in the "nitpick" category. 😃

Bad news is, you're still not done force-pushing this PR, because at least a couple of them are actually important nitpicks. 😦

(But I suggest you address these in new commits and rebase after the PR is approved—sound good?)

sopel/db.py

sopel/config/core_section.py

dgw

This is 👍 pending me getting a chance to put it through some paces on an actual instance (though I shouldn't need to if the DB tests pass, right? 😛)

Future plans: Documentation updates to the README, and possibly adding setuptools extras for the different DB options.

deathbybandaid · 2019-04-18T00:14:44Z

Very excited for this, will bot owners have any way to migrate existing sqlite content to another database?

RustyBower · 2019-04-18T00:16:59Z

Not currently, but there exist some tools like https://github.com/seanharr11/etlalchemy that can do the migrations between any of the now supported DBs

dgw · 2019-04-18T00:55:03Z

Not currently

Or ever, probably. I don't think it's Sopel's responsibility to offer migration from one database back-end to another. Existing users have numerous options from which to choose if they want to move away from SQLite, just like they had their choice of tools for moving to SQLite when Willie 5.0 came out.

If it "wasn't our problem" when dropping support for other database engines, it certainly "isn't our problem" when adding (back) support for engines other than SQLite while still keeping SQLite support.

deathbybandaid · 2019-04-18T01:21:45Z

It's still good to note that there is a solution though.

Exirel · 2019-05-08T12:19:33Z

I think this PR is ready to be merged. We should then improve the following points:

documentation,
driver support,
tests,

which can be all done in future PR.

dgw · 2019-05-10T04:29:57Z

Assigning to @RustyBower as a reminder. We agreed on IRC that I'll merge this once some rudimentary documentation on the more popular drivers is in place.

Basic docs (in the readme here and in a section at https://sopel.chat/usage/installing/ — a separate PR to sopel-irc/sopel.chat) should cover sqlite, mysql, and pgsql at minimum. Anything more obscure can wait, I think. Not many of us would want to buy, say, an Oracle license just to document it. 😁

README.rst

RustyBower · 2019-05-12T10:49:48Z

Should be all set with README configuration

RustyBower force-pushed the multi_db_support branch 5 times, most recently from 0890294 to 80cab9e Compare January 13, 2019 07:31

dgw added this to the 7.0.0 milestone Jan 13, 2019

dgw added the Feature label Jan 13, 2019

cerealcable reviewed Jan 13, 2019

View reviewed changes

dgw mentioned this pull request Jan 13, 2019

OperationalError: database is locked #736

Closed

RustyBower force-pushed the multi_db_support branch 4 times, most recently from c4a576d to 4b8ddc5 Compare January 14, 2019 02:35

dgw previously requested changes Feb 8, 2019

View reviewed changes

sopel/db.py Outdated Show resolved Hide resolved

sopel/config/core_section.py Outdated Show resolved Hide resolved

sopel/db.py Outdated Show resolved Hide resolved

RustyBower force-pushed the multi_db_support branch from e58d6f3 to 2c0b884 Compare February 8, 2019 13:24

RustyBower force-pushed the multi_db_support branch from 2c0b884 to fa5bb2b Compare March 16, 2019 08:20

dgw mentioned this pull request Mar 21, 2019

Adding plugin-based key-value store to SopelDB #1501

Closed

RustyBower force-pushed the multi_db_support branch 2 times, most recently from e57aa7f to 2c0b884 Compare March 22, 2019 01:56

Exirel requested changes Mar 25, 2019

View reviewed changes

Exirel reviewed Mar 25, 2019

View reviewed changes

sopel/db.py Show resolved Hide resolved

Exirel requested changes Mar 25, 2019

View reviewed changes

sopel/db.py Outdated Show resolved Hide resolved

sopel/db.py Outdated Show resolved Hide resolved

Exirel approved these changes Mar 26, 2019

View reviewed changes

RustyBower force-pushed the multi_db_support branch from c2afc79 to 8c2d8bc Compare March 28, 2019 20:24

dgw requested changes Mar 28, 2019

View reviewed changes

sopel/db.py Show resolved Hide resolved

sopel/db.py Outdated Show resolved Hide resolved

sopel/db.py Outdated Show resolved Hide resolved

sopel/config/core_section.py Show resolved Hide resolved

sopel/config/core_section.py Show resolved Hide resolved

Exirel mentioned this pull request Mar 29, 2019

db: add methods to delete a channel or nick's key from database #1526

Merged

RustyBower force-pushed the multi_db_support branch 2 times, most recently from e43a475 to b922cfb Compare April 1, 2019 16:40

dgw approved these changes Apr 1, 2019

View reviewed changes

dgw self-assigned this Apr 10, 2019

Exirel added the High Priority label May 8, 2019

Exirel mentioned this pull request May 8, 2019

db: make database path relative to config.core.homedir #1574

Merged

dgw assigned RustyBower and unassigned dgw May 10, 2019

db: adding multi database support

efd345b

RustyBower force-pushed the multi_db_support branch from b922cfb to efd345b Compare May 12, 2019 05:09

dgw reviewed May 12, 2019

View reviewed changes

README.rst Outdated Show resolved Hide resolved

RustyBower force-pushed the multi_db_support branch from 60a74a5 to 8a78393 Compare May 12, 2019 08:03

readme: add database configuration info

00ebf26

RustyBower force-pushed the multi_db_support branch from 8a78393 to 00ebf26 Compare May 12, 2019 10:49

dgw unassigned RustyBower May 12, 2019

dgw merged commit adc2fe7 into sopel-irc:master May 12, 2019

dgw mentioned this pull request May 30, 2022

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

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

db: adding multi database support #1446

db: adding multi database support #1446

RustyBower commented Jan 13, 2019 •

edited by dgw

Loading

cerealcable left a comment

cerealcable Jan 13, 2019

dgw Jan 13, 2019

cerealcable Jan 13, 2019

RustyBower Jan 14, 2019

dgw Jan 14, 2019

RustyBower Jan 14, 2019

RustyBower commented Jan 14, 2019 •

edited

Loading

dgw left a comment •

edited

Loading

dgw commented Mar 21, 2019

dgw commented Mar 22, 2019

RustyBower commented Mar 22, 2019

Exirel left a comment

Exirel commented Mar 25, 2019

Exirel left a comment

RustyBower commented Mar 27, 2019

dgw commented Mar 28, 2019

dgw left a comment

dgw left a comment

deathbybandaid commented Apr 18, 2019

RustyBower commented Apr 18, 2019 •

edited

Loading

dgw commented Apr 18, 2019

deathbybandaid commented Apr 18, 2019

Exirel commented May 8, 2019

dgw commented May 10, 2019 •

edited

Loading

RustyBower commented May 12, 2019

db: adding multi database support #1446

db: adding multi database support #1446

Conversation

RustyBower commented Jan 13, 2019 • edited by dgw Loading

cerealcable left a comment

Choose a reason for hiding this comment

cerealcable Jan 13, 2019

Choose a reason for hiding this comment

dgw Jan 13, 2019

Choose a reason for hiding this comment

cerealcable Jan 13, 2019

Choose a reason for hiding this comment

RustyBower Jan 14, 2019

Choose a reason for hiding this comment

dgw Jan 14, 2019

Choose a reason for hiding this comment

RustyBower Jan 14, 2019

Choose a reason for hiding this comment

RustyBower commented Jan 14, 2019 • edited Loading

dgw left a comment • edited Loading

Choose a reason for hiding this comment

dgw commented Mar 21, 2019

dgw commented Mar 22, 2019

RustyBower commented Mar 22, 2019

Exirel left a comment

Choose a reason for hiding this comment

Exirel commented Mar 25, 2019

Exirel left a comment

Choose a reason for hiding this comment

RustyBower commented Mar 27, 2019

dgw commented Mar 28, 2019

dgw left a comment

Choose a reason for hiding this comment

dgw left a comment

Choose a reason for hiding this comment

deathbybandaid commented Apr 18, 2019

RustyBower commented Apr 18, 2019 • edited Loading

dgw commented Apr 18, 2019

deathbybandaid commented Apr 18, 2019

Exirel commented May 8, 2019

dgw commented May 10, 2019 • edited Loading

RustyBower commented May 12, 2019

RustyBower commented Jan 13, 2019 •

edited by dgw

Loading

RustyBower commented Jan 14, 2019 •

edited

Loading

dgw left a comment •

edited

Loading

RustyBower commented Apr 18, 2019 •

edited

Loading

dgw commented May 10, 2019 •

edited

Loading