db: ordered migrations, user_items table, and shared inventory helpers - #32
Open
Seltraeh wants to merge 5 commits into
Open
db: ordered migrations, user_items table, and shared inventory helpers#32Seltraeh wants to merge 5 commits into
Seltraeh wants to merge 5 commits into
Conversation
Migrations previously lived in an unordered_map and therefore ran in hash order. They are now a vector so they run in declaration order, with a startup guard that aborts on a duplicate migration name to preserve the uniqueness the map gave for free. Adds 02072026_ExtendUserUnitsForUnitOps: 26 additive columns on user_units covering per-unit stats, the two sphere equipment slots and the favourite flag. This consolidates three earlier fork migrations onto the upstream table shape. Upstream columns (unit_lvl, base_rec, bb_*) remain the source of truth for upstream handlers; these serve handlers not yet moved to PacketInterface and are intended to be consolidated away as each one moves. Adds 05072026_CreateUserItemsTable: one row per item stack, keyed UNIQUE(user_id, item_id), with instance_id as the warehouse row id the client references. Common.hpp gains two helpers. addUserItem() upserts a stack, incrementing item_num when the species is already owned. returnEquippedSpheres() reads the sphere slots off units that are about to be deleted and credits them back to the warehouse; it must be called before the DELETE in any handler that consumes units, or the spheres are destroyed silently.
Tom2096
reviewed
Aug 6, 2026
| // shape. Upstream columns (unit_lvl/base_rec/ext_rec/bb_*) remain the | ||
| // source of truth for upstream handlers; these serve the not-yet-ported | ||
| // quests handlers and are consolidated away as each moves to | ||
| // PacketInterface. |
Collaborator
There was a problem hiding this comment.
I dont understand why this exists, a lot of it seems like duplicates of user units.
Tom2096
reviewed
Aug 6, 2026
Tom2096
reviewed
Aug 6, 2026
| .update = true, | ||
| .insert = true, | ||
| }), | ||
| // Extras the read/display path was missing — mapped to the quests mirror |
Collaborator
There was a problem hiding this comment.
i actually dont agree with adding everything here - it introduces bloat
for example, do we really need to add fe_bp? or leader_skill_id?
for each of these fields, we should only add them under 2 conditiions (this applies to every single other field as well)
- we understand fully what it does in the client
- the client needs it either for game functionailty or it crashes
Tom2096
reviewed
Aug 6, 2026
Tom2096
reviewed
Aug 6, 2026
Tom2096
reviewed
Aug 6, 2026
| } | ||
|
|
||
| /*! | ||
| * Returns any spheres equipped on soon-to-be-consumed units to the owner's |
Collaborator
There was a problem hiding this comment.
I dont understand the mechanism here - does the client delete the sphere if we fuse the unit or sell the unit?
Contributor
Author
There was a problem hiding this comment.
Yes, we have to free up the sphere attached to a unit prior to it's destruction by any means.
Addresses the review comments on this PR ("dont use co_await
database->execSqlCoro(, use the existing API"). Both flagged call sites used
SQL the typed interface could not express, which is why they bypassed it, so
this extends the interface instead of routing around it.
* DatabaseInterface::upsert(db, table, cells, conflict, accumulate)
INSERT ... ON CONFLICT(...) DO UPDATE. Columns in `accumulate` are added
to (col = col + excluded.col), the rest replaced. Plain insert only ever
ignored conflicts, so addUserItem's "add to the stack I already own" had
no expressible form.
* db::LookupIn(name, values) -> WHERE col IN ($1, $2, ...), one placeholder
per value; an empty list throws rather than letting a caller splice ids
into SQL. read/update/remove now share a single buildWhere() that handles
equality and IN with correct placeholder numbering.
addUserItem and returnEquippedSpheres now go through the interface.
Verified past compiling: the emitted statements were run against a copy of
deploy/gme.sqlite. Two grants of 3 then 4 leave item_num = 7, confirming the
conflict clause accumulates rather than replaces.
Two deliberate leftovers:
* addDefaultDecks still uses execSqlCoro. It is a recursive CTE that
generates ten deck rows in one INSERT...SELECT; the interface is
row-oriented and cannot express it. Converting it would mean ten
round trips to satisfy a rule, which is worse. Flagging rather than
hiding it.
* returnEquippedSpheres still takes the comma-joined id string its callers
build for their own DELETEs and splits it back into bound values. That
round trip disappears when those DELETEs move onto the interface.
Addresses both schema comments on this PR -- "a lot of it seems like duplicates of user units" on the 02072026 migration, and "why the rename?" on PacketInterfaceSchemas mapping ext_rec to "ext_heal". Same root cause. The client names the same stat differently in different packets: UserUnitInfo says base_rec / ext_rec / unit_lvl, while FriendInfo, ReinforcementInfo, FixedReinforcementInfo and UnitReinforceEntry say base_heal / ext_heal / add_heal / unit_lv. Both spellings are the client's, carried from IDA (see net/friends.kdl, whose field docs cite vtable offsets). A column was added per SPELLING, so one unit could hold two recovery values free to disagree. Now one column per concept. rec/lvl wins because 08032025_CreateUserUnitsTable established it. PacketInterfaceFor<T> maps either packet vocabulary onto the one column, which is what that indirection is for -- so a field<> whose packet and column names differ is correct here, not a smell. Packet field names are untouched; they are the client's, not ours to normalise. 06082026_ConsolidateUserUnitStatColumns merges duplicate values into the canonical column, DROPs unit_lv/base_heal/ext_heal and RENAMEs add_heal -> add_rec, limit_over_heal -> limit_over_rec. 02072026 is deliberately left as written, with a comment pointing forward: applied migrations are recorded by name, so editing it would change nothing for existing databases while diverging fresh ones. Merge rule: the duplicate wins only where the canonical column is still at its default, since quests-branch handlers wrote duplicates while upstream handlers wrote canonicals. Verified against a real database before writing it -- 0 rows held conflicting values and all 83 carried their value in a column being dropped, so the merge was lossless. After migrating: 44 columns -> 41, all 83 rows kept their level. Call sites were rewritten only inside quoted strings, since these identifiers appear as both a SQL column and a generated packet field, sometimes on one line (rd.base_heal = br["base_heal"]). A blanket find/replace would have silently broken four generated structs. The matching handler changes land on the branches that own those files: UnitEvo/UnitMix on split/05-units, CampaignBattleStart on split/10-campaign.
Applies the review's own test -- add a field only when we understand what it does AND the client demonstrably needs it -- to the two fields the review named. Both fail it. fe_bp and fe_max_usable_bp were INSERTed as the literal constants 100 and 200, read straight back into the packet, and never computed from or consumed by any handler; Frontier Evolution is not implemented. The KDL fields stay, so the client still receives both keys (defaulting to 0); only the per-user persistence goes. Re-add them alongside the subsystem that gives them meaning, when the stored values will mean something. 06082026_DropUnusedFeBpColumns removes the columns. 02072026 keeps its ADD lines so databases that already ran it and fresh ones converge, same pattern as the consolidation migration above it. Also drops a duplicate base_rec from the addUserUnit column list, left over from the rec/heal consolidation. SQLite tolerates a repeated column in an INSERT list and both values were the same bind, so nothing was written incorrectly -- the list just no longer matched intent. Verified on a real database: user_units 41 -> 39 columns, all 83 rows intact, server boots. Not dropped despite looking constant in a dev save: eqip_item_* (spheres -- returnEquippedSpheres depends on them), add_*/ext_*/limit_over_* (enhancement state, merely unexercised), user_id. A single-user database makes unused instance state look derivable, so column variance is a prompt to check a field's meaning rather than evidence on its own. The matching handler changes are on split/05-units.
Completes the field-bloat comment, which named fe_bp and leader_skill_id. leader_skill_id is species data -- every copy of a unit has the same leader skill -- so it belongs to UnitMst keyed by unit_id, not to a per-user row. UnitEvo already sourced it from targetMst; UnitMix was the only reader of the stored copy and now takes it from the MST too (it already held the handle for element/exp_pattern/max_lv, so no extra lookup). The packet field stays, so the client still receives the key. Only the redundant copy goes, removing one more place the database can drift from the MST. Verified on a real database: user_units 39 -> 38 columns, 83 rows intact, server boots. UserUnitInfo persistence is now 34 of 47 packet fields, down from 37. The matching UnitMix/UnitEvo changes are on split/05-units.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
db: ordered migrations, user_items table, and shared inventory helpers
Branch:
split/03-schema-foundationBase:
devMerge position: 03 of 13
Three files, but the highest-risk change in the whole series. Everything downstream depends on these table shapes.
What's included
Migration ordering. Migrations lived in an
unordered_mapand therefore ran in hash order. They are now avector, running in declaration order. Because a vector does not dedup, a startup guard aborts on a duplicate migration name, preserving the uniqueness the map gave for free.02072026_ExtendUserUnitsForUnitOps— 26 additive columns onuser_units: per-unit stats, two sphere equipment slots, and the favourite flag. Consolidates three earlier fork migrations onto the upstream table shape.05072026_CreateUserItemsTable— one row per item stack,UNIQUE(user_id, item_id), withinstance_idas the warehouse row id the client references.Structural tables for the later PRs — town, campaign, scenario and summon-ticket tables land here too, so
MigrationManager.cppis touched once rather than by five branches. All areCREATE TABLE IF NOT EXISTS, structural only, never seeded. Say the word if you'd rather each subsystem carried its own and I'll redistribute.Common.hpphelpersaddUserItem()— upserts a stack, incrementingitem_numwhen the species is already owned.returnEquippedSpheres()— reads sphere slots off units about to be deleted and credits them back to the warehouse. Must be called before the DELETE in any handler that consumes units.Known debt, flagging deliberately
The 26
user_unitscolumns duplicate upstream'sunit_lvl/base_rec/bb_*. Upstream columns remain the source of truth for upstream handlers; these serve handlers not yet onPacketInterface. The intent is to consolidate them away as each handler moves. If you'd rather that port happened before any of this merges, that's a reasonable call and I'd like to know now rather than later.Verification
Fresh-DB boot runs every migration in declared order; second boot skips recorded ones. Duplicate-name guard tested by temporarily doubling an entry.