Fix the CBOR Content Header-related issue. #464

akramtexas · 2020-03-12T15:43:02Z

This is the first of two PRs that will address Response of issueDeviceCommand is not providing CBOR Content-Type header edgex-go#2272 ("Response of issueDeviceCommand is not providing CBOR Content-Type header").
The second PR (in edgex-go) will be submitted subsequently, since it is dependent on the work done in the first PR (this current one).

internal/common/utils.go

akramtexas · 2020-03-12T15:51:52Z

Here are the set of steps used for testing, where some steps become applicable once the the second PR (in edgex-go) has been submitted, subsequent to this one:

Launch all edgex-go core and support services locally, running natively, without containerization (This will be needed for the next step).
Locally build this device-sdk-go branch.
Point device-virtual-go to use this device-sdk-go branch (i.e. the local build) and launch device-virtual-go locally.
At this point, the virtual device named "Random-Binary-Device" will get registered with EdegeX.
Issue the following command:

curl --location --request GET 'http://localhost:48082/api/v1/device/name/Random-Binary-Device' \
--header 'Content-Type: application/cbor' \
--data-raw ''

The preceding command should respond with two URLs, one each to use for PUT and GET, for example as follows:

{
   "id":"9f872d68-2281-4af4-959d-29e4d51c2192",
   "name":"Random-Binary-Device",
   "adminState":"UNLOCKED",
   "operatingState":"ENABLED",
   "labels":[
      "device-virtual-example"
   ],
   "commands":[
      {
         "created":1583181972178,
         "modified":1583181972178,
         "id":"b349df4a-6c3d-4218-b8bc-7ff8d51ea50d",
         "name":"Binary",
         "get":{
            "path":"/api/v1/device/{deviceId}/Binary",
            "responses":[
               {
                  "code":"200",
                  "expectedValues":[
                     "Binary"
                  ]
               },
               {
                  "code":"503",
                  "description":"service unavailable"
               }
            ],
            "url":"http://localhost:48082/api/v1/device/9f872d68-2281-4af4-959d-29e4d51c2192/command/b349df4a-6c3d-4218-b8bc-7ff8d51ea50d"
         },
         "put":{
            "path":"/api/v1/device/{deviceId}/Binary",
            "responses":[
               {
                  "code":"200"
               },
               {
                  "code":"503",
                  "description":"service unavailable"
               }
            ],
            "url":"http://localhost:48082/api/v1/device/9f872d68-2281-4af4-959d-29e4d51c2192/command/b349df4a-6c3d-4218-b8bc-7ff8d51ea50d",
            "parameterNames":[
               "EnableRandomization_Binary"
            ]
         }
      }
   ]
}

Use the URL for PUT from above response as follows, where the --data-binary is supplied via a local file, whose contents are °x�EnableRandomization_Binarydtrue, so you'll want to replace the path '@/Users/username/Decoded_Binary' by a suitably-located local file on your filesystem:

curl --location --request PUT 'http://localhost:48082/api/v1/device/9f872d68-2281-4af4-959d-29e4d51c2192/command/b349df4a-6c3d-4218-b8bc-7ff8d51ea50d' \
--header 'Content-Type: application/cbor' \
--data-binary '@/Users/username/Decoded_Binary'

And use the URL for GET from the same response above as follows:

curl --location --request GET 'http://localhost:48082/api/v1/device/9f872d68-2281-4af4-959d-29e4d51c2192/command/b349df4a-6c3d-4218-b8bc-7ff8d51ea50d' \
--header 'Content-Type: application/cbor'

akramtexas · 2020-03-12T16:24:49Z

