Implement property migration #268

Corecii · 2023-05-12T06:20:52Z

This is a fixed version of #253 because its git history was cursed and I didn't want to force push or anything else weird to fix it.

Preview

This PR allows us to specify migrations using the patches files and a Variant to Variant converter in the actual Rust code. This is better than having the Rust code do hard-coded checks against property names and such without being super complex to implement or think about. Writing new migrations this way is easy!

The patch files look like this:

Change:
  TextLabel:
    Font:
      Serialization:
        Type: Migrate
        Property: FontFace
        Migration: FontToFontFace

I've chosen to have migrations not be performed if the target property already exists. This matches Roblox's behavior where new properties always take priority. I figure making priority work both ways would be complex, and since backwards priority isn't known behavior of any present-day properties, it's unnecessary complexity. There's room to expand this in the future if we need to.

Why do we need this?

Two new properties -- FontFace and ScreenInsets -- are migrated from old properties. When both the old and new properties are present, Roblox always prefers the new properties.

The Roblox binary format requires that all objects of the same class have the same properties present. When mixing old models and new models into the same file using Rojo or Remodel, old models get assigned defaults for the new properties, which then take priority over the old properties.

This has two clear affects that users are actively experiencing in "preview" builds of Rojo and Remodel:

Mixing old and new models with UI via rojo build or Remodel scripts results in all old text UI having the Arial font.
Mixing old and new models with ScreenGuis via rojo build or Remodel scripts results in all old ScreenGuis losing their IgnoreGuiInset value.

The only work-around for users right now is to open their models in Roblox and re-save over them to get Roblox to do the migration. This is not a reasonable ask of a Rojo fully-managed project that may have hundreds of models.

As time goes on, more properties will get migrations like this and maintaining a fully-managed Rojo codebase will become an unreasonable task.

Implementing these migrations on the rbx-dom side is quite easy. If we don't do it, either someone else will add this feature themselves or people just won't do fully-managed Rojo due to the high risk of breakage.

Changes in this PR

Adds a new type of PropertySerialization, Migrate, where you specify the new property name to migrate to and the name of the migration.
Adds named migrations, which convert from one Variant to another Variant.
Changes deserializer code to respect the Migrate type.
Adds tests for these new migrations.
Adds documentation instructing how to add a new migration.

Corecii · 2023-05-12T07:25:01Z

I'd like to give this more time and attention, but I have a lot going on right now. I feel like this code can be improved, but any PR is a good start for now, right?

Dekkonot

Beyond needing a quick update to not update our workflows because of PR #270, looks good to me. My request to log is the only serious one; in-lining format arguments or not isn't really important but we have the ability so we may as well.

Dekkonot · 2023-05-22T16:50:20Z

rbx_binary/src/deserializer/state.rs

