-
Notifications
You must be signed in to change notification settings - Fork 551
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
fix for issue1281, ignore missing calcChain part #1288
Conversation
fixes #1281 |
{ | ||
// Fix bug https://github.com/OfficeDev/Open-XML-SDK/issues/1205 | ||
if (relationship.RelationshipType == @"http://schemas.microsoft.com/office/2006/relationships/recovered") | ||
@"http://schemas.openxmlformats.org/officeDocument/2006/relationships/calcChain", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something that is always missing? are there cases in which we wouldn't want to skip it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@twsouthwick read the comments on #1279, the Excel team and I discussed this. For opening a package with the SDK, it's safe to skip this regardless of whether the relationship is valid (i.e. has a valid part) or not. Excel will always create the relationship and part as needed and doesn't rely on reading it. And to answer your first question, no, it's not always missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
This should be stored in either OpenSettings or exposed as a feature (these concepts will probably be reconciled at some point, but OpenSettings allows you to set things that occur at opening). Should users be able to override this? Is there a use case for that? |
I don't see a need to make this an OpenSettings option. Users don't need to override the behavior. However, I do think this should be reworked to remove the relationship upon finding the part missing. I will do that. Edit: I think I misunderstood. If you mean, override the behavior of ignoring the relationship (new behavior), then perhaps there is a use case for an OpenSettings option which allows us to throw the exception when the relationship is broken (current behavior). I will create an option in OpenSettings for that. |
@twsouthwick I've added an option in openSettings which by default allows us to throw the exception if calcChain part is missing. when set to true, we don't throw the exception. I'm still on the fence actually about whether we should remove the relationship. Now it seems more sensible to leave the package as is either way. Example which allows us to throw: SpreadsheetDocument spd = SpreadsheetDocument.Open(@"c:\path\test.xlsx", true, new OpenSettings() { IgnoreExceptionOnCalcChainPartMissing = false }); |
@twsouthwick any thoughts on this openSettings approach? |
@Distancehe just to recap the functionality here as it may have gotten a little lost in the thread. Excel effectively ignores 3rd party authoring of the calcChain part (whether intact or broken) and will rewrite on save a new or updated part. With this change, the SDK's default behavior will still be to throw an exception on load if the calcChain relationship is broken, including the part being absent. I felt that this would be desirable for 3rd party producers using the SDK if they actually had some purpose for writing calcChain parts for their own use. This default behavior will do the same processing for calcChain that it does for any other part being loaded. Otherwise, the behavior is overridable with the OpenSettings IgnoreExceptionOnCalcChainPartMissing property. Overriding causes the SDK load path (i.e. SpreadsheetDocument.Open) to completely ignore the calcChain relationship, leaving the package as is wrt that part. Everyone, please post here if you have any concerns with this approach. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This is the simple approach of ignoring the missing part but leaving the relationship entry alone. Excel will handle this but if we feel it's better to remove the relationship, I can rework the fix.