Repository: hamba/avro
Pull request #238 (2023-02-22 15:44:38)

fix: cleanup tests and code
This PR cleans up some tests, comments and code


Patch:
diff --git a/internal/bytesx/reset.go b/internal/bytesx/reset.go
index 4ea67ef..0e2485c 100644
--- a/internal/bytesx/reset.go
+++ b/internal/bytesx/reset.go
@@ -10,7 +10,7 @@ type ResetReader struct {
 	tail int
 }
 
-// NewResetReader returns a new Reader reading from b.
+// NewResetReader returns a new ResetReader reading from b.
 func NewResetReader(b []byte) *ResetReader {
 	r := &ResetReader{}
 	r.Reset(b)
diff --git a/registry/client.go b/registry/client.go
index 20cb654..ff9343a 100644
--- a/registry/client.go
+++ b/registry/client.go
@@ -112,7 +112,9 @@ var defaultClient = &http.Client{
 			KeepAlive: 90 * time.Second,
 		}).DialContext,
 		TLSHandshakeTimeout: 3 * time.Second,
+		IdleConnTimeout:     90 * time.Second,
 	},
+	Timeout: 10 * time.Second,
 }
 
 // ClientFunc is a function used to customize the Client.
diff --git a/registry/decoder.go b/registry/decoder.go
index 43eb87c..329ebdd 100644
--- a/registry/decoder.go
+++ b/registry/decoder.go
@@ -11,22 +11,20 @@ import (
 // DecoderFunc is a function used to customize the Decoder.
 type DecoderFunc func(*Decoder)
 
-// WithAPI generates a new decoder with the same client as the one on
-// which the method is invoked and as api the one provided in input.
+// WithAPI sets the avro configuration on the decoder.
 func WithAPI(api avro.API) DecoderFunc {
 	return func(d *Decoder) {
 		d.api = api
 	}
 }
 
-// Decoder is entitled to receive raw messages as bytes, obtain the
-// related schema and then use it to deserialize the remaining content.
+// Decoder decodes confluent wire formatted avro payloads.
 type Decoder struct {
 	client *Client
 	api    avro.API
 }
 
-// NewDecoder shall return a new decoder given a client.
+// NewDecoder returns a decoder that will get schemas from client.
 func NewDecoder(client *Client, opts ...DecoderFunc) *Decoder {
 	d := &Decoder{
 		client: client,
@@ -38,42 +36,35 @@ func NewDecoder(client *Client, opts ...DecoderFunc) *Decoder {
 	return d
 }
 
-// Decode takes in input a payload to be deserialized, extrapolates
-// its schema id, gets the related schema and uses it to unmarshal the payload.
-// The payload shall be formatted according to:
-// https://docs.confluent.io/3.2.0/schema-registry/docs/serializer-formatter.html#wire-format .
-func (d *Decoder) Decode(
-	ctx context.Context,
-	payload []byte,
-	target interface{},
-) error {
-	if len(payload) < 6 {
-		return fmt.Errorf("payload not containing data")
+// Decode decodes data into v.
+// The data must be formatted using the Confluent wire format, otherwise
+// and error will be returned.
+// See:
+// https://docs.confluent.io/3.2.0/schema-registry/docs/serializer-formatter.html#wire-format.
+func (d *Decoder) Decode(ctx context.Context, data []byte, v any) error {
+	if len(data) < 6 {
+		return fmt.Errorf("data too short")
 	}
 
-	id, err := extractSchemaIDFromPayload(payload)
+	id, err := extractSchemaID(data)
 	if err != nil {
-		return fmt.Errorf("unable to extract schema id from payload, error: %w", err)
+		return fmt.Errorf("extracting schema id: %w", err)
 	}
 
 	schema, err := d.client.GetSchema(ctx, id)
 	if err != nil {
-		return fmt.Errorf("unable to obtain schema, error: %w", err)
+		return fmt.Errorf("getting schema: %w", err)
 	}
 
-	return d.api.Unmarshal(schema, payload[5:], target)
+	return d.api.Unmarshal(schema, data[5:], v)
 }
 
-// ExtractSchemaIDFromPayload extrapolates the schema id from a payload composed
-// of raw bytes containing a magic bytes, 4 bytes representing the schema encoding,
-// and the remaining payload being encoded with avro, as described in
-// https://docs.confluent.io/3.2.0/schema-registry/docs/serializer-formatter.html#wire-format .
-func extractSchemaIDFromPayload(payload []byte) (int, error) {
-	if len(payload) < 5 {
-		return 0, fmt.Errorf("payload too short to contain avro header")
+func extractSchemaID(data []byte) (int, error) {
+	if len(data) < 5 {
+		return 0, fmt.Errorf("data too short")
 	}
-	if payload[0] != 0 {
-		return 0, fmt.Errorf("magic byte value is %d, different from 0", payload[0])
+	if data[0] != 0 {
+		return 0, fmt.Errorf("invalid magic byte: %x", data[0])
 	}
-	return int(binary.BigEndian.Uint32(payload[1:5])), nil
+	return int(binary.BigEndian.Uint32(data[1:5])), nil
 }


Test patch:
diff --git a/gen/gen_test.go b/gen/gen_test.go
index 1af3cdb..51438da 100644
--- a/gen/gen_test.go
+++ b/gen/gen_test.go
@@ -3,7 +3,6 @@ package gen_test
 import (
 	"bytes"
 	"flag"
-	"fmt"
 	"os"
 	"regexp"
 	"strings"
@@ -97,7 +96,7 @@ func TestStruct_ConfigurableFieldTags(t *testing.T) {
 
 	for _, test := range tests {
 		test := test
-		t.Run(fmt.Sprintf("%s", test.tagStyle), func(t *testing.T) {
+		t.Run(string(test.tagStyle), func(t *testing.T) {
 			gc := gen.Config{
 				PackageName: "Something",
 				Tags: map[string]gen.TagStyle{
@@ -109,7 +108,7 @@ func TestStruct_ConfigurableFieldTags(t *testing.T) {
 			for _, expected := range []string{
 				"package something",
 				"type Test struct {",
-				fmt.Sprintf("SomeString string `avro:\"someString\" %s`", test.expectedTag),
+				"SomeString string `avro:\"someString\" " + test.expectedTag + "`",
 				"}",
 			} {
 				assert.Contains(t, lines, expected)
diff --git a/ocf/ocf_test.go b/ocf/ocf_test.go
index 880a43f..cd6f97b 100644
--- a/ocf/ocf_test.go
+++ b/ocf/ocf_test.go
@@ -146,7 +146,7 @@ func TestDecoder(t *testing.T) {
 	assert.Equal(t, 1, count)
 }
 
-func TestDecoderDeflate(t *testing.T) {
+func TestDecoder_WithDeflate(t *testing.T) {
 	unionStr := "union value"
 	want := FullRecord{
 		Strings: []string{"string1", "string2", "string3", "string4", "string5"},
@@ -195,7 +195,7 @@ func TestDecoderDeflate(t *testing.T) {
 	assert.Equal(t, 1, count)
 }
 
-func TestDecoderDeflate_InvalidData(t *testing.T) {
+func TestDecoder_WithDeflateHandlesInvalidData(t *testing.T) {
 	f, err := os.Open("testdata/deflate-invalid-data.avro")
 	if err != nil {
 		t.Error(err)
@@ -211,7 +211,7 @@ func TestDecoderDeflate_InvalidData(t *testing.T) {
 	assert.Error(t, dec.Error())
 }
 
-func TestDecoderSnappy(t *testing.T) {
+func TestDecoder_WithSnappy(t *testing.T) {
 	unionStr := "union value"
 	want := FullRecord{
 		Strings: []string{"string1", "string2", "string3", "string4", "string5"},
@@ -260,7 +260,7 @@ func TestDecoderSnappy(t *testing.T) {
 	assert.Equal(t, 1, count)
 }
 
-func TestDecoderSnappy_InvalidData(t *testing.T) {
+func TestDecoder_WithSnappyHandlesInvalidData(t *testing.T) {
 	f, err := os.Open("testdata/snappy-invalid-data.avro")
 	if err != nil {
 		t.Error(err)
@@ -276,7 +276,7 @@ func TestDecoderSnappy_InvalidData(t *testing.T) {
 	assert.Error(t, dec.Error())
 }
 
-func TestDecoderSnappy_ShortCRC(t *testing.T) {
+func TestDecoder_WithSnappyHandlesShortCRC(t *testing.T) {
 	f, err := os.Open("testdata/snappy-short-crc.avro")
 	if err != nil {
 		t.Error(err)
@@ -292,7 +292,7 @@ func TestDecoderSnappy_ShortCRC(t *testing.T) {
 	assert.Error(t, dec.Error())
 }
 
-func TestDecoderSnappy_InvalidCRC(t *testing.T) {
+func TestDecoder_WithSnappyHandlesInvalidCRC(t *testing.T) {
 	f, err := os.Open("testdata/snappy-invalid-crc.avro")
 	if err != nil {
 		t.Error(err)
diff --git a/registry/client_test.go b/registry/client_test.go
index ab4f186..e08e3d4 100644
--- a/registry/client_test.go
+++ b/registry/client_test.go
@@ -734,7 +734,7 @@ func TestError_Error(t *testing.T) {
 	assert.Equal(t, "Schema not found", str)
 }
 
-func TestError_ErrorEmptyMEssage(t *testing.T) {
+func TestError_ErrorEmptyMessage(t *testing.T) {
 	err := registry.Error{
 		StatusCode: 404,
 	}
diff --git a/registry/decoder_internal_test.go b/registry/decoder_internal_test.go
index 869e7c0..7faf213 100644
--- a/registry/decoder_internal_test.go
+++ b/registry/decoder_internal_test.go
@@ -1,42 +1,58 @@
 package registry
 
 import (
-	"encoding/binary"
-	"fmt"
 	"testing"
 
+	"github.com/hamba/avro/v2"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
-//series of tests for decoding
-
-func TestExtractSchemaIDFromPayload(t *testing.T) {
-	id := 42
-	payload := make([]byte, 5)
-	payload[0] = 0
-
-	binary.BigEndian.PutUint32(payload[1:5], uint32(id))
-
-	extracted_id, err := extractSchemaIDFromPayload(payload)
-
+func TestDecoder_WithAPI(t *testing.T) {
+	client, err := NewClient("http://example.com")
 	require.NoError(t, err)
 
-	assert.Equal(t, extracted_id, id)
-}
+	cfg := avro.Config{}.Freeze()
 
-func TestExtractSchemaIDFromPayload_HandlesError(t *testing.T) {
-	payload := make([]byte, 4)
-	payload[0] = 0
+	dec := NewDecoder(client, WithAPI(cfg))
 
-	_, err := extractSchemaIDFromPayload(payload)
-
-	assert.Equal(t, err.Error(), "payload too short to contain avro header")
-
-	payload = make([]byte, 5)
-	payload[0] = 1
-
-	_, err = extractSchemaIDFromPayload(payload)
+	assert.Equal(t, cfg, dec.api)
+}
 
-	assert.Equal(t, err.Error(), fmt.Sprintf("magic byte value is %d, different from 0", payload[0]))
+func TestExtractSchemaID(t *testing.T) {
+	tests := []struct {
+		name    string
+		data    []byte
+		wantID  int
+		wantErr require.ErrorAssertionFunc
+	}{
+		{
+			name:    "extracts id",
+			data:    []byte{0x0, 0x0, 0x0, 0x0, 0x2a},
+			wantID:  42,
+			wantErr: require.NoError,
+		},
+		{
+			name:    "handles short data",
+			data:    []byte{0x0, 0x0, 0x0, 0x2a},
+			wantID:  0,
+			wantErr: require.Error,
+		},
+		{
+			name:    "handles bad magic",
+			data:    []byte{0x1, 0x0, 0x0, 0x0, 0x2a},
+			wantID:  0,
+			wantErr: require.Error,
+		},
+	}
+
+	for _, test := range tests {
+		test := test
+		t.Run(test.name, func(t *testing.T) {
+			got, err := extractSchemaID(test.data)
+
+			test.wantErr(t, err)
+			assert.Equal(t, test.wantID, got)
+		})
+	}
 }
diff --git a/registry/decoder_test.go b/registry/decoder_test.go
index 4238ecf..bfe5a13 100644
--- a/registry/decoder_test.go
+++ b/registry/decoder_test.go
@@ -2,111 +2,70 @@ package registry_test
 
 import (
 	"context"
-	"encoding/binary"
-	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"testing"
 
-	"github.com/hamba/avro/v2"
 	"github.com/hamba/avro/v2/registry"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
-func TestDecoder_WithAPI(t *testing.T) {
-	client, err := registry.NewClient("http://example.com")
-	require.NoError(t, err)
-
-	registry.NewDecoder(client, registry.WithAPI(avro.DefaultConfig))
-}
-
 func TestDecoder_Decode(t *testing.T) {
-	//i declare a schema id and a struct to be deserialized
-	schema_id := 42
-	type Person struct {
-		Name string `avro:"name"`
-		Age  int    `avro:"age"`
+	tests := []struct {
+		name    string
+		data    []byte
+		schema  string
+		want    int
+		wantErr require.ErrorAssertionFunc
+	}{
+		{
+			name:    "decodes data",
+			data:    []byte{0x0, 0x0, 0x0, 0x0, 0x2a, 0x80, 0x2},
+			schema:  `{"schema":"int"}`,
+			want:    128,
+			wantErr: require.NoError,
+		},
+		{
+			name:    "handles short data",
+			data:    []byte{0x0, 0x0, 0x0, 0x0, 0x2a},
+			schema:  `{"schema":"int"}`,
+			wantErr: require.Error,
+		},
+		{
+			name:    "handles bad magic",
+			data:    []byte{0x1, 0x0, 0x0, 0x0, 0x2a, 0x80, 0x2},
+			schema:  `{"schema":"int"}`,
+			wantErr: require.Error,
+		},
+		{
+			name:    "handles bad schema",
+			data:    []byte{0x0, 0x0, 0x0, 0x0, 0x2a, 0x80, 0x2},
+			schema:  `{"schema":"nope"}`,
+			wantErr: require.Error,
+		},
 	}
-	john := &Person{
-		Name: "john",
-		Age:  28,
-	}
-
-	//i declare the string schema to be returned by the httptest server
-	var person_schema_string string = `{\"type\":\"record\",\"name\":\"person\",\"fields\":[{\"name\":\"name\",\"type\":\"string\"},{\"name\":\"age\",\"type\":\"int\"}]}`
-
-	//i declare the string schema to be parsed
-	var person_schema_string_to_parse string = `{"type":"record","name":"person","fields":[{"name":"name","type":"string"},{"name":"age","type":"int"}]}`
-
-	schema, err := avro.Parse(person_schema_string_to_parse)
-
-	//i marshal the payload
-	john_payload, err := avro.Marshal(schema, john)
-	require.NoError(t, err)
-
-	//i then add the magic byte to the payload, together with the encoded id
-	payload := make([]byte, 0, len(john_payload)+5)
-	payload = append(payload, 0)
-	binarySchemaId := make([]byte, 4)
-	binary.BigEndian.PutUint32(binarySchemaId, uint32(schema_id))
-	payload = append(payload, binarySchemaId...)
-	payload = append(payload, john_payload...)
-
-	//instantiating the test server
-	s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		assert.Equal(t, "GET", r.Method)
-		assert.Equal(t, fmt.Sprintf("/schemas/ids/%d", schema_id), r.URL.Path)
-
-		//_, _ = w.Write([]byte(fmt.Sprintf(`{"schema":""}`)))
-		_, _ = w.Write([]byte(fmt.Sprintf(`{"schema":"%s"}`, person_schema_string)))
-	}))
-	t.Cleanup(s.Close)
 
-	//creating the test client
-	client, _ := registry.NewClient(s.URL)
-	decoder := registry.NewDecoder(client)
+	for _, test := range tests {
+		test := test
+		t.Run(test.name, func(t *testing.T) {
+			h := http.NewServeMux()
+			h.Handle("/schemas/ids/42", http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
+				assert.Equal(t, "GET", req.Method)
 
-	target_john := &Person{}
-	err = decoder.Decode(context.Background(), payload, target_john)
+				_, _ = rw.Write([]byte(test.schema))
+			}))
+			srv := httptest.NewServer(h)
+			t.Cleanup(srv.Close)
 
-	require.NoError(t, err)
-	assert.Equal(t, john.Name, target_john.Name)
-	assert.Equal(t, john.Age, target_john.Age)
-}
-
-func TestDecoder_DecodeHandlesError(t *testing.T) {
-	schema_id := 42
-
-	//instantiating the test server
-	s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		assert.Equal(t, "GET", r.Method)
-		assert.Equal(t, fmt.Sprintf("/schemas/ids/%d", schema_id), r.URL.Path)
-
-		_, _ = w.Write([]byte(`{"schema":"boh"}`))
-	}))
-	t.Cleanup(s.Close)
-
-	//creating the test client
-	client, _ := registry.NewClient(s.URL)
-	decoder := registry.NewDecoder(client)
-
-	err := decoder.Decode(context.Background(), []byte{0}, nil)
+			client, _ := registry.NewClient(srv.URL)
+			decoder := registry.NewDecoder(client)
 
-	assert.Equal(t, "payload not containing data", err.Error())
+			var got int
+			err := decoder.Decode(context.Background(), test.data, &got)
 
-	err = decoder.Decode(context.Background(), []byte{1, 1, 1, 1, 1, 1, 1, 1, 1}, nil)
-
-	assert.Equal(t, "unable to extract schema id from payload, error: magic byte value is 1, different from 0", err.Error())
-
-	payload := make([]byte, 0, 6)
-	payload = append(payload, 0)
-	binarySchemaId := make([]byte, 4)
-	binary.BigEndian.PutUint32(binarySchemaId, uint32(schema_id))
-	payload = append(payload, binarySchemaId...)
-	payload = append(payload, []byte{0}...)
-
-	err = decoder.Decode(context.Background(), payload, nil)
-
-	assert.Equal(t, "unable to obtain schema, error: avro: unknown type: boh", err.Error())
+			test.wantErr(t, err)
+			assert.Equal(t, test.want, got)
+		})
+	}
 }