[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: |
|
||||||||||||||||||||
| Issue Links: |
|
||||||||||||||||||||
| 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. |
| Comment by Magnolia International [ 21/Jun/11 ] |
|
Need feedback for the next fix: 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 first case, we'll "fetch" all the time if it's a property we "know" does not exist. 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. |
| 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 |