Don't Bail If Optional Property Is Nullified by karthicks · Pull Request #441 · lightbend/config

karthicks · 2016-11-08T19:13:44Z

For details on the problem description, please refer to #440.

havocp · 2016-11-10T13:22:14Z

+                        if (!isOptionalProperty(clazz, beanProp)) {
+                            // bubble the {@link Missing} exception up
+                            throw e;
+                        }


Should this (sometimes?) be explicitly setting the field to null? I feel like if the config does not contain a setting, maybe we keep whatever the bean's constructor set, but if the config explicitly has null in it, we should probably explicitly set to null?

havocp · 2016-11-10T13:23:41Z

+                    }
+                } else {
+                    try {
+                        Object unwrapped = getValue(


I wonder if it would be better to handle null in getValue (would need to pass in to getValue whether the prop is optional and then use config.getIsNull I think). An advantage would be to avoid exceptions in normal control flow, which is nice because they are legitimately pretty slow to throw, and that could matter to some programs.

On second thought, would it be acceptable to treat null values as if they were missing, as the spec states? That way, we can avoid exceptions during the normal control flow.

havocp · 2016-11-10T13:25:27Z

        assertNotNull(beanConfig)
        assertNotNull(beanConfig.getValueObject)
        assertNull(beanConfig.getValueObject.getOptionalValue)
+        assertNull(beanConfig.getValueObject.getNullableValue)


maybe we could test the matrix of combinations of

bean constructor sets to null vs non-null

config explicitly sets to null or simply omits the setting

If only to be sure we don't accidentally change behavior of one of those matrix cells later.

Currently, we do kind of test the use case where a value is set to null versus a value that is simply missing or omitted. The ObjectsConfig#optionalValue field is being used to test the value being omitted, and the ObjectsConfig#nullableValue field for the value being set to null.

havocp · 2016-11-10T13:26:24Z

thanks for the PR!

karthicks · 2017-03-15T21:42:11Z

@havocp , sorry about the belated response. I tweaked the change so that it doesn't rely on the exception-based approach initially proposed. Can you take a look to see if this is more acceptable? -Karthick

havocp · 2017-03-16T16:10:07Z

+        }
+
+        public void setNullableValue(String nullableValue) {
+            this.nullableValue = nullableValue;


I think (could be wrong) that we aren't testing the behavior where the bean constructor sets a non-null value on the optional field, and then either we do not have it in the config, or it's set to null in the config. Intuitively I would think it should be overwritten with null only if explicitly set in the config?

Though i can see the argument for overwriting it with null always (always ignore the bean constructor) maybe ?

I think you're right. If an optional field is defaulted (either through an initializer or the default constructor), and if that field is missing in the configuration, then that default value should take effect. That does seem more intuitive to me.

havocp · 2017-03-16T16:16:47Z

                    }
+                    if (configValue == null && !isOptionalProperty(clazz, beanProp)) {
+                        SimpleConfig.addMissing(problems, expectedType, path, config.origin());
+                    }


Does this imply that the throw new ConfigException.Missing a few lines down is now unreachable? If so we might throw a BugOrBrokenException("Should have detected missing props earlier") down there instead of ConfigException.Missing.

Will do. That makes sense.

karthicks · 2017-03-16T23:19:12Z

@havocp ,

In order to handle fields in the bean that are initialized with a default value, I took the approach you initially suggested, viz., handle null values properly in getValue. Rather than changing that method's signature, I added an optional property to AbstractConfigValue and raise a ConfigException.Null exception only if the value is not optional.

What do you think?

-Karthick

havocp

Thanks for updating this, I apologize for taking a while to reply.

havocp · 2017-03-27T13:41:38Z


    final private SimpleConfigOrigin origin;

+    private boolean optional;


this field won't work for a couple reasons; 1) ConfigValue is immutable, 2) if this was changed to make a copy of the config value, it would still not be conceptually part of the value, it's part of the implied "schema" instead.

will suggest another way to do this below

havocp · 2017-03-27T13:48:08Z

+                    // Otherwise, raise a {@link BugOrBroken} exception
+                    throw new ConfigException.BugOrBroken("Should have detected missing props earlier");
                }
                Object unwrapped = getValue(clazz, parameterType, parameterClass, config, configPropName);


To let this getValue return null, I'd suggest passing in a parameter to getValue optional=true|false, and then at the top of getValue do something like:

if (optional and config.getIsNull(configPropName)) return null;

I think that will work ?

lightbend-cla-validator · 2017-03-31T19:30:11Z

Hi @yellowblood, @camullen,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

karthicks · 2017-03-31T20:20:26Z

@havocp ,

Updated the pull request as per your suggestion, which relies on the config.getIsNull method.

-Karthick Sankarachary

2m · 2017-10-06T12:27:47Z

-                    // Otherwise, raise a {@link Missing} exception right here
-                    throw new ConfigException.Missing(beanProp.getName());
+                    // Otherwise, raise a {@link BugOrBroken} exception
+                    throw new ConfigException.BugOrBroken("Should have detected missing props earlier");


This should be reverted to the original exception, because the related change (which was proposed here) is not in the PR anymore.

Just saw this comment. Don't know why I didn't get an email notification earlier. In any case, I've reverted to the original exception. Sorry for the belated response.

2m · 2017-10-06T12:28:30Z

I think this can be merged when the exception type is reverted.

havocp reviewed Nov 10, 2016

View reviewed changes

havocp reviewed Mar 16, 2017

View reviewed changes

havocp reviewed Mar 27, 2017

View reviewed changes

karthicks force-pushed the issue-440 branch 3 times, most recently from d38fba7 to 2546803 Compare March 31, 2017 20:14

Don't Bail If Optional Property Is Nullified

95592e1

karthicks force-pushed the issue-440 branch from 2546803 to 95592e1 Compare April 3, 2017 21:22

2m reviewed Oct 6, 2017

View reviewed changes

karthicks closed this Oct 30, 2017

karthicks deleted the issue-440 branch October 30, 2017 16:36

karthicks mentioned this pull request Oct 30, 2017

Don't Bail If Optional Property Is Nullified #522

Closed


		final private SimpleConfigOrigin origin;

		private boolean optional;

Conversation

karthicks commented Nov 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

havocp commented Nov 10, 2016

Uh oh!

karthicks commented Mar 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karthicks commented Mar 16, 2017

Uh oh!

havocp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lightbend-cla-validator commented Mar 31, 2017

Uh oh!

karthicks commented Mar 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

2m commented Oct 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants