Versioned Protos with Data Migration #866
Merged
Conversation
This file contains 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
clustered together. The primary purpose of this change is to create a test case for a "trivial but breaking" proto change where sim behavior is identical to before, but where local storage data and old sim links are completely broken due to incorrectly indexed stats arrays. However, once the proto versioning functionality that this change will test is implemented, it will result in a cleaner and more readable grouping of stats for future core work. On branch proto_versioning Changes to be committed: modified: assets/database/db.bin modified: assets/database/db.json modified: assets/database/leftover_db.bin modified: assets/database/leftover_db.json modified: proto/common.proto modified: sim/core/stats/stats.go modified: sim/death_knight/blood/TestBlood.results modified: sim/death_knight/frost/TestFrost.results modified: sim/death_knight/unholy/TestUnholy.results modified: sim/druid/feral/TestFeral.results modified: sim/druid/guardian/TestGuardian.results modified: sim/hunter/beast_mastery/TestBM.results modified: sim/hunter/marksmanship/TestMM.results modified: sim/hunter/survival/TestSV.results modified: sim/mage/arcane/TestArcane.results modified: sim/mage/fire/TestFire.results modified: sim/paladin/retribution/TestRetribution.results modified: sim/priest/shadow/TestShadow.results modified: sim/rogue/assassination/TestAssassination.results modified: sim/rogue/combat/TestCombat.results modified: sim/rogue/subtlety/TestSubtlety.results modified: sim/shaman/elemental/TestElemental.results modified: sim/shaman/enhancement/TestEnhancement.results modified: sim/warlock/affliction/TestAffliction.results modified: sim/warlock/demonology/TestDemonology.results modified: sim/warlock/destruction/TestDestruction.results modified: sim/warrior/arms/TestArms.results modified: sim/warrior/fury/TestFury.results modified: sim/warrior/protection/TestProtectionWarrior.results
current version, and applied them to the API change caused by the previous stats re-ordering. On branch proto_versioning Changes to be committed: modified: proto/common.proto modified: ui/core/constants/other.ts modified: ui/core/encounter.ts modified: ui/core/proto_utils/stats.ts modified: ui/core/proto_utils/utils.ts
file itself via a custom option. This allows both front-end and back-end code to automatically access the current proto version, rather than needing to manually increment two constants in the two codebases. On branch proto_versioning Changes to be committed: modified: proto/common.proto modified: ui/core/constants/other.ts
version from the custom option field. Modified back-end code that creates UnitStats protos to also populate the CurrentApiVersion field using this utility. This ensures that UnitStats protos delivered back to the UI from character stats and stat weights requests are correctly versioned, so that the front-end does not attempt to up-convert them. On branch proto_versioning Changes to be committed: modified: sim/core/character.go modified: sim/core/statweight.go modified: sim/core/utils.go
apiVersion field, leading to incorrect migration of the default target stats. On branch proto_versioning Changes to be committed: modified: ui/core/encounter.ts
NerdEgghead
requested review from
rosenrusinov,
kayla-glick,
ToxicKevinFerm and
1337LutZ
1337LutZ
reviewed
Jul 23, 2024
1337LutZ
reviewed
Jul 23, 2024
1337LutZ
approved these changes
Jul 23, 2024
There was a problem hiding this comment.
Very nice! This is how I had it in mind as well!
Only question I have is;
Should we also try/catch these migrations? And if failed > reset to defaults and show a Toast that we did so?
One idea:
Would be nice to have a test that will compare current and previous proto during build, and see if the api version was changed if there's other changes in the proto and fail the build.
methods, to clarify that these methods manipulate the existing object in the current iteration. If future data migrations require the creation of new objects to work properly, then return values can be added at that point. On branch proto_versioning Changes to be committed: modified: ui/core/encounter.ts modified: ui/core/proto_utils/stats.ts
detect breaking proto changes and enforce version number increases when appropriate. On branch proto_versioning Changes to be committed: new file: buf.yaml modified: package-lock.json modified: package.json modified: proto/common.proto new file: sim/core/TestProtoVersioning.results new file: sim/core/proto_test.go
|
Looks great, and easy to extend with new stuff! |
since this file is now automatically created/updated during CI. On branch proto_versioning Changes to be committed: modified: sim/core/proto_test.go
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR aims to solve a longstanding issue with our codebase, where changes to the size, indexing, or interpretation of protos completely break the sim for the majority of users, even when these changes are in principle a good idea. To solve this, a version number is introduced for our protos, in the form of a custom option for a placeholder
ProtoVersionmessage. The front-end code then compares this version to the stored version number for any protos it tries to load from local storage or links, and automatically runs up-conversion functions on the old data if they do not match.The functionality is tested against a "trivial but breaking" proto change where the Stat enum is simply re-ordered so that stats are grouped more intelligently. This has zero impact on core sim results (once the database is regenerated using the new schema), but would previously make the browser sim unusable without fully restoring defaults, since saved target armor values etc. would be incorrectly indexed. On this PR branch, local storage data and old sim links are correctly migrated to the new Stat schema upon a reload, making it possible to do more significant stats revisions in the future without adversely affecting our users.