fix: Invoke Validate function when adding yaml DeviceProfile #2597

weichou1229 · 2020-06-24T16:35:11Z

Fix #2596

PR Checklist

Please check if your PR fulfills the following requirements:

The commit subject follows the Conventional Commits spec
The commit message follows the EdgeX Contributor Guide
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)
make test has completed successfully

PR Type

What kind of change does this PR introduce?

What is the current behavior?

YAML DeviceProfile uploading API doesn't validate the content.

Issue Number: #2596

What is the new behavior?

Invoke Validate function when uploading yaml format DeviceProfile to prevent uploading invalid DeviceProfile.

Does this PR introduce a breaking change?

Yes
No

Are there any new imports or modules? If so, what are they used for and why?

Are there any specific instructions or things that should be known prior to reviewing?

Since deviceProfile.Validate() not validate the nested object, so create another function to validate that.

Other information

internal/core/metadata/rest_deviceprofile.go

lenny-goodell

LGTM

tonyespy

There are two issues that I'd like to see addressed in the code. The first involves error handling, and if there's no other way around it, we'll have to live with it. If so, we should ensure that we don't make the same mistake with our V2 APIs.

The only other comment I have about your commit message:

fix: Invoke Validate function when adding yaml DeviceProfile
Add yaml format DeviceProfile should invoke Validate function to prevent uploading invalid DeviceProfile.
     
Signed-off-by: weichou <[email protected]>

Please add a scope to your commit summary so others can tell what service is being modified, and second there should always be a blank line between the commit summary and the message body, so your commit should look like this:

fix(metadata): Invoke Validate function when adding YAML DeviceProfile

Add YAML format DeviceProfile should invoke Validate function to prevent uploading invalid DeviceProfile.
     
Signed-off-by: weichou <[email protected]>

Note - also acronyms like YAML, TOML, ..., should always be uppercase.

tonyespy · 2020-07-17T20:47:33Z

internal/core/metadata/rest_deviceprofile.go

@@ -281,6 +283,16 @@ func restAddProfileByYaml(
 		return
 	}

