[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: Text File log-trace-import-command.txt    
Issue Links:
Relates
relates to MGNLUI-3637 SystemLanguagesFieldDefinition should... Closed
causality
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:
I have added a method to test in info.magnolia.security.app.dialog.field.SystemLanguagesFieldDefinitionTest
in order to reproduce the bug.


 @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.
Add patch in info.magnolia.security.app.dialog.field.SystemLanguagesFieldDefinition#getOptions
Implementation 1:
Simple is by pass. If it have no context then return an empty list.

        if (!MgnlContext.hasInstance()) {
            return options;
        }
        Locale currentLocale = MgnlContext.getLocale();

Pros: Simple, ignore unwanted state.
Cons: Just fix exact this class. Don't fix the root cause.

Implementation 2:
Try to get Locale by fallbackLanguage of system or default language of JVM if any.

        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
Cons: Locale is unnecessary in this case. But useful in some cases. Don’t fix the root cause.

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)
When Map2Bean or Node2Bean mechanism transform yaml or node to bean, it will call get/ set methods in bean object. So it trigger info.magnolia.security.app.dialog.field.SystemLanguagesFieldDefinition#getOptions and throw NPE due to MgnlContext is null.
Null context is the root cause of the bug but it’s not the root cause of the issue. So what is the root cause?

  • Regard to YAML file, it will be observed for changes from info.magnolia.config.source.yaml.YamlConfigurationSource by using info.magnolia.resourceloader.ResourceOrigin#watchForChanges then it will call info.magnolia.config.source.yaml.YamlConfigurationSource#loadAndRegister to transform to bean from data read from yaml file if any changes.
  • Regard to JCR node, it will be observed for changes from info.magnolia.config.source.jcr.JcrConfigurationSource by using info.magnolia.config.source.jcr.RegistryBasedObservingManager and reload if needed and then transform node to bean by using Node2Bean meachanism in info.magnolia.config.source.jcr.JcrConfigurationSource#newProvider
    Then I believe that we need to initialize appropriate context when Map2Bean or Node2Bean are running in info.magnolia.config.source.yaml.YamlConfigurationSource#loadAndRegister and info.magnolia.config.source.jcr.JcrConfigurationSource#newProvider
// init context
MgnlContext.setInstance(new SimpleContext(Components.getComponent(SystemContext.class)));

Impaction: magnolia-configuration module
Pros: Fix the root cause
Cons: widespread impact, need more time for fixing and testing.

Comment by Richard Gange [ 23/Oct/15 ]

When Map2Bean or Node2Bean mechanism transform yaml or node to bean, it will call get/ set methods in bean object. So it trigger info.magnolia.security.app.dialog.field.SystemLanguagesFieldDefinition#getOptions and throw NPE due to MgnlContext is null.

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 ]

We saw some error logs look like the same in Node2Bean.

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.
See log trace at log-trace-import-command.txt

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:

but when I import a command config under dam-app, the error is the same.

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. Node2Bean

Let'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:

Return the entire set of properties for which the specified bean provides a read method. This map contains the to String converted property values for all properties for which a read method is provided.

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.
Those 2bean transformers should only set bean properties, so Map2BeanTransformer shouldn't use BeanUtilsBean#describe.

MgnlContext vs. Definitions

No-go. Ever. Period.

  1. Startup: Configuration is loaded once per instance, and is shared between all components, users, etc.
    • Making definitions return contextual info (e.g. currentLocale) defeats the purpose of having a single, valid, reliable configuration for all.
    • We don't get the NPE upon startup, only because we're lucky to be in system context.
  2. Reload is typically done asynchronously, be it via JCR or file observation.
    • Indeed in the thread executing file observation, there is rightfully no Context.

So what should we do?

  • Definitions should always be stupid POJOs.
  • The logic in #getOptions() should be moved to a custom SelectFieldFactory, overriding #getSelectFieldOptionDefinition()
    • the FieldTypeDefinition should be registered in in JCR config, under security-app module.

I will create a new ticket for this.

Comment by Ngoc Nguyenthanh [ 10/Nov/15 ]
  • Fixed by replace BeanUtilsBean#describe by org.apache.commons.beanutils.PropertyUtilsBean#getPropertyDescriptors in order to prevent Map2BeanTransformer call getters while trying to resolve bean object.
  • Add a unit-test to make sure all getters should not be called after bean object has been resolved.
Generated at Mon Feb 12 04:14:10 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.