From 5972b753fa186a40c398ed4d91801bcf7a85297b Mon Sep 17 00:00:00 2001 From: weichou Date: Thu, 25 Jun 2020 00:29:04 +0800 Subject: [PATCH] fix(metadata): Refactor deviceProfile JSON and YAML POST API Refactor deviceProfile adding func to use same validation logic for JSON and YAML format DeviceProfile. Signed-off-by: weichou --- internal/core/metadata/errors/types.go | 12 ++ internal/core/metadata/rest_deviceprofile.go | 113 +++++------------- .../core/metadata/rest_deviceprofile_test.go | 76 +++++++++--- internal/pkg/errorconcept/device_profile.go | 16 +++ openapi/v1/core-metadata.yaml | 9 +- 5 files changed, 125 insertions(+), 101 deletions(-) diff --git a/internal/core/metadata/errors/types.go b/internal/core/metadata/errors/types.go index f702577570..1b13c2d35c 100644 --- a/internal/core/metadata/errors/types.go +++ b/internal/core/metadata/errors/types.go @@ -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} +} diff --git a/internal/core/metadata/rest_deviceprofile.go b/internal/core/metadata/rest_deviceprofile.go index 941fd2a4a6..fe036a657f 100644 --- a/internal/core/metadata/rest_deviceprofile.go +++ b/internal/core/metadata/rest_deviceprofile.go @@ -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( @@ -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 @@ -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. @@ -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() diff --git a/internal/core/metadata/rest_deviceprofile_test.go b/internal/core/metadata/rest_deviceprofile_test.go index 35299e30ac..d374e52c3a 100644 --- a/internal/core/metadata/rest_deviceprofile_test.go +++ b/internal/core/metadata/rest_deviceprofile_test.go @@ -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) @@ -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 @@ -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", @@ -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) { @@ -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) @@ -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 { @@ -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", @@ -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) { diff --git a/internal/pkg/errorconcept/device_profile.go b/internal/pkg/errorconcept/device_profile.go index 630ce4dae4..4ddf4126c2 100644 --- a/internal/pkg/errorconcept/device_profile.go +++ b/internal/pkg/errorconcept/device_profile.go @@ -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 @@ -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 { diff --git a/openapi/v1/core-metadata.yaml b/openapi/v1/core-metadata.yaml index 1fd4ef8830..a98a0ae2a5 100644 --- a/openapi/v1/core-metadata.yaml +++ b/openapi/v1/core-metadata.yaml @@ -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}: @@ -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: @@ -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}: