Merge pull request #203 from oschwald/greg/cleanups

Misc cleanup

Author: oschwald

CHANGELOG.md

@@ -1,5 +1,10 @@
 # Changes
 
+## 2.2.0 - TBD
+
+- An error is returned when a `maxminddb` struct tag is clearly invalid (non
+  UTF-8) instead of silently ignoring validation failures.
+
 ## 2.1.1 - 2025-11-26
 
 - Fixed `runtime.AddCleanup` misuse that prevented the memory-mapped file from

internal/decoder/data_decoder.go

@@ -113,6 +113,8 @@ type DataDecoder struct {
 const (
 	// This is the value used in libmaxminddb.
 	maximumDataStructureDepth = 512
+	pointerBase2              = 2048
+	pointerBase3              = 526336
 )
 
 // NewDataDecoder creates a [DataDecoder].
@@ -240,9 +242,9 @@ func (d *DataDecoder) decodePointer(
 	case 1, 4:
 		pointerValueOffset = 0
 	case 2:
-		pointerValueOffset = 2048
+		pointerValueOffset = pointerBase2
 	case 3:
-		pointerValueOffset = 526336
+		pointerValueOffset = pointerBase3
 	default:
 		return 0, 0, mmdberrors.NewInvalidDatabaseError("invalid pointer size: %d", pointerSize)
 	}

internal/decoder/decoder.go

@@ -14,12 +14,12 @@ type Decoder struct {
 	d             DataDecoder
 	offset        uint
 	nextOffset    uint
-	opts          decoderOptions
 	hasNextOffset bool
 }
 
 type decoderOptions struct {
-	// Reserved for future options
+	// Intentionally empty for now. DecoderOption callbacks are still invoked so
+	// adding options in a future release is non-breaking.
 }
 
 // DecoderOption configures a Decoder.
@@ -34,13 +34,10 @@ func NewDecoder(d DataDecoder, offset uint, options ...DecoderOption) *Decoder {
 		option(&opts)
 	}
 
-	decoder := &Decoder{
+	return &Decoder{
 		d:      d,
 		offset: offset,
-		opts:   opts,
 	}
-
-	return decoder
 }
 
 // ReadBool reads the value pointed by the decoder as a bool.
@@ -124,7 +121,7 @@ func (d *Decoder) ReadFloat64() (float64, error) {
 	return value, nil
 }
 
-// ReadInt32 reads the value pointed by the decoder as a int32.
+// ReadInt32 reads the value pointed by the decoder as an int32.
 //
 // Returns an error if the database is malformed or if the pointed value is not an int32.
 func (d *Decoder) ReadInt32() (int32, error) {
@@ -144,7 +141,7 @@ func (d *Decoder) ReadInt32() (int32, error) {
 
 // ReadUint16 reads the value pointed by the decoder as a uint16.
 //
-// Returns an error if the database is malformed or if the pointed value is not an uint16.
+// Returns an error if the database is malformed or if the pointed value is not a uint16.
 func (d *Decoder) ReadUint16() (uint16, error) {
 	size, offset, err := d.decodeCtrlDataAndFollow(KindUint16)
 	if err != nil {
@@ -162,7 +159,7 @@ func (d *Decoder) ReadUint16() (uint16, error) {
 
 // ReadUint32 reads the value pointed by the decoder as a uint32.
 //
-// Returns an error if the database is malformed or if the pointed value is not an uint32.
+// Returns an error if the database is malformed or if the pointed value is not a uint32.
 func (d *Decoder) ReadUint32() (uint32, error) {
 	size, offset, err := d.decodeCtrlDataAndFollow(KindUint32)
 	if err != nil {
@@ -180,7 +177,7 @@ func (d *Decoder) ReadUint32() (uint32, error) {
 
 // ReadUint64 reads the value pointed by the decoder as a uint64.
 //
-// Returns an error if the database is malformed or if the pointed value is not an uint64.
+// Returns an error if the database is malformed or if the pointed value is not a uint64.
 func (d *Decoder) ReadUint64() (uint64, error) {
 	size, offset, err := d.decodeCtrlDataAndFollow(KindUint64)
 	if err != nil {
@@ -198,7 +195,7 @@ func (d *Decoder) ReadUint64() (uint64, error) {
 
 // ReadUint128 reads the value pointed by the decoder as a uint128.
 //
-// Returns an error if the database is malformed or if the pointed value is not an uint128.
+// Returns an error if the database is malformed or if the pointed value is not a uint128.
 func (d *Decoder) ReadUint128() (hi, lo uint64, err error) {
 	size, offset, err := d.decodeCtrlDataAndFollow(KindUint128)
 	if err != nil {

internal/decoder/decoder_test.go

@@ -245,7 +245,7 @@ func TestDecodeUint16(t *testing.T) {
 		"a1ff":   uint16(255),
 		"a201f4": uint16(500),
 		"a22a78": uint16(10872),
-		"a2ffff": uint16(65535), // [cite: 88] - Note: reflection test uses uint64 expected value
+		"a2ffff": uint16(65535),
 	}
 
 	for hexStr, expected := range tests {
@@ -337,16 +337,12 @@ func TestDecodeUint128(t *testing.T) {
 	}
 }
 
-// TestPointers requires a specific data file and structure.
 func TestPointersInDecoder(t *testing.T) {
-	// This test requires the 'maps-with-pointers.raw' file used in reflection_test [cite: 92]
-	// It demonstrates how to handle pointers using the basic Decoder.
-	bytes, err := os.ReadFile(testFile("maps-with-pointers.raw")) // [cite: 92]
+	bytes, err := os.ReadFile(testFile("maps-with-pointers.raw"))
 	require.NoError(t, err)
 	dd := NewDataDecoder(bytes)
 
 	expected := map[uint]map[string]string{
-		// Offsets and expected values from reflection_test.go [cite: 92]
 		0:  {"long_key": "long_value1"},
 		22: {"long_key": "long_value2"},
 		37: {"long_key2": "long_value1"},
@@ -358,12 +354,11 @@ func TestPointersInDecoder(t *testing.T) {
 	for startOffset, expectedValue := range expected {
 		t.Run(fmt.Sprintf("Offset_%d", startOffset), func(t *testing.T) {
 			decoder := NewDecoder(dd, startOffset) // Start at the specific offset
-			actualValue := make(map[string]string)
 
 			// Expecting a map at the target offset (may be behind a pointer)
 			mapIter, size, err := decoder.ReadMap()
 			require.NoError(t, err, "ReadMap failed")
-			_ = size // Use size if needed for pre-allocation
+			actualValue := make(map[string]string, int(size))
 			for keyBytes, errIter := range mapIter {
 				require.NoError(t, errIter)
 				key := string(keyBytes)
@@ -487,28 +482,18 @@ func TestPeekKind(t *testing.T) {
 // TestPeekKindWithPointer tests that PeekKind correctly follows pointers
 // to get the actual kind of the pointed-to value.
 func TestPeekKindWithPointer(t *testing.T) {
-	// Create a buffer with a pointer that points to a string
-	// This is a simplified test - in real MMDB files pointers are more complex
+	// Create a buffer with a pointer at offset 0 and a string at offset 5.
 	buffer := []byte{
-		// Pointer (TypePointer=1, size/pointer encoding)
-		0x20, 0x05, // Simple pointer to offset 5
-		// Target string at offset 5 (but we'll put it at offset 2 for this test)
-		0x44, 't', 'e', 's', 't', // String "test"
+		0x20, 0x05,
+		0x00, 0x00, 0x00,
+		0x44, 't', 'e', 's', 't',
 	}
 
 	decoder := NewDecoder(NewDataDecoder(buffer), 0)
 
-	// PeekKind should follow the pointer and return KindString
 	actualType, err := decoder.PeekKind()
 	require.NoError(t, err, "PeekKind with pointer failed")
-
-	// Note: This test may need adjustment based on actual pointer encoding
-	// The important thing is that PeekKind follows pointers
-	if actualType != KindPointer {
-		// If the implementation follows pointers completely, it should return the target kind
-		// If it just returns KindPointer, that's also acceptable behavior
-		t.Logf("PeekKind returned %d (this may be expected behavior)", actualType)
-	}
+	require.Equal(t, KindString, actualType)
 }
 
 // ExampleDecoder_PeekKind demonstrates how to use PeekKind for
@@ -556,12 +541,21 @@ func ExampleDecoder_PeekKind() {
 func TestDecoderOptions(t *testing.T) {
 	buffer := []byte{0x44, 't', 'e', 's', 't'} // String "test"
 	dd := NewDataDecoder(buffer)
+	optionCalled := false
+	option := func(*decoderOptions) {
+		optionCalled = true
+	}
 
-	// Test that options infrastructure works (even with no current options)
+	// Test that options infrastructure works (even with no current options).
 	decoder1 := NewDecoder(dd, 0)
 	require.NotNil(t, decoder1)
 
-	// Test that passing empty options slice works
+	// Test that passing options invokes each option callback.
+	decoderWithOption := NewDecoder(dd, 0, option)
+	require.NotNil(t, decoderWithOption)
+	require.True(t, optionCalled)
+
+	// Test that passing empty options slice works.
 	decoder2 := NewDecoder(dd, 0)
 	require.NotNil(t, decoder2)
 }

internal/decoder/error_context.go

@@ -1,24 +1,6 @@
 package decoder
 
-import (
-	"github.com/oschwald/maxminddb-golang/v2/internal/mmdberrors"
-)
-
-// errorContext provides zero-allocation error context tracking for Decoder.
-// This is only used when an error occurs, ensuring no performance impact
-// on the happy path.
-type errorContext struct {
-	path *mmdberrors.PathBuilder // Only allocated when needed
-}
-
-// BuildPath implements mmdberrors.ErrorContextTracker.
-// This is only called when an error occurs, so allocation is acceptable.
-func (e *errorContext) BuildPath() string {
-	if e.path == nil {
-		return "" // No path tracking enabled
-	}
-	return e.path.Build()
-}
+import "github.com/oschwald/maxminddb-golang/v2/internal/mmdberrors"
 
 // wrapError wraps an error with context information when an error occurs.
 // Zero allocation on happy path - only allocates when error != nil.
@@ -38,12 +20,3 @@ func (*Decoder) wrapErrorAtOffset(err error, offset uint) error {
 	}
 	return mmdberrors.WrapWithContext(err, offset, nil)
 }
-
-// Example of how to integrate into existing decoder methods:
-// Instead of:
-//   return mmdberrors.NewInvalidDatabaseError("message")
-// Use:
-//   return d.wrapError(mmdberrors.NewInvalidDatabaseError("message"))
-//
-// This adds zero overhead when no error occurs, but provides rich context
-// when errors do happen.

internal/decoder/error_context_test.go

@@ -18,7 +18,7 @@ func TestWrapError_ZeroAllocationHappyPath(t *testing.T) {
 	err := decoder.wrapError(nil)
 	require.NoError(t, err)
 
-	// DataDecoder should always have path tracking enabled
+	// Decoder should be initialized
 	require.NotNil(t, decoder.d)
 }
 
@@ -48,22 +48,18 @@ func TestPathBuilder(t *testing.T) {
 	// Test basic path building
 	require.Equal(t, "/", builder.Build())
 
-	builder.PushMap("city")
+	builder.PrependMap("city")
 	require.Equal(t, "/city", builder.Build())
 
-	builder.PushMap("names")
-	require.Equal(t, "/city/names", builder.Build())
-
-	builder.PushSlice(0)
-	require.Equal(t, "/city/names/0", builder.Build())
+	builder.PrependMap("names")
+	require.Equal(t, "/names/city", builder.Build())
 
-	// Test pop
-	builder.Pop()
+	builder = mmdberrors.NewPathBuilder()
+	builder.ParseAndExtend("/city/names")
 	require.Equal(t, "/city/names", builder.Build())
 
-	// Test reset
-	builder.Reset()
-	require.Equal(t, "/", builder.Build())
+	builder.PrependSlice(0)
+	require.Equal(t, "/0/city/names", builder.Build())
 }
 
 // Benchmark to verify zero allocation on happy path.
@@ -104,15 +100,14 @@ func BenchmarkWrapError_ErrorPath(b *testing.B) {
 func ExampleContextualError() {
 	// This would be internal to the decoder, shown for illustration
 	builder := mmdberrors.NewPathBuilder()
-	builder.PushMap("city")
-	builder.PushMap("names")
-	builder.PushMap("en")
+	builder.PrependMap("en")
+	builder.PrependMap("names")
+	builder.PrependMap("city")
 
 	// Simulate an error with context
 	originalErr := mmdberrors.NewInvalidDatabaseError("string too long")
 
-	contextTracker := &errorContext{path: builder}
-	wrappedErr := mmdberrors.WrapWithContext(originalErr, 1234, contextTracker)
+	wrappedErr := mmdberrors.WrapWithContext(originalErr, 1234, builder)
 
 	fmt.Println(wrappedErr.Error())
 	// Output: at offset 1234, path /city/names/en: string too long
@@ -211,13 +206,9 @@ func TestContextualErrorIntegration(t *testing.T) {
 		err := rd.Decode(0, &result)
 		require.Error(t, err)
 
-		// Debug: print the actual error and path
-		t.Logf("Error: %v", err)
-
 		// Should get a wrapped error with slice index in path
 		var contextErr mmdberrors.ContextualError
 		require.ErrorAs(t, err, &contextErr)
-		t.Logf("Path: %s", contextErr.Path)
 
 		// Verify we get the exact path with correct order
 		require.Equal(t, "/list/0/name", contextErr.Path)