Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ignore EventTime if it's not present #492

Merged
merged 2 commits into from
Jul 25, 2018
Merged

Ignore EventTime if it's not present #492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2040cc4
Ignore EventTime if it's not present
Jul 25, 2018
4dd7d7a
refactor if statement
Jul 25, 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
8 changes: 7 additions & 1 deletion Gopkg.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 13 additions & 5 deletions event/event.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ type Event struct {
CloudEventsVersion string `json:"cloudEventsVersion" validate:"required"`
Source string `json:"source" validate:"uri,required"`
EventID string `json:"eventID" validate:"required"`
EventTime time.Time `json:"eventTime,omitempty"`
EventTime *time.Time `json:"eventTime,omitempty"`
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 not sure I understand why we need this to be a pointer and not just the string representation of the time.Now()?

SchemaURL string `json:"schemaURL,omitempty"`
Extensions zap.MapStringInterface `json:"extensions,omitempty"`
ContentType string `json:"contentType,omitempty"`
Expand All @@ -43,12 +43,14 @@ type Event struct {

// New return new instance of Event.
func New(eventType TypeName, mimeType string, payload interface{}) *Event {
now := time.Now()

event := &Event{
EventType: eventType,
CloudEventsVersion: CloudEventsVersion,
Source: "https://serverless.com/event-gateway/#transformationVersion=" + TransformationVersion,
EventID: uuid.NewV4().String(),
EventTime: time.Now(),
EventTime: &now,
ContentType: mimeType,
Data: payload,
Extensions: map[string]interface{}{
Expand Down Expand Up @@ -128,7 +130,9 @@ func (e Event) MarshalLogObject(enc zapcore.ObjectEncoder) error {
enc.AddString("cloudEventsVersion", e.CloudEventsVersion)
enc.AddString("source", e.Source)
enc.AddString("eventID", e.EventID)
enc.AddString("eventTime", e.EventTime.String())
if e.EventTime != nil {
enc.AddString("eventTime", e.EventTime.String())
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we just make the Event struct EventTime into a string to begin with, then to time.Now().String() to populate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer time.Time because.. it's a time :) First of all it's good pattern to use types where they are meaningful, secondly it enabled all time operations on this data. Right now it's not used but eventually it will be. Also if someone uses our Event struct (it's based on CloudEvents spec) it's better to provide meaningful types.

}
if e.SchemaURL != "" {
enc.AddString("schemaURL", e.SchemaURL)
}
Expand Down Expand Up @@ -180,8 +184,12 @@ func parseAsCloudEventBinary(headers http.Header, payload interface{}) (*Event,
return nil, err
}

if val, err := time.Parse(time.RFC3339, headers.Get("CE-EventTime")); err == nil {
event.EventTime = val
if headers.Get("CE-EventTime") != "" {
val, err := time.Parse(time.RFC3339, headers.Get("CE-EventTime"))
if err != nil {
return nil, err
}
event.EventTime = &val
}

if val := headers.Get("CE-SchemaURL"); len(val) > 0 {
Expand Down
69 changes: 69 additions & 0 deletions event/event_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ import (
"net/http"
"net/url"
"testing"
"time"

"github.com/bouk/monkey"
eventpkg "github.com/serverless/event-gateway/event"
"github.com/stretchr/testify/assert"
)
Expand Down Expand Up @@ -36,6 +38,9 @@ func TestNew_Encoding(t *testing.T) {
}

func TestFromRequest(t *testing.T) {
patch := monkey.Patch(time.Now, func() time.Time { return testTime })
Copy link
Contributor Author

@mthenw mthenw Jul 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure convinced to this library but it works so it's kinda OK. And I still think it's better than abusing dependency injection pattern.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've never used it before myself, but ok! Wonder if we can just have a mock function to return that, like in the github.com/stretchr/testify/mock package?

In any case, if this works then I would leave it for now!

defer patch.Unpatch()

for _, testCase := range fromRequestTests {
t.Run(testCase.name, func(t *testing.T) {
url, _ := url.Parse("http://example.com")
Expand All @@ -49,6 +54,7 @@ func TestFromRequest(t *testing.T) {
assert.Equal(t, testCase.expectedError, err)
} else {
assert.Equal(t, testCase.expectedEvent.EventType, received.EventType, "EventType is not equal")
assert.Equal(t, testCase.expectedEvent.EventTime, received.EventTime, "EventTime is not equal")
assert.Equal(t, testCase.expectedEvent.Source, received.Source, "Source is not equal")
assert.Equal(t, testCase.expectedEvent.CloudEventsVersion, received.CloudEventsVersion, "CloudEventsVersion is not equal")
assert.Equal(t, testCase.expectedEvent.ContentType, received.ContentType, "ContentType is not equal")
Expand Down Expand Up @@ -107,6 +113,8 @@ var newTests = []struct {
},
}

var testTime = time.Date(1985, time.April, 12, 23, 20, 50, 00, time.UTC) //1985-04-12T23:20:50.00Z

var encodingTests = []struct {
name string
body []byte
Expand Down Expand Up @@ -151,20 +159,37 @@ var fromRequestTests = []struct {
requestHeaders: http.Header{"Content-Type": []string{"application/cloudevents+json"}},
requestBody: []byte(`{
"eventType": "user.created",
"eventTime": "1985-04-12T23:20:50.00Z",
"cloudEventsVersion": "0.1",
"source": "http://example.com",
"eventID": "6f6ada3b-0aa2-4b3c-989a-91ffc6405f11",
"contentType": "text/plain",
"data": "test"
}`),

expectedEvent: &eventpkg.Event{
EventType: eventpkg.TypeName("user.created"),
EventTime: &testTime,
CloudEventsVersion: "0.1",
Source: "http://example.com",
ContentType: "text/plain",
Data: "test",
},
},
{
name: "valid CloudEvent with invalid event time",
requestHeaders: http.Header{"Content-Type": []string{"application/cloudevents+json"}},
requestBody: []byte(`{
"eventType": "user.created",
"eventTime": "nottime",
"cloudEventsVersion": "0.1",
"source": "http://example.com",
"eventID": "6f6ada3b-0aa2-4b3c-989a-91ffc6405f11",
"contentType": "text/plain",
"data": "test"
}`),
expectedError: &time.ParseError{Layout: "\"2006-01-02T15:04:05Z07:00\"", Value: "\"nottime\"", LayoutElem: "2006", ValueElem: "nottime\"", Message: ""},
},
{
name: "error if invalid CloudEvent",
requestHeaders: http.Header{"Content-Type": []string{"application/cloudevents+json"}},
Expand Down Expand Up @@ -197,6 +222,47 @@ var fromRequestTests = []struct {
Extensions: map[string]interface{}{"myExtension": "ding"},
},
},
{
name: "valid CloudEvent in binary mode with valid event time",
requestHeaders: http.Header{
"Content-Type": []string{"text/plain"},
"Ce-Eventtype": []string{"myevent"},
"Ce-Eventtypeversion": []string{"0.1beta"},
"Ce-Cloudeventsversion": []string{"0.1"},
"Ce-Source": []string{"https://example.com"},
"Ce-Eventid": []string{"778d495b-a29e-48f9-a438-a26de1e33515"},
"Ce-Eventtime": []string{"1985-04-12T23:20:50.00Z"},
"Ce-Schemaurl": []string{"https://example.com"},
"Ce-X-MyExtension": []string{"ding"},
},
requestBody: []byte("hey there"),
expectedEvent: &eventpkg.Event{
EventType: eventpkg.TypeName("myevent"),
EventTime: &testTime,
CloudEventsVersion: "0.1",
Source: "https://example.com",
ContentType: "text/plain",
SchemaURL: "https://example.com",
Data: []byte("hey there"),
Extensions: map[string]interface{}{"myExtension": "ding"},
},
},
{
name: "valid CloudEvent in binary mode with invalid event time",
requestHeaders: http.Header{
"Content-Type": []string{"text/plain"},
"Ce-Eventtype": []string{"myevent"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be CE-<name> like the events/events.go package? I guess it doesn't really matter but for consistency...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how headers are stored internally in http.Header struct.

"Ce-Eventtypeversion": []string{"0.1beta"},
"Ce-Cloudeventsversion": []string{"0.1"},
"Ce-Source": []string{"https://example.com"},
"Ce-Eventid": []string{"778d495b-a29e-48f9-a438-a26de1e33515"},
"Ce-Eventtime": []string{"nottime"},
"Ce-Schemaurl": []string{"https://example.com"},
"Ce-X-MyExtension": []string{"ding"},
},
requestBody: []byte("hey there"),
expectedError: &time.ParseError{Layout: "2006-01-02T15:04:05Z07:00", Value: "nottime", LayoutElem: "2006", ValueElem: "nottime", Message: ""},
},
{
name: "error if invalid CloudEvent in binary mode",
requestHeaders: http.Header{
Expand All @@ -218,6 +284,7 @@ var fromRequestTests = []struct {
requestBody: []byte("hey there"),
expectedEvent: &eventpkg.Event{
EventType: eventpkg.TypeName("myevent"),
EventTime: &testTime,
CloudEventsVersion: "0.1",
Source: "https://serverless.com/event-gateway/#transformationVersion=0.1",
ContentType: "application/octet-stream",
Expand Down Expand Up @@ -259,6 +326,7 @@ var fromRequestTests = []struct {
}`),
expectedEvent: &eventpkg.Event{
EventType: eventpkg.TypeName("user.created"),
EventTime: &testTime,
CloudEventsVersion: "0.1",
Source: "https://serverless.com/event-gateway/#transformationVersion=0.1",
ContentType: "application/json",
Expand All @@ -275,6 +343,7 @@ var fromRequestTests = []struct {
requestBody: []byte(`{"key": "value"}`),
expectedEvent: &eventpkg.Event{
EventType: eventpkg.TypeHTTPRequest,
EventTime: &testTime,
CloudEventsVersion: "0.1",
Source: "https://serverless.com/event-gateway/#transformationVersion=0.1",
ContentType: "application/json",
Expand Down