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.

Sign up for GitHub

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

RDSK-9311: (gripper/input-controller) Guard against nil responses in rdk client/servers #4620

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

RDSK-9311: (gripper/input-controller) Guard against nil responses in rdk client/servers #4620

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
392e425
add nil geometries check to grippper
martha-johnston Dec 11, 2024
517c7c3
add nil geometries test to client
martha-johnston Dec 11, 2024
434ee1c
add input controller nil checks
martha-johnston Dec 11, 2024
5482948
lint fix
martha-johnston Dec 11, 2024
eb8d23e
fix tests:
martha-johnston Dec 11, 2024
e2962cf
remove unused inject input controller
martha-johnston Dec 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions components/gripper/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ func TestClient(t *testing.T) {
injectGripper2.StopFunc = func(ctx context.Context, extra map[string]interface{}) error {
return errStopUnimplemented
}
injectGripper2.GeometriesFunc = func(ctx context.Context) ([]spatialmath.Geometry, error) {
return nil, nil
}

gripperSvc, err := resource.NewAPIResourceCollection(
gripper.API,
Expand Down Expand Up @@ -145,6 +148,9 @@ func TestClient(t *testing.T) {
test.That(t, err, test.ShouldNotBeNil)
test.That(t, err.Error(), test.ShouldContainSubstring, errStopUnimplemented.Error())

_, err = client2.Geometries(context.Background(), extra)
test.That(t, err.Error(), test.ShouldContainSubstring, gripper.ErrGeometriesNil(failGripperName).Error())
Copy link
Member

Choose a reason for hiding this comment

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

wouldErrors.Is work here? I think we are trying to avoid using substring for errors test when possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the problem is that it adds "rpc error: code = Unknown desc = " before the actual error, so figured it was easier to use substring. in the server code it doesn't add that string, so you can just use test.ShouldBeError

Copy link
Contributor Author

Choose a reason for hiding this comment

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

although I guess I didn't try Errors.Is perhaps that's easier, I've never seen that in the test.That() format, I'll try it out

Copy link
Member

Choose a reason for hiding this comment

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

makes sense thanks for explaining! I meant test.ShouldBeError so prob fine to leave as is


test.That(t, client2.Close(context.Background()), test.ShouldBeNil)
test.That(t, conn.Close(), test.ShouldBeNil)
})
Expand Down
9 changes: 9 additions & 0 deletions components/gripper/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package gripper

import (
"context"
"fmt"

commonpb "go.viam.com/api/common/v1"
pb "go.viam.com/api/component/gripper/v1"
Expand All @@ -13,6 +14,11 @@ import (
"go.viam.com/rdk/spatialmath"
)

// ErrGeometriesNil is the returned error if gripper geometries are nil.
var ErrGeometriesNil = func(gripperName string) error {
return fmt.Errorf("gripper component %v Geometries should not return nil geometries", gripperName)
}

// serviceServer implements the GripperService from gripper.proto.
type serviceServer struct {
pb.UnimplementedGripperServiceServer
Expand Down Expand Up @@ -90,5 +96,8 @@ func (s *serviceServer) GetGeometries(ctx context.Context, req *commonpb.GetGeom
if err != nil {
return nil, err
}
if geometries == nil {
return nil, ErrGeometriesNil(req.GetName())
}
return &commonpb.GetGeometriesResponse{Geometries: spatialmath.NewGeometriesToProto(geometries)}, nil
}
22 changes: 22 additions & 0 deletions components/gripper/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,15 @@ import (
"errors"
"testing"

"github.com/golang/geo/r3"
pbcommon "go.viam.com/api/common/v1"
pb "go.viam.com/api/component/gripper/v1"
"go.viam.com/test"
"go.viam.com/utils/protoutils"

"go.viam.com/rdk/components/gripper"
"go.viam.com/rdk/resource"
"go.viam.com/rdk/spatialmath"
"go.viam.com/rdk/testutils/inject"
)

Expand Down Expand Up @@ -56,6 +59,14 @@ func TestServer(t *testing.T) {
extraOptions = extra
return nil
}
injectGripper.GeometriesFunc = func(ctx context.Context) ([]spatialmath.Geometry, error) {
box, err := spatialmath.NewBox(
spatialmath.NewPose(r3.Vector{X: 0, Y: 0, Z: 0}, spatialmath.NewZeroPose().Orientation()),
r3.Vector{},
testGripperName,
)
return []spatialmath.Geometry{box}, err
}

injectGripper2.OpenFunc = func(ctx context.Context, extra map[string]interface{}) error {
gripperOpen = testGripperName2
Expand All @@ -67,6 +78,9 @@ func TestServer(t *testing.T) {
injectGripper2.StopFunc = func(ctx context.Context, extra map[string]interface{}) error {
return errStopUnimplemented
}
injectGripper2.GeometriesFunc = func(ctx context.Context) ([]spatialmath.Geometry, error) {
return nil, nil
}

t.Run("open", func(t *testing.T) {
_, err := gripperServer.Open(context.Background(), &pb.OpenRequest{Name: missingGripperName})
Expand Down Expand Up @@ -122,4 +136,12 @@ func TestServer(t *testing.T) {
test.That(t, err, test.ShouldNotBeNil)
test.That(t, err, test.ShouldBeError, errStopUnimplemented)
})

t.Run("geometries", func(t *testing.T) {
_, err = gripperServer.GetGeometries(context.Background(), &pbcommon.GetGeometriesRequest{Name: testGripperName})
test.That(t, err, test.ShouldBeNil)

_, err = gripperServer.GetGeometries(context.Background(), &pbcommon.GetGeometriesRequest{Name: testGripperName2})
test.That(t, err, test.ShouldBeError, gripper.ErrGeometriesNil(testGripperName2))
})
}
8 changes: 4 additions & 4 deletions components/input/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,10 @@ func TestClient(t *testing.T) {

injectInputController2 := &inject.InputController{}
injectInputController2.ControlsFunc = func(ctx context.Context, extra map[string]interface{}) ([]input.Control, error) {
return nil, errControlsFailed
return nil, nil
}
injectInputController2.EventsFunc = func(ctx context.Context, extra map[string]interface{}) (map[input.Control]input.Event, error) {
return nil, errEventsFailed
return nil, nil
}

resources := map[resource.Name]input.Controller{
Expand Down Expand Up @@ -252,11 +252,11 @@ func TestClient(t *testing.T) {

_, err = client2.Controls(context.Background(), map[string]interface{}{})
test.That(t, err, test.ShouldNotBeNil)
test.That(t, err.Error(), test.ShouldContainSubstring, errControlsFailed.Error())
test.That(t, err.Error(), test.ShouldContainSubstring, input.ErrControlsNil(failInputControllerName).Error())

_, err = client2.Events(context.Background(), map[string]interface{}{})
test.That(t, err, test.ShouldNotBeNil)
test.That(t, err.Error(), test.ShouldContainSubstring, errEventsFailed.Error())
test.That(t, err.Error(), test.ShouldContainSubstring, input.ErrEventsNil(failInputControllerName).Error())
Copy link
Member

Choose a reason for hiding this comment

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

Should we still be testing the previous errors or is it not needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not needed because errControlsFailed is just an error that was defined in this package and was only returned from the injected input controller. now we are testing if the real function returns the expected error


event1 := input.Event{
Time: time.Now().UTC(),
Expand Down
18 changes: 18 additions & 0 deletions components/input/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package input

import (
"context"
"fmt"

"github.com/pkg/errors"
commonpb "go.viam.com/api/common/v1"
Expand All @@ -13,6 +14,17 @@ import (
"go.viam.com/rdk/resource"
)

var (
// ErrControlsNil is the returned error if input controller controls are nil.
ErrControlsNil = func(inputName string) error {
return fmt.Errorf("input controller component %v Controls should not return nil controls", inputName)
}
// ErrEventsNil is the returned error if input controller events are nil.
ErrEventsNil = func(inputName string) error {
return fmt.Errorf("input controller component %v Events should not return nil events", inputName)
}
)

// serviceServer implements the InputControllerService from proto.
type serviceServer struct {
pb.UnimplementedInputControllerServiceServer
Expand All @@ -39,6 +51,9 @@ func (s *serviceServer) GetControls(
if err != nil {
return nil, err
}
if controlList == nil {
return nil, ErrControlsNil(req.Controller)
}

resp := &pb.GetControlsResponse{}

Expand All @@ -62,6 +77,9 @@ func (s *serviceServer) GetEvents(
if err != nil {
return nil, err
}
if eventsIn == nil {
return nil, ErrEventsNil(req.Controller)
}

resp := &pb.GetEventsResponse{}

Expand Down
10 changes: 4 additions & 6 deletions components/input/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@ const (
)

var (
errControlsFailed = errors.New("can't get controls")
errEventsFailed = errors.New("can't get last events")
errTriggerEvent = errors.New("can't inject event")
errSendFailed = errors.New("send fail")
errRegisterFailed = errors.New("can't register callbacks")
Expand Down Expand Up @@ -98,10 +96,10 @@ func TestServer(t *testing.T) {
}

injectInputController2.ControlsFunc = func(ctx context.Context, extra map[string]interface{}) ([]input.Control, error) {
return nil, errControlsFailed
return nil, nil
}
injectInputController2.EventsFunc = func(ctx context.Context, extra map[string]interface{}) (map[input.Control]input.Event, error) {
return nil, errEventsFailed
return nil, nil
}
injectInputController2.RegisterControlCallbackFunc = func(
ctx context.Context,
Expand Down Expand Up @@ -137,7 +135,7 @@ func TestServer(t *testing.T) {
&pb.GetControlsRequest{Controller: failInputControllerName},
)
test.That(t, err, test.ShouldNotBeNil)
test.That(t, err.Error(), test.ShouldContainSubstring, errControlsFailed.Error())
test.That(t, err.Error(), test.ShouldContainSubstring, input.ErrControlsNil(failInputControllerName).Error())
})

t.Run("GetEvents", func(t *testing.T) {
Expand Down Expand Up @@ -186,7 +184,7 @@ func TestServer(t *testing.T) {
&pb.GetEventsRequest{Controller: failInputControllerName},
)
test.That(t, err, test.ShouldNotBeNil)
test.That(t, err.Error(), test.ShouldContainSubstring, errEventsFailed.Error())
test.That(t, err.Error(), test.ShouldContainSubstring, input.ErrEventsNil(failInputControllerName).Error())
})

t.Run("StreamEvents", func(t *testing.T) {
Expand Down
Loading