Add support for caller to determine origin for union_replace

[dplore]

Similar to #176 we've also run into several scenarios where it is more convenient for the caller to specify the origin when using union_replace batch functions. This changes origin to match whatever is passed in by the caller.

[coveralls]

Coverage Report for CI Build 25770024131

Coverage increased (+0.03%) to 4.469%

Details

• Coverage increased (+0.03%) from the base build.
• Patch coverage: 18 uncovered changes across 2 files (75 of 93 lines covered, 80.65%).
• No coverage regressions found.

Uncovered Changes

File	Changed	Covered	%
ygnmi/ygnmi.go	14	3	21.43%
ygnmi/gnmi.go	79	72	91.14%

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	52142
Covered Lines:	2330
Line Coverage:	4.47%
Coverage Strength:	0.05 hits per line

---

💛 - Coveralls

[robshakir]

I'm not sure I actually see the implementation of the description here (or at least it's not tested).

Can you please add unit tests that demonstrate the setting of the value of the origin to the value set by the caller?

[robshakir]

This seems a bit obtuse, why is it that setting val to string MUST mean that we are specifying a union replace with CLI? If I am setting /system/config/hostname then can't the value be a string here?

[dplore]

True, /system/config/hostname takes a string! This logic (or similar) is needed ygnmi's use of pointers under the hood for OpenConfig scalar leaf values compared to plain values for CLI text.

From what I can understand, this is how ygnmi distinguishes the two:

1. Origin openconfig schema Leaf Updates (e.g., /system/config/hostname)

When you invoke set or BatchUnionReplace on an OpenConfig leaf like /system/config/hostname:

• The framework checks if this query target is a leaf and scalar: q.isLeaf() && q.isScalar().
• Because OpenConfig fields in generated GoStructs use pointers to represent potentially unset values, the framework converts the value to a pointer type:

  if q.isLeaf() && q.isScalar() {
      setVal = &val
  }

• Next when populateSetRequest is called, the incoming dynamic val parameter is a *string, not a plain string.

• Inside gnmi.go line 520-523 block, the type assertion s, ok := val.(string) performs a lookup against the concrete string type. Because the type is *string, ok evaluates to false.

• Thus, the CLI-override block is bypassed, leaving isCLIUnionReplace = false. The request then correctly proceeds to translate *string into its JSON/JSON-IETF representation (e.g., "baz").

2. Origin cli Union Replace

On the other hand, native CLI configuration isn't in the OpenConfig schema tree. When you queue a CLI replacement using BatchUnionReplaceCLI:

You pass the raw CLI as a plain string (e.g., "interface Ethernet1\n mtu 1500").
Since the target is not a structured schema leaf, the framework leaves val as a plain string instead of converting it to a pointer.
In populateSetRequest, when the block evaluates:

  if s, ok := val.(string); ok && op == unionreplacePath {
      isCLIUnionReplace = true
      cliUnionReplaceVal = s
  }

the dynamic type is a plain string, making ok evaluate to true.

• This triggers isCLIUnionReplace = true, which correctly outputs a schema-free gpb.TypedValue_AsciiVal wrapping the raw CLI command.

[dplore]

Resolving, but feel free to provide suggestions on a different way to approach this. I am not sure what would be more elegant.

[robshakir]

It seems quite fragile to me to have this assumption be made -- how do we guarantee that all callers are aware that setting val to string means one thing and *string means another.

What is the value of path being set to when one is creating such an update? It seems like the tuple of path and val really specifies what one wants to set, rather than the type of val.

[dplore]

It seems the implementation took a shortcut by exploiting a difference in how
BatchUnionReplace and BatchUnionReplaceCLI populate their values:

In BatchUnionReplace, if a query is a scalar leaf (q.isLeaf() && q.isScalar()), its value is passed as a pointer (&val -> *string).

CLI Configs: In BatchUnionReplaceCLI, the ASCII snippet is passed as a raw, non-pointer string.

I will try having BatchUnionReplaceCLI wrap the CLI string in a dedicated custom type (type cliAsciiConfig string). Then populateSetRequest can type-assert on val.(cliAsciiConfig) without any risk of colliding with normal Go string values.

[dplore]

Please check now

[dplore]

Member

I'll raise a separate PR for this one, standby