[MAGNOLIA-3732] SystemContentWrapper fails to wrap hierarchy manager of its node data Created: 09/Jun/11  Updated: 16/Aug/11  Resolved: 16/Aug/11

Status: Closed
Project: Magnolia
Component/s: core
Affects Version/s: 4.4.4
Fix Version/s: 4.4.5

Type: Bug Priority: Neutral
Reporter: Jan Haderka Assignee: Jan Haderka
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: Text File mag3732-test.patch    
Issue Links:
dependency
depends upon MAGNOLIA-5124 Create TestAccessManager to allow tes... Closed
relation
supersession
supersedes MAGNOLIA-3729 Dialogs not found after update Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:

 Comments   
Comment by Jan Haderka [ 09/Jun/11 ]

The test to expose the issue. In order to fix it, SystemContentWrapper needs to override wrap(NodeData nd) method and wrap returned node data in SystemNodeDataWrapper which right now doesn't exists at all ... such wrapper should be created. The same applies to LazyContentWrapper and LazyNodeDataWrapper.

Comment by Jan Haderka [ 13/Jun/11 ]

Should be ported also to trunk. Otherwise it looks good.

Comment by Jan Haderka [ 21/Jun/11 ]

the wrapper should check that wrapped node data really exists and not wrap null data

java.lang.NullPointerException
	at info.magnolia.cms.util.NodeDataWrapper.getString(NodeDataWrapper.java:149)
	at info.magnolia.cms.security.MgnlUser.getLanguage(MgnlUser.java:277)
	at info.magnolia.context.UserContextImpl.setLocaleFor(UserContextImpl.java:98)
	at info.magnolia.context.UserContextImpl.getLocale(UserContextImpl.java:62)
	at info.magnolia.context.MgnlContext.getLocale(MgnlContext.java:98)
	at info.magnolia.freemarker.FreemarkerHelper.checkLocale(FreemarkerHelper.java:177)
	at info.magnolia.freemarker.FreemarkerHelper.render(FreemarkerHelper.java:144)
	at info.magnolia.freemarker.FreemarkerHelper.render(FreemarkerHelper.java:130)
	at info.magnolia.freemarker.FreemarkerUtil.process(FreemarkerUtil.java:135)
	at info.magnolia.cms.security.auth.callback.FormClientCallback.doCallback(FormClientCallback.java:74)
Comment by Magnolia International [ 21/Jun/11 ]

I don't think the npe occurs when the property does not exist, but simply because getWrappedNodeData() returns null .. because the value it returns is never set (LazyNodeDataWrapper implements a method called getWrappedContent instead of overriding getWrappedNodeData())

Comment by Magnolia International [ 21/Jun/11 ]

Comitted a first fix, but it's not enough.
Starting a simple magnolia-empty-webapp instance shows a further issue: nodeData.getJCRProperty() returns null, leading to yet another NPE in info.magnolia.cms.util.LazyNodeDataWrapper#getWrappedNodeData this time.

Comment by Magnolia International [ 21/Jun/11 ]

Need feedback for the next fix:
Replacing the

            if(nodeData == null || !nodeData.getJCRProperty().getSession().isLive()) {

by either

            if(nodeData == null || !nodeData.isExist() || !nodeData.getJCRProperty().getSession().isLive()) {

or

            if(nodeData == null || (nodeData.isExist() && !nodeData.getJCRProperty().getSession().isLive())) {

Works.

So in one case, we "fetch" if the property does not exist or if the session is dead.
In the second case, we only fetch if the property exists and the session is dead.

In the first case, we'll "fetch" all the time if it's a property we "know" does not exist.
In the second case, we'll never "fetch" that property, even though it might have been created in the meantime.

Both cases seem to solve at least the startup/basic use issues.

Comment by Jan Haderka [ 22/Jun/11 ]

I think it is more correct to fetch the property even if it doesn't exist. After all we allow multiple sources of content modification and repo might have been modified by other user or observer in the mean time. In such case proper relogin of the user and reinitialization of the session (or session.refresh() ) would allow us to obtain such property. There's no reason to force such refresh or relogin if we can deal with it automatically.

Comment by Magnolia International [ 22/Jun/11 ]

The thing that makes me hesitate is that this "if" block is in "lazy" nodedata. If the (wrapped) nodeData is not null, it means we already fetched it before. So it sounds like the reasonable thing to do is to refetch it only if the session is dead.
(much like we don't force a refresh on non-lazy implementations)

Comment by Jan Haderka [ 22/Jun/11 ]

Actually not true. If i read the code properly, there is an init method that let you to create it with repo name and path only.

Comment by Magnolia International [ 22/Jun/11 ]

Hu yes, in which case nodeData is null, thus entering the if block straight at the first condition

Comment by Philipp Bärfuss [ 30/Jun/11 ]

I favor the following:

if(nodeData == null || (nodeData.isExist() && !nodeData.getJCRProperty().getSession().isLive())) {

My argument is that if the nodedata doesn't exist then we don't want to access the repository on every method call. The disadvantage is that we will never load the nodedata when it is added later. But this is OK because it is not part of the contract that we automatically refresh the content.

Comment by Jan Haderka [ 25/Jul/11 ]

Waiting for review before backporting to trunk.

Comment by Jan Haderka [ 27/Jul/11 ]

getReferencedContent returns unwrapped content as well.

Comment by Philipp Bärfuss [ 02/Aug/11 ]

port to trunk let the build fail

Generated at Mon Feb 12 03:49:09 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.