Skip to content
This repository was archived by the owner on Dec 9, 2024. It is now read-only.

Make sure event body is a string #393

Merged
merged 17 commits into from
Mar 26, 2018
Merged

Make sure event body is a string #393

Changes from 2 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
f12f965
Add form to string conversion in eventFromRequest
RaeesBhatti Mar 19, 2018
5969a23
Reformat logic
RaeesBhatti Mar 19, 2018
56cffd8
Change a func name
RaeesBhatti Mar 20, 2018
b4d20c7
Update the switch case logic
RaeesBhatti Mar 20, 2018
74512a1
Remove the function
RaeesBhatti Mar 20, 2018
dbedb9e
Add comment to explain the process
RaeesBhatti Mar 20, 2018
1a4d8ab
Update comment
RaeesBhatti Mar 20, 2018
4ef70a4
Merge branch 'master' into fixEncoding
RaeesBhatti Mar 22, 2018
1e761d7
Add Provider mock
RaeesBhatti Mar 23, 2018
fcbbd29
Add encoding test
RaeesBhatti Mar 23, 2018
7cce702
Add missing break
RaeesBhatti Mar 23, 2018
83f57ae
Fix the test
RaeesBhatti Mar 23, 2018
841fb1d
Remove breaks
RaeesBhatti Mar 23, 2018
1c37d3d
Rename a test func
RaeesBhatti Mar 26, 2018
14dae4e
Add multiple scenarios in the test
RaeesBhatti Mar 26, 2018
6b11d76
Change a key name
RaeesBhatti Mar 26, 2018
9e63be3
Add test case for multipart/form-data
RaeesBhatti Mar 26, 2018
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
30 changes: 25 additions & 5 deletions router/event.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ type HTTPResponse struct {

const (
mimeJSON = "application/json"
mimeFormMultipart = "multipart/form-data"
mimeFormURLEncoded = "application/x-www-form-urlencoded"
)

func isHTTPEvent(r *http.Request) bool {
Expand Down Expand Up @@ -63,11 +65,9 @@ func (router *Router) eventFromRequest(r *http.Request) (*eventpkg.Event, string
}

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

if err := correctEventData(event, body, mime); err != nil {
return nil, "", err
}

if event.Type == eventpkg.TypeHTTP {
Expand Down Expand Up @@ -124,3 +124,23 @@ func transformHeaders(req http.Header) map[string]string {

return headers
}


func correctEventData(event *eventpkg.Event, body []byte, mime string) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add comment explaining what this function does?

if len(body) > 0 {
switch mime {
case mimeJSON:
err := json.Unmarshal(body, &event.Data)
if err != nil {
return errors.New("malformed JSON body")
}
break
default:
Copy link
Contributor

@alexdebrie alexdebrie Mar 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two notes:

  1. 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.

  2. 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)
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if byteSlice, ok := event.Data.([]byte);
ok && (strings.HasPrefix(mime, mimeFormMultipart) || mime == mimeFormURLEncoded) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

event.Data = string(byteSlice)
}
}
}
return nil
}