New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maps with null values cause an unclear exception #184

Closed
StFS opened this Issue Sep 18, 2016 · 6 comments

Comments

Projects
None yet
2 participants
@StFS
Contributor

StFS commented Sep 18, 2016

We have a rather complex application that is getting its configuration from all over the place.

We're in the process of cleaning all of that up and OWNER is a big part of that effort. However, we do have to ease into this and so we have configuration coming from many different sources and we're funneling that into OWNER.

Working on this I just stumbled upon a "problem" where one of the maps that I was passing into OWNER when creating a configuration interface via the ConfigFactory contained a null value.

This resulted in a very cryptic NullPointerException where I had no idea what was causing the problem. It took me quite a while to track down the problem.

I've created a unit test that demonstrates the problem in its simplest form and it's at the bottom of this issue.

I'm wondering whether doing a sanity check to make sure that the maps passed into ConfigFactory.create() do not contain null values would be a good idea? If such a property is encountered, an IllegalArgumentException could be thrown with the key name in the message.

public class Issue184 {

    public interface MyConfig extends Config {

        @Key("null.value.key")
        @DefaultValue("1")
        Integer getNullValueKey();
    }

    private Issue184.MyConfig cfg;

    @Before
    public void before() {
        Map<String,String> propsMapWithNullValue = new HashMap<String,String>();
        propsMapWithNullValue.put("null.value.key", null);
        Map<String,String> propsMapWithValidValue = new HashMap<String,String>();
        propsMapWithValidValue.put("null.value.key", "42");
        cfg = ConfigFactory.create(Issue184.MyConfig.class, propsMapWithValidValue, propsMapWithNullValue);
    }


    @Test
    public void testInvalidValueArray() throws Exception {
        cfg.getNullValueKey();
    }
}

The above code will throw the following exception:

java.lang.NullPointerException
    at java.util.Hashtable.put(Hashtable.java:459)
    at java.util.Hashtable.putAll(Hashtable.java:523)
    at org.aeonbits.owner.PropertiesManager.merge(PropertiesManager.java:349)
    at org.aeonbits.owner.PropertiesManager.load(PropertiesManager.java:219)
    at org.aeonbits.owner.PropertiesManager.load(PropertiesManager.java:207)
    at org.aeonbits.owner.PropertiesInvocationHandler.<init>(PropertiesInvocationHandler.java:54)
    at org.aeonbits.owner.DefaultFactory.create(DefaultFactory.java:46)
    at org.aeonbits.owner.ConfigFactory.create(ConfigFactory.java:66)
    at org.aeonbits.owner.issues.Issue184.before(Issue184.java:31)
...
@StFS

This comment has been minimized.

Contributor

StFS commented Sep 19, 2016

Of course, another option (rather than throwing an IllegalArgumentException with the key name in the message) would be to remove illegal entries from the imports and then just use the modified maps as imports.

@lviggiano

This comment has been minimized.

Owner

lviggiano commented Sep 20, 2016

Hi @StFS

java.util.Properties extends java.util.Hashtable that does not accept null and throws the NullPointerException.

I'll look in your change and I'll make sure that next release will just ignore the nulls instead of throwing this NullPointerException.

Thank you.

lviggiano added a commit that referenced this issue Feb 10, 2017

Merge pull request #185 from StFS/184-null-value-maps
Fix for #184 - have ConfigFactory throw exeption on illegal import maps

@StFS StFS closed this Apr 6, 2017

@StFS StFS reopened this Apr 6, 2017

@StFS

This comment has been minimized.

Contributor

StFS commented Apr 6, 2017

This was fixed with the following PRs:

#185
#196

@lviggiano

This comment has been minimized.

Owner

lviggiano commented Mar 1, 2018

I released today, I think that some documentation for this feature may help users. 👍

@StFS

This comment has been minimized.

Contributor

StFS commented Mar 1, 2018

great to hear! Can you refresh my memory... I've added a small clause about this in the documentation but I'm trying to generate the website but mvn site only seems to generate some maven stuff. How do I generate the documentation website locally so I can verify that everything is looking okay?

@StFS

This comment has been minimized.

Contributor

StFS commented Mar 1, 2018

nevermind... figured it out.

StFS added a commit to StFS/owner that referenced this issue Mar 1, 2018

lviggiano added a commit that referenced this issue Mar 2, 2018

Merge pull request #224 from StFS/184-documentation
Adding some documentation for #184
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment