Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 2 additions & 21 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ if (TradeData.TryParse(receivedBuffer, out var decoded, out _))
}
```

**Messages with Repeating Groups (Recommended - Order-Safe API):**
**Messages with Repeating Groups (Span-Based API):**
```csharp
// Create message with groups
var orderBook = new OrderBookData { InstrumentId = 42 };
Expand Down Expand Up @@ -146,17 +146,7 @@ bool success = OrderBookData.TryEncode(
);
```

**Messages with Repeating Groups (Traditional API):**
```csharp
// Alternative: Manual API for advanced scenarios
Span<byte> buffer = stackalloc byte[1024];
orderBook.BeginEncoding(buffer, out var writer);
OrderBookData.TryEncodeBids(ref writer, bids);
OrderBookData.TryEncodeAsks(ref writer, asks);
int bytesWritten = writer.BytesWritten;
```

**Messages with Variable-Length Data (Recommended - Order-Safe API):**
**Messages with Variable-Length Data (Span-Based API):**
```csharp
// Encode message with varData
var order = new NewOrderData { OrderId = 123, Price = 9950 };
Expand All @@ -181,15 +171,6 @@ decoded.ConsumeVariableLengthSegments(
);
```

**Messages with Variable-Length Data (Traditional API):**
```csharp
// Alternative: Manual API for advanced scenarios
Span<byte> buffer = stackalloc byte[512];
order.BeginEncoding(buffer, out var writer);
NewOrderData.TryEncodeSymbol(ref writer, symbolBytes);
int bytesWritten = writer.BytesWritten;
```

## Example Schema

```xml
Expand Down
80 changes: 44 additions & 36 deletions docs/ENCODING_ORDER_FIX.md
Original file line number Diff line number Diff line change
Expand Up @@ -133,35 +133,57 @@ All tests updated to use new API:
| **Intent Clarity** | ⚠️ Chained calls | ✅ Single call, clear order |
| **Type Safety** | ⚠️ Can call in wrong order | ✅ Compiler enforces order |
| **Performance** | ✅ Zero overhead | ✅ Zero overhead |
| **Backward Compat** | N/A | ✅ Traditional API still works |
| **Backward Compat** | N/A | ⚠️ Old API removed (breaking change) |

## Final State

**As of this implementation, the old error-prone APIs have been removed:**
- ❌ `BeginEncoding()` method removed
- ❌ `TryEncode{GroupName}()` public methods removed (now private helpers)
- ❌ `TryEncode{VarDataName}()` public methods removed (now private helpers)
- ✅ Only the comprehensive `TryEncode()` methods remain (span-based and callback-based)

This is a **breaking change** but necessary to prevent encoding errors in production systems.

## Migration Guide

### Before (Removed - Incorrect Design)
### Before (Fluent API - Removed)
```csharp
var encoder = orderBook.CreateEncoder(buffer)
.WithBids(bids)
.WithAsks(asks);
int bytesWritten = encoder.BytesWritten;
```

### After (New Order-Safe Design)
### Before (Traditional API - Removed)
```csharp
orderBook.BeginEncoding(buffer, out var writer);
OrderBookData.TryEncodeBids(ref writer, bids);
OrderBookData.TryEncodeAsks(ref writer, asks);
int bytesWritten = writer.BytesWritten;
```

### After (Current - Only Available API)
```csharp
// Span-based API (simple)
bool success = OrderBookData.TryEncode(
orderBook,
buffer,
bids,
asks,
out int bytesWritten
);
```

### Traditional API (Still Available)
```csharp
orderBook.BeginEncoding(buffer, out var writer);
OrderBookData.TryEncodeBids(ref writer, bids);
OrderBookData.TryEncodeAsks(ref writer, asks);
int bytesWritten = writer.BytesWritten;
// OR Callback-based API (zero-allocation)
bool success = OrderBookData.TryEncode(
orderBook,
buffer,
bidCount: 3,
bidsEncoder: (int index, ref BidsData item) => { /* populate */ },
askCount: 2,
asksEncoder: (int index, ref AsksData item) => { /* populate */ },
out int bytesWritten
);
```

## Validation
Expand All @@ -186,37 +208,23 @@ TryEncode(message, buffer, bids, asks, out bytesWritten)

✅ Order matches schema exactly

### Binary Compatibility Test

```csharp
// Old API
orderBook.BeginEncoding(bufferOld, out var writerOld);
OrderBookData.TryEncodeBids(ref writerOld, bids);
OrderBookData.TryEncodeAsks(ref writerOld, asks);

// New API
OrderBookData.TryEncode(orderBook, bufferNew, bids, asks, out _);

// Validation
Assert.True(bufferOld.SequenceEqual(bufferNew)); // ✅ PASSES
```

Both APIs produce **identical binary output**.

## Conclusion

@pedrosakuma's feedback identified a critical design flaw that could have caused data corruption in production. The new design:
The old error-prone encoding APIs have been successfully removed. The codebase now enforces compile-time safety through the comprehensive `TryEncode` methods:

1. ✅ **Prevents encoding errors by design** - impossible to encode in wrong order
2. ✅ **Enforces schema order at compile time** - compiler is the safety net
3. ✅ **Simplifies the API** - single method call instead of builder pattern
4. ✅ **Maintains backward compatibility** - traditional API still works
5. ✅ **Zero performance overhead** - direct encoding, no intermediate state
2. ✅ **Enforces schema order at compile-time** - compiler is the safety net
3. ✅ **Simplifies the API** - single method call instead of multiple steps
4. ✅ **Zero performance overhead** - direct encoding, no intermediate state
5. ✅ **Two variants available**:
- Span-based API for simple use cases with array allocation
- Callback-based API for zero-allocation high-performance scenarios

This is a **significant improvement** over the original fluent API and demonstrates the value of thorough code review.
This is a **breaking change** from the previous API, but it significantly improves safety and prevents data corruption bugs in production.

---

**Commit**: 1e4cf6a
**Tests**: 104/104 passing ✅
**Backward Compatible**: Yes ✅
**Implementation Date**: 2025-10-30
**Tests**: All passing ✅
**Breaking Change**: Yes - old APIs removed

41 changes: 8 additions & 33 deletions src/SbeCodeGenerator/Generators/Types/MessageDefinition.cs
Original file line number Diff line number Diff line change
Expand Up @@ -154,37 +154,14 @@ private void AppendEncodeHelpers(StringBuilder sb, int tabs)

