Proposed patch:
Additional discussion needed, since this may broke existing implementations,
which used null values to distinguish between list not present vs. list is empty.
Reopened, bugfix will be merged on 27-07-2014, before 7AM PST.
Some regressions may be introduced probably.
Meanwhile also separately raised on https://wiki.opendaylight.org/view/YANG_Tools:Design:Binding_Specification_v2_Analysis#Major.2C_low-impact:_DataObject_Builder_should_initialize_List_.26_Set_.28all_collection.29_fields_to_be_non-null_.28Bug_1097.29
rovarga on IRC chat suggested to first "check if v2 [binding2] produces nulls".
This would be confusing for end users, as they would see an empty list even when that list is required to have a certain number of elements.
YANGTOOLS-585 solves this the other way around: empty lists are never observed, so at least half of this issue is solved, while maintaining API compatibility and consistent behavior: when the user sees a list, it is guaranteed to contain the number of elements specified by YANG schema.
Reopening this for a follow-up, as it seems we have a ton of code which assumes empty lists are present – https://logs.opendaylight.org/releng/vex-yul-odl-jenkins-1/netvirt-csit-1node-1cmb-0ctl-0cmp-openstack-queens-upstream-stateful-neon/31/ is an example.
One thing we can do is to have LazyDataObject return empty lists when the list is not constrained by min-elements > 0.
Codegen cannot ensure this, as generated classes are not guaranteed to have an accurate view of the model (because of class reuse between groupings and their instantiations). Codec has the accurate schema, hence it should be able to perform this accurately.
Actually codegen needs to participate in this, too, as we simply cannot ensure sane .equals()/.hashCode() without that. I am not sure how feasible this is.
https://git.opendaylight.org/gerrit/77102 introduces a helper method, but that is kind of inconvenient for users. I think the best approach here is to generate a default non-null getter (nonnullFoo()), which would perform wrapping to an empty list if the property is null. This has the advantage of being at the user's fingertips. Furthermore we can extend this to normal getters, which can perform an assertion – as opposed to returning a null which can fail at some different call site.
We can also annotate these methods with @NonNull, which will make them quite friendly.
This affects runtime, too, as LazyDataObject needs to understand and simulate default methods. This is an unfortunate consequence of how default methods work with dynamic proxies plus the fact that there is no consistent way to invoke a default method on both JDK8 and JDK9+.
This is what we get for any list:
/** * @return <code>java.util.List</code> <code>grplst</code>, or <code>null</code> if not present */ @Nullable List<Grplst> getGrplst(); /** * @return <code>java.util.List</code> <code>grplst</code> */ default @NonNull List<Grplst> nonnullGrplst() { return CodeHelpers.nonnull(getGrplst()); }
this does not include leaf-lists, as they do not have the same lifecycle rules as lists.
rovarga in terms of "IDE auto-completion discoverability" for users, how about naming it getGrplstNonNull() instead of nonnullGrplst() ? Or were you worried about name clashes? It's very unlikely that the YANG model would contain a property name grplstNonNull ... Or if that's a total absolute no go for you (shame!), then how about at least the prettier camel nonNullGrplst() instead of nonnullGrplst() ?
In terms of mutability of the returned list, skitt and I were chatting about this on IRC earlier this week... so in Binding v1 getGrplst() still returns a mutable list, doesn't it? Whether that is right or wrong should be a separate discussion in another issue, fact is that today it is. Theferefore, if the non-null variant returns an immutable list (but only if it's null, otherwise it's the original, which is mutable), then that is very inconsistent and could lead to very strange and hard to discover bugs in any code that relies on getters of lists returning mutable lists (just because it DOES today, and people could, and we have observed in some cases have, relied on that).
See also new MDSAL-384 & MDSAL-385, for separate future discussion.
No, binding v1 doesn’t always return a mutable list.
OK, but even if it doesn't always return a mutable list, but just sometimes, my point stands?
As for naming, we cannot use suffixes because we have no control over YANG names, think about this:
container foo {
leaf bar {
type string;
}
leaf bar-non-null {
type string;
}
}
As for mutability, it is left unspecified in the generated interface, which is completely on purpose. Generated builders retain whatever the user specified (which may or may not be mutable), while the DOM-backed implementation always returns immutable (as it is a view). Therefore immutable list is all we can offer in the contract.
The only way you can rely on getGrplst() returning a mutable list is when you control the source of the object – in which case you can initialize it to an ArrayList and nonnullGrplst() will do the right thing.
It's really confused (at my first glance) that when to use 'getGrplst()' or 'nonnullGrplst()' 
and once people are used to calling 'nonnullGrplst()' then ‘getGrplst()' may not be necessary.
To what extent could this be possible:
if (myObjects.getListParam() == null) { // mean one thing } if (myObjects.getListParam() != null && myObjects.getListParam().size() == 0) { // mean another thing }
Probably internally calling 'nonnullGrplst()' by 'getGrplst()' would be fine in my opinion.
Major difference is that getListParam() is a property accessor, whereas nonnullListParam() is a convenience mapper. The former contributes to hashCode/equals, where null is not the same thing as equals(). We cannot treat the two methods as equivalent, as for example, datastore merge on with a non-present list is a different thing than a merge with an empty list.
Furthermore, as noted above, returning an empty list where the model dictates min-elements > 0 would end up being confusing and there is no way we can make that work in Binding V1, as interfaces are not generated for all instantiations:
grouping foo {
list bar;
}
container baz {
uses foo;
}
container zyx {
uses foo {
augment bar {
min-elements 1;
}
}
}
Both instantiations of 'list bar' are backed by the same interface (generated at 'grouping foo'), but its getBar() has a different meaning: in may be absent from 'baz', but it must be present in 'zyx'.
The proposed approach is the only way I see to deliver requested functionality, without changing DTO semantics and thereby introducing unintended side-effects and breakage.
Clear enough, returning an empty list has different semantics,
as for ' getListParam()', an empty list means there exists an data node in the data tree whereas returning 'null' means not existing.
and for ' nonnullListParam()', an empty list is just a mapping result value which could only be used for the convenience coding.
To be honest，it's more convoluted to users to catch these mess above than directly do a 'null' checking.
I think it does help users; instead of
if (blah.getListParam() != null) { for (Param param : blah.getListParam()) { // ... } }
they can write
for (Param param : blah.nonnullListParam()) { // ... }
directly. It seems like a minor gain when considered in isolation, but for code which manipulates a lot of lists it definitely makes the code more readable.
It also helps a lot when writing streaming code.
It also has the benefit of accessing the property once, which has performance benefits with LazyDataObject. It also helps Eclipse nullness analysis – which does not regard the null check above to guarantee the next invocation returning non-null (because it cannot know the two invocations are guaranteed to return the same thing). Doing the equivalent is quite a bit of work:
final List<Param> local = blah.getParam(); if (local != null) { for (Param param : local) { // ... } }
This introduces a regresion in RPC interfaces:
@CheckReturnValue @Nullable ListenableFuture<RpcResult<IsClientAbortedOutput>> isClientAborted();
That Nullable should not be there – which means that method generation is slightly busted.