+            Some(_) => {
+                let (migration, property) = migrated_to.unwrap();
+                if !instance.builder.has_property(property.as_ref()) {
+                    match perform_migration(*migration, &value) {


Suggested change

match perform_migration(*migration, &value) {

log::trace!("Attempting migrate property {canonical_name} to {property}");

match perform_migration(*migration, &value) {

If we're going to log when one fails, it makes sense for us to log when one starts as well.

Dekkonot · 2023-05-22T16:50:28Z

rbx_binary/src/deserializer/state.rs

+                            log::warn!(
+                                "Failed to migrate property {} to {} because: {}",
+                                canonical_name,
+                                property,
+                                e.to_string()
+                            );


Suggested change

log::warn!(

"Failed to migrate property {} to {} because: {}",

canonical_name,

property,

e.to_string()

);

log::warn!(

"Failed to migrate property {canonical_name} to {property} because: {}",

e.to_string()

);

Dekkonot · 2023-05-22T16:53:42Z

.github/workflows/ci.yml

@@ -17,7 +17,7 @@ jobs:
    strategy:
      fail-fast: false
      matrix:
-        rust_version: [stable, "1.63.0"]
+        rust_version: [stable, "1.65.0"]


Suggested change

rust_version: [stable, "1.65.0"]

rust_version: [stable]

Dekkonot · 2023-05-22T16:53:57Z

.github/workflows/clippy.yml

@@ -16,7 +16,7 @@ jobs:

    strategy:
      matrix:
-        rust_version: ["1.64.0"]
+        rust_version: ["1.65.0"]


Suggested change

rust_version: ["1.65.0"]

rust_version: [stable]

Dekkonot · 2023-05-24T23:32:00Z

...I should have thought about this beforehand but I just merged #249 which changes the format of patches a bit and swaps us over to using a polished version of generate_reflection named rbx_reflector. You'll need to use that tool in this PR instead because it'll get archived Soon(tm).

Beyond having a better UX, we now no longer need to add properties to the database, only modify their serialization. That meshes well with migrations and should hopefully make it reasonable to have those migrations live as patches.

nezuo · 2023-05-28T01:12:25Z

docs/patching-database.md

+## Roblox added a new property, but it's a migration from an existing property, and the existing property no longer loads
+Sometimes Roblox migrates an existing property whose type is too constrained to a new property with a more flexible type.
+
+Without special handling, this can cause problems for binary files because when old and new models are mixed together, the binary serializer must add the new property to the old models. Without special instruction, it'll just add the default value. This can result in weird behavior like old text UI all having the Arial font, because the default value of a new property took priority.


when old and new models are mixed together

What do you mean by "mixing" models?

nezuo · 2023-05-28T01:21:54Z

docs/patching-database.md

+## Roblox added a new property, but it's a migration from an existing property, and the existing property no longer loads
+Sometimes Roblox migrates an existing property whose type is too constrained to a new property with a more flexible type.
+
+Without special handling, this can cause problems for binary files because when old and new models are mixed together, the binary serializer must add the new property to the old models. Without special instruction, it'll just add the default value. This can result in weird behavior like old text UI all having the Arial font, because the default value of a new property took priority.


the binary serializer must add

Is this talking about rbx_binary or saving from Studio?

nezuo · 2023-05-28T01:25:22Z

docs/patching-database.md

If I understand correctly, migrations swap out the old property for the new one in deserialization. It doesn't actual migrate the model file, it will still have the old property. I think this would be useful information to include.

Based on PR #268 Co-authored-by: Shae A <shae@uplift.games>

Dekkonot · 2023-06-15T01:22:53Z

Work on this has been superseded by PR #283! Sorry that we ended up bungling this, I'll try to be more mindful of how the order things get merged in the future.

Based on PR rojo-rbx#268 Co-authored-by: Shae A <shae@uplift.games>

Implement migration, tests, and docs

9d50a60

Corecii requested review from LPGhatguy, Kampfkarren and Dekkonot as code owners May 12, 2023 06:20

Fix clippy lints

8033c1e

Corecii mentioned this pull request May 12, 2023

Add files for testing that migrations work correctly rojo-rbx/rbx-test-files#20

Merged

Corecii added 2 commits May 12, 2023 02:21

Fix tests

c1aebcb

Up rust version for time

8f436da

Corecii changed the title ~~Implement migration, tests, and docs~~ Implement property migration May 12, 2023

use test-files/master

c1c83cf

Dekkonot requested changes May 22, 2023

View reviewed changes

nezuo reviewed May 28, 2023

View reviewed changes

kennethloeffler mentioned this pull request Jun 13, 2023

Implement property migration #283

Merged

2 tasks

Dekkonot pushed a commit that referenced this pull request Jun 15, 2023

Implement property migration (#283)

e99f37d

Based on PR #268 Co-authored-by: Shae A <shae@uplift.games>

Dekkonot closed this Jun 15, 2023

Dekkonot pushed a commit to Dekkonot/rbx-dom that referenced this pull request Jun 20, 2023

Implement property migration (rojo-rbx#283)

8595c34

Based on PR rojo-rbx#268 Co-authored-by: Shae A <shae@uplift.games>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement property migration #268

Implement property migration #268

Corecii commented May 12, 2023 •

edited

Loading

Corecii commented May 12, 2023

Dekkonot left a comment

Dekkonot May 22, 2023

Dekkonot May 22, 2023

Dekkonot May 22, 2023

Dekkonot May 22, 2023

Dekkonot commented May 24, 2023

nezuo May 28, 2023

nezuo May 28, 2023

nezuo May 28, 2023

Dekkonot commented Jun 15, 2023

	match perform_migration(*migration, &value) {
	log::trace!("Attempting migrate property {canonical_name} to {property}");
	match perform_migration(*migration, &value) {

Implement property migration #268

Implement property migration #268

Conversation

Corecii commented May 12, 2023 • edited Loading

Preview

Why do we need this?

Changes in this PR

Corecii commented May 12, 2023

Dekkonot left a comment

Choose a reason for hiding this comment

Dekkonot May 22, 2023

Choose a reason for hiding this comment

Dekkonot May 22, 2023

Choose a reason for hiding this comment

Dekkonot May 22, 2023

Choose a reason for hiding this comment

Dekkonot May 22, 2023

Choose a reason for hiding this comment

Dekkonot commented May 24, 2023

nezuo May 28, 2023

Choose a reason for hiding this comment

nezuo May 28, 2023

Choose a reason for hiding this comment

nezuo May 28, 2023

Choose a reason for hiding this comment

Dekkonot commented Jun 15, 2023

Corecii commented May 12, 2023 •

edited

Loading