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: Invoke Validate function when adding yaml DeviceProfile #2597

Merged
merged 1 commit into from
Jul 30, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 12 additions & 0 deletions internal/core/metadata/errors/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -183,3 +183,15 @@ func NewErrNameCollision(name, fromID, toID string) ErrNameCollision {
toID: toID,
}
}

type ErrDeviceProfileMarshalJson struct {
msg string
}

func (e ErrDeviceProfileMarshalJson) Error() string {
return e.msg
}

func NewErrDeviceProfileMarshalJson(message string) error {
return ErrDeviceProfileMarshalJson{msg: message}
}
113 changes: 33 additions & 80 deletions internal/core/metadata/rest_deviceprofile.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,37 +68,13 @@ func restAddDeviceProfile(
vdc coredata.ValueDescriptorClient,
configuration *config.ConfigurationStruct) {

var dp models.DeviceProfile

if err := json.NewDecoder(r.Body).Decode(&dp); err != nil {
errorHandler.Handle(w, err, errorconcept.Common.InvalidRequest_StatusBadRequest)
data, err := ioutil.ReadAll(r.Body)
if err != nil {
errorHandler.Handle(w, err, errorconcept.DeviceProfile.ReadFile)
return
}

if configuration.Writable.EnableValueDescriptorManagement {
// Check if the device profile name is unique so that we do not create ValueDescriptors for a DeviceProfile that
// will fail during the creation process later on.
nameOp := device_profile.NewGetProfileName(dp.Name, dbClient)
_, err := nameOp.Execute()
// The operator will return an DuplicateName error if the DeviceProfile exist.
if err == nil {
errorHandler.Handle(w, err, errorconcept.DeviceProfile.DuplicateName)
return
}

op := device_profile.NewAddValueDescriptorExecutor(r.Context(), vdc, lc, dp.DeviceResources...)
err = op.Execute()
if err != nil {
errorHandler.HandleOneVariant(
w,
err,
errorconcept.NewServiceClientHttpError(err),
errorconcept.Default.InternalServerError)
return
}
}

addDeviceProfile(dp, dbClient, w, errorHandler)
addDeviceProfile(data, lc, vdc, dbClient, r, w, errorHandler, configuration)
}

func restUpdateDeviceProfile(
Expand Down Expand Up @@ -281,52 +257,13 @@ func restAddProfileByYaml(
return
}

if configuration.Writable.EnableValueDescriptorManagement {
// Check if the device profile name is unique so that we do not create ValueDescriptors for a DeviceProfile that
// will fail during the creation process later on.
nameOp := device_profile.NewGetProfileName(dp.Name, dbClient)
_, err := nameOp.Execute()
// The operator will return an DuplicateName error if the DeviceProfile exist.
if err == nil {
errorHandler.Handle(w, err, errorconcept.DeviceProfile.DuplicateName)
return
}

op := device_profile.NewAddValueDescriptorExecutor(r.Context(), vdc, lc, dp.DeviceResources...)
err = op.Execute()
if err != nil {
errorHandler.HandleOneVariant(
w,
err,
errorconcept.NewServiceClientHttpError(err),
errorconcept.Default.InternalServerError)
return
}
}

// Avoid using the 'addDeviceProfile' function because we need to be backwards compatibility for API response codes.
// The difference is the mapping of 'ErrContractInvalid' to a '409(Conflict)' rather than a '400(Bad request).
// Disregarding backwards compatibility, the 'addDeviceProfile' function is the correct implementation to use in the
// 'ErrContractInvalid' since a '400(Bad Request)' is the correct response.
op := device_profile.NewAddDeviceProfileExecutor(dp, dbClient)
id, err := op.Execute()

jsonBytes, err := json.Marshal(dp)
if err != nil {
errorHandler.HandleManyVariants(
w,
err,
[]errorconcept.ErrorConceptType{
errorconcept.DeviceProfile.InvalidState_StatusBadRequest,
errorconcept.DeviceProfile.ContractInvalid_StatusConflict,
errorconcept.Common.DuplicateName,
errorconcept.DeviceProfile.EmptyName,
},
errorconcept.Default.InternalServerError)
errorHandler.Handle(w, err, errorconcept.DeviceProfile.MarshalJson)
return
}

w.WriteHeader(http.StatusOK)
w.Write([]byte(id))
addDeviceProfile(jsonBytes, lc, vdc, dbClient, r, w, errorHandler, configuration)
}

// Add a device profile with YAML content
Expand Down Expand Up @@ -355,6 +292,32 @@ func restAddProfileByYamlRaw(
return
}

jsonBytes, err := json.Marshal(dp)
if err != nil {
errorHandler.Handle(w, err, errorconcept.DeviceProfile.MarshalJson)
return
}

addDeviceProfile(jsonBytes, lc, vdc, dbClient, r, w, errorHandler, configuration)
}

