Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

PostgreSQL driver, tests against DB backends, and general drivers cleanup #2723

Merged
merged 33 commits into from Aug 27, 2019

Conversation

Tobotimus
Copy link
Member

@Tobotimus Tobotimus commented May 22, 2019

Yay! The PostgreSQL driver is here. This comes with quite a few changes to the driver system, the data backup/migration/deletion system, and tests. The reason I made so many changes is because I wanted to harmonise backend-agnostic operations rather than add more driver-specific code where it didn't need to be. There are also just a few general cleanups in here too for the other drivers. This includes breaking changes for any dev using the drivers directly, but I think devs who are doing so should expect such breaking changes to occur (it's not exactly an interface we encourage devs to use).

The PostgreSQL driver

  • Uses the (fastest-available) asyncpg driver
  • Each cog has a schema named in the form "cog_name.cog_identifier".
  • Each category has its own table within this schema.
  • When a schema is created, an entry is added to the red_cogs table in the red_config schema, to track which schemas actually contain red data, and to assist with migrations.
  • Each entry in a table contains primary key columns, and a JSONB column, which corresponds to a single document (much like in Mongo).
  • The redbot/core/driver/postgres/ddl.sql contains all of the actual logic behind the driver, mostly in PL/pgSQL. At least we're diversifying our code base 馃槈
  • Implements the inc() and toggle() methods in anticipation for #2635.

Changes to drivers more generally

  • Driver modules have been renamed to remove the unusual and unnecessary red_ prefix. Also, driver classes are now available as attributes to the redbot.core.drivers package, and extra requirements (motor, asyncpg) are imported conditionally.
  • The functions to get drivers have changed in the redbot.core.drivers package:
    • get_driver() now takes the cog name, identifier, driver-specific kwargs (if needed) and an optional storage_type kwarg. If storage_type is omitted, the function will default to data_manager.storage_type().
    • There is also a new function, get_driver_class(), which simply takes an optional storage_type and returns the class for the driver.
  • BaseDriver.get_config_details() is now an abstract staticmethod. With the Mongo driver, for some reason it was a module-level function, and was being called in a non-abstract and non-agnostic way.
  • BaseDriver.initialize(**storage_details) and BaseDriver.teardown() are two new abstract coroutine classmethods to initialise and teardown the driver (db connections, DDL etc). For the JSON driver, these do nothing. BaseDriver.initialize() will raise a new exception, redbot.core.errors.MissingExtraRequirements, when the required extra has not been installed for that driver.
  • The JSON driver no longer requires the data_path_override kwarg to create standard drivers for a settings file in a core/cog data path. The kwarg is still available, however (for unit tests).
  • IdentifierData.custom_group_data is no longer a dict, has been renamed to IdentifierData.primary_key_len and will always be an int (it was actually unclear what it was meant to be beforehand). This simplifies quite a bit of code where the primary key length is needed. Also, ConfigCategory.get_pkey_info() is a new classmethod for the enum which returns a (primary_key_len, is_custom) tuple based on the passed category and custom group data dict.
  • BaseDriver.delete_all_data() is a new coroutine method for deleting all data from the given backend. There is a generic implementation of this in the base driver, but the Mongo and Postgres drivers have overridden methods with extra options (e.g. allowing the data to be deleted by simply dropping the entire database). This replaces the logic which was previously in redbot.setup.remove_instance()
  • BaseDriver.aiter_cogs() is an abstract asynchonous iterator which yields (cog_name, cog_id) tuples which refer to cogs which have data stored on that driver's backend. This is useful for migrations, as discussed below.
  • BaseDriver.migrate_to() is a new coroutine method which is a driver-agnostic (non-abstract) way of migrating data from one backend to another. It makes use of the new BaseDriver.aiter_cogs() method and existing BaseDriver.(import|export)_data() methods. Further discussed below.

Changes to migrations and backups

  • All of the backend-specific code in redbot.setup, except for that regarding MongoV1, has been removed.
  • redbot.core.config.migrate() is a new coroutine function for migrating from one driver to another. It uses BaseDriver.migrate_to() under the hood, but abstracts away the retrieval of custom group data. It is used to migrate data to the JSON driver prior to a backup.
  • The [p]backup command no longer contains driver-specific code
  • For a new driver to be compatible with migrations and backups, it now simply needs to implement the whole BaseDriver ABC.

