Make SingleFile BundleID the expected length (12 bytes) #116656
Conversation
|
Tagging subscribers to this area: @vitek-karas, @agocke |
There was a problem hiding this comment.
Pull Request Overview
This PR corrects the substring usage when generating the deterministic BundleID so that the ID is the intended fixed length rather than the remainder of the string.
- Changed the
Substringcall to use a start index of 0 and a specified length. - Ensures the generated BundleID is exactly
BundleIdLengthcharacters.
|
We would need to bump the major version, but otherwise it seems ok. How long is the bundle id right now? |
|
Also needs a unit test |
Haven't validated by looking at an actual bundle yet, but it should be 32 bytes long now (SHA-256 hash : 32 bytes => Base 64 Encoded : 44 bytes => Substring(12) : 32 bytes). Seems like a reasonable length dispite being maybe overkill for a unique identifier. It might make more sense to leave this as 32 bytes and update the code to make it look intentionally 32 bytes as long there's no good reason to change it back to 12 bytes. From what I gather from previous discussions, the 12 bytes was arbitrarily chosen to match GetRandomFileName(). |
|
Works for me. Do we actually use the bundle ID as a file name anywhere? Maybe during extraction? The problem with long file names is windows path lengths. |
|
I think either way (going back to 12 bytes per the original intent of making it deterministic or making the code/naming indicate what it is currently doing) is actually fine. The bundle ID is expected to be variable-length data (written/read as a length-prefixed string), so it shouldn't be breaking. |
Yeah, for extraction we do |
| @@ -134,7 +133,7 @@ private string GenerateDeterministicId() | |||
| bundleHash.Dispose(); | |||
| bundleHash = null; | |||
|
|
|||
| return Convert.ToBase64String(manifestHash).Substring(BundleIdLength).Replace('/', '_'); | |||
| return Convert.ToBase64String(manifestHash).Substring(0, BundleIdLength).Replace('/', '_'); | |||
| } | |||
|
|
|||
| public long Write(BinaryWriter writer) | |||
There was a problem hiding this comment.
Maybe Debug.Assert(BundleID.Length == BundleIdLength) after we set it below?
It looks like #52930 meant to have the BundleID be 12 characters, but used
Substring(12), which created a substring starting at offset 12 instead. This may end up being more of breaking change, though.