[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 |
||
| 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 { 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. Fix of As for *Builder vs *Factory, there already are multiple "manual" union "builders", so it is too late to change API or naming. |
| 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 |