[MDSAL-667] Binding DTO bindingHashCode() is inconsistent with augmentations Created: 15/Jun/21 Updated: 01/Dec/21 |
|
| Status: | Confirmed |
| Project: | mdsal |
| Component/s: | Binding codegen |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Bug | Priority: | Medium |
| Reporter: | Robert Varga | Assignee: | Unassigned |
| Resolution: | Unresolved | Votes: | 0 |
| Labels: | pt | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||
| Description |
|
eOur implementation of bindingHashCode() is wrong as demonstrated (but not reported) in Our implementation treats augmentations attached to a particular object as a Map and performs:
final int prime = 31; int result = 1; result = prime * result + obj.augmentations().hashCode(); return result; This looks okay and works in most situations, but unfortunately it breaks down when equivalent augmentations are involved.
Equivalent augmentations come from the combination of our design choice, where we do not follow the effective YANG model, but rather a declared/effective hybrid. In this hybrid world, we generate interfaces for groupings and use composition of those to model 'uses foo' inheritence. This leads to improved data mobility, as we have a common construct which is shared by all instantiations of a particular grouping. It also reduces the number of generated classes by a combinatoric factor. Unfortunately this does not play nicely with the concept of augmentations, as these operate on effective model, but has a tangled story when it comes to manifestations. One of the tangles is evident openflowplugin-extension-nicira-match.yang, where we are augmenting multiple (hopefully all) use sites of a grouping with the same content – effectively saying that the original grouping should have an extension, in this case should include 'all-matches-grouping': augment "/sal-flow:add-flow/sal-flow:input/sal-flow:match/ext-gen:extension-list/ext-gen:extension" {
ext:augment-identifier "nx-aug-match-rpc-add-flow";
uses all-matches-grouping;
}
augment "/sal-flow:remove-flow/sal-flow:input/sal-flow:match/ext-gen:extension-list/ext-gen:extension" {
ext:augment-identifier "nx-aug-match-rpc-remove-flow";
uses all-matches-grouping;
}
With these we have a number of classes generated for the same augment, and since we are pointing to a grouping, their type indicates they are augmentations of the same construct (in the hybrid world) but they are distinct in effective world. This breaks down type safety, as these augments can be used interchangeably and can be transported all over the place via composition of the grouping. This, obviously, is not acceptable in YANG instantiated world, as represented via NormalizedNode structures – and therefore mdsal-binding-dom-codec has to deal with interpreting augmentations correctly. As a side-effect, incidental augmentation misuse morphs to the correct augmentation when we reconstruct a DTO from NormalizedNodes – and we have to deal with aliases in DataObjectCodecContext.mismatchedAugmentationByClass(). We therefore cannot use the augment's class identity in either hashCode() nor equals() implementation, simply because if we happen to have a mismatched augmentation, storing the object and then reading it back will result in an object which does not hash (nor compare) as equal. Our hashing (and equality) contract must not short-circuit to Map.hashCode()/equals(), but rather treat the attached augmentations as a Set – for both hashCode() and equivalence purposes. |
| Comments |
| Comment by Robert Varga [ 15/Jun/21 ] |
|
There is a problem with implementing equality contract, though, as at least one of the participants in an foo.equals(bar) invocation must be able to translate the invocation. This is possible for binding-dom-codec, as it has the corresponding schema and class world view, but is problematic for generated code due to incomplete view of the world with split-compilation. One possible approach to solving this would be to explictly mark augmentation equivalence for this sort of use case with a yang-ext.yang extension, such as: extension augment-equivalent {
argument augment-reference;
}
and the block above could be: prefix foo;
augment "/sal-flow:add-flow/sal-flow:input/sal-flow:match/ext-gen:extension-list/ext-gen:extension" {
ext:augment-identifier "nx-aug-match-rpc-add-flow";
uses all-matches-grouping;
}
augment "/sal-flow:remove-flow/sal-flow:input/sal-flow:match/ext-gen:extension-list/ext-gen:extension" {
ext:augment-equivalent "nx-aug-match-rpc-add-flow";
uses all-matches-grouping;
}
The cross-module use case is also supported with proper prefix handling, i.e. "foo:nx-aug-match-rpc-add-flow". Augmentations tagged with augment-equivalent would not have a generated class, but rather would be checked at codegen (and runtime) for compatibility with the referenced augmentation (which must be tagged with augment-identifier, obviously) and be reused. This side-steps the problem by eliminating the secondary classes, not completely solving it.
|