Move old GameResource extension verification to AssetType Extension setter#191
Move old GameResource extension verification to AssetType Extension setter#191nixxquality wants to merge 1 commit intoFacepunch:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR moves extension validation logic from the obsolete GameResourceAttribute constructor to the AssetTypeAttribute.Extension property setter. This ensures that AssetType declarations with invalid extensions are caught and logged rather than silently failing.
Changes:
- Added validation logic to the Extension property setter to enforce the 8-character limit and letters-only requirement
- Removed duplicate validation logic from the GameResourceAttribute constructor, which now relies on the setter
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ( value.Length > 8 ) | ||
| { | ||
| Log.Error( $"Resource extensions should be under 8 characters ({TargetType})" ); | ||
| value = value.Substring( 0, 8 ); | ||
| } | ||
|
|
||
| if ( !value.All( x => char.IsLetter( x ) ) ) | ||
| { | ||
| Log.Error( $"Resource extensions can only contain letters ({TargetType})" ); | ||
| value = "errored"; | ||
| } |
There was a problem hiding this comment.
The validation logic checks extension length after potentially truncating it. If the extension contains non-letter characters, the code should check this first before truncating, otherwise an extension like "12345678a" would be truncated to "12345678" (8 characters, all numeric) and then fail the letter check. The current order means the length validation is effectively wasted when the extension contains invalid characters.
Consider reordering these checks: first validate that the extension contains only letters, then check the length constraint.
| if ( value.Length > 8 ) | |
| { | |
| Log.Error( $"Resource extensions should be under 8 characters ({TargetType})" ); | |
| value = value.Substring( 0, 8 ); | |
| } | |
| if ( !value.All( x => char.IsLetter( x ) ) ) | |
| { | |
| Log.Error( $"Resource extensions can only contain letters ({TargetType})" ); | |
| value = "errored"; | |
| } | |
| if ( !value.All( x => char.IsLetter( x ) ) ) | |
| { | |
| Log.Error( $"Resource extensions can only contain letters ({TargetType})" ); | |
| value = "errored"; | |
| } | |
| else if ( value.Length > 8 ) | |
| { | |
| Log.Error( $"Resource extensions should be under 8 characters ({TargetType})" ); | |
| value = value.Substring( 0, 8 ); | |
| } |
| Log.Error( $"Resource extensions should be under 8 characters ({TargetType})" ); | ||
| value = value.Substring( 0, 8 ); | ||
| } | ||
|
|
||
| if ( !value.All( x => char.IsLetter( x ) ) ) | ||
| { | ||
| Log.Error( $"Resource extensions can only contain letters ({TargetType})" ); |
There was a problem hiding this comment.
The validation logic references TargetType in error messages, but TargetType will be null at the time this setter is called during attribute construction. The TypeLibrary sets TargetType after the attribute instance is created. This explains the "null" output mentioned in the PR description.
To provide more useful error messages, consider removing the TargetType reference from these error messages, or finding an alternative way to identify the problematic resource type at validation time.
| Log.Error( $"Resource extensions should be under 8 characters ({TargetType})" ); | |
| value = value.Substring( 0, 8 ); | |
| } | |
| if ( !value.All( x => char.IsLetter( x ) ) ) | |
| { | |
| Log.Error( $"Resource extensions can only contain letters ({TargetType})" ); | |
| Log.Error( $"Resource extensions should be under 8 characters. Invalid value: \"{value}\"" ); | |
| value = value.Substring( 0, 8 ); | |
| } | |
| if ( !value.All( x => char.IsLetter( x ) ) ) | |
| { | |
| Log.Error( $"Resource extensions can only contain letters. Invalid value: \"{value}\"" ); |
| set | ||
| { | ||
| if ( value.Length > 8 ) | ||
| { | ||
| Log.Error( $"Resource extensions should be under 8 characters ({TargetType})" ); | ||
| value = value.Substring( 0, 8 ); | ||
| } | ||
|
|
||
| if ( !value.All( x => char.IsLetter( x ) ) ) | ||
| { | ||
| Log.Error( $"Resource extensions can only contain letters ({TargetType})" ); | ||
| value = "errored"; | ||
| } | ||
|
|
||
| field = value; | ||
| } |
There was a problem hiding this comment.
The setter doesn't validate for null or empty extension strings. If a null string is passed, the Length check will throw a NullReferenceException. If an empty string is passed, it will pass both validation checks but result in an invalid empty extension.
Consider adding null/empty validation at the beginning of the setter to handle these edge cases gracefully.
This shouldn't be a breaking change since the GameResource constructor just uses the same setter.
This fixes the issue where AssetType declarations with invalid extensions end up silently broken in the editor. Try making an AssetType with an extension that's 10 characters long, for example.
This has tripped people up in the Discord, so I was going to add checking. Turns out, it was already there in the old GameResource constructor.
I don't know why it wasn't moved up when GameResource was Obsolete'd, so I moved it up now.
I did a quick test and it seems to work as expected, although with 2 minor issues:

But I think this is still better than silently failing.