From 0cfc8f4f4126d6a5fb39ef6c80d23506b61d5068 Mon Sep 17 00:00:00 2001 From: Mark Mandel Date: Tue, 17 Jan 2023 15:24:05 -0800 Subject: [PATCH] Review updates * Reorder in alias.go * More coverage of TestNewEventsFromHTTPRequest * tests for `to_events(...)` * Better comment documentation. Signed-off-by: Mark Mandel --- v2/alias.go | 2 +- v2/binding/format/format.go | 3 + v2/binding/to_event.go | 3 +- v2/binding/to_event_test.go | 50 +++++++++++ v2/protocol/http/utility.go | 9 +- v2/protocol/http/utility_test.go | 145 ++++++++++++++++++------------- 6 files changed, 147 insertions(+), 65 deletions(-) diff --git a/v2/alias.go b/v2/alias.go index 337774ec4..2fbfaa9a7 100644 --- a/v2/alias.go +++ b/v2/alias.go @@ -140,8 +140,8 @@ var ( NewEventFromHTTPResponse = http.NewEventFromHTTPResponse NewEventsFromHTTPRequest = http.NewEventsFromHTTPRequest NewEventsFromHTTPResponse = http.NewEventsFromHTTPResponse - NewHTTPRequestFromEvents = http.NewHTTPRequestFromEvents NewHTTPRequestFromEvent = http.NewHTTPRequestFromEvent + NewHTTPRequestFromEvents = http.NewHTTPRequestFromEvents IsHTTPBatch = http.IsHTTPBatch // HTTP Messages diff --git a/v2/binding/format/format.go b/v2/binding/format/format.go index 4db6db948..6bdd1842b 100644 --- a/v2/binding/format/format.go +++ b/v2/binding/format/format.go @@ -51,6 +51,9 @@ func (jb jsonBatchFmt) MediaType() string { return event.ApplicationCloudEventsBatchJSON } +// Marshal will return an error for jsonBatchFmt since the Format interface doesn't support batch Marshalling, and we +// know it's structured batch json, we'll go direct to the json.UnMarshall() (see `ToEvents()`) since that is the best +// way to support batch operations for now. func (jb jsonBatchFmt) Marshal(e *event.Event) ([]byte, error) { return nil, errors.New("not supported for batch events") } diff --git a/v2/binding/to_event.go b/v2/binding/to_event.go index e6cfaacd2..d3332c158 100644 --- a/v2/binding/to_event.go +++ b/v2/binding/to_event.go @@ -77,8 +77,7 @@ func ToEvents(ctx context.Context, message MessageReader, body io.Reader) ([]eve // Since Format doesn't support batch Marshalling, and we know it's structured batch json, we'll go direct to the // json.UnMarshall(), since that is the best way to support batch operations for now. var events []event.Event - err := json.NewDecoder(body).Decode(&events) - return events, err + return events, json.NewDecoder(body).Decode(&events) } type messageToEventBuilder event.Event diff --git a/v2/binding/to_event_test.go b/v2/binding/to_event_test.go index 0a0e00c00..185744a23 100644 --- a/v2/binding/to_event_test.go +++ b/v2/binding/to_event_test.go @@ -7,8 +7,12 @@ package binding_test import ( "context" + "io" + nethttp "net/http" + "strings" "testing" + "github.com/cloudevents/sdk-go/v2/protocol/http" "github.com/stretchr/testify/require" "github.com/cloudevents/sdk-go/v2/binding" @@ -159,3 +163,49 @@ func TestToEvent_transformers_applied_once(t *testing.T) { } }) } + +func TestToEvents(t *testing.T) { + fixture := map[string]struct { + contentType string + jsn string + expected func(*testing.T, []event.Event, error) + }{ + "valid event": { + jsn: `[{"data":"foo","datacontenttype":"application/json","id":"id","source":"source","specversion":"1.0","type":"type"}]`, + expected: func(t *testing.T, list []event.Event, err error) { + require.NoError(t, err) + require.Len(t, list, 1) + require.Equal(t, "id", list[0].ID()) + }, + }, + "invalid event": { + jsn: `[{"data":"foo","datacontenttype":"application/json","specversion":"0.1","type":"type"}]`, + expected: func(t *testing.T, _ []event.Event, err error) { + require.ErrorContains(t, err, "specversion: unknown value") + }, + }, + "bad content type": { + jsn: `[{"data":"foo","datacontenttype":"application/json","id":"id","source":"source","specversion":"1.0","type":"type"}]`, + contentType: event.ApplicationJSON, + expected: func(t *testing.T, _ []event.Event, err error) { + require.ErrorContains(t, err, "cannot convert message to batched events") + }, + }, + } + + for k, v := range fixture { + t.Run(k, func(t *testing.T) { + header := nethttp.Header{} + if len(v.contentType) == 0 { + header.Set(http.ContentType, event.ApplicationCloudEventsBatchJSON) + } else { + header.Set(http.ContentType, v.contentType) + } + + buf := io.NopCloser(strings.NewReader(v.jsn)) + msg := http.NewMessage(header, buf) + list, err := binding.ToEvents(context.Background(), msg, msg.BodyReader) + v.expected(t, list, err) + }) + } +} diff --git a/v2/protocol/http/utility.go b/v2/protocol/http/utility.go index 9d194af0a..350fc1cf6 100644 --- a/v2/protocol/http/utility.go +++ b/v2/protocol/http/utility.go @@ -27,19 +27,20 @@ func NewEventFromHTTPResponse(resp *nethttp.Response) (*event.Event, error) { return binding.ToEvent(context.Background(), msg) } -// NewEventsFromHTTPRequest returns a batched set of Events from a http.Request +// NewEventsFromHTTPRequest returns a batched set of Events from a HTTP Request func NewEventsFromHTTPRequest(req *nethttp.Request) ([]event.Event, error) { msg := NewMessageFromHttpRequest(req) return binding.ToEvents(context.Background(), msg, msg.BodyReader) } -// NewEventsFromHTTPResponse returns a batched set of Events from a http.Response +// NewEventsFromHTTPResponse returns a batched set of Events from a HTTP Response func NewEventsFromHTTPResponse(resp *nethttp.Response) ([]event.Event, error) { msg := NewMessageFromHttpResponse(resp) return binding.ToEvents(context.Background(), msg, msg.BodyReader) } // NewHTTPRequestFromEvent creates a http.Request object that can be used with any http.Client for a singular event. +// This is an HTTP POST action to the provided url. func NewHTTPRequestFromEvent(ctx context.Context, url string, event event.Event) (*nethttp.Request, error) { if err := event.Validate(); err != nil { return nil, err @@ -57,7 +58,7 @@ func NewHTTPRequestFromEvent(ctx context.Context, url string, event event.Event) } // NewHTTPRequestFromEvents creates a http.Request object that can be used with any http.Client for sending -// a batched set of events. +// a batched set of events. This is an HTTP POST action to the provided url. func NewHTTPRequestFromEvents(ctx context.Context, url string, events []event.Event) (*nethttp.Request, error) { // Sending batch events is quite straightforward, as there is only JSON format, so a simple implementation. for _, e := range events { @@ -81,7 +82,7 @@ func NewHTTPRequestFromEvents(ctx context.Context, url string, events []event.Ev return request, nil } -// IsHTTPBatch returns of the current http.Request or http.Response is a batch event operation, by checking the +// IsHTTPBatch returns if the current http.Request or http.Response is a batch event operation, by checking the // header `Content-Type` value. func IsHTTPBatch(header nethttp.Header) bool { return header.Get(ContentType) == event.ApplicationCloudEventsBatchJSON diff --git a/v2/protocol/http/utility_test.go b/v2/protocol/http/utility_test.go index a58cb0df2..a0583094e 100644 --- a/v2/protocol/http/utility_test.go +++ b/v2/protocol/http/utility_test.go @@ -94,14 +94,17 @@ func TestNewEventFromHttpResponse(t *testing.T) { } func TestNewEventsFromHTTPRequest(t *testing.T) { + type expected struct { - len int - ids []string + len int + ids []string + errorContains string } fixtures := map[string]struct { - jsn string - expected expected + jsn string + contentType string + expected expected }{ "single": { jsn: `[{"data":"foo","datacontenttype":"application/json","id":"id","source":"source","specversion":"1.0","type":"type"}]`, @@ -117,35 +120,44 @@ func TestNewEventsFromHTTPRequest(t *testing.T) { ids: []string{"id1", "id2", "id3"}, }, }, + "invalid header": { + jsn: `{"data":"foo","datacontenttype":"application/json","id":"id","source":"source","specversion":"1.0","type":"type"}`, + contentType: event.ApplicationJSON, + expected: expected{ + errorContains: "cannot convert message to batched events", + }, + }, + "invalid event": { + jsn: `[{"data":"foo","datacontenttype":"application/json","specversion":"0.1","type":"type"}]`, + expected: expected{ + errorContains: "specversion: unknown value", + }, + }, } for k, v := range fixtures { t.Run(k, func(t *testing.T) { req := httptest.NewRequest("POST", "http://localhost", bytes.NewReader([]byte(v.jsn))) - req.Header.Set(ContentType, event.ApplicationCloudEventsBatchJSON) + if len(v.contentType) == 0 { + req.Header.Set(ContentType, event.ApplicationCloudEventsBatchJSON) + } else { + req.Header.Set(ContentType, v.contentType) + } events, err := NewEventsFromHTTPRequest(req) - require.NoError(t, err) - require.Len(t, events, v.expected.len) - for i, e := range events { - test.AssertEvent(t, e, test.IsValid()) - require.Equal(t, v.expected.ids[i], events[i].ID()) + if len(v.expected.errorContains) == 0 { + require.NoError(t, err) + require.Len(t, events, v.expected.len) + for i, e := range events { + test.AssertEvent(t, e, test.IsValid()) + require.Equal(t, v.expected.ids[i], events[i].ID()) + } + } else { + require.Error(t, err) + require.ErrorContainsf(t, err, v.expected.errorContains, "error should include message") } }) } - - t.Run("bad request", func(t *testing.T) { - e := event.New() - e.SetID(uuid.New().String()) - e.SetSource("example/uri") - e.SetType("example.type") - require.NoError(t, e.SetData(event.ApplicationJSON, map[string]string{"hello": "world"})) - req, err := NewHTTPRequestFromEvent(context.Background(), "http://localhost", e) - require.NoError(t, err) - - _, err = NewEventsFromHTTPRequest(req) - require.ErrorContainsf(t, err, "cannot convert message to batched events", "error should include message") - }) } func TestNewEventsFromHTTPResponse(t *testing.T) { @@ -164,12 +176,6 @@ func TestNewEventsFromHTTPResponse(t *testing.T) { } func TestNewHTTPRequestFromEvent(t *testing.T) { - e := event.New() - e.SetID(uuid.New().String()) - e.SetSource("example/uri") - e.SetType("example.type") - require.NoError(t, e.SetData(event.ApplicationJSON, map[string]string{"hello": "world"})) - // echo back what we get, so we can compare events at either side. ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set(ContentType, r.Header.Get(ContentType)) @@ -188,33 +194,32 @@ func TestNewHTTPRequestFromEvent(t *testing.T) { })) defer ts.Close() - req, err := NewHTTPRequestFromEvent(context.Background(), ts.URL, e) - require.NoError(t, err) - - resp, err := ts.Client().Do(req) - require.NoError(t, err) + t.Run("valid event", func(t *testing.T) { + e := event.New() + e.SetID(uuid.New().String()) + e.SetSource("example/uri") + e.SetType("example.type") + require.NoError(t, e.SetData(event.ApplicationJSON, map[string]string{"hello": "world"})) - result, err := NewEventFromHTTPResponse(resp) - require.NoError(t, err) - require.Equal(t, &e, result) -} + req, err := NewHTTPRequestFromEvent(context.Background(), ts.URL, e) + require.NoError(t, err) -func TestNewHTTPRequestFromEvents(t *testing.T) { - var events []event.Event - e := event.New() - e.SetID(uuid.New().String()) - e.SetSource("example/uri") - e.SetType("example.type") - require.NoError(t, e.SetData(event.ApplicationJSON, map[string]string{"hello": "world"})) - events = append(events, e.Clone()) + resp, err := ts.Client().Do(req) + require.NoError(t, err) - e.SetID(uuid.New().String()) - require.NoError(t, e.SetData(event.ApplicationJSON, map[string]string{"goodbye": "world"})) - events = append(events, e) + result, err := NewEventFromHTTPResponse(resp) + require.NoError(t, err) + require.Equal(t, &e, result) + }) - require.Len(t, events, 2) - require.NotEqual(t, events[0], events[1]) + t.Run("invalid event", func(t *testing.T) { + e := event.New() + _, err := NewHTTPRequestFromEvent(context.Background(), ts.URL, e) + require.ErrorContains(t, err, "id: MUST be a non-empty string") + }) +} +func TestNewHTTPRequestFromEvents(t *testing.T) { // echo back what we get, so we can compare events at either side. ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set(ContentType, r.Header.Get(ContentType)) @@ -226,15 +231,39 @@ func TestNewHTTPRequestFromEvents(t *testing.T) { })) defer ts.Close() - req, err := NewHTTPRequestFromEvents(context.Background(), ts.URL, events) - require.NoError(t, err) + t.Run("valid events", func(t *testing.T) { + var events []event.Event + e := event.New() + e.SetID(uuid.New().String()) + e.SetSource("example/uri") + e.SetType("example.type") + require.NoError(t, e.SetData(event.ApplicationJSON, map[string]string{"hello": "world"})) + events = append(events, e.Clone()) - resp, err := ts.Client().Do(req) - require.NoError(t, err) + e.SetID(uuid.New().String()) + require.NoError(t, e.SetData(event.ApplicationJSON, map[string]string{"goodbye": "world"})) + events = append(events, e) + + require.Len(t, events, 2) + require.NotEqual(t, events[0], events[1]) + + req, err := NewHTTPRequestFromEvents(context.Background(), ts.URL, events) + require.NoError(t, err) + + resp, err := ts.Client().Do(req) + require.NoError(t, err) + + result, err := NewEventsFromHTTPResponse(resp) + require.NoError(t, err) + require.Equal(t, events, result) + }) + + t.Run("invalid events", func(t *testing.T) { + events := []event.Event{event.New()} + _, err := NewHTTPRequestFromEvents(context.Background(), ts.URL, events) + require.ErrorContains(t, err, "id: MUST be a non-empty string") + }) - result, err := NewEventsFromHTTPResponse(resp) - require.NoError(t, err) - require.Equal(t, events, result) } func TestIsHTTPBatch(t *testing.T) {