Perform property migrations in rbx_binary serializer #339
Conversation
This is necessary because if collect_type_info processes a migrated-to property first, and then processes a migrated property later, PropInfo.migration will still be None and we won't be able to apply migrations This would not work if a logical property had multiple, different migration operations associated with it, but thankfully that is not the case!
This is kind of ugly... Because migrated and migrated-to properties can both exist in the same DOM, we're won't necessarily get a consistent variant type here, so we just ignore any migration failures and assume values that fail are already the correct type
Woo, lookin good!
| if let Some(migration) = prop_info.migration { | ||
| match migration.perform(&value) { | ||
| Ok(new_value) => Cow::Owned(new_value), | ||
| Err(_) => value, | ||
| } | ||
| } else { | ||
| value | ||
| } |
There was a problem hiding this comment.
Migrations seem unlikely to fail anyway, so I think this is fine. The one we might run into is someone setting an invalid Font enum value, but I don't think that's something we can reasonably account for right now.
There was a problem hiding this comment.
I think this might be a little stranger than you're realizing - the Err branch will be hit under normal operation. The reason why is because in this PR, all migrated properties are collated under a single PropInfo. For reference, here's what PropInfo looks like during the migrated properties test:
PropInfo {
prop_type: Color3uint8,
serialized_name: "Color3uint8",
aliases: {
"BrickColor",
"brickColor",
},
default_value: Color3uint8(
Color3uint8 {
r: 163,
g: 162,
b: 165,
},
),
migration: Some(
PropertyMigration {
new_property_name: "Color",
migration: BrickColorToColor,
},
),
}
This means that in serialize_properties, some properties will be Variant::BrickColor, some properties will be Variant::Color3uint8 or Variant::Color3, and the migration will fail - but we expect it to!
There was a problem hiding this comment.
Oh that's kinda gross. I wonder if it's worth attempting a conversion in the same way we do with rbx_xml or if it's just a lost cause?
There was a problem hiding this comment.
Since this is likely throwaway code I'm not sure if it matters too much - any mismatches will be caught later in the match prop_info.prop_type. I tried to make this less ugly, but I wasn't able to do so while still having one prop chunk per logical property. I don't think there's a good way to do it without significantly changing the binary serializer
There was a problem hiding this comment.
I suppose another option is we make the migration catch multiple types of variants? That feels less technically correct but is probably safer anyway.
There was a problem hiding this comment.
I don't think that's necessary. Besides applying migrations for select properties, there's no other changes to the behavior of serialize_properties as a result of this, and I'll want to delete this code ASAP regardless
There was a problem hiding this comment.
That's fair enough. We're probably good to merge this then.
Related: #335
Yeah, so this sucked pretty hard. rbx_xml will be easier (I hope)...
I've added descriptions to some of the commits here in an attempt to elucidate my thought process, if anything is unclear please ask questions