Separately, I propose my updating the EdgeX doc Sending and Consuming Binary Data From EdgeX Device Services to include the steps (including the golang code used to create the payload that was referenced in the testing steps above (i.e. --data-binary '@/Users/username/Decoded_Binary'.

internal/common/utils.go

AnthonyMBonafide · 2020-03-13T15:48:40Z

internal/common/utils.go

+	ChecksumAlgoxxHash = "xxHash"
+	checksumContextKey = "payload-checksum"


Following my other comment regarding unused functionality, this would also be removed if the other "Reader" functionality is removed.

internal/handler/command.go

internal/common/types.go

AnthonyMBonafide · 2020-03-13T16:20:19Z

internal/autoevent/executor.go

@@ -75,7 +75,7 @@ func readResource(e *executor) (*dsModels.Event, common.AppError) {
 	vars[common.NameVar] = e.deviceName
 	vars[common.CommandVar] = e.autoEvent.Resource

-	evt, appErr := handler.CommandHandler(vars, "", common.GetCmdMethod, "")
+	evt, appErr := handler.CommandHandler(vars, "", common.GetCmdMethod, "", nil)


Quickly looking at this, I think there may be a bug here. We are passing in nil for the r *http.Request. However, in the called function it never checks if the value is nil but performs some operations on r. For example:

device-sdk-go/internal/handler/command.go

Lines 488 to 494 in 58d8c2f

if r.Header.Get(clients.ContentType) == clients.ContentTypeCBOR {

// Handle Content-type CBOR and populate paramMap

err = codec.NewDecoderBytes([]byte(params), &codec.CborHandle{}).Decode(&paramMap)

} else if r.Header.Get(clients.ContentType) == clients.ContentTypeJSON {

// Handle Content-type JSON and populate paramMap

err = json.Unmarshal([]byte(params), &paramMap)

}

In revisiting this, one approach would be to change the signature of the following function (in /device-sdk-go/internal/autoevent/executor.go) as indicated below, which could then call evt, appErr := handler.CommandHandler(vars, "", req)

func readResource(e *executor, req *http.Request) (*dsModels.Event, common.AppError) { ... ... evt, appErr := handler.CommandHandler(vars, "", req) ... ... }

The challenge will be the call evt, appErr := readResource(e, nil) from its calling function, which is as follows and, specifically, how to introduce and pass in a parameter of type *http.Request:

// Run triggers this Executor executes the handler for the resource periodically func (e *executor) Run() { ... }

AnthonyMBonafide · 2020-03-13T16:25:04Z

internal/handler/command_test.go

@@ -452,3 +466,9 @@ func TestCommandHandler(t *testing.T) {
 		})
 	}
 }
