Make sure event body is a string #393

RaeesBhatti · 2018-03-19T14:52:24Z

No description provided.

alexdebrie · 2018-03-19T16:14:37Z

router/router.go

+		if contentType, ok := httpdata.Headers["Content-Type"]; ok &&
+			(strings.HasPrefix(contentType, mimeFormMultipart) || contentType == mimeFormURLEncoded) {
+			if byteSlice, ok := httpdata.Body.([]byte); ok {
+				httpdata.Body = string(byteSlice)


@elsteelbrain I think this is generally the right approach -- stringifying the request Body -- but I don't know that this is the right place to do it. It would only fix it for http events, rather than all events.

This method might be better. cc @mthenw

Yup, you are right @alexdebrie. We discussed it earlier today.

RaeesBhatti · 2018-03-19T20:08:54Z

@mthenw @alexdebrie Changed as requested.

Changed

RaeesBhatti · 2018-03-19T20:51:12Z

@alexdebrie @mthenw Please take a look now

alexdebrie · 2018-03-19T21:57:08Z

router/event.go

+				return errors.New("malformed JSON body")
+			}
+			break
+		default:


Two notes:

We check properties of the body at the beginning of this block, but then check the properties of event.Data within the default statement, though those should be the same thing since body is assigned to event.Data earlier.

The first case statement checks the mimetype, and the default statement also checks the mimetype in an if statement. How about we move that if statement into a second case statement?

Combining these two, we'd have something like:

if len(body) > 0 { switch mime { case mimeJSON: err := json.Unmarshal(body, &event.Data) if err != nil { return errors.New("malformed JSON body") } case mimeFormMultipart, mimeFormURLEncoded: event.Data = string(body) } }

@alexdebrie About the second note, mimeFormMultipart mime will be a bit different based on what user agent is sending them. The user agent specifies the body boundary, which is mentioned in the mime header, so, we cannot add that into a case statement. The other one we can, but then we'll have to duplicate the code for them.

About the first one, we cannot assign string to a body of type []byte because that is not allowed. The type interface{} of event.Data allows us to do that.

@elsteelbrain Good point on the multipart/form-data boundary. I hadn't thought of that.

About the first one, we cannot assign string to a body of type []byte because that is not allowed. The type interface{} of event.Data allows us to do that.

I don't understand this one. We're not assigning string to body, we're assigning the stringified version of body to event.Data, right? Like this: event.Data = string(body).

I'm gonna second what Alex said, those should be two case statements. I don't understand why is it not possible.

What about this:

if len(body) > 0 { switch { case mime == mimeJSON: err = json.Unmarshal(body, &event.Data) if err != nil { return nil, "", errors.New("malformed JSON body") } case strings.HasPrefix(mime, mimeFormMultipart), mime == mimeFormURLEncoded: event.Data = string(body) } }

This also gets the complexity down under the limit without needing an additional function.

cc @elsteelbrain @mthenw

mthenw · 2018-03-20T11:04:27Z

router/event.go

@@ -124,3 +124,23 @@ func transformHeaders(req http.Header) map[string]string {

 	return headers
 }
+
+
+func correctEventData(event *eventpkg.Event, body []byte, mime string) error {


Can you add comment explaining what this function does?

mthenw · 2018-03-20T11:35:13Z

router/event.go

+				return errors.New("malformed JSON body")
+			}
+			break
+		default:


I'm gonna second what Alex said, those should be two case statements. I don't understand why is it not possible.

mthenw · 2018-03-20T11:35:27Z

router/event.go

+			break
+		default:
+			if byteSlice, ok := event.Data.([]byte);
+				ok && (strings.HasPrefix(mime, mimeFormMultipart) || mime == mimeFormURLEncoded) {


Why you check for prefix here, not the full header?

Because the content type is different based on the user agent. For example, Insomnia send this as content type: multipart/form-data; boundary=X-INSOMNIA-BOUNDARY.

Updated as requested

Updated and explained

codecov-io · 2018-03-23T09:36:25Z

Codecov Report

Merging #393 into master will increase coverage by 2.41%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #393      +/-   ##
==========================================
+ Coverage   56.55%   58.97%   +2.41%     
==========================================
  Files          24       24              
  Lines        1434     1438       +4     
==========================================
+ Hits          811      848      +37     
+ Misses        577      543      -34     
- Partials       46       47       +1

Impacted Files	Coverage Δ
router/event.go	`86.56% <100%> (+0.85%)`	⬆️
router/router.go	`60.92% <0%> (+10.92%)`	⬆️

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 bf66945...9e63be3. Read the comment docs.

alexdebrie · 2018-03-23T13:52:26Z

router/event.go

+			if err != nil {
+				return nil, "", errors.New("malformed JSON body")
+			}
+			break


@elsteelbrain break isn't needed in Golang case statements -- https://tour.golang.org/flowcontrol/9

alexdebrie · 2018-03-23T13:52:34Z

router/event.go

+			break
+		case strings.HasPrefix(mime, mimeFormMultipart), mime == mimeFormURLEncoded:
+			event.Data = string(body)
+			break


@elsteelbrain Same as above

mthenw

Looks good, can you also add test case for multipart/form-data?

RaeesBhatti requested review from mthenw and alexdebrie March 19, 2018 16:12

alexdebrie previously requested changes Mar 19, 2018

View reviewed changes

Add form to string conversion in eventFromRequest

f12f965

RaeesBhatti force-pushed the fixEncoding branch from fea34bc to f12f965 Compare March 19, 2018 20:08

Reformat logic

5969a23

alexdebrie previously requested changes Mar 19, 2018

View reviewed changes

Change a func name

56cffd8

mthenw previously requested changes Mar 20, 2018

View reviewed changes

Update the switch case logic

b4d20c7

RaeesBhatti added 3 commits March 20, 2018 20:11

Remove the function

74512a1

Add comment to explain the process

dbedb9e

Update comment

1a4d8ab

RaeesBhatti added 3 commits March 22, 2018 22:58

Merge branch 'master' into fixEncoding

4ef70a4

Add Provider mock

1e761d7

Add encoding test

fcbbd29

RaeesBhatti added 3 commits March 23, 2018 17:42

Add missing break

7cce702

Fix the test

83f57ae

Remove breaks

841fb1d

alexdebrie reviewed Mar 23, 2018

View reviewed changes

RaeesBhatti added 2 commits March 26, 2018 04:02

Rename a test func

1c37d3d

Add multiple scenarios in the test

14dae4e

mthenw suggested changes Mar 26, 2018

View reviewed changes

RaeesBhatti added 2 commits March 26, 2018 13:37

Change a key name

6b11d76

Add test case for multipart/form-data

9e63be3

mthenw approved these changes Mar 26, 2018

View reviewed changes

RaeesBhatti merged commit 0da4cd7 into master Mar 26, 2018

RaeesBhatti deleted the fixEncoding branch March 26, 2018 10:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure event body is a string #393

Make sure event body is a string #393

RaeesBhatti commented Mar 19, 2018

alexdebrie Mar 19, 2018

mthenw Mar 19, 2018

RaeesBhatti commented Mar 19, 2018

RaeesBhatti commented Mar 19, 2018

alexdebrie Mar 19, 2018 •

edited

Loading

RaeesBhatti Mar 20, 2018

RaeesBhatti Mar 20, 2018

alexdebrie Mar 20, 2018

mthenw Mar 20, 2018

alexdebrie Mar 20, 2018

mthenw Mar 20, 2018

mthenw Mar 20, 2018

mthenw Mar 20, 2018

RaeesBhatti Mar 20, 2018

codecov-io commented Mar 23, 2018 •

edited by codecov bot

Loading

alexdebrie Mar 23, 2018

alexdebrie Mar 23, 2018

mthenw left a comment

Make sure event body is a string #393

Make sure event body is a string #393

Conversation

RaeesBhatti commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RaeesBhatti commented Mar 19, 2018

RaeesBhatti commented Mar 19, 2018

alexdebrie Mar 19, 2018 • 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

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

codecov-io commented Mar 23, 2018 • edited by codecov bot Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mthenw left a comment

Choose a reason for hiding this comment

alexdebrie Mar 19, 2018 •

edited

Loading

codecov-io commented Mar 23, 2018 •

edited by codecov bot

Loading