Skip to content

Add Property Migration #253

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 28 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
9752641
Implement Font type
Corecii Jan 12, 2023
c9a798f
Error when not impl future FontStyles
Jan 15, 2023
cca1027
Add docs
Jan 15, 2023
d345184
Merge remote-tracking branch 'upstream/master' into implement-font-type
Jan 15, 2023
5fd55f5
Delete old tests
Jan 15, 2023
5edf212
Add new test
Jan 15, 2023
ff6e9f1
Fix formatting consistency issues in spec file
Corecii Jan 16, 2023
575486e
Address PR comments
Jan 18, 2023
2f9efd3
IgnoreGuiInset no longer saves
Jan 19, 2023
fb1c248
Fix typo in binary.md
Corecii Feb 12, 2023
24293fa
Fix font weight casing
Feb 12, 2023
bb86ea1
Remove unused constructor
Feb 12, 2023
7b7029e
Fix snapshot tests for PascalCase fonts
Feb 12, 2023
9f591ad
Don't cast around u16 for xml
Feb 12, 2023
881f4b5
Remote Other for Font Weight and Style
Feb 12, 2023
feed1e1
Fix camelCase attribute on FontStyle
Corecii Feb 13, 2023
b25efb0
Add PR changes to CHANGELOG files
Corecii Feb 13, 2023
36279e5
Fix Font types from being excluded
Feb 28, 2023
0df4bb7
Update database to reflect Font inclusion
Feb 28, 2023
4b162bb
Use if...else
Feb 28, 2023
bf6b91e
Fix minor formatting
Feb 28, 2023
3760427
to_x -> as_x
Feb 28, 2023
06dcf3b
Update Database
Corecii Mar 12, 2023
37bf303
Add property migration
Corecii Mar 12, 2023
c675e7c
Add tests for migrations
Corecii Mar 12, 2023
2f88f0b
Add migration patching instructions
Corecii Mar 12, 2023
897b532
Merge remote-tracking branch 'upstream/master' into property-migration-2
Corecii May 12, 2023
9e860c3
Update database
Corecii May 12, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 29 additions & 2 deletions docs/patching-database.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# How to Fix a New Property Added by Roblox
When Roblox introduces new properties, usually tools like Rojo can use them without any additional changes. Sometimes, though, properties are added with different names, multiple serialized forms, or aren't listed at all in the reflection dump that Roblox gives us.
When Roblox introduces new properties, usually tools like Rojo can use them without any additional changes. Sometimes, though, properties are added with different names, multiple serialized forms, need to be migrated to a new property, or aren't listed at all in the reflection dump that Roblox gives us.

This document describes some common scenarios, and what work needs to happen to fix them.

Expand Down Expand Up @@ -91,6 +91,32 @@ Change:
AliasFor: MaxPlayers
```

## 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.

To fix this, we need to write a migration (in Rust) and apply it is as a patch (using database patch files).

First, add your migration to the PropertyMigration enum in [`rbx_reflection/src/migrations`][migrations]. The migration should be named after the properties it's migrating. For example, migrating from `Font` to `FontFace` would be named `FontToFontFace`.

Next, add code to convert from the old property's type to the new property's type. This code should be a new match arm in the `perform_migration` function in [`rbx_reflection/src/migrations`][migrations].

Finally, add a patch in the [patches](patches) folder. This patch should change the old property's serialization type to `Migrate`, specifying the new property name and the migration name.

For example, the patch for fonts looks like:
```yaml
Change:
TextLabel:
Font: # Property we're migrating *from*
Serialization:
Type: Migrate
Property: FontFace # Property we're migrating *to*
Migration: FontToFontFace # Migration we're using
```

If this property is present on multiple classes, you may need to specify the Serialization change for multiple properties on multiple classes. For example, the `Font` property is present on `TextLabel`, `TextButton`, `TextBox` without being derived from a superclass, so the real patch is approximately 3 times as long since it needs to be applied to each class.

## Roblox added a new property, but modifying it from Lua requires a special API
Sometimes a property is added that cannot be assigned directly from Lua.

Expand Down Expand Up @@ -148,4 +174,5 @@ These pull requests outline how we implemented support for Attributes in rbx-dom

[rbx-dom]: https://github.com/rojo-rbx/rbx-dom
[patches]: https://github.com/rojo-rbx/rbx-dom/tree/master/patches
[custom-properties]: https://github.com/rojo-rbx/rbx-dom/blob/master/rbx_dom_lua/src/customProperties.lua
[custom-properties]: https://github.com/rojo-rbx/rbx-dom/blob/master/rbx_dom_lua/src/customProperties.lua
[migrations]: https://github.com/rojo-rbx/rbx-dom/blob/master/rbx_reflection/src/migration.rs
15 changes: 14 additions & 1 deletion generate_reflection/src/property_patches.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ use std::path::Path;

use anyhow::{anyhow, bail, Context};
use rbx_reflection::{
DataType, PropertyDescriptor, PropertyKind, ReflectionDatabase, Scriptability,
DataType, PropertyDescriptor, PropertyKind, PropertyMigration, ReflectionDatabase,
Scriptability,
};
use serde::Deserialize;

Expand Down Expand Up @@ -84,6 +85,11 @@ pub enum PropertySerialization {
#[serde(rename = "As")]
serializes_as: String,
},
#[serde(rename_all = "PascalCase")]
Migrate {
property: String,
migration: PropertyMigration,
},
}

impl From<PropertySerialization> for rbx_reflection::PropertySerialization<'_> {
Expand All @@ -96,6 +102,13 @@ impl From<PropertySerialization> for rbx_reflection::PropertySerialization<'_> {
PropertySerialization::SerializesAs { serializes_as } => {
rbx_reflection::PropertySerialization::SerializesAs(Cow::Owned(serializes_as))
}
PropertySerialization::Migrate {
property,
migration,
} => rbx_reflection::PropertySerialization::Migrate {
property: Cow::Owned(property),
migration,
},
}
}
}
Expand Down
7 changes: 7 additions & 0 deletions patches/screengui.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
Change:
ScreenGui:
IgnoreGuiInset:
Serialization:
Type: Migrate
Property: ScreenInsets
Migration: IgnoreGuiInsetToScreenInsets
19 changes: 19 additions & 0 deletions patches/text-gui.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
Change:
TextLabel:
Font:
Serialization:
Type: Migrate
Property: FontFace
Migration: FontToFontFace
TextButton:
Font:
Serialization:
Type: Migrate
Property: FontFace
Migration: FontToFontFace
TextBox:
Font:
Serialization:
Type: Migrate
Property: FontFace
Migration: FontToFontFace
4 changes: 3 additions & 1 deletion rbx_binary/src/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -434,7 +434,9 @@ fn find_serialized_from_canonical<'a>(
match serialization {
// This property serializes as-is. This is the happiest path: both the
// canonical and serialized descriptors are the same!
PropertySerialization::Serializes => Some(canonical),
PropertySerialization::Serializes | PropertySerialization::Migrate { .. } => {
Some(canonical)
}

// This property serializes under an alias. That property should have a
// corresponding property descriptor within the same class descriptor.
Expand Down
Loading