Unit tests

  • New tox recipes have been added for testing against the mongo and postgres backends. They won't be run unless specified explicitly by the -e option or the TOXENV env var. Options for the driver backend can be modified through environment variables (see tox.ini). Travis now has two extra jobs for these extra tox recipes.
  • tests/conftest.py has been added back in because pytest-asyncio is doing weird shit with the event_loop fixture (see pytest-dev/pytest-asyncio#75). It also contains the important new (autouse) fixture for setting up and tearing down the drivers. Now this may cause issues with 3rd party cog test suites which are making use of our fixtures, but I doubt it.

That's it. I'm sorry it's such a massive PR. But all of these changes ultimately made it much easier to write and verify the new driver.

Signed-off-by: Toby Harradine <tobyharradine@gmail.com>
Tobotimus added 3 commits May 22, 2019
Signed-off-by: Toby Harradine <tobyharradine@gmail.com>
Signed-off-by: Toby Harradine <tobyharradine@gmail.com>
@Tobotimus Tobotimus requested a review from mikeshardmind as a code owner May 22, 2019
@Tobotimus Tobotimus changed the base branch from V3/develop to V3/feature/pimp_my_config May 22, 2019
@mikeshardmind mikeshardmind added this to the 3.2.0 milestone May 23, 2019
Tobotimus added 15 commits May 26, 2019
Most of the logic is now in PL/pgSQL.

This completely avoids the use of Python f-strings to format identifiers into queries. Although an SQL-injection attack would have been impossible anyway (only the owner would have ever had the ability to do that), using PostgreSQL's format() is more reliable for unusual identifiers. Performance-wise, I'm not sure whether this is an improvement, but I highly doubt that it's worse.

Signed-off-by: Toby Harradine <tobyharradine@gmail.com>
Signed-off-by: Toby Harradine <tobyharradine@gmail.com>
Signed-off-by: Toby Harradine <tobyharradine@gmail.com>
Signed-off-by: Toby Harradine <tobyharradine@gmail.com>
Signed-off-by: Toby Harradine <tobyharradine@gmail.com>
Signed-off-by: Toby Harradine <tobyharradine@gmail.com>
Signed-off-by: Toby Harradine <tobyharradine@gmail.com>
Signed-off-by: Toby Harradine <tobyharradine@gmail.com>
Signed-off-by: Toby Harradine <tobyharradine@gmail.com>
Signed-off-by: Toby Harradine <tobyharradine@gmail.com>
Signed-off-by: Toby Harradine <tobyharradine@gmail.com>
Signed-off-by: Toby Harradine <tobyharradine@gmail.com>
Signed-off-by: Toby Harradine <tobyharradine@gmail.com>
Signed-off-by: Toby Harradine <tobyharradine@gmail.com>
Signed-off-by: Toby Harradine <tobyharradine@gmail.com>
@Tobotimus Tobotimus changed the base branch from V3/feature/pimp_my_config to V3/develop Jun 24, 2019
@Tobotimus Tobotimus added Big fella Breaking Change and removed V3 labels Jun 29, 2019
Tobotimus and others added 8 commits Jul 2, 2019
# Conflicts:
#	Pipfile
#	Pipfile.lock
#	redbot/__main__.py
#	redbot/core/config.py
#	redbot/core/core_commands.py
#	redbot/core/drivers/json.py
#	redbot/core/json_io.py
#	redbot/setup.py
#	setup.cfg
Signed-off-by: Toby Harradine <tobyharradine@gmail.com>
Signed-off-by: Toby Harradine <tobyharradine@gmail.com>
Signed-off-by: Toby Harradine <tobyharradine@gmail.com>

# Conflicts:
# redbot/core/config.py
# redbot/core/drivers/red_base.py
Signed-off-by: Toby Harradine <tobyharradine@gmail.com>
mikeshardmind
mikeshardmind previously requested changes Jul 27, 2019
Copy link
Contributor

@mikeshardmind mikeshardmind left a comment

This is not at all a full review, but unless we intend on intentionally allowing users to use the drivers directly, in line with our updated version guarantees, the drivers should be private by convention.

@Tobotimus
Copy link
Member Author

@Tobotimus Tobotimus commented Jul 28, 2019

@mikeshardmind The next PR for Config I do will be reorganising config into a package with the drivers as a private subpackage. I think that should be sufficient

@mikeshardmind mikeshardmind dismissed their stale review Jul 28, 2019

(Dismissing based on future plans)

Copy link
Contributor

@mikeshardmind mikeshardmind left a comment

Appears to work as intended, I still have a few more things to look at before I'm confident enough for a 馃憤 though.

Tobotimus added 3 commits Aug 27, 2019
Signed-off-by: Toby Harradine <tobyharradine@gmail.com>
Signed-off-by: Toby Harradine <tobyharradine@gmail.com>
Signed-off-by: Toby Harradine <tobyharradine@gmail.com>
Copy link
Contributor

@mikeshardmind mikeshardmind left a comment

After the stuff which was brought up in discord was handled (ident based auth support, not requiring being a pg superuser) This is now ready for use.

@mikeshardmind mikeshardmind merged commit d1a46ac into Cog-Creators:V3/develop Aug 27, 2019
1 check passed
@Tobotimus Tobotimus deleted the V3/postgres_driver branch Aug 27, 2019
@jack1142 jack1142 added the Type: Bug label Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Type: Bug Type: Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants