Honor content type overrides - #1016
Conversation
db8f4bc to
6cd7611
Compare
This comment was marked as resolved.
This comment was marked as resolved.
dtivel
left a comment
dtivel
left a comment
There was a problem hiding this comment.
@kennethr127, thank you for contributing this fix!
I’m requesting a few focused changes before merging:
- Avoid stripping trailing slashes when matching override part names, since that can make malformed names match valid parts.
- Make the
OverrideandDefaultmodes explicit when selecting content types. - Simplify the test to open the source VSIX once in read-only mode.
- Use separate
NotNullandEqualassertions for clearer failures.
With those changes, this should be ready to merge.
| string path; | ||
| using (var package = ShadowCopyPackage(SamplePackageWithOverrides, out path, OpcPackageFileMode.ReadWrite)) | ||
| { | ||
| var partToCheck = new Uri("/extension.vsixmanifest", UriKind.Relative); | ||
| var part = package.GetPart(partToCheck); | ||
| Assert.True(part != null && part.ContentType == "text/xml"); | ||
| } | ||
| using (var package = OpcPackage.Open(path, OpcPackageFileMode.ReadWrite)) | ||
| { | ||
| var partToCheck = new Uri("/extension.vsixmanifest", UriKind.Relative); | ||
| var part = package.GetPart(partToCheck); | ||
| Assert.True(part != null && part.ContentType == "text/xml"); | ||
| } |
There was a problem hiding this comment.
@kennethr127, is the second open/assertion necessary? Nothing modifies the package between assertions, and ContentType does not depend on read/write mode. Opening SamplePackageWithOverrides once in read-only mode with OpcPackage.Open(...) should provide the same coverage while removing the shadow copy and duplicate verification.
| var defaultContentType = Package.ContentTypes.FirstOrDefault(ct => string.Equals(ct.Extension, extension, StringComparison.OrdinalIgnoreCase)); | ||
| var overrideContentType = Package.ContentTypes.FirstOrDefault(ct => string.Equals(ct.PartName?.Trim('/'), _path, StringComparison.OrdinalIgnoreCase)); |
There was a problem hiding this comment.
@kennethr127, use TrimStart('/') instead of Trim('/'). A trailing slash is invalid for an OPC part name, but removing it can silently make a malformed override match a different valid part. For example:
/folder/file.txt/would incorrectly matchfolder/file.txt./extensionless/would incorrectly match the partextensionless.
Removing only the required leading slash avoids accepting malformed override names. Also filter Mode by OpcContentTypeMode.Override and Default to make the intended precedence explicit.
There was a problem hiding this comment.
Might also be good to add some tests for those cases.
| { | ||
| var partToCheck = new Uri("/extension.vsixmanifest", UriKind.Relative); | ||
| var part = package.GetPart(partToCheck); | ||
| Assert.True(part != null && part.ContentType == "text/xml"); |
There was a problem hiding this comment.
@kennethr127, use separate assertions so failures identify whether the part was missing or had the wrong content type:
Assert.NotNull(part);
Assert.Equal("text/xml", part.ContentType);Assert.True(part != null && part.ContentType == "text/xml") loses that diagnostic information.
4 participants
Currently, the Overrides in the [Content_Types].xml file in the package are not being honored during signing. This update addresses that issue by checking for any overrides when retrieving the ContentType.
Fixes #1015