The fix revision 720997 was aimed to remove an NPE bug on the "this._moniker" in the method "toString" of the file "/poi/trunk/src/java/org/apache/poi/hssf/record/HyperlinkRecord.java" , but it is incomplete. Since the "this._moniker" is a class field and also could be null during the run-time execution, it should also be null-checked before being dereferenced in other methods. The buggy code locations the same fix needs to be applied at are as bellows: Line 559 of the method "serialize". public void serialize(LittleEndianOutput out) { _range.serialize(out); _guid.serialize(out); out.writeInt(0x00000002); // TODO const out.writeInt(_linkOpts); if ((_linkOpts & HLINK_LABEL) != 0){ out.writeInt(_label.length()); StringUtil.putUnicodeLE(_label, out); } if ((_linkOpts & HLINK_TARGET_FRAME) != 0){ out.writeInt(_targetFrame.length()); StringUtil.putUnicodeLE(_targetFrame, out); } if ((_linkOpts & HLINK_UNC_PATH) != 0) { out.writeInt(_address.length()); StringUtil.putUnicodeLE(_address, out); } else if ((_linkOpts & HLINK_URL) != 0){ [Line 559] _moniker.serialize(out); if(URL_MONIKER.equals(_moniker)){ if ((_linkOpts & HLINK_TARGET_FRAME) != 0) { out.writeInt(_address.length()*2); StringUtil.putUnicodeLE(_address, out); } else { out.writeInt(_address.length()*2 + TAIL_SIZE); StringUtil.putUnicodeLE(_address, out); writeTail(_uninterpretedTail, out); } } else if (FILE_MONIKER.equals(_moniker)){ out.writeShort(_fileOpts); out.writeInt(_shortFilename.length()); StringUtil.putCompressedUnicode(_shortFilename, out); writeTail(_uninterpretedTail, out); if (_address == null) { out.writeInt(0); } else { int addrLen = _address.length() * 2; out.writeInt(addrLen + 6); out.writeInt(addrLen); out.writeShort(0x0003); // TODO const StringUtil.putUnicodeLE(_address, out); } } } if((_linkOpts & HLINK_PLACE) != 0){ out.writeInt(_textMark.length()); StringUtil.putUnicodeLE(_textMark, out); } }
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.