From e8712795a6e121f8320e30c6409a5c564387216f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 30 Oct 2025 17:13:48 +0000 Subject: [PATCH 1/3] Initial plan From 61162307b42877c6694797317cdf7a80822709ec Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 30 Oct 2025 17:24:46 +0000 Subject: [PATCH 2/3] Remove error-prone encoding flows, keep only comprehensive TryEncode methods Co-authored-by: pedrosakuma <39205549+pedrosakuma@users.noreply.github.com> --- README.md | 23 +- docs/ENCODING_ORDER_FIX.md | 80 +++--- .../Generators/Types/MessageDefinition.cs | 41 +-- .../FluentEncoderIntegrationTests.cs | 45 --- .../GroupAndVarDataEncodingTests.cs | 271 ++++++++++-------- 5 files changed, 213 insertions(+), 247 deletions(-) diff --git a/README.md b/README.md index 44cb390..31366f8 100644 --- a/README.md +++ b/README.md @@ -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 }; @@ -146,17 +146,7 @@ bool success = OrderBookData.TryEncode( ); ``` -**Messages with Repeating Groups (Traditional API):** -```csharp -// Alternative: Manual API for advanced scenarios -Span 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 }; @@ -181,15 +171,6 @@ decoded.ConsumeVariableLengthSegments( ); ``` -**Messages with Variable-Length Data (Traditional API):** -```csharp -// Alternative: Manual API for advanced scenarios -Span buffer = stackalloc byte[512]; -order.BeginEncoding(buffer, out var writer); -NewOrderData.TryEncodeSymbol(ref writer, symbolBytes); -int bytesWritten = writer.BytesWritten; -``` - ## Example Schema ```xml diff --git a/docs/ENCODING_ORDER_FIX.md b/docs/ENCODING_ORDER_FIX.md index 86965f5..5204852 100644 --- a/docs/ENCODING_ORDER_FIX.md +++ b/docs/ENCODING_ORDER_FIX.md @@ -133,11 +133,21 @@ 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) @@ -145,8 +155,17 @@ var encoder = orderBook.CreateEncoder(buffer) 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, @@ -154,14 +173,17 @@ bool success = OrderBookData.TryEncode( 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 @@ -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**: October 30, 2025 +**Tests**: All passing ✅ +**Breaking Change**: Yes - old APIs removed + diff --git a/src/SbeCodeGenerator/Generators/Types/MessageDefinition.cs b/src/SbeCodeGenerator/Generators/Types/MessageDefinition.cs index b3dce6c..7760cdf 100644 --- a/src/SbeCodeGenerator/Generators/Types/MessageDefinition.cs +++ b/src/SbeCodeGenerator/Generators/Types/MessageDefinition.cs @@ -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("/// ", 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("/// ", tabs); - sb.AppendLine("/// The destination buffer.", tabs); - sb.AppendLine("/// The writer positioned after the message header for writing variable data.", tabs); - sb.AppendLine("/// True if encoding started successfully; otherwise, false.", tabs); - sb.AppendLine($"public bool BeginEncoding(Span 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()) { sb.AppendLine("/// ", 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("/// ", tabs); - sb.AppendLine("/// The writer to use for encoding.", tabs); - sb.AppendLine($"/// The {group.Name} entries to encode.", tabs); - sb.AppendLine("/// True if encoding succeeded; otherwise, false.", 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); @@ -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()) { sb.AppendLine("/// ", 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("/// ", tabs); - sb.AppendLine("/// The writer to use for encoding.", tabs); - sb.AppendLine($"/// The {data.Name} data to encode.", tabs); - sb.AppendLine("/// True if encoding succeeded; otherwise, false.", tabs); - sb.AppendLine($"public static bool TryEncode{data.Name.FirstCharToUpper()}(ref SpanWriter writer, ReadOnlySpan data)", tabs); + sb.AppendLine($"private static bool TryEncode{data.Name.FirstCharToUpper()}(ref SpanWriter writer, ReadOnlySpan data)", tabs); sb.AppendLine("{", tabs++); sb.AppendLine($"// Write length prefix (uint8 for VarString8)", tabs); sb.AppendLine($"if (data.Length > 255)", tabs); diff --git a/tests/SbeCodeGenerator.IntegrationTests/FluentEncoderIntegrationTests.cs b/tests/SbeCodeGenerator.IntegrationTests/FluentEncoderIntegrationTests.cs index d8b7260..5cb28f7 100644 --- a/tests/SbeCodeGenerator.IntegrationTests/FluentEncoderIntegrationTests.cs +++ b/tests/SbeCodeGenerator.IntegrationTests/FluentEncoderIntegrationTests.cs @@ -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 bufferOld = stackalloc byte[1024]; - Span 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() { diff --git a/tests/SbeCodeGenerator.IntegrationTests/GroupAndVarDataEncodingTests.cs b/tests/SbeCodeGenerator.IntegrationTests/GroupAndVarDataEncodingTests.cs index a9029e4..b76aef3 100644 --- a/tests/SbeCodeGenerator.IntegrationTests/GroupAndVarDataEncodingTests.cs +++ b/tests/SbeCodeGenerator.IntegrationTests/GroupAndVarDataEncodingTests.cs @@ -7,63 +7,36 @@ namespace SbeCodeGenerator.IntegrationTests { /// - /// Integration tests for Phase 3: Group and VarData encoding support + /// Integration tests for Group and VarData encoding using the comprehensive TryEncode API + /// These tests verify that the allocation-free and allocation-based encoding flows work correctly /// public class GroupAndVarDataEncodingTests { [Fact] - public void GroupEncoding_TryEncodeBids_EncodesCorrectly() + public void GroupEncoding_EmptyGroups_EncodesCorrectly() { - // Arrange - Create buffer and bids data - Span buffer = stackalloc byte[512]; - var bids = new Integration.Test.V0.OrderBookData.BidsData[] - { - new Integration.Test.V0.OrderBookData.BidsData { Price = 1000, Quantity = 100 }, - new Integration.Test.V0.OrderBookData.BidsData { Price = 1010, Quantity = 101 }, - new Integration.Test.V0.OrderBookData.BidsData { Price = 1020, Quantity = 102 } - }; - - // Act - Encode using TryEncodeBids - var writer = new Integration.Test.V0.Runtime.SpanWriter(buffer); - var success = Integration.Test.V0.OrderBookData.TryEncodeBids(ref writer, bids); - - // Assert - Assert.True(success); - Assert.Equal(Integration.Test.V0.GroupSizeEncoding.MESSAGE_SIZE + 3 * Integration.Test.V0.OrderBookData.BidsData.MESSAGE_SIZE, writer.BytesWritten); - - // Verify the encoded data can be read back - var reader = new Integration.Test.V0.Runtime.SpanReader(buffer); - Assert.True(reader.TryRead(out var header)); - Assert.Equal((ushort)Integration.Test.V0.OrderBookData.BidsData.MESSAGE_SIZE, header.BlockLength); - Assert.Equal(3u, header.NumInGroup); - - for (int i = 0; i < 3; i++) - { - Assert.True(reader.TryRead(out var bid)); - Assert.Equal(bids[i].Price.Value, bid.Price.Value); - Assert.Equal(bids[i].Quantity, bid.Quantity); - } - } - - [Fact] - public void GroupEncoding_EmptyGroup_EncodesCorrectly() - { - // Arrange - Empty group + // Arrange - Empty groups Span buffer = stackalloc byte[512]; + var orderBook = new Integration.Test.V0.OrderBookData { InstrumentId = 42 }; var bids = Array.Empty(); - - // Act - var writer = new Integration.Test.V0.Runtime.SpanWriter(buffer); - var success = Integration.Test.V0.OrderBookData.TryEncodeBids(ref writer, bids); + var asks = Array.Empty(); + + // Act - Use comprehensive TryEncode with empty groups + bool success = Integration.Test.V0.OrderBookData.TryEncode( + orderBook, + buffer, + bids, + asks, + out int bytesWritten + ); // Assert Assert.True(success); - Assert.Equal(Integration.Test.V0.GroupSizeEncoding.MESSAGE_SIZE, writer.BytesWritten); - - // Verify header - var reader = new Integration.Test.V0.Runtime.SpanReader(buffer); - Assert.True(reader.TryRead(out var header)); - Assert.Equal(0u, header.NumInGroup); + Assert.True(bytesWritten > 0); + + // Decode and verify + Assert.True(Integration.Test.V0.OrderBookData.TryParse(buffer, out var decoded, out var variableData)); + Assert.Equal(42, decoded.InstrumentId); } [Fact] @@ -71,6 +44,7 @@ public void GroupEncoding_TwoGroups_EncodesCorrectly() { // Arrange - Create buffer and data for both bids and asks Span buffer = stackalloc byte[512]; + var orderBook = new Integration.Test.V0.OrderBookData { InstrumentId = 99 }; var bids = new Integration.Test.V0.OrderBookData.BidsData[] { new Integration.Test.V0.OrderBookData.BidsData { Price = 1000, Quantity = 100 }, @@ -81,68 +55,104 @@ public void GroupEncoding_TwoGroups_EncodesCorrectly() new Integration.Test.V0.OrderBookData.AsksData { Price = 2000, Quantity = 200 } }; - // Act - Encode both groups - var writer = new Integration.Test.V0.Runtime.SpanWriter(buffer); - Assert.True(Integration.Test.V0.OrderBookData.TryEncodeBids(ref writer, bids)); - Assert.True(Integration.Test.V0.OrderBookData.TryEncodeAsks(ref writer, asks)); + // Act - Use comprehensive TryEncode + bool success = Integration.Test.V0.OrderBookData.TryEncode( + orderBook, + buffer, + bids, + asks, + out int bytesWritten + ); - // Assert - Verify both groups are encoded correctly - var reader = new Integration.Test.V0.Runtime.SpanReader(buffer); + // Assert + Assert.True(success); + Assert.True(bytesWritten > 0); - // Verify bids - Assert.True(reader.TryRead(out var bidsHeader)); - Assert.Equal(2u, bidsHeader.NumInGroup); - for (int i = 0; i < 2; i++) - { - Assert.True(reader.TryRead(out var bid)); - Assert.Equal(bids[i].Price.Value, bid.Price.Value); - } - - // Verify asks - Assert.True(reader.TryRead(out var asksHeader)); - Assert.Equal(1u, asksHeader.NumInGroup); - Assert.True(reader.TryRead(out var ask)); - Assert.Equal(2000, ask.Price.Value); + // Decode and verify both groups + Assert.True(Integration.Test.V0.OrderBookData.TryParse(buffer, out var decoded, out var variableData)); + Assert.Equal(99, decoded.InstrumentId); + + var decodedBids = new List(); + var decodedAsks = new List(); + + decoded.ConsumeVariableLengthSegments( + variableData, + bid => decodedBids.Add(bid), + ask => decodedAsks.Add(ask) + ); + + Assert.Equal(2, decodedBids.Count); + Assert.Single(decodedAsks); + Assert.Equal(1000, decodedBids[0].Price.Value); + Assert.Equal(2000, decodedAsks[0].Price.Value); } [Fact] public void GroupEncoding_InsufficientBuffer_ReturnsFalse() { // Arrange - Buffer too small - Span buffer = stackalloc byte[10]; // Too small for group header + Span buffer = stackalloc byte[10]; // Too small + 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 = Array.Empty(); // Act - var writer = new Integration.Test.V0.Runtime.SpanWriter(buffer); - var success = Integration.Test.V0.OrderBookData.TryEncodeBids(ref writer, bids); + bool success = Integration.Test.V0.OrderBookData.TryEncode( + orderBook, + buffer, + bids, + asks, + out int bytesWritten + ); // Assert Assert.False(success); + Assert.Equal(0, bytesWritten); } [Fact] - public void VarDataEncoding_TryEncodeSymbol_EncodesCorrectly() + public void VarDataEncoding_WithSymbol_EncodesCorrectly() { // Arrange Span buffer = stackalloc byte[512]; + var order = new Integration.Test.V0.NewOrderData + { + OrderId = 123, + Price = 9950, + Quantity = 100, + Side = Integration.Test.V0.OrderSide.Buy, + OrderType = Integration.Test.V0.OrderType.Limit + }; string symbol = "AAPL"; var symbolBytes = Encoding.UTF8.GetBytes(symbol); - // Act - Encode varData - var writer = new Integration.Test.V0.Runtime.SpanWriter(buffer); - var success = Integration.Test.V0.NewOrderData.TryEncodeSymbol(ref writer, symbolBytes); + // Act - Use comprehensive TryEncode + bool success = Integration.Test.V0.NewOrderData.TryEncode( + order, + buffer, + symbolBytes, + out int bytesWritten + ); // Assert Assert.True(success); - Assert.Equal(1 + symbolBytes.Length, writer.BytesWritten); // 1 byte for length + data - - // Verify the encoded data can be read back - Assert.Equal((byte)symbolBytes.Length, buffer[0]); // Length prefix - var decodedBytes = buffer.Slice(1, symbolBytes.Length); - var decodedSymbol = Encoding.UTF8.GetString(decodedBytes); + Assert.True(bytesWritten > 0); + + // Decode and verify + Assert.True(Integration.Test.V0.NewOrderData.TryParse(buffer, out var decoded, out var variableData)); + Assert.Equal(123, decoded.OrderId.Value); + + string decodedSymbol = ""; + decoded.ConsumeVariableLengthSegments( + variableData, + symbolData => { + decodedSymbol = Encoding.UTF8.GetString(symbolData.VarData.Slice(0, symbolData.Length)); + } + ); + Assert.Equal(symbol, decodedSymbol); } @@ -151,16 +161,27 @@ public void VarDataEncoding_EmptyData_EncodesCorrectly() { // Arrange Span buffer = stackalloc byte[512]; + var order = new Integration.Test.V0.NewOrderData + { + OrderId = 456, + Price = 1000, + Quantity = 10, + Side = Integration.Test.V0.OrderSide.Sell, + OrderType = Integration.Test.V0.OrderType.Market + }; var emptyData = Array.Empty(); // Act - var writer = new Integration.Test.V0.Runtime.SpanWriter(buffer); - var success = Integration.Test.V0.NewOrderData.TryEncodeSymbol(ref writer, emptyData); + bool success = Integration.Test.V0.NewOrderData.TryEncode( + order, + buffer, + emptyData, + out int bytesWritten + ); // Assert Assert.True(success); - Assert.Equal(1, writer.BytesWritten); // Just the length byte (0) - Assert.Equal(0, buffer[0]); + Assert.True(bytesWritten > 0); } [Fact] @@ -168,29 +189,55 @@ public void VarDataEncoding_TooLong_ReturnsFalse() { // Arrange - Data longer than 255 bytes (max for uint8 length) Span buffer = stackalloc byte[512]; + var order = new Integration.Test.V0.NewOrderData + { + OrderId = 789, + Price = 2000, + Quantity = 20, + Side = Integration.Test.V0.OrderSide.Buy, + OrderType = Integration.Test.V0.OrderType.Limit + }; var tooLongData = new byte[256]; // 256 > 255 // Act - var writer = new Integration.Test.V0.Runtime.SpanWriter(buffer); - var success = Integration.Test.V0.NewOrderData.TryEncodeSymbol(ref writer, tooLongData); + bool success = Integration.Test.V0.NewOrderData.TryEncode( + order, + buffer, + tooLongData, + out int bytesWritten + ); // Assert Assert.False(success); // Should fail because length > 255 + Assert.Equal(0, bytesWritten); } [Fact] public void VarDataEncoding_InsufficientBuffer_ReturnsFalse() { // Arrange - Buffer too small - Span buffer = stackalloc byte[2]; // Only room for length + 1 byte - var data = new byte[5]; // Need 6 bytes total + Span buffer = stackalloc byte[2]; // Too small + var order = new Integration.Test.V0.NewOrderData + { + OrderId = 111, + Price = 3000, + Quantity = 30, + Side = Integration.Test.V0.OrderSide.Sell, + OrderType = Integration.Test.V0.OrderType.Limit + }; + var data = new byte[5]; // Act - var writer = new Integration.Test.V0.Runtime.SpanWriter(buffer); - var success = Integration.Test.V0.NewOrderData.TryEncodeSymbol(ref writer, data); + bool success = Integration.Test.V0.NewOrderData.TryEncode( + order, + buffer, + data, + out int bytesWritten + ); // Assert Assert.False(success); + Assert.Equal(0, bytesWritten); } [Fact] @@ -217,12 +264,18 @@ public void CompleteMessageWithGroups_RoundTrip_PreservesData() new Integration.Test.V0.OrderBookData.AsksData { Price = 2010, Quantity = 201 } }; - // Act - Encode message with groups using BeginEncoding - Assert.True(orderBook.BeginEncoding(buffer, out var writer)); - Assert.True(Integration.Test.V0.OrderBookData.TryEncodeBids(ref writer, bids)); - Assert.True(Integration.Test.V0.OrderBookData.TryEncodeAsks(ref writer, asks)); + // Act - Encode message with groups using comprehensive TryEncode + bool success = Integration.Test.V0.OrderBookData.TryEncode( + orderBook, + buffer, + bids, + asks, + out int totalBytesWritten + ); - int totalBytesWritten = writer.BytesWritten; + // Assert + Assert.True(success); + Assert.True(totalBytesWritten > 0); // Decode the fixed part Assert.True(Integration.Test.V0.OrderBookData.TryParse(buffer, out var decodedMessage, out var variableData)); @@ -273,9 +326,17 @@ public void CompleteMessageWithVarData_RoundTrip_PreservesData() string symbol = "AAPL"; var symbolBytes = Encoding.UTF8.GetBytes(symbol); - // Act - Encode message with varData using BeginEncoding - Assert.True(order.BeginEncoding(buffer, out var writer)); - Assert.True(Integration.Test.V0.NewOrderData.TryEncodeSymbol(ref writer, symbolBytes)); + // Act - Encode message with varData using comprehensive TryEncode + bool success = Integration.Test.V0.NewOrderData.TryEncode( + order, + buffer, + symbolBytes, + out int bytesWritten + ); + + // Assert + Assert.True(success); + Assert.True(bytesWritten > 0); // Decode the fixed part Assert.True(Integration.Test.V0.NewOrderData.TryParse(buffer, out var decodedOrder, out var variableData)); @@ -299,19 +360,5 @@ public void CompleteMessageWithVarData_RoundTrip_PreservesData() Assert.Equal(symbol, decodedSymbol); Assert.Equal((byte)symbolBytes.Length, symbolLength); } - - [Fact] - public void BeginEncoding_InsufficientBuffer_ReturnsFalse() - { - // Arrange - Buffer too small for message header - Span buffer = stackalloc byte[2]; - var orderBook = new Integration.Test.V0.OrderBookData { InstrumentId = 42 }; - - // Act - var success = orderBook.BeginEncoding(buffer, out var writer); - - // Assert - Assert.False(success); - } } } From 645050433a883ec182550c6b59d15b89609fc2f3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 30 Oct 2025 17:29:59 +0000 Subject: [PATCH 3/3] Address code review feedback - improve comments and clarity Co-authored-by: pedrosakuma <39205549+pedrosakuma@users.noreply.github.com> --- docs/ENCODING_ORDER_FIX.md | 4 ++-- .../GroupAndVarDataEncodingTests.cs | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/docs/ENCODING_ORDER_FIX.md b/docs/ENCODING_ORDER_FIX.md index 5204852..d58687d 100644 --- a/docs/ENCODING_ORDER_FIX.md +++ b/docs/ENCODING_ORDER_FIX.md @@ -133,7 +133,7 @@ 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 | ✅ Old API removed (breaking change) | +| **Backward Compat** | N/A | ⚠️ Old API removed (breaking change) | ## Final State @@ -224,7 +224,7 @@ This is a **breaking change** from the previous API, but it significantly improv --- -**Implementation Date**: October 30, 2025 +**Implementation Date**: 2025-10-30 **Tests**: All passing ✅ **Breaking Change**: Yes - old APIs removed diff --git a/tests/SbeCodeGenerator.IntegrationTests/GroupAndVarDataEncodingTests.cs b/tests/SbeCodeGenerator.IntegrationTests/GroupAndVarDataEncodingTests.cs index b76aef3..66635db 100644 --- a/tests/SbeCodeGenerator.IntegrationTests/GroupAndVarDataEncodingTests.cs +++ b/tests/SbeCodeGenerator.IntegrationTests/GroupAndVarDataEncodingTests.cs @@ -8,7 +8,8 @@ namespace SbeCodeGenerator.IntegrationTests { /// /// Integration tests for Group and VarData encoding using the comprehensive TryEncode API - /// These tests verify that the allocation-free and allocation-based encoding flows work correctly + /// These tests verify the span-based (allocation-based) encoding flow + /// For callback-based (allocation-free) tests, see FluentEncoderIntegrationTests.cs /// public class GroupAndVarDataEncodingTests { @@ -345,7 +346,7 @@ out int bytesWritten Assert.Equal(100, decodedOrder.Quantity); // Decode the varData using ConsumeVariableLengthSegments - string decodedSymbol = ""; + string? decodedSymbol = null; byte symbolLength = 0; decodedOrder.ConsumeVariableLengthSegments( variableData, @@ -357,6 +358,7 @@ out int bytesWritten ); // Assert - Verify symbol matches + Assert.NotNull(decodedSymbol); Assert.Equal(symbol, decodedSymbol); Assert.Equal((byte)symbolBytes.Length, symbolLength); }