+	err = validateDeviceProfileStruct(dp)
+	if err != nil {
+		if err.Error() == duplicatedCommandNameError {


It seems pretty fragile to have to define the constant duplicatedCommandNameError in this file in order to be able to distinguish between two different errors conditions detected by the underlying DeviceProfile.Validate() method that's called by validateDeviceProfileStruct. This seems like a possible flaw in our original object validation approach implemented for our V1 REST endpoints.

The OpenAPI error description for this specific case says:

409: 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.

This description is poorly worded. I think what it means is this:

If the device profile defines more than one core command with the same name, or if it defines a core command which conflicts with an already existing core command from another device profile.

Yes, it is fragile, but it is a quick workaround to avoid breaking API backward compatibility. Do you have any other suggestion?

Your first statement is enough. We allow the same command name in different profile.
https://github.com/edgexfoundry/go-mod-core-contracts/blob/0746ea1719ff74015da8ef926f06a40e1e317cc2/models/deviceprofile.go#L87-L95

May we update the Swagger file in this PR to the following description?

If the device profile defines more than one core command with the same name.

Yes, it is fragile, but it is a quick workaround to avoid breaking API backward compatibility. Do you have any other suggestion?

Sure, but we never validated device profiles when using this endpoint... That said, if this aligns with the deviceprofile import using JSON, I guess we can leave it as is. Does our new approach to validation being used for V2 allow more flexible error reporting? I would hate to see us make the same mistake twice...

Your first statement is enough. We allow the same command name in different profile.
https://github.com/edgexfoundry/go-mod-core-contracts/blob/0746ea1719ff74015da8ef926f06a40e1e317cc2/models/deviceprofile.go#L87-L95

May we update the Swagger file in this PR to the following description?

If the device profile defines more than one core command with the same name.

Yes

Sure, but we never validated device profiles when using this endpoint... That said, if this aligns with the deviceprofile import using JSON, I guess we can leave it as is. Does our new approach to validation being used for V2 allow more flexible error reporting? I would hate to see us make the same mistake twice...

We will avoid this situation in V2

May we update the Swagger file in this PR to the following description?
If the device profile defines more than one core command with the same name.

Swagger file updated.

Yes, it is fragile, but it is a quick workaround to avoid breaking API backward compatibility. Do you have any other suggestion?

Is it breaking backward compatibility because the code doesn't match our OpenAPI documentation or is it breaking because the non-YAML deviceprofile endpoint can return 409, and thus we want the deviceprofile/upload, and ../uploadfile endpoints to do the same???

I would argue that if none of the deviceprofile endpoints ever return 409, then it's not breaking backward compatibility if the YAML endpoints don't return it either...

So I actually did a bunch of testing this afternoon using Bruce's PR, as well as against the geneva snap (1.2.1). I found that 400 is returned for "duplicate core command names" not 409. In fact, I haven't looked at the actual code path, but even without the fix, the uploadfile endpoint does still return a 400 error in this situation

For json format:
I look into the source code
https://github.com/edgexfoundry/edgex-go/blob/master/internal/core/metadata/rest_deviceprofile.go#L74

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

It only return 400 when uploading the invalid device profile and the implementation not match the API doc.
https://github.com/edgexfoundry/edgex-go/blob/master/api/openapi/v1/core-metadata.yaml#L933

internal/core/metadata/rest_deviceprofile.go

tonyespy · 2020-07-18T00:59:54Z

@weichou1229 also your branch is out of sync with master, so please re-base.

weichou1229 · 2020-07-20T08:15:48Z

@tonyespy

The only other comment I have about your commit message...

also your branch is out of sync with master, so please re-base.

No problem, modified the commit message and rebase on the master branch.

internal/core/metadata/rest_deviceprofile.go

tonyespy

Please see my comments regarding the actual error that's supposed to be returned when an imported device profile contains core commands with duplicate names. Maybe the OpenAPI definition wasn't ever validated against the real service?

tonyespy · 2020-07-21T19:20:52Z

api/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 device profile defines more than one core command with the same name.


I ran some tests, and 409 is actually returned when a device profile with the same name already exists, not when duplicate core commands exist within a device profile (JSON or YAML). I checked against the geneva version 1.2.1 (using the snap), and sure enough when a device profile is imported with duplicate command names, the return code is 400.

So this description should really be:

If the profile's name matches an existing device profile.

Do you mean?

The deviceProfile POST API should return 400 for func (dp DeviceProfile) Validate() error.

The deviceProfile POST API should return 409 If the profile's name matches an existing device profile.

tonyespy · 2020-07-21T23:00:52Z

internal/core/metadata/rest_deviceprofile.go

@@ -281,6 +283,16 @@ func restAddProfileByYaml(
 		return
 	}

+	err = validateDeviceProfileStruct(dp)
+	if err != nil {
+		if err.Error() == duplicatedCommandNameError {


So I actually did a bunch of testing this afternoon using Bruce's PR, as well as against the geneva snap (1.2.1). I found that 400 is returned for "duplicate core command names" not 409. In fact, I haven't looked at the actual code path, but even without the fix, the uploadfile endpoint does still return a 400 error in this situation

internal/core/metadata/rest_deviceprofile.go

internal/pkg/errorconcept/device_profile.go

Refactor deviceProfile adding func to use same validation logic for JSON and YAML format DeviceProfile. Signed-off-by: weichou <[email protected]>

sonarqubecloud · 2020-07-30T07:15:11Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
1 Code Smell

No Coverage information
0.0% Duplication

lenny-goodell

LGTM

tonyespy

LGTM

cloudxxx8 added this to the Hanoi milestone Jun 28, 2020

cloudxxx8 requested review from rsdmike, tonyespy and lenny-goodell June 29, 2020 02:50

lenny-goodell requested changes Jul 16, 2020

View reviewed changes

internal/core/metadata/rest_deviceprofile.go Outdated Show resolved Hide resolved

rsdmike previously approved these changes Jul 17, 2020

View reviewed changes

cloudxxx8 requested a review from lenny-goodell July 17, 2020 16:14

lenny-goodell previously approved these changes Jul 17, 2020

View reviewed changes

tonyespy requested changes Jul 17, 2020

View reviewed changes

weichou1229 dismissed stale reviews from lenny-goodell and rsdmike via 2c86856 July 20, 2020 08:12

weichou1229 requested a review from tonyespy July 20, 2020 08:16

lenny-goodell requested changes Jul 21, 2020

View reviewed changes

internal/core/metadata/rest_deviceprofile.go Outdated Show resolved Hide resolved

internal/core/metadata/rest_deviceprofile.go Outdated Show resolved Hide resolved

tonyespy requested changes Jul 21, 2020

View reviewed changes

weichou1229 requested a review from lenny-goodell July 28, 2020 06:13

lenny-goodell requested changes Jul 28, 2020

View reviewed changes

internal/core/metadata/rest_deviceprofile.go Outdated Show resolved Hide resolved

internal/pkg/errorconcept/device_profile.go Outdated Show resolved Hide resolved

weichou1229 requested a review from lenny-goodell July 29, 2020 02:41

fix(metadata): Refactor deviceProfile JSON and YAML POST API

5972b75

Refactor deviceProfile adding func to use same validation logic for JSON and YAML format DeviceProfile. Signed-off-by: weichou <[email protected]>

lenny-goodell approved these changes Jul 30, 2020

View reviewed changes

tonyespy approved these changes Jul 30, 2020

View reviewed changes

lenny-goodell merged commit 9098740 into edgexfoundry:master Jul 30, 2020

jinlinGuan mentioned this pull request Jul 31, 2020

Update core-metadata deviceprofile tests edgexfoundry/blackbox-testing#465

Closed

weichou1229 deleted the issue-2596 branch October 14, 2020 06:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Invoke Validate function when adding yaml DeviceProfile #2597

fix: Invoke Validate function when adding yaml DeviceProfile #2597

weichou1229 commented Jun 24, 2020 •

edited

Loading

lenny-goodell left a comment

tonyespy left a comment

tonyespy Jul 17, 2020

cloudxxx8 Jul 18, 2020

tonyespy Jul 20, 2020

cloudxxx8 Jul 21, 2020 •

edited

Loading

weichou1229 Jul 21, 2020

tonyespy Jul 21, 2020

tonyespy Jul 21, 2020

weichou1229 Jul 23, 2020

tonyespy commented Jul 18, 2020

weichou1229 commented Jul 20, 2020

tonyespy left a comment

tonyespy Jul 21, 2020

weichou1229 Jul 23, 2020 •

edited

Loading

tonyespy Jul 21, 2020

sonarqubecloud bot commented Jul 30, 2020

lenny-goodell left a comment

tonyespy left a comment

fix: Invoke Validate function when adding yaml DeviceProfile #2597

fix: Invoke Validate function when adding yaml DeviceProfile #2597

Conversation

weichou1229 commented Jun 24, 2020 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Are there any new imports or modules? If so, what are they used for and why?

Are there any specific instructions or things that should be known prior to reviewing?

Other information

lenny-goodell left a comment

Choose a reason for hiding this comment

tonyespy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudxxx8 Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonyespy commented Jul 18, 2020

weichou1229 commented Jul 20, 2020

tonyespy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weichou1229 Jul 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jul 30, 2020

lenny-goodell left a comment

Choose a reason for hiding this comment

tonyespy left a comment

Choose a reason for hiding this comment

weichou1229 commented Jun 24, 2020 •

edited

Loading

cloudxxx8 Jul 21, 2020 •

edited

Loading

weichou1229 Jul 23, 2020 •

edited

Loading