XRENDERING-771: Null pointer exception when list items occur without lists

[michitux]

Jira URL

https://jira.xwiki.org/browse/XRENDERING-771

Changes

Description

• Protect against list items without wrapping list.
• Add tests.

Clarifications

• This will most likely result in incorrect outputs as the rest of the events will believe they are inside a list. I'm not sure though it makes sense to fix this.
• I'm wondering if the renderer should log a warning to notify the admin that something wrong happened. The problem with this is that I don't know what context the log could provide to let the admin understand where the problem happened. However, maybe the log message itself could have enough context (URL?)

Screenshots & Video

No UI changes.

Executed Tests

LANG=C.UTF-8 mvn clean install -Pdocker,legacy,integration-tests,snapshotModules,quality,distribution,flavor-integration-tests,standalone

Expected merging strategy

• Prefers squash: Yes
• Backport on branches:
  • stable-16.10.x
  • stable-16.4.x

[surli]

Why not encapsulating the content of the method in an if (!this.definitionListDepth.isEmpty()) { instead of adding artificially an element? Would that break something else?

[michitux]

Hm, I thought so, but now I'm not so sure anymore. It seems that getListItemIndex already checks if the stack is non-empty, so this might actually be a better solution... It will break the printing of the list items in both XWiki syntax and plain text, but it might be cleaner.

[surli]

And +1 for having a warning in the else branch then.

[michitux]

I've adjusted the code. I've decided not to log a warning as it is hard to provide relevant information for admins to perform any actions, in particular without a stack trace. For now, I'm just logging a debug message. A possibility could also be to make this a warning when deprecation logging is enabled as in https://github.com/xwiki/xwiki-platform/blob/05d353688490b416a90fcf9ba116cca04e8a9b06/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/doc/XWikiDocument.java#L2434-L2437 but to me that doesn't really seem fitting, what do you think?

[tmortagne]

to me that doesn't really seem fitting

Yeah, does not feel like a deprecation. In the case of cache document modification, I used it because modifying directly the cached document used to be kind of officially OK.