osuAkatsuki / bancho.py

An osu! server for the generic public, optimized for maintainability in modern python
https://akatsuki.gg
MIT License
201 stars 125 forks source link

[v5.1.0] Refactor the rest of the repositories to use sqlalchemy core 1.4 #632

Closed cmyui closed 4 months ago

cmyui commented 4 months ago

Co-authored-by: James Wilson tsunyoku@users.noreply.github.com

Describe your changes

Related Issues / Projects

Resolves #394

Checklist

tsunyoku commented 4 months ago

@cmyui should we do alembic stuff in this PR or another one? if the latter, i want to merge this into master

tsunyoku commented 4 months ago

also, i was thinking we could create a database adapter that wraps around databases.Database to take a ClauseElement and we do the query compilation stuff there so that we're not doing compiled = stmt.compile(dialect=DIALECT), str(compiled) and compiled.params everywhere

cmyui commented 4 months ago

should we do alembic stuff in this PR or another one? if the latter, i want to merge this into master

I'm confident there are quite a few bugs in this so I'd like to diff against the generated alembic migrations first. I think a new alembic branch can be based on this; that way we can treat them separately, and merge this without necessarily merging the alembic work. Easy to change alembics base to master if we merge this (in fact, I think github may do so automatically).

cmyui commented 4 months ago

also, i was thinking we could create a database adapter that wraps around databases.Database to take a ClauseElement and we do the query compilation stuff there so that we're not doing compiled = stmt.compile(dialect=DIALECT), str(compiled) and compiled.params everywhere

Yup, that's a good idea. It seemed very error prone since stmt alone will be accepted by the db functions -- very easy to miss one.

tsunyoku commented 4 months ago

sorry for the loaded commit - was unavoidable. latest commit adds a wrapper around databases as discussed above, and additionally ensures all of our queries are compatible with sqlalchemy 2.0 (all of the selects needed a slight syntax change)

squashed quite a few bugs in the process; there were a couple random ones but the majority were the sqlalchemy statements being incorrectly passed to the database

in the future i want to refactor the database wrapper to be strict and only accept ClauseElement rather than ClauseElement | str however until every query is using sqlalchemy this isn't possible.

cmyui commented 4 months ago

Alright did my most thorough manual testing in a while on this PR & I think I've caught the major bugs. It still likely has some diffs in terms of the existing db schema vs. what this would generate, but I don't think anything major -- just things like incorrect server_default. I'll tackle those in an upcoming PR, but from a runtime perspective this looks to be working as expected for all major flows (score fetch, submit, multiplayer, spectator, etc.) and commands.

cmyui commented 4 months ago

Ty for your support on this PR @tsunyoku!! Much appreciated 🎉🎉🎉