Review updates

* Reorder in alias.go
* More coverage of TestNewEventsFromHTTPRequest
* tests for `to_events(...)`
* Better comment documentation.

Signed-off-by: Mark Mandel <markmandel@google.com>

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

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")
 }

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

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)
+		})
+	}
+}

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

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) {