[MDSAL-218] *Builder classes for "type union" YANG gen. code should have private constructor Created: 09/Jan/17  Updated: 09/Mar/18  Resolved: 13/Feb/18

Status: Resolved
Project: mdsal
Component/s: Binding codegen, Binding V2 codegen
Affects Version/s: None
Fix Version/s: Oxygen

Type: Bug
Reporter: Michael Vorburger Assignee: Jie Han
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified
Environment:

Operating System: All
Platform: All


External issue ID: 7498

 Description   

Background: I'm currently working on a number of fixes for https://github.com/vorburger/xtendbeans (AKA AssertBeans), as Genius & netvirt developers are starting to pick up using this utility, respectively its adaptation to ODL known as AssertDataObjects from org.opendaylight.mdsal.binding.testutils org.opendaylight.mdsal:mdsal-binding-test-utils, because that utility does not yet work with "type union" YANG gen. code, such as e.g. the IpAddress type. (FYI the https://git.opendaylight.org/gerrit/#/c/50137/ has a non-regression type for the upcoming fixes in org.opendaylight.mdsal.binding.testutils / ch.vorburger.xtendbeans.)

While doing this work, I noticed that YANG gen. (stub) code for union type *Builder classes does not have a private constructor, as I would argue they all should have - even if users manually extend those *Builder classes - because users would be adding constructors with additional arguments, never implement the default constructor.

For example, the org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.yangtools.test.union.rev150121.UnionTestTypeBuilder found in org.opendaylight.mdsal:mdsal-binding-test-model:0.10.0-SNAPSHOT looks like this:

public class UnionTestTypeBuilder {
public static UnionTestType getDefaultInstance(java.lang.String defaultValue) {

but, IMHO, is missing this:

private UnionTestTypeBuilder() { }

I can see that e.g. IpAddressBuilder (from mdsal/model/ietf/ietf-inet-types) has such a private constructor, but that seems to have been manually added after that *Builder was originally generated. A number of other code. gen. and not manually edited union type *Builder classes do not have this private constructor - this bug suggests to extend the code generator templates to add this.

This would help make it clear to any developers that these kind of *Builder classes are never meant to be instantiated with their no-args default constructor.

BTW: Personally I find the naming of these *Builder classes a little unfortunate, because they look more like *Factory than *Builder - like they don't have any setter methods, don't implement org.opendaylight.yangtools.concepts.Builder and thus no build() - these really are a different concept, and having named them *Builder is more confusing than if they were simply named *Factory, or anything else other than *Builder - but it's probably way too late to change this - or is it?



 Comments   
Comment by Vratko Polak [ 09/Jan/17 ]

If I understand it correctly, "typedef" may contain both "default" and "type" based on "union", so zero argument constructor could make sense for constructing the default value.
But there may be better mechanisms for creating the default instance than argument-less constructor.

Fix of MDSAL-49 would probably also fix this, but it may be easier to fix this without fixing MDSAL-49.

As for *Builder vs *Factory, there already are multiple "manual" union "builders", so it is too late to change API or naming.
But future auto-generated classes are free to implement Builder interface.

Comment by Peter Kajsa [ 10/Jan/17 ]

binding generator issue, hence moved to md-sal

Comment by Jie Han [ 07/Feb/18 ]

https://git.opendaylight.org/gerrit/68004
https://git.opendaylight.org/gerrit/68005

Generated at Wed Feb 07 20:09:06 UTC 2024 using Jira 8.20.10#820010-sha1:ace47f9899e9ee25d7157d59aa17ab06aee30d3d.