Ignore EventTime if it's not present #492
Conversation
| @@ -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 }) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
Codecov Report
@@ Coverage Diff @@
## master #492 +/- ##
========================================
+ Coverage 71% 71.2% +0.2%
========================================
Files 37 37
Lines 2190 2195 +5
========================================
+ Hits 1555 1563 +8
+ Misses 572 571 -1
+ Partials 63 61 -2
Continue to review full report at Codecov.
|
| @@ -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()) | |||
There was a problem hiding this comment.
Should we just make the Event struct EventTime into a string to begin with, then to time.Now().String() to populate?
There was a problem hiding this comment.
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.
| @@ -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"` | |||
There was a problem hiding this comment.
I'm not sure I understand why we need this to be a pointer and not just the string representation of the time.Now()?
event/event.go
Outdated
| if val, err := time.Parse(time.RFC3339, headers.Get("CE-EventTime")); err == nil { | ||
| event.EventTime = val | ||
| if headers.Get("CE-EventTime") != "" { | ||
| if val, err := time.Parse(time.RFC3339, headers.Get("CE-EventTime")); err == nil { |
There was a problem hiding this comment.
I would flip this around, the val is stlll local to the outer if block
if headers.Get("CE-EventTime") != "" {
val, err := time.Parse(time.RFC3339, headers.Get("CE-EventTime"))
if err != nil {
return nil, err
}
event.EventTime = &val
}
| @@ -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 }) | |||
There was a problem hiding this comment.
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!
| name: "valid CloudEvent in binary mode with invalid event time", | ||
| requestHeaders: http.Header{ | ||
| "Content-Type": []string{"text/plain"}, | ||
| "Ce-Eventtype": []string{"myevent"}, |
There was a problem hiding this comment.
Should these be CE-<name> like the events/events.go package? I guess it doesn't really matter but for consistency...
There was a problem hiding this comment.
This is how headers are stored internally in http.Header struct.
What did you implement:
This PR fixes parsing CloudEvent EventTime field. If EventTime is not preset it will not fill it with default value (now EventTime is pointer to time.Time)
It also makes consistent parsing functions (parsing in binary mode should also return error if it's impossible to parse time).
Todos:
Is this ready for review?: Yes
Is it a breaking change?: NO