Skip to content
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

Nestings: Inconsistent handling of 'null' in outer property #9

Closed
acepe opened this issue Apr 27, 2015 · 5 comments
Closed

Nestings: Inconsistent handling of 'null' in outer property #9

acepe opened this issue Apr 27, 2015 · 5 comments

Comments

@acepe
Copy link

acepe commented Apr 27, 2015

Hello Nicolai,
I just noticed some inconsistencies with handling null-values in Nestings.
If you create a nesting, and the outer property ('currentEmployeeProperty' in your example) contains 'null', a NullPointerExcepton is thrown.
However if the nesting was already created (using a non-null Employee) and the currentEmployeeProperty's value is set to 'null' at a later time, no exception is thrown.Additionally, the NestedProperty is not updated and keeps its old value. I have created a simple test showing the behaviour.

I know that 'null' in the outer property (Employee) is ambigous. What should happen if there is a Nesting on, for example Employee::nameProperty, the Employee is null, and the Name is changed? Either a Exception could be thrown (there is no Employee whose name can be set), or a new Employee could be created, and the name set.

I would personally really like it if Nestings could handle both described alternatives.

  • If the outer properties value becomes 'null', the inner property should become null, too (not keep its last value, as it is currently).
  • Throwing a NullPointerException could be the default way of handling 'null' in outer properties when writing to inner-properties.
  • As an alternative one could specifiy an optional Supplier, which creates a new value for the outer property.
@nipafx
Copy link
Owner

nipafx commented Apr 28, 2015

Hi @acepe,

thanks for this detailed description! I'll look into it as soon as I find some time.

nipafx pushed a commit that referenced this issue Apr 28, 2015
As pointed out by #9, creating a 'DeepNesting' on an outer observable which contains null does not work as intended. Its 'innerObservableProperty' will contain null instead of an empty 'Oprional'. The same faulty behavior was found when one of the nested observables contains null.

The problem was that the nesting's initialization could not distinguish between uninitialized values (which were represented as null) and null values contained in those observable. This lead to the initialization following an incorrect (and ultimately incomplete) computational path.

The fix changes the way the value array is created. Instead of containing null for uninitialized values, it contains arbitrary instances which are not null an equal no instances which appears "in the wild".
@nipafx
Copy link
Owner

nipafx commented Apr 28, 2015

I found some time to look into the matter:

NPE During Construction

As the code is written at the moment, NullPointerExceptions should never occur. That it does when the outer observable (or any other observable, really) contains null, is a bug. Thank you for spotting it! Great work (and also awesome that you provided a test! 👍 ).

I squashed it in e46cc83 and will soon release a version 0.2.1 which contains the fix.

Setting Null (Present)

The behavior you observe when one of the observables gets a null value is as intended (see Javadoc for NestedProperty).

In short: When one of the observables gets a null value, the inner observable can not be accessed anymore (is missing). But the nested property does not take on a new value - this is largely due to the fact that it is unclear what this value would be (null is ambiguous and not possible for primitive wrapping properties). To give you an opportunity to deal with this scenario, the property has an innerObservablePresentProperty which indicates this.

Just to avoid misunderstandings: It would be great if you could confirm that the behavior described here and in the documentation is what you observe. As soon as you do, I will release 0.2.1.

Setting Null (Future)

That behavior is stupid. 😉 Well, not really but it isn't great either. And your suggestions are very much in line with what I had in mind but didn't implement yet.

If you open another ticket (which I will mark as a feature request), we can discuss which options exist and I will implement it for 0.3.0 which I plan for sometime in May.

@acepe
Copy link
Author

acepe commented May 1, 2015

Hello @nicolaiparlog. Thanks for the quick fix. I tested it, setting null works as described.

@nipafx
Copy link
Owner

nipafx commented May 1, 2015

Ok, I will release 0.2.1 this afternoon (hopefully).

Btw, regarding the future of Setting Null, see #3. We can discuss your ideas there.

@nipafx
Copy link
Owner

nipafx commented May 1, 2015

I just released 0.2.1 which contains this fix.

@nipafx nipafx closed this as completed May 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants