dita-ot icon indicating copy to clipboard operation
dita-ot copied to clipboard

NullPointerException crash occurs when a peer mapref and local mapref use the same keyscope

Open chrispy-snps opened this issue 2 years ago • 10 comments

Expected Behavior

When I run a transformation on the following files, where (1) there is a peer and local mapref of the same submap, and (2) there is a keyref inside a submap topic:

bug

I expect it to complete successfully.

Actual Behavior

The transformation fails as follows:

keyref:
   [keyref] Reading file:/tmp/temp20220915211429383/map.ditamap
Error: java.lang.NullPointerException: Cannot invoke "org.dita.dost.util.KeyScope.equals(Object)" because "rt.scope" is null

In submap.ditamap, if I comment out one of the two <topicref> elements, then it fails as follows:

keyref:
   [keyref] Reading file:/tmp/temp20220915212742017/map.ditamap
Error: java.lang.NullPointerException: element cannot be mapped to a null key

The issue seems to somehow be caused by the following:

  • The peer and local maprefs sharing the same keyscope value in map.ditamap
  • The key references to be resolved in the topic files

If I remove any of these, the crash does not occur.

Possible Solution

When I swap the order of the peer mapref and the local mapref, the crash no longer occurs. Maybe the key resolution attempt is not handling the peer mapref properly.

Steps to Reproduce

  1. Download and extract the following testcase: 4007.zip
  2. Run the following command: dita -i map.ditamap -f html5

Environment

  • DITA-OT version 3.7.3
  • Operating system and version: Linux (Ubuntu 20.04)
  • How did you run DITA-OT? dita command
  • Transformation type: html5

chrispy-snps avatar Sep 16 '22 01:09 chrispy-snps

If I use ditaot_save_preprocessing.pl to look at the contents of map.ditamap going into the keyref preprocessing stage, the keyscoped peer mapref causing the issue is as follows:

<map>
   <title>Home</title>
   <keydef keys="Tool" processing-role="resource-only">
      <topicmeta>
         <keywords>
            <keyword>My Product</keyword>
         </keywords>
      </topicmeta>
   </keydef>

   <!-- this is causing the issue (I think) -->
   <mapref format="ditamap"
           href="submap.ditamap"
           keyscope="submap"
           processing-role="resource-only"
           scope="peer"/>

   <submap keyscope="submap">
      <submap-topicmeta>
         <submap-title>Submap</submap-title>
      </submap-topicmeta>
      <topicgroup keyscope="topics">
         <topicref href="topic1.dita"/>
         <topicref href="topic2.dita"/>
      </topicgroup>
   </submap>
</map>

This issue occurs in a top-level help collection map that contains dozens of submaps and thousands of topic files. Some of the submaps are also published by themselves, so they have peer maprefs in them for cross-book links. The peer and local maprefs are actually separated by levels of non-keyscoped maprefs in our production content; the testcase simply shows the simplified structure with these non-keyscoped maprefs collapsed.

chrispy-snps avatar Sep 16 '22 10:09 chrispy-snps

In the attached sample "4007/submap.ditamap" the reference to the RNG file using xml-model should be "urn:oasis:names:tc:dita:rng:map.rng", instead of referencing a bookmap. If I change that, I can reproduce the NPE, leaving below more of the stack trace:

    /Users/../DITA-OT3.x/plugins/org.dita.base/build_preprocess.xml:232: java.lang.NullPointerException: Cannot invoke "org.dita.dost.util.KeyScope.equals(Object)" because "rt.scope" is null
at org.dita.dost.module.KeyrefModule.lambda$walkMap$20(KeyrefModule.java:327)
at java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:178)
at java.base/java.util.ArrayList$ArrayListSpliterator.tryAdvance(ArrayList.java:1602)
at java.base/java.util.stream.ReferencePipeline.forEachWithCancel(ReferencePipeline.java:129)
at java.base/java.util.stream.AbstractPipeline.copyIntoWithCancel(AbstractPipeline.java:527)
at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:513)
at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
at java.base/java.util.stream.FindOps$FindOp.evaluateSequential(FindOps.java:150)
at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
at java.base/java.util.stream.ReferencePipeline.findAny(ReferencePipeline.java:652)
at org.dita.dost.module.KeyrefModule.walkMap(KeyrefModule.java:388)
at org.dita.dost.module.KeyrefModule.walkMap(KeyrefModule.java:428)
at org.dita.dost.module.KeyrefModule.walkMap(KeyrefModule.java:428)
at org.dita.dost.module.KeyrefModule.walkMap(KeyrefModule.java:428)
at org.dita.dost.module.KeyrefModule.walkMap(KeyrefModule.java:452)
at org.dita.dost.module.KeyrefModule.collectProcessingTopics(KeyrefModule.java:233)
at org.dita.dost.module.KeyrefModule.execute(KeyrefModule.java:135)

raducoravu avatar Sep 16 '22 11:09 raducoravu

Thanks @raducoravu! I have corrected the testcase archive in-place.

chrispy-snps avatar Sep 16 '22 11:09 chrispy-snps

@chrispy-snps how about if you guard the NPE, in https://github.com/dita-ot/dita-ot/blob/develop/src/main/java/org/dita/dost/module/KeyrefModule.java at line 327:

                                             .filter(rt -> rt.scope.equals(s) && rt.in.uri.equals(fi.uri))

change it to:

                                             .filter(rt -> rt.scope != null && rt.scope.equals(s) && rt.in.uri.equals(fi.uri))

raducoravu avatar Sep 19 '22 05:09 raducoravu

With your change, the small 4007.zip testcase above now gives the following:

keyref:
   [keyref] Reading file:/tmp/temp20220919082024727/map.ditamap
Error: java.lang.NullPointerException: element cannot be mapped to a null key

but our production content gives a different message:

keyref:
   [keyref] Reading file:/tmp/temp20220919081841677/olh_pwre.ditamap
Error: java.lang.NullPointerException: Cannot read field "keyDefinition" because "scope" is null

perhaps because different follow-up bugs are now sensitized due to the testcase simplification. I will see if I can simplify a testcase for this second message.

I do not get stack traces when running the dita command, perhaps due to #3731?

chrispy-snps avatar Sep 19 '22 12:09 chrispy-snps

For the following message:

keyref:
   [keyref] Reading file:/tmp/temp20220919090811619/map.ditamap
Error: java.lang.NullPointerException: Cannot read field "keyDefinition" because "scope" is null

Here is a testcase:

4007-scope-is-null.zip

It contains the following file structure:

4007-scope-is-null

chrispy-snps avatar Sep 19 '22 13:09 chrispy-snps

@chrispy-snps how about if you revert the change I suggested and in the method "org.dita.dost.module.KeyrefModule.walkMap(FileInfo, XdmNode, List<KeyScope>, List<ResolveTask>, Receiver)" there is this code:

                final List<KeyScope> ss = node.attribute(ATTRIBUTE_NAME_KEYSCOPE) != null
                        ? Stream.of(node.attribute(ATTRIBUTE_NAME_KEYSCOPE).trim().split("\\s+"))
                        .flatMap(keyscope -> scope.stream().map(s -> s.getChildScope(keyscope)))
                        .collect(Collectors.toList())
                        : scope;

replace the third line:

                        .flatMap(keyscope -> scope.stream().map(s -> s.getChildScope(keyscope)))

with:

                       .flatMap(keyscope -> scope.stream().map(s -> s.getChildScope(keyscope))).filter(Objects::nonNull)

I think this should fix the NPE. Does that produce the desired output?

raducoravu avatar Sep 20 '22 10:09 raducoravu

@raducoravu - this fix works perfectly, in both 4007.zip and 4007-scope-is-null.zip, with one topic reference and two topic references, and in our production content! Thank you!

I can submit a pull request for this change. It probably deserves unit test coverage, but I am not sure how to do that so I asked about it in #4009.

chrispy-snps avatar Sep 20 '22 10:09 chrispy-snps

@raducoravu - I don't think I will be able to write a test for this. Do you want me to submit a pull request for the code change and see where it goes, or would you prefer to submit it?

chrispy-snps avatar Sep 20 '22 20:09 chrispy-snps

@chrispy-snps thanks for confirming the fix works, I will try to add a pull request for it.

raducoravu avatar Sep 21 '22 06:09 raducoravu