Modernize Database layer

[tdesveaux]

As per #7385 , I'm adding type information here and there as I work on other things.

One thing I'm noticing is that the database models do not have mapped python classes to interact, which would help typing. Is that by choice or a legacy implementation?

If I understand the code correctly, database data is passed around as dict which then need tests to validate that the data is as expected with Validator (defined here)?

Some things that could be done from the top of my head:

• require sqlalchemy >=1.4 (first released on march 2021) which is the version made to be a transition point to SQLAlchemy 2.0 (this would allow to fix deprecation warnings ignore here)
• Implement DB models as mapped classes (or make dataclasses first, then make these dataclasses mapped afterward using Hybrid Declaration)

Also, it seems DB queries are run from a threadpool, an obvious reason is that queries are synchronous. If there is no other reason, SQLAlchemy 1.4.3 (March 2021) introduced asyncio support at a beta level (although there is bugfixes as recently as a critical one in 1.4.51 (Jan 2024), so there is reason to hold off on it.

Here is as far as I got looking into it for now. As I don't have a good understanding of Buildbot's internals nor history, I wanted to get maintainers opinion on this. I'm willing to submit PRs on this (hopefully, finding a way to make them as small as possible).

[p12tic]

I think what you wrote is mostly correct.

Dataclasses is a welcome addition.

One problem with asyncio is that Twisted doesn't work well with it and needs hacks, at least the last time I checked. Also, last time I checked there's no way to write asyncio tests that mock the clock. So for now I would like to not introduce asyncio as much as possible.

So about your suggestions:

require sqlalchemy >=1.4 (first released on march 2021) which is the version made to be a transition point to SQLAlchemy 2.0 (this would allow to fix deprecation warnings ignore here)

Seems great, please go ahead.

Implement DB models as mapped classes (or make dataclasses first, then make these dataclasses mapped afterward using Hybrid Declaration)

Also seems great. I think what you did in #7598 is the way to go. I just need to think a bit about migrations because this is part of public API. Is it possible to override [] operator on a dataclass and provide an interface just like a dict? This way any existing third-party code would still work albeit with a lot of warnings.

[tdesveaux]

Is it possible to override [] operator on a dataclass and provide an interface just like a dict? This way any existing third-party code would still work albeit with a lot of warnings.

Absolutely. In fact, it's what I did in the beginning before removing it in the last commit.
It's a simple:

def __getitem__(self, key: str):
        if hasattr(self, key):
            return getattr(self, key)
        raise KeyError(key)

I'll remove the commit, and maybe add a deprecation warning?

As for the AsyncEngine, I think it's perfectly fine to postpone this. Current implementation using threads is perfectly fine, so using async would be of little gain.

[p12tic]

I'll remove the commit, and maybe add a deprecation warning?

Yes, please do.