Consolidate Stores

[jvc56]

We should weto this:

return gameplay.TimedOut(ctx, b.gameStore, b.userStore, b.notorietyStore, b.listStatStore, b.tournamentStore, evt.UserId, evt.GameId)

And replace it with something like this:

return gameplay.TimedOut(ctx, gameplayStores, evt.UserId, evt.GameId)

In all places where the stores are getting out of hand.

[andy-k]

stores are getting out of hand, but consolidating them into a struct doesn't solve the problems they're causing.

• having independent stores makes it impossible to write code that performs a transaction across more than one store because game, user, stats, tournament (among others) are ultimately backed by different db connections (ignoring redis). but actually they connect to the same db.
• stores tend to perform deserialization even when not necessary (for example when deleting a user's sought game).
• some stores cache to mitigate the cost of deserialization. but the cached objects also hold locks. by getting a game, then getting many hundreds of other games to overflow the cache, then getting the first game again, it is possible that the last .Get returns an independently deserialized copy of the same game as the first game, with independent locks.
• stores obscure logic bug in code like this one in the tournament store.

but i don't have a good solution to this that doesn't essentially amount to a full rewrite.

[domino14]

These started as different stores because we didn't even add Postgres until several months after we started the project. We used to store games in memory, users were all anonymous, etc. Once we started adding persistence we started creating db stores gradually (while some things remained in memory until much later, like SoughtGame, some things moved to Redis..)

• Consolidating them into a struct will help in keeping function signatures from going crazy and having to change a hundred different ones every time we add a new one.
• Stores performing unnecessary deserializations is a different problem, it's not because it's a store that it does that. We can fix that issue.
• We probably need to greatly minimize the amount of caching that we do, but I wouldn't remove it altogether (for example for GetBriefProfile and other function calls that do benefit from caching)

Some possible solutions:

• Can we just make the DB connection in each store shared? i.e the db *gorm.DB (or its standard lib SQL equivalent). Then we can pass one connection object to all stores
• A rewrite could use CQRS (https://threedots.tech/post/basic-cqrs-in-go/). We can determine which queries we absolutely need, and separate them into write/read. The write ones would all be in the same package (and talk to different tables transactionally as needed).

[andy-k]

• yeah, we can hide the problem in a big struct. it wouldn't be more wrong than they currently are. the only issue is it'd make us less aware of the problem. this one line return gameplay.TimedOut(ctx, b.gameStore, b.userStore, b.notorietyStore, b.listStatStore, b.tournamentStore, evt.UserId, evt.GameId) implies that gameplay.TimedOut cannot possibly be transactionally correct. the more ergonomic line hides this fact.
  • if we implement this anyway, we'd probably have allStores instead of just gameplayStores. either way, we'll have to always defensively instantiate the whole set since it's not going to be clear which stores are required by any particular code path.
  • maybe we can even hide allStores into ctx. then nobody knows which functions touch the db.
• stores performing unnecessary deserializations is a direct consequence of having stores at all, because in trying to be generic enough to serve any current and future callers, the callers are not being made aware of what happens behind the scenes. different callers rely on different parts of the behavior (for example the aforementioned tournament store logic that depended on .Get having a cache) and then each function in the store accumulates responsibilities that are not relevant to some of its callers. (but yes, it is a different problem from methods taking many stores being wordy.)
• i don't think we have proven that GetBriefProfile benefits from caching. but i'm fine with GetBriefProfile's cache, because it's readonly, updates happen separately and would just invalidate the record instead of also being updated non-transactionally, the cache is time-bound rather than size-bound, there's no behavior/locks in there, and the read is for user consumption (it doesn't affect updates). the other caches we have tend to be full-blown entity objects that can be evicted unexpectedly, come with locks, and they are sometimes updated in-place and may go out of sync if the db fails. it's those caches that are problematic.
• yeah we should have a single DB object. and migrate away from automigrate.
  • maybe the stores shouldn't own DB.
  • store functions should take Tx (not DB) as argument so that the caller can compose. but that alone may not be good enough because for update is situational.
• CQRS seems to be inappropriate, we don't want eventual consistency and we do need things like "update ... returning" (in more places than we currently do) instead of what it recommends "call the command; when it succeeds, call the query" (in a separate transaction of course, during which time other changes may have taken place). read queries may also benefit from joins rather than separate queries. or maybe i'm misunderstanding it.

[jvc56]

This is blocking the migration of action histories, which just adds more tech debt to this problem. I think we should make this a more urgent priority, even if that amounts to a complete rewrite.

[domino14]

There is a

type Stores struct {
    UserStore       user.Store
    GameStore       gameplay.GameStore
    SoughtGameStore gameplay.SoughtGameStore
    PresenceStore   user.PresenceStore
    ChatStore       user.ChatStore
    ListStatStore   stats.ListStatStore
    NotorietyStore  mod.NotorietyStore
    TournamentStore tournament.TournamentStore
    ConfigStore     config.ConfigStore
    SessionStore    sessions.SessionStore
}

in bus.go that gets passed around a few places. Can we add whatever you need there and pass it to more places? If we do that though, I may want to pass a pointer around.