search

Couchers-org / couchers

The next-generation couch surfing platform. Free forever. Community‑led. Non‑profit. Modern. Chuck us a star :)
https://couchers.org
MIT License
375 stars 77 forks source link

SQLAlchemy 2.0 migration/refactor #903

Open aapeliv opened 3 years ago

aapeliv commented 3 years ago

SQLAlchemy is making some large changes from 1.3 to 1.4 (a kind of 2.0 migration release). This is causing a bunch of annoying warnings in the console, and we should in general refactor how database queries are done. I.e. read the SQLA docs about the recommended patterns, decide how much to rely on the ORM vs Core and then standardize across the codebase.

ShmuelTreiger commented 3 years ago

Warnings taken care of; reading docs, standardizing, and refactoring have not.

ShmuelTreiger commented 3 years ago

Here's the guideline for migrating to SQLA 2.0*: https://docs.sqlalchemy.org/en/14/changelog/migration_20.html#migration-core-connection-transaction

I'm sure there's more to be done. In particular, I think some of the new/changed autocommit stuff may effect us.

There's also this warning message when you run tests, but I don't understand locations stuff well enough to solve (the other warning message seems safe to ignore):

/couchers/couchers/app/backend/src/couchers/models.py:277: SAWarning: Multiple rows returned with uselist=False for lazily-loaded attribute 'User.timezone_area' return self.timezone_area.tzid if self.timezone_area else "Etc/UTC"

That being said, I think I've done what I can here. Someone else is welcome to take over.

*The steps above the specific link location have 100% been taken care of, excepting the warning included here.

aapeliv commented 3 years ago

There's also this warning message when you run tests, but I don't understand locations stuff well enough to solve (the other warning message seems safe to ignore):

I think that's only in the test using test data which has overlapping timezone areas. Shouldn't be an issue in prod.

ShmuelTreiger commented 3 years ago

That makes sense. Should we adjust the test? Seems weird we run a test that could never be in prod?

aapeliv commented 3 years ago

It would be good to update the sample timezone regions, but it's a fair bit of effort. Basically there is a UTC region that covers the whole world, and then a few low-poly regions that I've drawn just to have some more real ones. It would require cutting out those low-poly regions from the UTC polygon.

ShmuelTreiger commented 3 years ago

Low priority issue?

aapeliv commented 3 years ago

Yep