openconfig / ygot

A YANG-centric Go toolkit - Go/Protobuf Code Generation; Validation; Marshaling/Unmarshaling
Apache License 2.0
284 stars 106 forks source link

Doesn't choice and case statement in YANG result in oneof fields in proto? #948

Open Baalajee-S opened 8 months ago

Baalajee-S commented 8 months ago

Input YANG file:

module test {

  yang-version "1";

  namespace "urn:test";
  prefix "test";

  import openconfig-extensions { prefix oc-ext; }

  // meta
  organization "None";

  contact
    "s.baalajee@gmail.com";

  description
    "Defines a data model for test purpose.";

  // Correct?
  oc-ext:openconfig-version "0.3.0";

  revision "2023-12-30" {
    description
      "Test";
    // Correct?
    reference "0.3.0";
  }

  container top {
    choice test-choice {
      case case-foo {
        leaf foo {
          type uint8;
        }
      }
      case case-bar {
        leaf bar {
          type boolean;
        }
      }
    }
  }
}

Output proto file:

// openconfig.test is generated by proto_generator as a protobuf
// representation of a YANG schema.
//
// Input schema modules:
//  - yang/bgpls_topology/test.yang
// Include paths:
//   - yang/...
syntax = "proto3";

package openconfig.test;

import "github.com/openconfig/ygot/proto/ywrapper/ywrapper.proto";
import "github.com/openconfig/ygot/proto/yext/yext.proto";

option go_package = "bgpls_topology_proto/openconfig/test";

message Top {
  ywrapper.BoolValue bar = 292023054 [(yext.schemapath) = "/top/bar"];
  ywrapper.UintValue foo = 298489470 [(yext.schemapath) = "/top/foo"];
}

Should the generated proto be?

// openconfig.test is generated by proto_generator as a protobuf
// representation of a YANG schema.
//
// Input schema modules:
//  - yang/bgpls_topology/test.yang
// Include paths:
//   - yang/...
syntax = "proto3";

package openconfig.test;

import "github.com/openconfig/ygot/proto/ywrapper/ywrapper.proto";
import "github.com/openconfig/ygot/proto/yext/yext.proto";

option go_package = "bgpls_topology_proto/openconfig/test";

message Top {
  oneof foo_bar {
    ywrapper.BoolValue bar = 292023054 [(yext.schemapath) = "/top/bar"];
    ywrapper.UintValue foo = 298489470 [(yext.schemapath) = "/top/foo"];
  }
}
wenovus commented 8 months ago

Hi Baalajee, your proposal makes sense. It would be a breaking change but it conforms to YANG's specs better.

The entrypoint to changing this would be here: https://github.com/openconfig/ygot/blob/f6d9cff4bdcf98553af92a5fef4932cdbc9e8444/genutil/common.go#L267-L286

Since we're maintaining ygot on a best-effort basis, I can't give any estimates on when this can be done, but you're welcome to contribute and I'd be happy to give hints and code review.

robshakir commented 7 months ago

Doing this behind a flag would solve the backwards incompatibility issues at least initially. Some care needs to be taken with what the naming of the oneof is -- but this does seem reasonable. It wasn't something I focused on when implementing this because OpenConfig doesn't use choice/case. I'm also open to helping review a contribution here.

r.

On Thu, Jan 11, 2024 at 10:45 AM Wen Bo Li @.***> wrote:

Hi Baalajee, your proposal makes sense. It would be a breaking change but it conforms to YANG's specs better.

The entrypoint to changing this would be here:

https://github.com/openconfig/ygot/blob/f6d9cff4bdcf98553af92a5fef4932cdbc9e8444/genutil/common.go#L267-L286

Since we're maintaining ygot on a best-effort basis, I can't give any estimates on when this can be done, but you're welcome to contribute and I'd be happy to give hints and code review.

— Reply to this email directly, view it on GitHub https://github.com/openconfig/ygot/issues/948#issuecomment-1887757567, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABE43VOVAOW2MLANR7RG5GDYOAXONAVCNFSM6AAAAABBWU72VKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBXG42TONJWG4 . You are receiving this because you are subscribed to this thread.Message ID: @.***>

--

robjs VRTS

TZ: Pacific (America/Los_Angeles) Desk: SFO-SPE

Please do not feel obligated to reply outside your working hours!

What is now proved was once only imagined - William Blake.