private void AppendVariableDataEncoding(StringBuilder sb, int tabs)
{
// Generate BeginEncoding method that returns a SpanWriter positioned after the message
sb.AppendLine("/// <summary>", tabs);
sb.AppendLine($"/// Begins encoding this {Name} message with variable-length data support.", tabs);
sb.AppendLine("/// Use the returned writer to encode groups and varData fields.", tabs);
sb.AppendLine("/// </summary>", tabs);
sb.AppendLine("/// <param name=\"buffer\">The destination buffer.</param>", tabs);
sb.AppendLine("/// <param name=\"writer\">The writer positioned after the message header for writing variable data.</param>", tabs);
sb.AppendLine("/// <returns>True if encoding started successfully; otherwise, false.</returns>", tabs);
sb.AppendLine($"public bool BeginEncoding(Span<byte> buffer, out SpanWriter writer)", tabs);
sb.AppendLine("{", tabs++);
sb.AppendLine("if (buffer.Length < MESSAGE_SIZE)", tabs);
sb.AppendLine("{", tabs++);
sb.AppendLine("writer = default;", tabs);
sb.AppendLine("return false;", tabs);
sb.AppendLine("}", --tabs);
sb.AppendLine("", tabs);
sb.AppendLine("writer = new SpanWriter(buffer);", tabs);
sb.AppendLine("writer.Write(this);", tabs);
sb.AppendLine("return true;", tabs);
sb.AppendLine("}", --tabs);

// Generate helper methods for each group
// Generate private helper methods for each group (used internally by comprehensive TryEncode)
foreach (var group in Groups.Cast<GroupDefinition>())
{
sb.AppendLine("/// <summary>", tabs);
sb.AppendLine($"/// Encodes a {group.Name} group into the buffer.", tabs);
sb.AppendLine($"/// Internal helper: Encodes a {group.Name} group into the buffer.", tabs);
sb.AppendLine("/// Use the comprehensive TryEncode method instead.", tabs);
sb.AppendLine("/// </summary>", tabs);
sb.AppendLine("/// <param name=\"writer\">The writer to use for encoding.</param>", tabs);
sb.AppendLine($"/// <param name=\"entries\">The {group.Name} entries to encode.</param>", tabs);
sb.AppendLine("/// <returns>True if encoding succeeded; otherwise, false.</returns>", tabs);
sb.AppendLine($"public static bool TryEncode{group.Name}(ref SpanWriter writer, ReadOnlySpan<{group.Name}Data> entries)", tabs);
sb.AppendLine($"private static bool TryEncode{group.Name}(ref SpanWriter writer, ReadOnlySpan<{group.Name}Data> entries)", tabs);
sb.AppendLine("{", tabs++);
sb.AppendLine($"// Write group header", tabs);
sb.AppendLine($"var header = new {group.DimensionType}", tabs);
Expand All @@ -207,16 +184,14 @@ private void AppendVariableDataEncoding(StringBuilder sb, int tabs)
sb.AppendLine("}", --tabs);
}

// Generate helper methods for each varData field
// Generate private helper methods for each varData field (used internally by comprehensive TryEncode)
foreach (var data in Datas.Cast<DataFieldDefinition>())
{
sb.AppendLine("/// <summary>", tabs);
sb.AppendLine($"/// Encodes a {data.Name} varData field into the buffer.", tabs);
sb.AppendLine($"/// Internal helper: Encodes a {data.Name} varData field into the buffer.", tabs);
sb.AppendLine("/// Use the comprehensive TryEncode method instead.", tabs);
sb.AppendLine("/// </summary>", tabs);
sb.AppendLine("/// <param name=\"writer\">The writer to use for encoding.</param>", tabs);
sb.AppendLine($"/// <param name=\"data\">The {data.Name} data to encode.</param>", tabs);
sb.AppendLine("/// <returns>True if encoding succeeded; otherwise, false.</returns>", tabs);
sb.AppendLine($"public static bool TryEncode{data.Name.FirstCharToUpper()}(ref SpanWriter writer, ReadOnlySpan<byte> data)", tabs);
sb.AppendLine($"private static bool TryEncode{data.Name.FirstCharToUpper()}(ref SpanWriter writer, ReadOnlySpan<byte> data)", tabs);
sb.AppendLine("{", tabs++);
sb.AppendLine($"// Write length prefix (uint8 for VarString8)", tabs);
sb.AppendLine($"if (data.Length > 255)", tabs);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -201,51 +201,6 @@ out int bytesWritten
Assert.Equal(2, askCount);
}

[Fact]
public void TryEncode_CompareWithOldAPI_ProducesSameResult()
{
// This test demonstrates that the new API produces identical output to the old API

// Arrange
Span<byte> bufferOld = stackalloc byte[1024];
Span<byte> bufferNew = stackalloc byte[1024];

var orderBook = new Integration.Test.V0.OrderBookData
{
InstrumentId = 42
};

var bids = new Integration.Test.V0.OrderBookData.BidsData[]
{
new Integration.Test.V0.OrderBookData.BidsData { Price = 1000, Quantity = 100 }
};

var asks = new Integration.Test.V0.OrderBookData.AsksData[]
{
new Integration.Test.V0.OrderBookData.AsksData { Price = 2000, Quantity = 200 }
};

// Act - Old API (manual BeginEncoding + individual TryEncode calls)
Assert.True(orderBook.BeginEncoding(bufferOld, out var writerOld));
Assert.True(Integration.Test.V0.OrderBookData.TryEncodeBids(ref writerOld, bids));
Assert.True(Integration.Test.V0.OrderBookData.TryEncodeAsks(ref writerOld, asks));
int bytesWrittenOld = writerOld.BytesWritten;

// Act - New API (single TryEncode call with all parameters)
bool successNew = Integration.Test.V0.OrderBookData.TryEncode(
orderBook,
bufferNew,
bids,
asks,
out int bytesWrittenNew
);

// Assert - Both produce identical results
Assert.True(successNew);
Assert.Equal(bytesWrittenOld, bytesWrittenNew);
Assert.True(bufferOld.Slice(0, bytesWrittenOld).SequenceEqual(bufferNew.Slice(0, bytesWrittenNew)));
}

[Fact]
public void TryEncode_CallbackBased_ZeroAllocation()
{
Expand Down
Loading