// This function centralizes the common logic for adding a device profile to the database and dealing with the return
func addDeviceProfile(
jsonBytes []byte,
lc logger.LoggingClient,
vdc coredata.ValueDescriptorClient,
dbClient interfaces.DBClient,
r *http.Request,
w http.ResponseWriter,
errorHandler errorconcept.ErrorHandler,
configuration *config.ConfigurationStruct) {

var dp models.DeviceProfile
if err := dp.UnmarshalJSON(jsonBytes); err != nil {
errorHandler.Handle(w, err, errorconcept.Common.InvalidRequest_StatusBadRequest)
return
}

if configuration.Writable.EnableValueDescriptorManagement {
// Check if the device profile name is unique so that we do not create ValueDescriptors for a DeviceProfile that
// will fail during the creation process later on.
Expand All @@ -378,16 +341,6 @@ func restAddProfileByYamlRaw(
}
}

addDeviceProfile(dp, dbClient, w, errorHandler)
}

// This function centralizes the common logic for adding a device profile to the database and dealing with the return
func addDeviceProfile(
dp models.DeviceProfile,
dbClient interfaces.DBClient,
w http.ResponseWriter,
errorHandler errorconcept.ErrorHandler) {

op := device_profile.NewAddDeviceProfileExecutor(dp, dbClient)
id, err := op.Execute()

Expand Down
76 changes: 61 additions & 15 deletions internal/core/metadata/rest_deviceprofile_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -940,8 +940,7 @@ func TestDeleteDeviceProfileByName(t *testing.T) {

func TestAddProfileByYaml(t *testing.T) {
// we have to overwrite the CoreCommands so that their isValidated is false
dp := TestDeviceProfile
dp.CoreCommands = []contract.Command{TestCommand}
dp := TestDeviceProfileValidated

okBody, _ := yaml.Marshal(dp)

Expand All @@ -953,6 +952,16 @@ func TestAddProfileByYaml(t *testing.T) {
emptyName.Name = ""
emptyBody, _ := yaml.Marshal(emptyName)

emptyDeviceResource := createTestDeviceProfile()
emptyDeviceResource.DeviceCommands[0].Get = []contract.ResourceOperation{
{Index: "test index", Operation: "test operation", DeviceResource: ""},
}
emptyDeviceResourceBody, _ := yaml.Marshal(emptyDeviceResource)

emptyCoreCmdName := createTestDeviceProfile()
emptyCoreCmdName.CoreCommands[0].Name = ""
emptyCoreCmdNameBody, _ := yaml.Marshal(emptyCoreCmdName)

emptyFileRequest := createDeviceProfileRequestWithFile(okBody)
emptyFileRequest.MultipartForm = new(multipart.Form)
emptyFileRequest.MultipartForm.File = nil
Expand Down Expand Up @@ -1006,15 +1015,10 @@ func TestAddProfileByYaml(t *testing.T) {
{
"Duplicate command name",
createDeviceProfileRequestWithFile(dupeBody),
createDBClientWithOutlines([]mockOutline{
{"AddDeviceProfile", []interface{}{dp}, []interface{}{TestDeviceProfileID, nil}},
{"GetDeviceProfileByName", []interface{}{TestDeviceProfileName}, []interface{}{contract.DeviceProfile{}, db.ErrNotFound}},
}),
createMockValueDescriptorClient([]mockOutline{
{"Add", []interface{}{TestDeviceProfileValidated}, []interface{}{}},
}, requestContext, TestDeviceProfile, nil),
createDBClientWithOutlines([]mockOutline{}),
createMockValueDescriptorClient([]mockOutline{}, requestContext, TestDeviceProfile, nil),
true,
http.StatusConflict,
http.StatusBadRequest,
},
{
"Duplicate profile name",
Expand Down Expand Up @@ -1064,6 +1068,22 @@ func TestAddProfileByYaml(t *testing.T) {
false,
http.StatusOK,
},
{
"Resource operation's deviceResource is empty",
createDeviceProfileRequestWithFile(emptyDeviceResourceBody),
createDBClientWithOutlines([]mockOutline{}),
createMockValueDescriptorClient([]mockOutline{}, requestContext, TestDeviceProfile, nil),
true,
http.StatusBadRequest,
},
{
"Core command name is empty",
createDeviceProfileRequestWithFile(emptyCoreCmdNameBody),
createDBClientWithOutlines([]mockOutline{}),
createMockValueDescriptorClient([]mockOutline{}, requestContext, TestDeviceProfile, nil),
true,
http.StatusBadRequest,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand All @@ -1088,8 +1108,7 @@ func TestAddProfileByYaml(t *testing.T) {

func TestAddProfileByYamlRaw(t *testing.T) {
// we have to overwrite the CoreCommands so that their isValidated is false
dp := TestDeviceProfile
dp.CoreCommands = []contract.Command{TestCommand}
dp := TestDeviceProfileValidated

okBody, _ := yaml.Marshal(dp)

Expand All @@ -1100,6 +1119,17 @@ func TestAddProfileByYamlRaw(t *testing.T) {
emptyName := dp
emptyName.Name = ""
emptyBody, _ := yaml.Marshal(emptyName)

emptyDeviceResource := createTestDeviceProfile()
emptyDeviceResource.DeviceCommands[0].Get = []contract.ResourceOperation{
{Index: "test index", Operation: "test operation", DeviceResource: ""},
}
emptyDeviceResourceBody, _ := yaml.Marshal(emptyDeviceResource)

emptyCoreCmdName := createTestDeviceProfile()
emptyCoreCmdName.CoreCommands[0].Name = ""
emptyCoreCmdNameBody, _ := yaml.Marshal(emptyCoreCmdName)

requestContext := httptest.NewRequest(http.MethodPost, AddressableTestURI, nil).Context()

tests := []struct {
Expand All @@ -1124,12 +1154,12 @@ func TestAddProfileByYamlRaw(t *testing.T) {
http.StatusOK,
},
{
"YAML unmarshal error",
createDeviceProfileRequestWithFile(dupeBody),
"Duplicate command name",
httptest.NewRequest(http.MethodPut, AddressableTestURI, bytes.NewBuffer(dupeBody)),
nil,
createMockValueDescriptorClient([]mockOutline{}, requestContext, TestDeviceProfile, nil),
true,
http.StatusServiceUnavailable,
http.StatusBadRequest,
},
{
"Empty device profile name",
Expand Down Expand Up @@ -1168,6 +1198,22 @@ func TestAddProfileByYamlRaw(t *testing.T) {
false,
http.StatusOK,
},
{
"Resource operation's deviceResource is empty",
httptest.NewRequest(http.MethodPut, AddressableTestURI, bytes.NewBuffer(emptyDeviceResourceBody)),
createDBClientWithOutlines([]mockOutline{}),
createMockValueDescriptorClient([]mockOutline{}, requestContext, TestDeviceProfile, nil),
true,
http.StatusBadRequest,
},
{
"Core command name is empty",
httptest.NewRequest(http.MethodPut, AddressableTestURI, bytes.NewBuffer(emptyCoreCmdNameBody)),
createDBClientWithOutlines([]mockOutline{}),
createMockValueDescriptorClient([]mockOutline{}, requestContext, TestDeviceProfile, nil),
true,
http.StatusBadRequest,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down
16 changes: 16 additions & 0 deletions internal/pkg/errorconcept/device_profile.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ type deviceProfileErrorConcept struct {
InvalidState_StatusBadRequest deviceProfileInvalidState_StatusBadRequest
InvalidState_StatusConflict deviceProfileInvalidState_StatusConflict
MarshalYaml deviceProfileMarshalYaml
MarshalJson deviceProfileMarshalJson
MissingFile deviceProfileMissingFile
NotFound deviceProfileNotFound
ReadFile deviceProfileReadFile
Expand Down Expand Up @@ -114,6 +115,21 @@ func (r deviceProfileInvalidState_StatusConflict) message(err error) string {
return err.Error()
}

type deviceProfileMarshalJson struct{}

func (r deviceProfileMarshalJson) httpErrorCode() int {
return http.StatusInternalServerError
}

func (r deviceProfileMarshalJson) isA(err error) bool {
_, ok := err.(metadataErrors.ErrDeviceProfileMarshalJson)
return ok
}

func (r deviceProfileMarshalJson) message(err error) string {
return err.Error()
}

type deviceProfileMarshalYaml struct{}

func (r deviceProfileMarshalYaml) httpErrorCode() int {
Expand Down
9 changes: 3 additions & 6 deletions openapi/v1/core-metadata.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -931,8 +931,7 @@ paths:
400:
description: For malformed or unparsable requests
409:
description: If an associated command's name is a duplicate for the profile
or if the name is determined to not be unique with regard to others.
description: If the profile's name matches an existing device profile.
500:
description: For unknown or unanticipated issues
/v1/deviceprofile/id/{id}:
Expand Down Expand Up @@ -1114,8 +1113,7 @@ paths:
400:
description: If the YAML file is empty
409:
description: If an associated command's name is a duplicate for the profile
or if the name is determined to not be unique with regard to others
description: If the profile's name matches an existing device profile.
500:
description: For unknown or unanticipated issues
/v1/deviceprofile/uploadfile:
Expand All @@ -1128,8 +1126,7 @@ paths:
400:
description: If the YAML file is empty
409:
description: If an associated command's name is a duplicate for the profile
or if the name is determined to not be unique with regard to others
description: If the profile's name matches an existing device profile.
500:
description: for unknown or unanticipated issues
/v1/deviceprofile/yaml/name/{name}:
Expand Down