+
+func newRequestWithContentType(contentType string) *http.Request {


I do not see any tests that cover using the application/cbor content type nor an empty, or invalid content type. Can we add some tests to cover those cases?

pkg/models/event.go

AnthonyMBonafide · 2020-03-13T16:34:59Z

Would be better to land the PR for edgexfoundry/edgex-go#2272 first? The reason I ask is because we can:

Manually end-to-end test this functionality
Better know what to expect for the headers. For example, we may want to start requiring clients to provide a content type, or we may do the assumption in core-command that the data is JSON and add the header so it is consistent within the system. I am not suggesting any of these thoughts just providing some concrete examples to my idea.

Also, if you decide to use the "Reader" functionality, it exists in edgex-go already so maybe we can move it to a common place and start reusing it in other places.

akramtexas · 2020-03-13T16:47:33Z

Would be better to land the PR for edgexfoundry/edgex-go#2272 first? The reason I ask is because we can:

Manually end-to-end test this functionality

Better know what to expect for the headers. For example, we may want to start requiring clients to provide a content type, or we may do the assumption in core-command that the data is JSON and add the header so it is consistent within the system. I am not suggesting any of these thoughts just providing some concrete examples to my idea.

Also, if you decide to use the "Reader" functionality, it exists in edgex-go already so maybe we can move it to a common place and start reusing it in other places.

I am absolutely open to the idea, if we can make it work. In fact, I refer to my comment at the outset in the set of steps used for testing, where I noted that some steps become applicable only once the other PR (the one you also refer to, for the edgex-go repo) has been landed.
I will revisit the initial challenge, which has to do with the setup required to manually end-to-end test this functionality, and whose testing requires running a local instance of device-virtual-go which has been built after pointing it to the updates (local at this time, the ones in this, the currently landed PR) in device-sdk-go.
Definitely worth a second look. I will investigate 👍

AnthonyMBonafide · 2020-03-19T14:01:21Z

As per my conversation with @akramtexas, we are putting these changes on hold in favor of completing edgexfoundry/edgex-go#2272 first. This will allow us to ensure that the Content-type header is actually being passed through core-command so that we can perform the logic in this PR.

akramtexas · 2020-03-19T14:03:43Z

As per my conversation with @akramtexas, we are putting these changes on hold in favor of completing edgexfoundry/edgex-go#2272 first. This will allow us to ensure that the Content-type header is actually being passed through core-command so that we can perform the logic in this PR.

Yes, exactly, @AnthonyMBonafide and thanks for the excellent feedback on this and the other PR 👍

AnthonyMBonafide

Please re-request me for a review once the depending issue edgexfoundry/edgex-go#2272 has been completed.

akramtexas · 2020-03-19T14:04:15Z

Please re-request me for a review once the depending issue edgexfoundry/edgex-go#2272 has been completed.

Will do ✅

AnthonyMBonafide · 2020-04-09T15:48:56Z

This PR is associated with #479

codecov-io · 2020-04-09T16:39:23Z

Codecov Report

Merging #464 into master will decrease coverage by 2.88%.
The diff coverage is 62.79%.

@@            Coverage Diff             @@
##           master     #464      +/-   ##
==========================================
- Coverage   57.50%   54.61%   -2.89%     
==========================================
  Files          25       25              
  Lines        3144     2479     -665     
==========================================
- Hits         1808     1354     -454     
+ Misses       1211     1019     -192     
+ Partials      125      106      -19

Impacted Files	Coverage Δ
internal/autoevent/executor.go	`37.50% <0.00%> (-1.39%)`	⬇️
internal/common/types.go	`0.00% <ø> (ø)`
pkg/models/event.go	`0.00% <ø> (ø)`
internal/controller/restfuncs.go	`24.21% <50.00%> (-5.99%)`	⬇️
internal/handler/command.go	`77.29% <60.00%> (-7.70%)`	⬇️
internal/handler/test_utils.go	`100.00% <100.00%> (ø)`
pkg/models/commandvalue.go	`61.96% <0.00%> (-1.65%)`	⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc4d8ba...f9e2813. Read the comment docs.

akramtexas · 2020-04-14T19:02:21Z

Rebased.

tsconn23 · 2020-04-16T14:34:29Z

example/cmd/device-simple/Attribution.txt

@@ -96,6 +96,9 @@ https://github.com/pmezard/go-difflib/blob/master/LICENSE
 stretchr/testify (MIT) https://github.com/stretchr/testify
 https://github.com/stretchr/testify/blob/master/LICENSE

+ugorji/go (MIT) https://github.com/ugorji/go
+https://github.com/ugorji/go/blob/master/LICENSE
+


Please update the reference to the CBOR library per edgexfoundry/edgex-go#2490

Signed-off-by: Akram Ahmad <sftwr2020@gmail.com>

akramtexas · 2020-04-16T15:30:19Z

Rebased

AnthonyMBonafide · 2020-04-17T14:47:23Z

Closing this PR as it is not needed to fix the bug we were originally intending to fix.

Although this functionality is great it is not needed. A new feature request was created to add this functionality later( #488 ). There is a reference to this PR which can be used when we decide to take on that work.

akramtexas self-assigned this Mar 12, 2020

jdharms reviewed Mar 12, 2020

View reviewed changes

internal/common/utils.go Outdated Show resolved Hide resolved

akramtexas force-pushed the fix_device_sdk_go_response_cbor_issue_2272 branch from 659b91f to 58d8c2f Compare March 12, 2020 15:50

akramtexas requested a review from jdharms March 12, 2020 15:50

akramtexas requested a review from michaelestrin March 13, 2020 10:40

michaelestrin requested review from AnthonyMBonafide and removed request for michaelestrin March 13, 2020 11:33

AnthonyMBonafide suggested changes Mar 13, 2020

View reviewed changes

akramtexas force-pushed the fix_device_sdk_go_response_cbor_issue_2272 branch 2 times, most recently from 121ae82 to 8eaf976 Compare March 17, 2020 15:47

akramtexas requested a review from AnthonyMBonafide March 17, 2020 15:47

akramtexas force-pushed the fix_device_sdk_go_response_cbor_issue_2272 branch from 92ab471 to 9dc7c52 Compare March 17, 2020 18:47

cloudxxx8 requested a review from FelixTing March 18, 2020 05:50

AnthonyMBonafide added 2-medium priority denoting issues with cross-cutting project impact bug Something isn't working enhancement New feature or request hold Intended for PRs we want to flag for ongoing review and removed enhancement New feature or request labels Mar 19, 2020

AnthonyMBonafide suggested changes Mar 19, 2020

View reviewed changes

akramtexas mentioned this pull request Mar 20, 2020

Fix core-command response when handling a CBOR result. edgexfoundry/edgex-go#2446

Merged

akramtexas force-pushed the fix_device_sdk_go_response_cbor_issue_2272 branch 2 times, most recently from 6e80231 to a3da746 Compare April 8, 2020 16:39

akramtexas force-pushed the fix_device_sdk_go_response_cbor_issue_2272 branch 2 times, most recently from 1556322 to c997980 Compare April 9, 2020 16:31

akramtexas force-pushed the fix_device_sdk_go_response_cbor_issue_2272 branch from c997980 to 093062c Compare April 14, 2020 19:02

akramtexas force-pushed the fix_device_sdk_go_response_cbor_issue_2272 branch 2 times, most recently from 5992b07 to 1343d84 Compare April 15, 2020 21:59

akramtexas requested a review from AnthonyMBonafide April 15, 2020 22:00

tsconn23 suggested changes Apr 16, 2020

View reviewed changes

Fix the Content Header-related issue with CBOR.

f9e2813

Signed-off-by: Akram Ahmad <sftwr2020@gmail.com>

akramtexas force-pushed the fix_device_sdk_go_response_cbor_issue_2272 branch from 1343d84 to f9e2813 Compare April 16, 2020 15:30

AnthonyMBonafide closed this Apr 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the CBOR Content Header-related issue. #464

Fix the CBOR Content Header-related issue. #464

akramtexas commented Mar 12, 2020

akramtexas commented Mar 12, 2020 •

edited

Loading

akramtexas commented Mar 12, 2020

AnthonyMBonafide Mar 13, 2020

AnthonyMBonafide Mar 13, 2020

akramtexas Mar 17, 2020 •

edited

Loading

AnthonyMBonafide Mar 13, 2020

AnthonyMBonafide commented Mar 13, 2020

akramtexas commented Mar 13, 2020

AnthonyMBonafide commented Mar 19, 2020

akramtexas commented Mar 19, 2020

AnthonyMBonafide left a comment

akramtexas commented Mar 19, 2020

AnthonyMBonafide commented Apr 9, 2020

codecov-io commented Apr 9, 2020 •

edited

Loading

akramtexas commented Apr 14, 2020

tsconn23 Apr 16, 2020

akramtexas commented Apr 16, 2020

AnthonyMBonafide commented Apr 17, 2020

		ChecksumAlgoxxHash = "xxHash"
		checksumContextKey = "payload-checksum"

	if r.Header.Get(clients.ContentType) == clients.ContentTypeCBOR {
	// Handle Content-type CBOR and populate paramMap
	err = codec.NewDecoderBytes([]byte(params), &codec.CborHandle{}).Decode(&paramMap)
	} else if r.Header.Get(clients.ContentType) == clients.ContentTypeJSON {
	// Handle Content-type JSON and populate paramMap
	err = json.Unmarshal([]byte(params), &paramMap)
	}

Fix the CBOR Content Header-related issue. #464

Fix the CBOR Content Header-related issue. #464

Conversation

akramtexas commented Mar 12, 2020

akramtexas commented Mar 12, 2020 • edited Loading

akramtexas commented Mar 12, 2020

AnthonyMBonafide Mar 13, 2020

Choose a reason for hiding this comment

AnthonyMBonafide Mar 13, 2020

Choose a reason for hiding this comment

akramtexas Mar 17, 2020 • edited Loading

Choose a reason for hiding this comment

AnthonyMBonafide Mar 13, 2020

Choose a reason for hiding this comment

AnthonyMBonafide commented Mar 13, 2020

akramtexas commented Mar 13, 2020

AnthonyMBonafide commented Mar 19, 2020

akramtexas commented Mar 19, 2020

AnthonyMBonafide left a comment

Choose a reason for hiding this comment

akramtexas commented Mar 19, 2020

AnthonyMBonafide commented Apr 9, 2020

codecov-io commented Apr 9, 2020 • edited Loading

Codecov Report

akramtexas commented Apr 14, 2020

tsconn23 Apr 16, 2020

Choose a reason for hiding this comment

akramtexas commented Apr 16, 2020

AnthonyMBonafide commented Apr 17, 2020

akramtexas commented Mar 12, 2020 •

edited

Loading

akramtexas Mar 17, 2020 •

edited

Loading

codecov-io commented Apr 9, 2020 •

edited

Loading