Summary: | An incomplete fix for the NPE bug in HyperlinkRecord.java | ||
---|---|---|---|
Product: | POI | Reporter: | lianggt08 |
Component: | HSSF | Assignee: | POI Developers List <dev> |
Status: | RESOLVED WORKSFORME | ||
Severity: | critical | ||
Priority: | P2 | ||
Version: | unspecified | ||
Target Milestone: | --- | ||
Hardware: | PC | ||
OS: | All |
Description
lianggt08
2012-02-14 08:50:50 UTC
I believe that a monkier is required for both of the two kinds of links that are being serialized there. If the monkier is null at this point, then the problem lies earlier. We can't just skip writing it, as the record wouldn't be valid How are you ending up without one? The text diff of the revision 720997 is as bellows: --- poi/trunk/src/java/org/apache/poi/hssf/record/HyperlinkRecord.java 2008/11/26 22:00:07 720996 +++ poi/trunk/src/java/org/apache/poi/hssf/record/HyperlinkRecord.java 2008/11/26 22:00:29 720997 @@ -628,7 +628,7 @@ if ((_linkOpts & HLINK_TARGET_FRAME) != 0) { buffer.append(" .targetFrame= ").append(getTargetFrame()).append("\n"); } - if((_linkOpts & HLINK_URL) != 0) { + if((_linkOpts & HLINK_URL) != 0 && _moniker != null) { buffer.append(" .moniker = ").append(_moniker.formatAsString()).append("\n"); } if ((_linkOpts & HLINK_PLACE) != 0) { we can see that only checking "(_linkOpts & HLINK_URL) != 0" can not guarantee that " _moniker != null". If the revision 720997 is a right fix, then I think the Line 559 of the method "serialize" should also be fixed this way. I think it will be more reasonable to treat them in a same way. I would say that toString() must correspond to serialize(), not the other way around. The code in HyperlinkRecord.serialize is correct. If a link is URL and not a UNC path then the moniker field MUST be set. If it is null then you are constructing wrong data. HyperlinkRecord.toString() uses slightly different logic and prints the moniker if it is not null and hyperlink type is URL (UNC path option is not tested here). This is not quite correct but acceptable because toString() is used mainly for debugging purposes. In any case, I'm not going to change it without a unit test that demonstrates the NPE issue. If you are able to construct one, please upload and re-open this ticket. Yegor (In reply to comment #2) > The text diff of the revision 720997 is as bellows: > --- poi/trunk/src/java/org/apache/poi/hssf/record/HyperlinkRecord.java > 2008/11/26 22:00:07 720996 > +++ poi/trunk/src/java/org/apache/poi/hssf/record/HyperlinkRecord.java > 2008/11/26 22:00:29 720997 > @@ -628,7 +628,7 @@ > if ((_linkOpts & HLINK_TARGET_FRAME) != 0) { > buffer.append(" .targetFrame= > ").append(getTargetFrame()).append("\n"); > } > - if((_linkOpts & HLINK_URL) != 0) { > + if((_linkOpts & HLINK_URL) != 0 && _moniker != null) { > buffer.append(" .moniker = > ").append(_moniker.formatAsString()).append("\n"); > } > if ((_linkOpts & HLINK_PLACE) != 0) { > > we can see that only checking "(_linkOpts & HLINK_URL) != 0" can not guarantee > that " _moniker != null". > > If the revision 720997 is a right fix, then I think the Line 559 of the method > "serialize" should also be fixed this way. > > I think it will be more reasonable to treat them in a same way. Closing based on previous comment and non-update for a long time. |