Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Wildcard entries to return empty data instead of invalid path #299

Merged
merged 15 commits into from
Nov 11, 2024
171 changes: 167 additions & 4 deletions gnmi_server/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1297,11 +1297,11 @@ func runGnmiTestGet(t *testing.T, namespace string) {
},
{
desc: "Get valid but non-existing node",
pathTarget: "COUNTERS_DB",
pathTarget: stateDBPath,
textPbPath: `
elem: <name: "MyCounters" >
`,
wantRetCode: codes.NotFound,
elem: <name: "TRANSCEIVER_DOM_SENSOR" >
`,
wantRetCode: codes.OK,
}, {
desc: "Get COUNTERS_PORT_NAME_MAP",
pathTarget: "COUNTERS_DB",
Expand Down Expand Up @@ -3487,6 +3487,169 @@ func TestClientConnections(t *testing.T) {
}
}

func TestWildcardTableNoError(t *testing.T) {
s := createServer(t, 8081)
go runServer(t, s)
defer s.ForceStop()

fileName := "../testdata/NEIGH_STATE_TABLE_MAP.txt"
neighStateTableByte, err := ioutil.ReadFile(fileName)
if err != nil {
t.Fatalf("read file %v err: %v", fileName, err)
}

var neighStateTableJson interface{}
json.Unmarshal(neighStateTableByte, &neighStateTableJson)

tests := []struct {
desc string
q client.Query
wantNoti []client.Notification
poll int
}{
{
desc: "poll query for NEIGH_STATE_TABLE",
poll: 1,
q: client.Query{
Target: "STATE_DB",
Type: client.Poll,
Queries: []client.Path{{"NEIGH_STATE_TABLE"}},
TLS: &tls.Config{InsecureSkipVerify: true},
},
wantNoti: []client.Notification{
client.Update{Path: []string{"NEIGH_STATE_TABLE"}, TS: time.Unix(0, 200), Val: neighStateTableJson},
client.Update{Path: []string{"NEIGH_STATE_TABLE"}, TS: time.Unix(0, 200), Val: neighStateTableJson},
},
},
}
namespace, _ := sdcfg.GetDbDefaultNamespace()
prepareStateDb(t, namespace)
var mutexNoti sync.Mutex
for _, tt := range tests {

t.Run(tt.desc, func(t *testing.T) {
q := tt.q
q.Addrs = []string{"127.0.0.1:8081"}
c := client.New()
var gotNoti []client.Notification
q.NotificationHandler = func(n client.Notification) error {
mutexNoti.Lock()
if nn, ok := n.(client.Update); ok {
nn.TS = time.Unix(0, 200)
gotNoti = append(gotNoti, nn)
}
mutexNoti.Unlock()
return nil
}

wg := new(sync.WaitGroup)
wg.Add(1)

go func() {
defer wg.Done()
if err := c.Subscribe(context.Background(), q); err != nil {
t.Errorf("c.Subscribe(): got error %v, expected nil", err)
}
}()

wg.Wait()

for i := 0; i < tt.poll; i++ {
if err := c.Poll(); err != nil {
t.Errorf("c.Poll(): got error %v, expected nil", err)
}
}

mutexNoti.Lock()

if len(gotNoti) == 0 {
t.Errorf("expected non zero notifications")
}

if diff := pretty.Compare(tt.wantNoti, gotNoti); diff != "" {
t.Log("\n Want: \n", tt.wantNoti)
t.Log("\n Got: \n", gotNoti)
Copy link
Collaborator

@qiluo-msft qiluo-msft Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to verify if the gnmi response is correct.
Seems like a bug in your manual test: "{}" #Closed

Copy link
Contributor Author

@zbud-msft zbud-msft Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added test to verify gnmi response is empty json, please take a look at EMPTY_JSON.txt.

It is not a bug, as gnmi_client as part of subscribe will print out json_ietf_value as json string and "{}" is a valid response.

json_ietf structured value should be a valid json string, which empty json represented as string, "{}", is.
https://github.com/openconfig/reference/blob/master/rpc/gnmi/gnmi-specification.md#23-structured-data-types

If we use gnmi_get which prints out json and not json string, we indeed get expected empty json
[elem {
name: "PSU_INFO"
}
]
The GetResponse is below

{}

t.Errorf("unexpected updates: \n%s", diff)
}

mutexNoti.Unlock()

c.Close()
})
}
}

func TestNonExistentTableNoError(t *testing.T) {
Copy link
Collaborator

@qiluo-msft qiluo-msft Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestNonExistentTableNoError

Is there a test case for existing table no error? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added test

s := createServer(t, 8081)
go runServer(t, s)
defer s.ForceStop()

tests := []struct {
desc string
q client.Query
want []client.Notification
poll int
}{
{
desc: "poll query for TRANSCEIVER_DOM_SENSOR",
poll: 10,
q: client.Query{
Target: "STATE_DB",
Type: client.Poll,
Queries: []client.Path{{"TRANSCEIVER_DOM_SENSOR"}},
TLS: &tls.Config{InsecureSkipVerify: true},
},
want: []client.Notification{
client.Connected{},
client.Sync{},
},
},
}
namespace, _ := sdcfg.GetDbDefaultNamespace()
for _, tt := range tests {
prepareStateDb(t, namespace)
t.Run(tt.desc, func(t *testing.T) {
q := tt.q
q.Addrs = []string{"127.0.0.1:8081"}
c := client.New()
var gotNoti []client.Notification
q.NotificationHandler = func(n client.Notification) error {
if nn, ok := n.(client.Update); ok {
nn.TS = time.Unix(0, 200)
gotNoti = append(gotNoti, nn)
} else {
gotNoti = append(gotNoti, n)
}
return nil
}

wg := new(sync.WaitGroup)
wg.Add(1)

go func() {
defer wg.Done()
if err := c.Subscribe(context.Background(), q); err != nil {
t.Errorf("c.Subscribe(): got error %v, expected nil", err)
}
}()

wg.Wait()

for i := 0; i < tt.poll; i++ {
if err := c.Poll(); err != nil {
t.Errorf("c.Poll(): got error %v, expected nil", err)
}
}

if len(gotNoti) == 0 {
t.Errorf("expected non zero notifications")
}

c.Close()
})
}
}

func TestConnectionDataSet(t *testing.T) {
s := createServer(t, 8081)
go runServer(t, s)
Expand Down
10 changes: 6 additions & 4 deletions sonic_data_client/db_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -670,11 +670,13 @@ func populateDbtablePath(prefix, path *gnmipb.Path, pathG2S *map[*gnmipb.Path][]
// <5> DB Table Key Key Field
switch len(stringSlice) {
case 2: // only table name provided
res, err := redisDb.Keys(tblPath.tableName + "*").Result()
if err != nil || len(res) < 1 {
log.V(2).Infof("Invalid db table Path %v %v", target, dbPath)
return fmt.Errorf("Failed to find %v %v %v %v", target, dbPath, err, res)
wildcardTableName := tblPath.tableName + "*"
log.V(6).Infof("Fetching all keys for %v with table name %s", target, wildcardTableName)
res, err := redisDb.Keys(wildcardTableName).Result()
if err != nil {
return fmt.Errorf("redis Keys op failed for %v %v, got err %v %v", target, dbPath, err, res)
}
log.V(6).Infof("Result of keys operation for %v %v, got %v", target, dbPath, res)
tblPath.tableKey = ""
case 3: // Third element could be table key; or field name in which case table name itself is the key too
n, err := redisDb.Exists(tblPath.tableName + tblPath.delimitor + mappedKey).Result()
Expand Down
Loading