From a320eed376dce4f8431f17db9fc05aa3dd6aa7e9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9=20Sterba?= <hda@andre-sterba.de>
Date: Sun, 22 Oct 2023 11:54:39 +0000
Subject: [PATCH] Add validation tests for topology
See merge request danet/gosdn!573
---
controller/northbound/server/topology.go | 35 ++++++--
controller/northbound/server/topology_test.go | 86 ++++++++++++++++---
2 files changed, 102 insertions(+), 19 deletions(-)
diff --git a/controller/northbound/server/topology.go b/controller/northbound/server/topology.go
index 085e359b1..099afe534 100644
--- a/controller/northbound/server/topology.go
+++ b/controller/northbound/server/topology.go
@@ -2,6 +2,7 @@ package server
import (
"context"
+ "errors"
"time"
topopb "code.fbi.h-da.de/danet/gosdn/api/go/gosdn/topology"
@@ -15,6 +16,7 @@ import (
"github.com/google/uuid"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
+ "google.golang.org/protobuf/reflect/protoreflect"
)
// TopologyServer holds a topologyService and represents a TopologyServiceServer.
@@ -41,10 +43,29 @@ func NewTopologyServer(
}
}
+func (t TopologyServer) checkForValidationErrors(request protoreflect.ProtoMessage) error {
+ err := t.protoValidator.Validate(request)
+ if err != nil {
+ var valErr *protovalidate.ValidationError
+
+ if ok := errors.As(err, &valErr); ok {
+ protoErr := valErr.ToProto()
+ grpcError, _ := status.New(codes.Aborted, "Validation failed").WithDetails(protoErr)
+
+ return grpcError.Err()
+ }
+
+ return status.Errorf(codes.Aborted, "%v", err)
+ }
+
+ return nil
+}
+
// AddLink adds a new link to the topology.
func (t *TopologyServer) AddLink(ctx context.Context, request *topopb.AddLinkRequest) (*topopb.AddLinkResponse, error) {
- if err := t.protoValidator.Validate(request); err != nil {
- return nil, status.Errorf(codes.Aborted, "%v", err)
+ err := t.checkForValidationErrors(request)
+ if err != nil {
+ return nil, err
}
sourceNode, sourcePort, err := t.ensureNodeAndPortExists(request.Link.SourceNode, request.Link.SourcePort)
@@ -77,8 +98,9 @@ func (t *TopologyServer) AddLink(ctx context.Context, request *topopb.AddLinkReq
// GetTopology returns the current topology in the form of all links.
func (t *TopologyServer) GetTopology(ctx context.Context, request *topopb.GetTopologyRequest) (*topopb.GetTopologyResponse, error) {
- if err := t.protoValidator.Validate(request); err != nil {
- return nil, status.Errorf(codes.Aborted, "%v", err)
+ err := t.checkForValidationErrors(request)
+ if err != nil {
+ return nil, err
}
topo, err := t.topologyService.GetAll()
@@ -127,8 +149,9 @@ func (t *TopologyServer) GetTopology(ctx context.Context, request *topopb.GetTop
// DeleteLink deletes a link.
func (t *TopologyServer) DeleteLink(ctx context.Context, request *topopb.DeleteLinkRequest) (*topopb.DeleteLinkResponse, error) {
- if err := t.protoValidator.Validate(request); err != nil {
- return nil, status.Errorf(codes.Aborted, "%v", err)
+ err := t.checkForValidationErrors(request)
+ if err != nil {
+ return nil, err
}
linkID, err := uuid.Parse(request.Id)
diff --git a/controller/northbound/server/topology_test.go b/controller/northbound/server/topology_test.go
index a1066224c..5306b61c6 100644
--- a/controller/northbound/server/topology_test.go
+++ b/controller/northbound/server/topology_test.go
@@ -5,6 +5,7 @@ import (
"reflect"
"testing"
+ "buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go/buf/validate"
apb "code.fbi.h-da.de/danet/gosdn/api/go/gosdn/topology"
eventservice "code.fbi.h-da.de/danet/gosdn/controller/eventService"
"code.fbi.h-da.de/danet/gosdn/controller/topology"
@@ -244,11 +245,12 @@ func TestTopology_AddLink(t *testing.T) {
request *apb.AddLinkRequest
}
tests := []struct {
- name string
- fields fields
- args args
- want *apb.AddLinkResponse
- wantErr bool
+ name string
+ fields fields
+ args args
+ want *apb.AddLinkResponse
+ wantErr bool
+ validationErrors []*validate.Violation
}{
{
name: "should add a new link",
@@ -281,6 +283,33 @@ func TestTopology_AddLink(t *testing.T) {
},
want: &apb.AddLinkResponse{},
wantErr: true,
+ validationErrors: []*validate.Violation{
+ {
+ FieldPath: "link.name",
+ ConstraintId: "string.min_len",
+ Message: "value length must be at least 1 characters",
+ },
+ {
+ FieldPath: "link.sourceNode",
+ ConstraintId: "required",
+ Message: "value is required",
+ },
+ {
+ FieldPath: "link.targetNode",
+ ConstraintId: "required",
+ Message: "value is required",
+ },
+ {
+ FieldPath: "link.sourcePort",
+ ConstraintId: "required",
+ Message: "value is required",
+ },
+ {
+ FieldPath: "link.targetPort",
+ ConstraintId: "required",
+ Message: "value is required",
+ },
+ },
},
}
for _, tt := range tests {
@@ -289,7 +318,8 @@ func TestTopology_AddLink(t *testing.T) {
_, err := tr.AddLink(tt.args.ctx, tt.args.request)
if err != nil {
if tt.wantErr {
- // TODO: check error
+ assertValidationErrors(t, err, tt.validationErrors)
+
return
}
@@ -318,7 +348,7 @@ func TestTopology_GetTopology(t *testing.T) {
wantErr bool
}{
{
- name: "should add a new link",
+ name: "should get an existing link",
fields: fields{
ports: []ports.Port{getTestSourcePort(), getTestTargetPort()},
nodes: []nodes.Node{getTestSourceNode(), getTestTargetNode()},
@@ -391,14 +421,15 @@ func TestTopology_DeleteLink(t *testing.T) {
request *apb.DeleteLinkRequest
}
tests := []struct {
- name string
- fields fields
- args args
- want *apb.DeleteLinkResponse
- wantErr bool
+ name string
+ fields fields
+ args args
+ want *apb.DeleteLinkResponse
+ wantErr bool
+ validationErrors []*validate.Violation
}{
{
- name: "should add a new link",
+ name: "should delete an existing link",
fields: fields{
ports: []ports.Port{getTestSourcePort(), getTestTargetPort()},
nodes: []nodes.Node{getTestSourceNode(), getTestTargetNode()},
@@ -413,6 +444,28 @@ func TestTopology_DeleteLink(t *testing.T) {
want: &apb.DeleteLinkResponse{},
wantErr: false,
},
+ {
+ name: "should error due to missing link id",
+ fields: fields{
+ ports: []ports.Port{getTestSourcePort(), getTestTargetPort()},
+ nodes: []nodes.Node{getTestSourceNode(), getTestTargetNode()},
+ links: []links.Link{getTestLinkInternal()},
+ },
+ args: args{
+ ctx: context.TODO(),
+ request: &apb.DeleteLinkRequest{
+ Id: "",
+ },
+ },
+ want: &apb.DeleteLinkResponse{},
+ wantErr: true,
+ validationErrors: []*validate.Violation{
+ {
+ FieldPath: "id",
+ ConstraintId: "required",
+ Message: "value is required",
+ }},
+ },
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
@@ -423,11 +476,18 @@ func TestTopology_DeleteLink(t *testing.T) {
return
}
+ if tt.wantErr {
+ assertValidationErrors(t, err, tt.validationErrors)
+ // There is no need to fetch again if we are expecting a validation error.
+ return
+ }
+
gotAfterDelete, err := tr.GetTopology(tt.args.ctx, &apb.GetTopologyRequest{})
if (err != nil) != tt.wantErr {
t.Errorf("Topology.GetTopology() error = %v, wantErr %v", err, tt.wantErr)
return
}
+
if !reflect.DeepEqual(gotAfterDelete.Toplogy, &apb.Topology{}) {
t.Errorf("Topology.GetTopology() = %v, want %v", got, tt.want)
}
--
GitLab