[MAGNOLIA-6400] Map2Bean tranformer throws NPE if definition class uses MgnlContext Created: 04/Oct/15 Updated: 13/Nov/15 Resolved: 10/Nov/15 |
|
| Status: | Closed |
| Project: | Magnolia |
| Component/s: | configuration |
| Affects Version/s: | 5.4.2 |
| Fix Version/s: | 5.4.4 |
| Type: | Bug | Priority: | Neutral |
| Reporter: | Richard Gange | Assignee: | Ngoc Nguyenthanh |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | Map2Bean, quickwin, support | ||
| Remaining Estimate: | 1d 7h | ||
| Time Spent: | 5d 5h | ||
| Original Estimate: | 0.25d | ||
| 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: | |||||||||||||
| Sprint: | Saigon 15, Saigon 18 | ||||||||||||
| Story Points: | 8 | ||||||||||||
| Description |
|
For an example of this see info.magnolia.security.app.dialog.field.SystemLanguagesFieldDefinition. It has a method getOptions() which uses MgnlContext. When I configure this field in the JCR I never pass through SystemLanguagesFieldDefinition#getOptions(). So that is why we don't see this MgnlContext issue when configuring the dialog in JCR. On the other hand, when configuring in yaml, when I save the dialog I pass through SystemLanguagesFieldDefinition#getOptions() for whatever reason. The dialog is still usable but you do receive a warning when the dialog saves. A possible workaround would be to move the loading of options to the factory class or just ignore the warning. |
| Comments |
| Comment by Ngoc Nguyenthanh [ 23/Oct/15 ] |
|
Reproduce:
@Test
public void shouldPassWhenContextIsNull() throws Exception {
// GIVEN
MgnlContext.setInstance(null);
final Map<String, Object> map = new LinkedHashMap<String, Object>();
map.put("class", "info.magnolia.security.app.dialog.field.SystemLanguagesFieldDefinition");
MockComponentProvider componentProvider = new MockComponentProvider();
TypeMapping mapping = new TypeMappingImpl();
PreConfiguredBeanUtils beanUtils = new PreConfiguredBeanUtils();
Map2BeanTransformer map2BeanTransformer = new Map2BeanTransformer(componentProvider, mapping, beanUtils);
// WHEN
SystemLanguagesFieldDefinition definition = map2BeanTransformer.toBean(map, SystemLanguagesFieldDefinition.class);
// THEN
assertTrue(definition != null);
}
|
| Comment by Ngoc Nguyenthanh [ 23/Oct/15 ] |
|
Solution 1: Fix the bug with minimum impaction.
if (!MgnlContext.hasInstance()) {
return options;
}
Locale currentLocale = MgnlContext.getLocale();
Pros: Simple, ignore unwanted state. Implementation 2:
Locale currentLocale = null;
if (MgnlContext.hasInstance()) {
currentLocale = MgnlContext.getLocale();
} else {
final String fallbackLanguagePath = "fallbackLanguage";
if (systemLanguages.hasProperty(fallbackLanguagePath)) {
String langCode = systemLanguages.getProperty(fallbackLanguagePath).getString();
if (StringUtils.isNotEmpty(langCode)) {
currentLocale = new Locale(langCode);
}
}
}
if (currentLocale == null) {
currentLocale = Locale.getDefault();
}
Pros: Same with implementation 1 but add ability to get a fallback Locale Solution impaction: info.magnolia.security.app.dialog.field.SystemLanguagesFieldDefinitionTest in security-app module. |
| Comment by Richard Gange [ 23/Oct/15 ] |
|
IMHO We should try to figure why this behavior is different in Map2Bean. In Node2Bean we don't have this issue, why? I think this indicates there is an issue with Map2Bean that needs to be fixed. |
| Comment by Ngoc Nguyenthanh [ 23/Oct/15 ] |
|
Solution 2: Fix the root cause related to Map2BeanTransformer and Node2BeanTransformer didn’t provide MgnlContext From org.apache.commons.beanutils.PropertyUtilsBean#getIndexedProperty (line:542 - Call the property getter and return the value)
// init context MgnlContext.setInstance(new SimpleContext(Components.getComponent(SystemContext.class))); Impaction: magnolia-configuration module |
| Comment by Richard Gange [ 23/Oct/15 ] |
This isn't an issue in Node2Bean... |
| Comment by Ngoc Nguyenthanh [ 23/Oct/15 ] |
|
Hi rgange We saw some error logs look like the same in Node2Bean. Could you please see my solution 2 above and give me some comments? Thanks |
| Comment by Ngoc Nguyenthanh [ 23/Oct/15 ] |
|
I have run the unit-test, so the bug has been introduced from 5.4. I didn’t test for 5.3 and below. |
| Comment by Ngoc Nguyenthanh [ 23/Oct/15 ] |
|
Hi mgeljic Please have a look and give me some comments. Thanks |
| Comment by Richard Gange [ 23/Oct/15 ] |
Can I see your stack trace because I cannot reproduce it using the Node2Bean mechanism (a.k.a configuring in config app)? If Node2Bean had this issue wouldn't we had seen it a long time ago? Map2Bean is newer. I think the issue is Map2Bean might be calling every setter rather than just the setters for configured properties. Which would be inefficient. I don't know that for sure though, it's just my hunch. |
| Comment by Sang Ngo Huu [ 23/Oct/15 ] |
|
rgange, I don't know Node2Bean is error or not, but when I import a command config under dam-app, the error is the same. ngoc.nguyenthanh Please investigate too. |
| Comment by Richard Gange [ 23/Oct/15 ] |
|
sang.ngo Thanks for that. But this is not the same issue I reported. You mentioned:
It's not the same. The root cause is the same, MgnlContext not set, but not for the same reason. In this case guice cannot inject the context . That's different and should be investigated in another ticket. My issue is this. Try and configure this field on a dialog in the JCR using SystemLanguagesFieldDefinition. After you configure the field's class property to SystemLanguagesFieldDefinition everything works fine. No context error. Now try and configure that same field in a dialog in YAML. When you save the YAML file it fails. So what I'm trying to figure out here is why is getOptions() called when I configure in YAML. Why is a get function being called in a set scenario. When I save my configuration the bean should be set for next use. |
| Comment by Mikaël Geljić [ 28/Oct/15 ] |
|
Hi guys! Map2Bean vs. Node2BeanLet's leave the command issue apart, even if symptoms are similar; I also agree w/ Rich, it must first be understood why reloading this SystemLanguagesFieldDefinition results in an NPE under map2bean, and not under node2bean (which I can confirm). And looking that up (drumroll), the issue stems from Map2BeanTransformer invoking BeanUtilsBean#describe; (node2bean doesn't do that). Reading its javadoc:
In effect, this means that getters are actually called "indirectly" by map2bean. Besides, we don't care about those string-converted values, we don't use them. MgnlContext vs. DefinitionsNo-go. Ever. Period.
So what should we do?
I will create a new ticket for this. |
| Comment by Ngoc Nguyenthanh [ 10/Nov/15 ] |
|