[ECO-4567] Support msgpack #1311
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughMigrates from legacy MsgPack.Cli to MessagePack‑CSharp v3, removes the IO.Ably.Shared.MsgPack shared project and related CI/script items, adds IMessagePackFormatter formatters and resolver, annotates many models with MessagePack attributes, updates request/protocol runtime selection, and replaces generated serializers with source‑gen compatible formatters and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Library consumer
participant AblyReq as AblyRequest
participant AblyHttp as AblyHttpClient
participant MsgPackH as MsgPackHelper
participant MPSerializer as MessagePackSerializer
note right of AblyReq: AblyRequest constructed with explicit Protocol
Client->>AblyReq: new AblyRequest(path, method, protocol)
AblyReq->>AblyHttp: Build HttpRequestMessage (Accept header per protocol)
AblyHttp->>MsgPackH: If response is binary -> ToJsonString/DecodeMsgPackObject (for logging/parsing)
AblyHttp->>Client: Return Response (JSON or binary)
alt Protocol is MsgPack
Client->>MPSerializer: Serialize/Deserialize with Options (AblyResolver + GeneratedResolver)
MPSerializer-->>Client: Typed object (via formatters)
else Protocol is Json
Client->>Client: Use Newtonsoft.Json (existing JSON paths)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
temporarily deployed
to
staging/pull/1311/features
- Added cake script to build IO.Ably.Dll with all dependencies merged - Removed old dependencies from Ably/Plugins - Updated README and CONTRIBUTING guide
- Updated test assertions to support native NUnit asserts - Updated PlayMode.asmdef to exclude WebGL
sacOO7
changed the base branch from
fix/upgrade-newtonsoft-dep
to
fix/newtonsoft-unity
github-actions
bot
temporarily deployed
to
staging/pull/1311/features
compilation flags
github-actions
bot
temporarily deployed
to
staging/pull/1311/features
sacOO7
force-pushed
the
fix/newtonsoft-unity
branch
from
438dc8a to
4df62c1
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1311/features
- Formatted Main solution with all relevant C# files - Fixed import issues in MsgPack serializer/deserializers
sacOO7
force-pushed
the
feature/support-msgpack
branch
from
3c139d5 to
73d7fce
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1311/features
github-actions
bot
temporarily deployed
to
staging/pull/1311/features
sacOO7
force-pushed
the
feature/support-msgpack
branch
from
ddddad5 to
b374fe2
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1311/features
…ilar to Newtonsoft
sacOO7
force-pushed
the
feature/support-msgpack
branch
from
b374fe2 to
b7b7995
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1311/features
sacOO7
force-pushed
the
fix/newtonsoft-unity
branch
from
61b484c to
7d0629b
Compare
- Updated IMPLEMENTATION_MSGPACK_FROM_NEWTONSOFT plan - Accordingly added msgpack annotations for relevant models - Created separate project for generating msgpack serializers pre-compile time
github-actions
bot
temporarily deployed
to
staging/pull/1311/features
- Created plan for migrating from msgpack-cli to msgpack-csharp - Implemented the plan, removed previous generated serializers from the code. - Updated dependencies to right versions for netstandard and netframework - Fixed build and msgpack related tests
github-actions
bot
temporarily deployed
to
staging/pull/1311/features
github-actions
bot
temporarily deployed
to
staging/pull/1311/features
- Added extra MsgPack tests to MessageEncodersAcceptanceTests - Update Base64EncoderTests with Encode specific tests - Removed unncessary Defaults.MsgPackEnabled checks from test fixtures - Removed unncessary mono build step from CI since its irrelevant (we already support netstandard2.0)
sacOO7
force-pushed
the
feature/support-msgpack
branch
from
2ce4c27 to
3d56db4
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1311/features
…ck_test_fixtures.json
github-actions
bot
temporarily deployed
to
staging/pull/1311/features
github-actions
bot
temporarily deployed
to
staging/pull/1311/features
sacOO7
force-pushed
the
feature/support-msgpack
branch
from
6789e5c to
39a2c58
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1311/features
github-actions
bot
temporarily deployed
to
staging/pull/1311/features
sacOO7
force-pushed
the
feature/support-msgpack
branch
from
8f855e5 to
5d7a02f
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1311/features
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/IO.Ably.Tests.Shared/MsgPackMessageSerializerTests.cs (2)
319-334: Test uses circular dependency—won't catch serialization bugs.Line 326 creates the test input by calling
MsgPackHelper.Serialise, which is the same serializer being tested. This means if theCountfield has a serialization bug, the test will pass because both serialization and deserialization use the same buggy code.Replace the serialization with manual byte-array construction like other tests:
public void DeserializesMessageCorrectly_Count(int count) { // Arrange - byte[] expectedMessage = MsgPackHelper.Serialise(new ProtocolMessage() { Count = count }) as byte[]; + List<byte> expectedMessage = new List<byte>(); + expectedMessage.Add(0x81); // map with 1 element + expectedMessage.AddRange(SerializeString("count")); + // Add count value encoding based on value range + if (count >= 0 && count <= 127) + { + expectedMessage.Add((byte)count); + } + else if (count < 0 && count >= -32) + { + expectedMessage.Add((byte)count); + } + // Add other range handlers similar to SerializeMsgSerial
277-296: Fix the test to properly deserialize ConnectionKey from ConnectionDetails.The test is attempting to access
target.ConnectionKey, butProtocolMessagedoes not have a directConnectionKeyproperty. The property is nested underConnectionDetails:target.ConnectionDetails?.ConnectionKey.Additionally, the MessagePack binary structure is incorrect. Currently, it creates a top-level
connectionKeyfield, but sinceProtocolMessagehas no such property, it is silently ignored during deserialization. The binary should instead create a nestedconnectionDetailsobject containing theconnectionKeyfield.Update the test to:
- Create the binary with the correct nested structure:
connectionDetails→connectionKey- Change the assertion to
Assert.Equal(connectionKey, target.ConnectionDetails?.ConnectionKey);
🧹 Nitpick comments (3)
src/IO.Ably.Tests.Shared/Realtime/ChannelSandboxSpecs.cs (1)
1005-1020: Consider parameterizing this test for both protocols.While this test focuses on thread safety, consider whether it should also test both MsgPack and Json protocols, given that protocol-specific serialization could have concurrency implications.
Apply this diff if you want to parameterize by protocol:
- [Fact] + [Theory] + [ProtocolData] [Trait("bug", "102")] - public async Task WhenAttachingToAChannelFromMultipleThreads_ItShouldNotThrowAnError() + public async Task WhenAttachingToAChannelFromMultipleThreads_ItShouldNotThrowAnError(Protocol protocol) { - var client1 = await GetRealtimeClient(Protocol.Json); + var client1 = await GetRealtimeClient(protocol); var channel = client1.Channels.Get("test".AddRandomSuffix());src/IO.Ably.Tests.Shared/MsgPackMessageSerializerTests.cs (2)
412-602: Consider the maintainability cost of manual byte-array construction.These helpers manually encode MessagePack byte sequences, which provides independent verification but creates a brittle dependency on serialization format details (field count, order, encoding). Any change to the MessagePack schema will require updating these methods.
Recommendations:
- Document the fragility with inline warnings
- Consider generating expected bytes from a reference implementation or verified fixtures
- Ensure integration tests cover round-trip serialization/deserialization to catch divergence
For now, the extensive comments help, but be aware that schema changes will require careful updates here.
718-722: Address the TODO for round-trip encoding verification.The TODO indicates missing test coverage for re-encoding decoded messages and verifying they match the original MessagePack bytes. This round-trip test would strengthen confidence in serialization consistency.
Would you like me to generate a helper method to re-encode the decoded message and compare it with the original, or open a separate issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/IO.Ably.Tests.Shared/MsgPackMessageSerializerTests.cs(9 hunks)src/IO.Ably.Tests.Shared/Realtime/ChannelSandboxSpecs.cs(5 hunks)src/IO.Ably.Tests.Shared/Rest/ChannelSandboxSpecs.cs(5 hunks)src/IO.Ably.Tests.Shared/Rest/PresenceSandboxSpecs.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/IO.Ably.Tests.Shared/Rest/PresenceSandboxSpecs.cs
🧰 Additional context used
🧬 Code graph analysis (3)
src/IO.Ably.Tests.Shared/Rest/ChannelSandboxSpecs.cs (1)
src/IO.Ably.Tests.Shared/Helpers/JAssert.cs (2)
JAssert(6-22)DeepEquals(11-21)
src/IO.Ably.Tests.Shared/Realtime/ChannelSandboxSpecs.cs (2)
src/IO.Ably.Tests.Shared/Helpers/JAssert.cs (2)
JAssert(6-22)DeepEquals(11-21)src/IO.Ably.Tests.Shared/Rest/SandboxSpecs.cs (5)
Task(83-90)Task(92-97)Task(99-114)Task(116-119)Task(121-138)
src/IO.Ably.Tests.Shared/MsgPackMessageSerializerTests.cs (5)
src/IO.Ably.Shared.MsgPack/MsgPackHelper.cs (3)
MsgPackHelper(11-97)Serialise(20-33)Deserialise(35-49)src/IO.Ably.Shared/Types/Message.cs (6)
ToString(99-108)Message(34-44)Message(178-181)Message(189-192)Message(201-213)Message(222-234)src/IO.Ably.Shared/Types/ProtocolMessage.cs (4)
ToString(282-306)ProtocolMessage(104-108)ProtocolMessage(110-114)ProtocolMessage(116-120)src/IO.Ably.Tests.Shared/Infrastructure/ResourceHelper.cs (2)
ResourceHelper(8-41)GetResource(10-23)src/IO.Ably.Tests.Shared/Helpers/JAssert.cs (2)
JAssert(6-22)DeepEquals(11-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: check
- GitHub Check: check (net7.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net7.0)
🔇 Additional comments (10)
src/IO.Ably.Tests.Shared/Realtime/ChannelSandboxSpecs.cs (5)
14-14: LGTM: Using statement supports helper usage.The new using statement is correctly added to support the
JAssert.DeepEqualscall at line 914.
872-878: LGTM: Fixture data now tests both protocols unconditionally.The fixture data correctly generates test cases for both MsgPack and Json protocols, aligning with the PR objective to remove compile-time guards and enable both protocols.
914-914: LGTM: Enhanced diagnostics for JSON comparisons.The updated call to
JAssert.DeepEqualswith theOutputparameter provides better diagnostics when JSON assertions fail, as it will write differences to the test output.
929-935: LGTM: Test properly parameterized for both protocols.The migration from
InlineDatatoProtocolDatacorrectly parameterizes the test to run with both MsgPack and Json protocols for each channel state, ensuring comprehensive protocol coverage.
970-976: LGTM: Consistent protocol parameterization.The changes consistently apply the protocol parameterization pattern, ensuring this test also runs with both MsgPack and Json protocols.
src/IO.Ably.Tests.Shared/Rest/ChannelSandboxSpecs.cs (3)
14-14: LGTM!The import is necessary to support the
JAsserthelper used in the JSON comparison assertions below.
372-372: Improved test diagnostics.The change from
JToken.DeepEqualstoJAssert.DeepEqualsadds automatic difference logging when JSON comparisons fail, making test failures easier to diagnose.Also applies to: 412-412
598-601: LGTM!The refactoring correctly uses
awaitand extracts the response data through the structuredresponse.ItemsAPI, consistent with the new Request-based approach.src/IO.Ably.Tests.Shared/MsgPackMessageSerializerTests.cs (2)
16-18: Clear documentation of architectural change.The comment effectively explains the removal of manual serializer generation in favor of build-time source generation.
84-91: LGTM: Helpful debugging utility.This helper improves test diagnostics by logging byte-level differences when assertions fail.
| /// <summary> | ||
| /// Loads msgpack test fixtures from the embedded resource. | ||
| /// </summary> | ||
| private static List<MsgpackTestFixture> LoadMsgpackFixtures() | ||
| { | ||
| var json = ResourceHelper.GetResource("msgpack_test_fixtures.json"); | ||
| return JsonConvert.DeserializeObject<List<MsgpackTestFixture>>(json); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the embedded test fixture resource exists.
The method loads msgpack_test_fixtures.json as an embedded resource. Ensure this file is included in the test project with the correct build action (EmbeddedResource) and namespace path.
Run the following to verify the resource is properly embedded:
🏁 Script executed:
#!/bin/bash
# Search for the fixture file in the repository
fd -e json msgpack_test_fixtures
# Check csproj files for EmbeddedResource entries
rg -nP --type=xml '<EmbeddedResource.*msgpack_test_fixtures' -g '*.csproj'Length of output: 335
---
🏁 Script executed:
```shell
#!/bin/bash
# Find ResourceHelper implementation
rg -nP 'class ResourceHelper|static.*GetResource' --type=cs -C3
# Search for Shared project csproj
find . -name "*Tests.Shared*.csproj" -o -name "IO.Ably.Tests.Shared.csproj"
Length of output: 1818
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full ResourceHelper.GetResource implementation
cat git/src/IO.Ably.Tests.Shared/Infrastructure/ResourceHelper.cs
# Find the Shared project csproj file
find . -name "IO.Ably.Tests.Shared.csproj"
Length of output: 93
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and display ResourceHelper content using fd and cat
fd -t f ResourceHelper.cs -x cat
# Find all csproj files in Tests directories
fd -t f '\.csproj$' -p '*Tests*'
Length of output: 1588
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for all csproj files
find . -name "*.csproj" -type f | grep -i tests
# Check specifically for Shared project
find . -name "*Shared*.csproj" -type f
Length of output: 121
Add Link attribute to NETFramework project's EmbeddedResource entry.
The embedded resource is configured in both test projects, but the NETFramework project is missing the Link attribute required for the simple filename lookup. Without it, the resource will be embedded with the full path structure, causing ResourceHelper.GetResource("msgpack_test_fixtures.json") to fail at runtime in NETFramework tests.
Update src/IO.Ably.Tests.NETFramework/IO.Ably.Tests.NETFramework.csproj line 70:
<EmbeddedResource Include="..\..\common\test-resources\msgpack_test_fixtures.json" Link="msgpack_test_fixtures.json" />The DotNET project already has this configured correctly.
🤖 Prompt for AI Agents
In src/IO.Ably.Tests.Shared/MsgPackMessageSerializerTests.cs around lines 636 to
643: tests load an embedded resource by simple filename
("msgpack_test_fixtures.json"), but the NETFramework test project embeds the
file with its full path so ResourceHelper.GetResource fails; update
src/IO.Ably.Tests.NETFramework/IO.Ably.Tests.NETFramework.csproj (around line
70) to add a Link attribute to the EmbeddedResource entry so the resource is
embedded with the simple filename (set Link="msgpack_test_fixtures.json"),
saving the project and rebuilding so the NETFramework tests can find the
resource at runtime.
| { | ||
| case "string": | ||
| decodedMsg.Data.Should().BeOfType<string>(); | ||
| var expectedString = string.Concat(Enumerable.Repeat(fixture.Data.ToString(), fixture.NumRepeat)); | ||
| decodedMsg.Data.Should().Be(expectedString); | ||
| (decodedMsg.Data as string).Length.Should().Be(fixture.NumRepeat); | ||
| break; | ||
|
|
||
| case "binary": | ||
| decodedMsg.Data.Should().BeOfType<byte[]>(); | ||
| var expectedBytes = System.Text.Encoding.UTF8.GetBytes( | ||
| string.Concat(Enumerable.Repeat(fixture.Data.ToString(), fixture.NumRepeat))); | ||
| (decodedMsg.Data as byte[]).Should().Equal(expectedBytes); | ||
| (decodedMsg.Data as byte[]).Length.Should().Be(fixture.NumRepeat); | ||
| break; |
There was a problem hiding this comment.
Fix incorrect length assertions.
Lines 689 and 697 check that the decoded length equals fixture.NumRepeat, but this is only correct if each repeat element has length 1. When you concatenate a string N times, the length is original.Length × N, not just N.
For example:
- If
fixture.Data = "x"andNumRepeat = 5→ expected: "xxxxx" (length 5) ✓ - If
fixture.Data = "ab"andNumRepeat = 5→ expected: "ababababab" (length 10) ✗
Line 688 already validates the full data, so the length check is redundant. Either remove it or fix it:
- (decodedMsg.Data as string).Length.Should().Be(fixture.NumRepeat);
+ (decodedMsg.Data as string).Length.Should().Be(expectedString.Length);Apply the same fix on Line 697 for binary:
- (decodedMsg.Data as byte[]).Length.Should().Be(fixture.NumRepeat);
+ (decodedMsg.Data as byte[]).Length.Should().Be(expectedBytes.Length);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| case "string": | |
| decodedMsg.Data.Should().BeOfType<string>(); | |
| var expectedString = string.Concat(Enumerable.Repeat(fixture.Data.ToString(), fixture.NumRepeat)); | |
| decodedMsg.Data.Should().Be(expectedString); | |
| (decodedMsg.Data as string).Length.Should().Be(fixture.NumRepeat); | |
| break; | |
| case "binary": | |
| decodedMsg.Data.Should().BeOfType<byte[]>(); | |
| var expectedBytes = System.Text.Encoding.UTF8.GetBytes( | |
| string.Concat(Enumerable.Repeat(fixture.Data.ToString(), fixture.NumRepeat))); | |
| (decodedMsg.Data as byte[]).Should().Equal(expectedBytes); | |
| (decodedMsg.Data as byte[]).Length.Should().Be(fixture.NumRepeat); | |
| break; | |
| { | |
| case "string": | |
| decodedMsg.Data.Should().BeOfType<string>(); | |
| var expectedString = string.Concat(Enumerable.Repeat(fixture.Data.ToString(), fixture.NumRepeat)); | |
| decodedMsg.Data.Should().Be(expectedString); | |
| (decodedMsg.Data as string).Length.Should().Be(expectedString.Length); | |
| break; | |
| case "binary": | |
| decodedMsg.Data.Should().BeOfType<byte[]>(); | |
| var expectedBytes = System.Text.Encoding.UTF8.GetBytes( | |
| string.Concat(Enumerable.Repeat(fixture.Data.ToString(), fixture.NumRepeat))); | |
| (decodedMsg.Data as byte[]).Should().Equal(expectedBytes); | |
| (decodedMsg.Data as byte[]).Length.Should().Be(expectedBytes.Length); | |
| break; |
🤖 Prompt for AI Agents
In src/IO.Ably.Tests.Shared/MsgPackMessageSerializerTests.cs around lines 684 to
698, the length assertions for string and binary data are wrong: they assert
decoded length equals fixture.NumRepeat but the actual length should be
original.Length * fixture.NumRepeat (or the check is redundant because the
full-data equality is already asserted). Fix by either removing the redundant
length assertions or changing them to compare against expectedString.Length for
the string case and expectedBytes.Length for the binary case (use the expected
value already constructed above).
| var client1 = await GetRestClient(protocol); | ||
| await client1.AblyAuth.AddAuthHeader(request); | ||
| await httpClient.Execute(request); | ||
| client1.Request(HttpMethod.Post, $"/channels/{channelName}/messages", null, rawMessage); |
There was a problem hiding this comment.
Missing await keyword causes potential race condition.
The Request method returns a Task that should be awaited (as shown on Line 598). Without await, the POST request may not complete before the test continues to check history on Line 553, potentially causing intermittent test failures.
Apply this diff to fix the issue:
- client1.Request(HttpMethod.Post, $"/channels/{channelName}/messages", null, rawMessage);
+ await client1.Request(HttpMethod.Post, $"/channels/{channelName}/messages", null, rawMessage);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| client1.Request(HttpMethod.Post, $"/channels/{channelName}/messages", null, rawMessage); | |
| await client1.Request(HttpMethod.Post, $"/channels/{channelName}/messages", null, rawMessage); |
🤖 Prompt for AI Agents
In src/IO.Ably.Tests.Shared/Rest/ChannelSandboxSpecs.cs around line 548, the
call to client1.Request(HttpMethod.Post, $"/channels/{channelName}/messages",
null, rawMessage) is not awaited which can cause a race where the POST hasn't
completed before the test checks history; update the call to await the returned
Task (i.e., prepend await) so the test waits for the POST to finish, ensuring
the test method remains async and returns a Task if not already.
…to JsonConverters
sacOO7
force-pushed
the
feature/support-msgpack
branch
from
819d6b9 to
a1ceae7
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1311/features
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/IO.Ably.Tests.Shared/MsgPackMessageSerializerTests.cs (1)
685-699: Fix incorrect length assertions.Lines 690 and 698 check that the decoded length equals
fixture.NumRepeat, but this is only correct if each repeat element has length 1. When you concatenate a string or byte sequence N times, the length isoriginal.Length × N, not justN.Example:
- If
fixture.Data = "x"andNumRepeat = 5→ expected: "xxxxx" (length 5) ✓- If
fixture.Data = "ab"andNumRepeat = 5→ expected: "ababababab" (length 10) ✗Lines 689 and 697 already validate the full data equality, so the length checks are redundant. Either remove them or fix them:
- (decodedMsg.Data as string).Length.Should().Be(fixture.NumRepeat); + (decodedMsg.Data as string).Length.Should().Be(expectedString.Length);Apply the same fix on line 698 for binary:
- (decodedMsg.Data as byte[]).Length.Should().Be(fixture.NumRepeat); + (decodedMsg.Data as byte[]).Length.Should().Be(expectedBytes.Length);
🧹 Nitpick comments (2)
src/IO.Ably.Shared.MsgPack/CustomSerialisers/TimespanMessagePackSerializer.cs (2)
9-9: Class name is slightly inconsistent withTimeSpantype spellingThe formatter is named
TimespanFormatterwhile the BCL type isTimeSpan. If this type is not part of the public surface or can be safely renamed, considerTimeSpanFormatterfor consistency with other formatters and the underlying type.
11-15: Confirm that truncating to whole milliseconds is acceptable for your protocol
Serializewrites(long)value.TotalMilliseconds, which drops any sub‑millisecond ticks from theTimeSpan. If earlier MsgPack usage or protocol expectations relied on full tick precision or a different unit (e.g., ticks or seconds), this could be a subtle behavior change.Please confirm this representation matches existing payloads and that losing sub‑millisecond precision is acceptable for all
TimeSpanuses here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/IO.Ably.Shared.MsgPack/CustomSerialisers/DateTimeOffsetMessagePackSerializer.cs(1 hunks)src/IO.Ably.Shared.MsgPack/CustomSerialisers/TimespanMessagePackSerializer.cs(1 hunks)src/IO.Ably.Tests.Shared/MsgPackMessageSerializerTests.cs(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/IO.Ably.Shared.MsgPack/CustomSerialisers/TimespanMessagePackSerializer.cs (2)
src/IO.Ably.Shared.MsgPack/CustomSerialisers/AblyResolver.cs (1)
IMessagePackFormatter(24-27)src/IO.Ably.Shared.MsgPack/CustomSerialisers/DateTimeOffsetMessagePackSerializer.cs (1)
Serialize(12-15)
src/IO.Ably.Shared.MsgPack/CustomSerialisers/DateTimeOffsetMessagePackSerializer.cs (3)
src/IO.Ably.Shared.MsgPack/CustomSerialisers/AblyResolver.cs (1)
IMessagePackFormatter(24-27)src/IO.Ably.Shared.MsgPack/CustomSerialisers/TimespanMessagePackSerializer.cs (1)
Serialize(12-15)src/IO.Ably.Shared/Extensions/DateExtensions.cs (1)
ToUnixTimeInMilliseconds(30-33)
src/IO.Ably.Tests.Shared/MsgPackMessageSerializerTests.cs (4)
src/IO.Ably.Shared.MsgPack/MsgPackHelper.cs (3)
MsgPackHelper(11-97)Serialise(20-33)Deserialise(35-49)src/IO.Ably.Shared/Types/Message.cs (6)
ToString(99-108)Message(34-44)Message(178-181)Message(189-192)Message(201-213)Message(222-234)src/IO.Ably.Tests.Shared/Infrastructure/ResourceHelper.cs (2)
ResourceHelper(8-41)GetResource(10-23)src/IO.Ably.Tests.Shared/Helpers/JAssert.cs (2)
JAssert(6-22)DeepEquals(11-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: check (net9.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net7.0)
- GitHub Check: check
🔇 Additional comments (8)
src/IO.Ably.Shared.MsgPack/CustomSerialisers/DateTimeOffsetMessagePackSerializer.cs (3)
11-15: LGTM!The serialization logic correctly writes the Unix timestamp in milliseconds.
20-27: Integer path looks correct.This branch correctly handles the primary deserialization case matching what
Serializeproduces.
29-34: Float branch is necessary and correct for defensive deserialization.The
MessagePackType.Floatbranch is intentional and justified. BothDateTimeOffsetFormatterandTimespanFormatterimplement identical defensive deserialization patterns:
- Serialize writes
Int64only- Deserialize accepts Integer, Float, and String types
This asymmetry is deliberate for cross-language interoperability and backward compatibility with legacy packed data. While the truncation via
(long)cast limits precision, it aligns with the formatter's millisecond granularity.src/IO.Ably.Tests.Shared/MsgPackMessageSerializerTests.cs (5)
84-91: Nice debugging utility!This helper will make it much easier to diagnose serialization mismatches during test failures.
564-602: No issues found. The 8-field count is correct.The PresenceMessage type definition in
src/IO.Ably.Shared/Types/PresenceMessage.cscontains exactly 8 fields marked with[Key(...)]attributes in declaration order: id, action, clientId, connectionId, connectionKey, data, encoding, and timestamp. This matches the serialization logic in the helper, which declares a map with 8 elements and serializes each field in the same order.
520-562: No issues found—the 9-field count is accurate.The Message class at
src/IO.Ably.Shared/Types/Message.cshas exactly 9 fields with Key attributes (id, clientId, connectionId, connectionKey, name, timestamp, data, extras, encoding), matching the test's 0x89 map serialization in declaration order. The test correctly reflects the current schema.
402-474: Field count and order are correct—no issues found.The helper accurately reflects the current ProtocolMessage structure: it has exactly 15 fields in the declared order (params, action, auth, flags, count, error, id, channel, channelSerial, connectionId, msgSerial, timestamp, messages, presence, connectionDetails), and the 0x8F marker (15-element map) is appropriate. The [Key] attributes in ProtocolMessage.cs confirm this ordering.
295-295: No changes needed — the commented assertion is correct.ProtocolMessage does not have a ConnectionKey property. The class contains ConnectionId (line 167), ConnectionDetails (line 189), and other properties, but no ConnectionKey. The assertion is correctly commented out because attempting to access
target.ConnectionKeywould fail. The test properly validates that deserialization handles the incoming "connectionKey" msgpack field without error.Likely an incorrect or invalid review comment.
| // Handle string type (parse DateTimeOffset string representation) | ||
| if (nextType == MessagePackType.String) | ||
| { | ||
| var value = reader.ReadString(); | ||
| if (DateTimeOffset.TryParse(value, out var result)) | ||
| { | ||
| return result; | ||
| } | ||
| } | ||
|
|
||
| // Return MinValue if unable to parse | ||
| return DateTimeOffset.MinValue; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify asymmetric deserialization and silent failure pattern.
The deserializer accepts Integer, Float, and String inputs, but Serialize only produces integers. Additionally, the fallback silently returns DateTimeOffset.MinValue instead of throwing an exception when encountering unexpected formats.
Please confirm:
- Is the permissive deserialization intentional (e.g., for backward compatibility or cross-platform interop)?
- Is returning
MinValueas a silent failure consistent with the library's error handling strategy? - Should string parsing use culture-invariant methods to avoid locale-dependent behavior?
🏁 Script executed:
#!/bin/bash
# Check if other formatters in this codebase follow similar silent failure patterns
rg -n -A5 "IMessagePackFormatter" --type=cs -g "*Formatter.cs" -g "*Serializer.cs" | rg -A5 "\.MinValue|\.Default|return default"
# Check if there are tests covering these edge cases
rg -n "DateTimeOffsetFormatter|DateTimeOffsetMessagePackSerializer" --type=cs -g "*Test*.cs" -g "*Spec*.cs"Length of output: 0
I need to broaden my search strategy to gather more information about the codebase patterns.
</web_search>
🏁 Script executed:
#!/bin/bash
# Find all MessagePackFormatter implementations
echo "=== Formatter implementations ==="
rg -n "class.*IMessagePackFormatter|class.*Formatter.*:" --type=cs
echo ""
echo "=== TryParse usage patterns ==="
rg -n "TryParse" --type=cs | head -20
echo ""
echo "=== Error handling: throw vs return default ==="
rg -n "throw new|return.*MinValue|return.*Default" --type=cs | head -20
echo ""
echo "=== Test files related to serialization ==="
fd -e cs -path "*[Tt]est*" | grep -i "serializ\|formatter"Length of output: 5535
🌐 Web query:
MessagePack C# library error handling deserialize format convention
💡 Result:
Short summary — how MessagePack for C# handles deserialization errors and format conventions:
- Exceptions: deserialization and formatter errors are wrapped in MessagePackSerializationException (includes target type/context). Formatters should throw this for format errors. [1].
- Format conventions: two common object encodings — array (IntKey / ordered numeric keys) and map (StringKey / named keys). Contractless resolvers let you skip attributes and use property names. IntKey (array) is fastest; StringKey (map) is more version-tolerant. [5][4].
- Missing/extra fields & constructors: for array (IntKey) it matches by index; for map (StringKey) it matches by name (case‑insensitive). Deserializer picks the best constructor; use [SerializationConstructor] to control it. Unmatched constructor parameters cause formatter resolution errors. [4].
- Typeless/obj: Typeless format embeds .NET type as an ext (typecode 100). You can provide a Type binder (TypelessFormatter.BindToType) if names changed. Typeless is unsafe for untrusted data. [3][2].
- Security/tolerance: use MessagePackSerializerOptions (e.g., WithSecurity(MessagePackSecurity.UntrustedData)) to harden against untrusted input. For more tolerant decoding, use Contractless* resolvers or allow-private variants; for strict/fast use IntKey and generated resolvers. [3][5].
- Best practices on error handling: validate reader state in custom formatters (depth tracking), return/throw MessagePackSerializationException on invalid formats, and prefer explicit resolvers/options for versioning or untrusted data. [1][2].
If you want, I can show example code for: a) catching MessagePackSerializationException, b) configuring Contractless resolver, or c) implementing a safe typeless BindToType.
The web search reveals important convention information. Let me verify the parallel formatter implementations:
🏁 Script executed:
#!/bin/bash
# Compare error handling in TimespanFormatter and DateTimeOffsetFormatter
echo "=== TimespanFormatter error handling ==="
cat src/IO.Ably.Shared.MsgPack/CustomSerialisers/TimespanMessagePackSerializer.cs
echo ""
echo "=== DateTimeOffsetFormatter error handling ==="
cat src/IO.Ably.Shared.MsgPack/CustomSerialisers/DateTimeOffsetMessagePackSerializer.cs
echo ""
echo "=== Check for MessagePackSerializationException usage in codebase ==="
rg -n "MessagePackSerializationException" --type=csLength of output: 3921
Throw MessagePackSerializationException instead of silently returning MinValue.
Both DateTimeOffsetFormatter and the parallel TimespanFormatter violate MessagePack C# library conventions by returning default values on deserialization failures instead of throwing exceptions. Per the official MessagePack C# documentation, custom formatters must throw MessagePackSerializationException when encountering invalid or unexpected data formats.
The current pattern silently discards malformed data, which can hide corruption or format mismatches:
Fix required in:
src/IO.Ably.Shared.MsgPack/CustomSerialisers/DateTimeOffsetMessagePackSerializer.cs(lines 36–47)src/IO.Ably.Shared.MsgPack/CustomSerialisers/TimespanMessagePackSerializer.cs(lines 32–48)
Replace return DateTimeOffset.MinValue and return TimeSpan.MinValue with:
throw new MessagePackSerializationException($"Cannot deserialize {nameof(DateTimeOffset)} from {nextType}");🤖 Prompt for AI Agents
src/IO.Ably.Shared.MsgPack/CustomSerialisers/DateTimeOffsetMessagePackSerializer.cs
lines 36-47, replace the silent fallback that returns DateTimeOffset.MinValue
with throwing a MessagePackSerializationException when deserialization fails:
after failing to parse the string (or when encountering an unexpected
MessagePackType), throw new MessagePackSerializationException($"Cannot
deserialize {nameof(DateTimeOffset)} from {nextType}"); apply the same pattern
to src/IO.Ably.Shared.MsgPack/CustomSerialisers/TimespanMessagePackSerializer.cs
lines 32-48, replacing return TimeSpan.MinValue with throwing
MessagePackSerializationException including the offending nextType.
| public TimeSpan Deserialize(ref MessagePackReader reader, MessagePackSerializerOptions options) | ||
| { | ||
| var data = unpacker.LastReadData; | ||
| return TimeSpan.FromMilliseconds(data.AsInt64()); | ||
| var nextType = reader.NextMessagePackType; | ||
|
|
||
| // Handle integer types (Int64, Int32, etc.) | ||
| if (nextType == MessagePackType.Integer) | ||
| { | ||
| var milliseconds = reader.ReadInt64(); | ||
| return TimeSpan.FromMilliseconds(milliseconds); | ||
| } | ||
|
|
||
| // Handle float types (Single, Double) | ||
| if (nextType == MessagePackType.Float) | ||
| { | ||
| var milliseconds = reader.ReadDouble(); | ||
| return TimeSpan.FromMilliseconds(milliseconds); | ||
| } | ||
|
|
||
| // Handle string type (parse TimeSpan string representation) | ||
| if (nextType == MessagePackType.String) | ||
| { | ||
| var value = reader.ReadString(); | ||
| if (TimeSpan.TryParse(value, out var result)) | ||
| { | ||
| return result; | ||
| } | ||
| } | ||
|
|
||
| // Return null TimeSpan if unable to parse | ||
| return TimeSpan.MinValue; |
There was a problem hiding this comment.
🧩 Analysis chain
Ensure Deserialize consumes unsupported values and handles nil correctly
Deserialize peeks at reader.NextMessagePackType and only consumes the value for Integer/Float/String. For any other type (including Nil), it returns TimeSpan.MinValue without advancing reader. Because NextMessagePackType does not consume the value, the next formatter will see the same token, which can corrupt the overall deserialization stream.
The comment “Return null TimeSpan” is also misleading, since a non‑nullable TimeSpan is returned and TimeSpan.MinValue is effectively used as a sentinel.
Consider updating Deserialize to:
- Explicitly consume
nil(e.g., viareader.TryReadNil()) and return your sentinel/default. - Consume unsupported types (e.g., via
reader.Skip()or the equivalent in your MessagePack version) before returning, so the reader remains in a consistent state. - Optionally clarify the comment to reflect that
TimeSpan.MinValueis used as the fallback.
For example:
- public TimeSpan Deserialize(ref MessagePackReader reader, MessagePackSerializerOptions options)
- {
- var nextType = reader.NextMessagePackType;
-
- // Handle integer types (Int64, Int32, etc.)
- if (nextType == MessagePackType.Integer)
- {
- var milliseconds = reader.ReadInt64();
- return TimeSpan.FromMilliseconds(milliseconds);
- }
-
- // Handle float types (Single, Double)
- if (nextType == MessagePackType.Float)
- {
- var milliseconds = reader.ReadDouble();
- return TimeSpan.FromMilliseconds(milliseconds);
- }
-
- // Handle string type (parse TimeSpan string representation)
- if (nextType == MessagePackType.String)
- {
- var value = reader.ReadString();
- if (TimeSpan.TryParse(value, out var result))
- {
- return result;
- }
- }
-
- // Return null TimeSpan if unable to parse
- return TimeSpan.MinValue;
- }
+ public TimeSpan Deserialize(ref MessagePackReader reader, MessagePackSerializerOptions options)
+ {
+ // Treat nil as an "empty" value but make sure we consume it.
+ if (reader.TryReadNil())
+ {
+ return TimeSpan.MinValue;
+ }
+
+ var nextType = reader.NextMessagePackType;
+
+ switch (nextType)
+ {
+ case MessagePackType.Integer:
+ {
+ var milliseconds = reader.ReadInt64();
+ return TimeSpan.FromMilliseconds(milliseconds);
+ }
+
+ case MessagePackType.Float:
+ {
+ var milliseconds = reader.ReadDouble();
+ return TimeSpan.FromMilliseconds(milliseconds);
+ }
+
+ case MessagePackType.String:
+ {
+ var value = reader.ReadString();
+ if (TimeSpan.TryParse(value, out var result))
+ {
+ return result;
+ }
+
+ return TimeSpan.MinValue;
+ }
+
+ default:
+ // Skip unsupported value so we advance the reader.
+ reader.Skip();
+ return TimeSpan.MinValue;
+ }
+ }Adjust the sentinel (TimeSpan.MinValue vs another default) as needed to match how callers interpret “no/invalid duration”.
I'll help you verify this critical review comment. Let me gather the necessary information about the code and the MessagePack library being used.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and examine the TimespanMessagePackSerializer file
find . -name "TimespanMessagePackSerializer.cs" -type f 2>/dev/null | head -20
Length of output: 79
🏁 Script executed:
#!/bin/bash
# Check project file for MessagePack dependencies and version
find . -name "*.csproj" -o -name "*.fsproj" | xargs grep -l "MessagePack" 2>/dev/null | head -5Length of output: 278
🏁 Script executed:
#!/bin/bash
# Search for csproj files in the shared project directory
find . -path "*/IO.Ably.Shared.MsgPack*" -name "*.csproj" 2>/dev/nullLength of output: 0
🏁 Script executed:
#!/bin/bash
# Look for usages of TimespanMessagePackSerializer
rg "TimespanMessagePackSerializer" -n -A 2 -B 2 2>/dev/null || echo "Pattern not found"Length of output: 810
---
🏁 Script executed:
```shell
#!/bin/bash
# Read the entire TimespanMessagePackSerializer.cs file
cat ./src/IO.Ably.Shared.MsgPack/CustomSerialisers/TimespanMessagePackSerializer.cs
Length of output: 1854
🏁 Script executed:
#!/bin/bash
# Get MessagePack version from NETStandard project
cat ./src/IO.Ably.NETStandard20/IO.Ably.NETStandard20.csproj | grep -A 2 -B 2 "MessagePack"Length of output: 215
🏁 Script executed:
#!/bin/bash
# Also check Android project
cat ./src/IO.Ably.Android/IO.Ably.Android.csproj | grep -A 2 -B 2 "MessagePack"Length of output: 900
🏁 Script executed:
#!/bin/bash
# Search for other deserializers in same directory for comparison
ls -la ./src/IO.Ably.Shared.MsgPack/CustomSerialisers/Length of output: 747
🏁 Script executed:
#!/bin/bash
# Look for tests related to TimespanMessagePackSerializer
rg "TimeSpan|Timespan" -i ./src/IO.Ably.Tests.DotNET ./src/IO.Ably.Tests.NETFramework -n | head -30Length of output: 0
---
🌐 Web query:
```web
MessagePack C# 3.1.4 MessagePackReader TryReadNil Skip methods
💡 Result:
MessagePack for C# (v3.1.4) — key facts about MessagePackReader.TryReadNil() and Skip(...)
-
TryReadNil(): Attempts to read a MessagePack nil at the current position. Returns true and advances the reader if the next token is nil; returns false and does not advance if it is not nil. Use it to detect nulls before calling other Read* methods. [1][2]
-
Skip(SerializationContext): Advances the reader past the next full MessagePack value (of any type). Useful when you need to ignore unknown map keys or values. It requires a SerializationContext (used for stack-guarding / recursion limits) and will consume the entire value (including nested arrays/maps). [2]
Minimal examples
-
Detect nil:
var reader = new MessagePackReader(...);
if (reader.TryReadNil()) { /* value was nil / } else { / not nil — reader not advanced */ } -
Skip unknown value (inside a custom converter):
int count = reader.ReadMapHeader();
for (int i = 0; i < count; i++)
{
if (KnownKey.TryRead(ref reader)) { /* read value(s) for key */ }
else
{
// skip unknown key and its value
reader.Skip(context); // skips key (if not already consumed)
reader.Skip(context); // skips value
}
}
Sources:
[1] MessagePack (NuGet package) — version listing for 3.1.4.
[2] Nerdbank.MessagePack / MessagePackReader API docs (TryReadNil usage shown in examples; Skip(SerializationContext) documented).
---
🏁 Script executed:
```shell
#!/bin/bash
# Check DateTimeOffsetMessagePackSerializer for comparison pattern
cat ./src/IO.Ably.Shared.MsgPack/CustomSerialisers/DateTimeOffsetMessagePackSerializer.cs
Length of output: 1906
🏁 Script executed:
#!/bin/bash
# Check another serializer for comparison
cat ./src/IO.Ably.Shared.MsgPack/CustomSerialisers/MessageExtrasMessagePackSerializer.csLength of output: 1781
🏁 Script executed:
#!/bin/bash
# Search for formatter tests in test projects
find . -path "*/Tests*" -name "*.cs" -exec grep -l "Formatter\|Serialiser" {} \; | head -10Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for any test that validates Deserialize behavior with different types
rg "Deserialize|NextMessagePackType" -n ./src/IO.Ably.Tests.DotNET --type cs 2>/dev/null | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check MessagePackSerializerOptions to see what's available
rg "MessagePackSerializerOptions" -n -A 5 -B 2 --type cs ./src/IO.Ably.Shared.MsgPack/ | head -40Length of output: 4203
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for SerializationContext usage in the codebase
rg "SerializationContext" -n --type cs ./src/IO.Ably.Shared.MsgPack/
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there are any other custom formatters that handle unsupported types
rg "reader.Skip\|TryReadNil\|Nil" -n --type cs ./src/IO.Ably.Shared.MsgPack/CustomSerialisers/ -A 2 -B 2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the MsgPackHelper to understand how options are configured
cat ./src/IO.Ably.Shared.MsgPack/MsgPackHelper.csLength of output: 2941
🏁 Script executed:
#!/bin/bash
# Search for MessagePackReader in documentation or other examples in the code
rg "MessagePackReader" -n --type cs ./src/IO.Ably.Shared.MsgPack/ -A 3 | head -50Length of output: 3067
---
🌐 Web query:
```web
MessagePack C# 3.1.4 MessagePackReader skip unsupported value without SerializationContext
💡 Result:
Use MessagePackReader.Skip() — it advances the reader past the current MessagePack value (including nested arrays/maps) and does not require a SerializationContext/SerializationOptions. Example:
using System;
using System.Buffers;
using MessagePack;
ReadOnlyMemory data = ...;
var reader = new MessagePackReader(data.Span);
if (/* value is unsupported */) {
reader.Skip(); // skips the whole value
}
The MessagePackReader primitive API (Skip) is documented in the MessagePack-CSharp README. [1][2]
Sources:
[1] MessagePack-CSharp README — MessagePackReader (Skip). (github.com)
[2] MessagePack NuGet (3.1.4). (nuget.org)
Citations:
- 1: https://github.com/MessagePack-CSharp/MessagePack-CSharp?utm_source=openai
- 2: https://www.nuget.org/packages/MessagePack/3.1.4?utm_source=openai
Fix stream corruption in TimespanFormatter.Deserialize by consuming all message pack values
The current implementation correctly identifies that Deserialize peeks at NextMessagePackType without consuming unsupported types (including nil), leaving the reader in an inconsistent state for subsequent deserializations.
Verification confirms:
- The
TryReadNil()method is the correct pattern for handling nil values, as demonstrated in other formatters likeMessageExtrasFormatterandJObjectMessagePackSerializer MessagePackReader.Skip()advances the reader past unsupported values without requiring additional parameters- The suggested diff structure and implementation approach is correct and will compile
Required changes to TimespanFormatter.Deserialize (lines 18–47):
- Call
reader.TryReadNil()first to consume nil tokens before checking other types - Use
reader.Skip()in the default case to advance past unsupported message pack types - Update the misleading comment since
TimeSpan.MinValueis a sentinel, not "null"
Apply the suggested diff from the review comment. Additionally, note that DateTimeOffsetFormatter (in DateTimeOffsetMessagePackSerializer.cs) has the same issue and should be fixed identically.
🤖 Prompt for AI Agents
In src/IO.Ably.Shared.MsgPack/CustomSerialisers/TimespanMessagePackSerializer.cs
around lines 18 to 47, the Deserialize method peeks at NextMessagePackType but
does not consume unsupported or nil values, leaving the MessagePackReader in an
inconsistent state; update the method to first call reader.TryReadNil() to
consume nil tokens, then handle Integer, Float, and String cases as before, and
in the default/unsupported branch call reader.Skip() to advance past the value;
also update the comment to reflect that TimeSpan.MinValue is used as a sentinel
and apply the identical fix to DateTimeOffsetMessagePackSerializer.cs
(DateTimeOffsetFormatter).
- Removed all references to separate msgpack project from solutions, projects - Added custom msgpack serializer tests for each msgpack formatter
github-actions
bot
temporarily deployed
to
staging/pull/1311/features
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/IO.Ably.Tests.Shared/MsgPack/MsgPackHelperTests.cs (1)
106-112: Use explicit cast instead ofasin base64 byte[] round‑trip testSame pattern here:
as List<Message>followed byFirst()can yield a null dereference if deserialisation ever returns an unexpected type. An explicit cast better reflects the test’s intent that deserialisation must succeed.- var message = new Message() { Name = "example", Data = "AAECAwQFBgcICQoLDA0ODw==".FromBase64() }; - var serialised = MsgPackHelper.Serialise(new List<Message> { message }); - var resultMessage = MsgPackHelper.Deserialise(serialised, typeof(List<Message>)) as List<Message>; + var message = new Message() { Name = "example", Data = "AAECAwQFBgcICQoLDA0ODw==".FromBase64() }; + var serialised = MsgPackHelper.Serialise(new List<Message> { message }); + var resultMessage = (List<Message>)MsgPackHelper.Deserialise(serialised, typeof(List<Message>)); var data = resultMessage.First().Data as byte[]; data.Should().BeEquivalentTo(message.Data as byte[]); resultMessage.First().Name.Should().Be(message.Name);
♻️ Duplicate comments (5)
src/IO.Ably.Tests.Shared/MsgPack/MsgPackHelperTests.cs (1)
21-27: Avoid potential nullresultin message list deserialisation test
MsgPackHelper.Deserialisereturnsobject, and usingas List<Message>meansresultcould be null, yet it’s immediately dereferenced withFirst(). In tests it’s clearer and safer to use an explicit cast so a mismatch fails fast with a cast exception instead of a possible NRE.- var serialised = MsgPackHelper.Serialise(new List<Message> { message }); - - var result = MsgPackHelper.Deserialise(serialised, typeof(List<Message>)) as List<Message>; - var resultMessage = result.First(); + var serialised = MsgPackHelper.Serialise(new List<Message> { message }); + + var result = (List<Message>)MsgPackHelper.Deserialise(serialised, typeof(List<Message>)); + var resultMessage = result.First();src/IO.Ably.Shared/MsgPack/MsgPackHelper.cs (1)
73-96: Consider logging or narrowing the catch in DecodeMsgPackObject
DecodeMsgPackObjectcatches allExceptionand silently falls back to a hex string, which is fine as a last‑resort debug view but makes it hard to see whether the failure was in MessagePack conversion or JSON parsing. Consider at least logging the exception, or restricting the catch to expected types (e.g., MessagePack/JSON parsing errors) so unexpected failures aren’t completely hidden.src/IO.Ably.Shared/MsgPack/CustomSerialisers/DateTimeOffsetMessagePackSerializer.cs (1)
18-47: Throw on invalid DateTimeOffset payload instead of returning MinValue
Deserializecurrently falls back toDateTimeOffset.MinValuefor unsupported types or parse failures, which silently hides invalid or corrupted MsgPack data and leaves reader state ambiguous for unsupportednextTypevalues. Prefer throwing aMessagePackSerializationExceptionso callers get a clear failure instead of a sentinel value; this aligns with typical MessagePack formatter patterns and the earlier review comment on this class.Example change:
- // Handle string type (parse DateTimeOffset string representation) + // Handle string type (parse DateTimeOffset string representation) if (nextType == MessagePackType.String) { var value = reader.ReadString(); - if (DateTimeOffset.TryParse(value, out var result)) + if (DateTimeOffset.TryParse(value, out var result)) { return result; } } - - // Return MinValue if unable to parse - return DateTimeOffset.MinValue; + throw new MessagePackSerializationException( + $"Cannot deserialize {nameof(DateTimeOffset)} from {nextType}.");src/IO.Ably.Shared/MsgPack/CustomSerialisers/TimespanMessagePackSerializer.cs (1)
36-47: Throw on invalid TimeSpan payload and fix misleading commentLike the DateTimeOffset formatter,
Deserializehere returnsTimeSpan.MinValuewhen parsing fails or the type is unsupported, silently masking bad data. It’s safer to throw aMessagePackSerializationExceptionso invalid payloads are surfaced rather than turned into a sentinel value; also, the comment mentions “null TimeSpan”, which isn’t possible for a value type.Suggested change:
- // Handle string type (parse TimeSpan string representation) + // Handle string type (parse TimeSpan string representation) if (nextType == MessagePackType.String) { var value = reader.ReadString(); if (TimeSpan.TryParse(value, out var result)) { return result; } } - - // Return null TimeSpan if unable to parse - return TimeSpan.MinValue; + throw new MessagePackSerializationException( + $"Cannot deserialize {nameof(TimeSpan)} from {nextType}.");src/IO.Ably.Tests.Shared/MsgPack/MsgPackMessageSerializerTests.cs (1)
681-697: Fix incorrect length assertions in TestMsgpackDecodingFor the
stringandbinarycases, you buildexpectedString/expectedBytesby repeatingfixture.DataNumRepeattimes, then assert both full equality and that the decoded length equalsfixture.NumRepeat. The latter is only true when the original data length is 1; otherwise the length isoriginal.Length × NumRepeat, so these assertions are wrong and redundant.Either drop the length checks or compare to the already-constructed expected values:
- decodedMsg.Data.Should().Be(expectedString); - (decodedMsg.Data as string).Length.Should().Be(fixture.NumRepeat); + decodedMsg.Data.Should().Be(expectedString); + (decodedMsg.Data as string).Length.Should().Be(expectedString.Length); ... - (decodedMsg.Data as byte[]).Should().Equal(expectedBytes); - (decodedMsg.Data as byte[]).Length.Should().Be(fixture.NumRepeat); + (decodedMsg.Data as byte[]).Should().Equal(expectedBytes); + (decodedMsg.Data as byte[]).Length.Should().Be(expectedBytes.Length);
🧹 Nitpick comments (13)
src/IO.Ably.Tests.Shared/CustomSerializers/JsonSerializationTests.cs (1)
1-36: Test implementation is correct and validates TokenResponse JSON deserialization.The test properly deserializes a TokenResponse from JSON and validates all key fields. The structure is clear and assertions are comprehensive.
Optional: Consider consistent spelling in method name.
The method name uses British spelling "CanDeserialiseTokenResponse" while the codebase uses American spelling (e.g.,
JsonHelper.Deserialize). For consistency, consider:- public void CanDeserialiseTokenResponse() + public void CanDeserializeTokenResponse()Optional: Consider adding edge-case tests.
The current test covers the happy path. You might consider adding tests for:
- Missing or null fields
- Invalid JSON structure
- Empty capability objects
- Boundary conditions for timestamps
However, these can be deferred based on testing priorities.
src/IO.Ably.Tests.Shared/IO.Ably.Tests.Shared.projitems (1)
83-83: Consider relocating test utility to Infrastructure or Helpers folder.
SimpleBufferWriter.csdoesn't follow the*Tests.csnaming convention, suggesting it's a test utility rather than a test class. Test helpers in this project are typically organized underInfrastructure\orHelpers\folders for better discoverability. However, keeping it adjacent to the MsgPack tests may be intentional for convenience.src/IO.Ably.Shared/MsgPack/CustomSerialisers/AblyGeneratedResolver.cs (1)
5-8: Consider adding a documentation comment.Since this is an empty partial class that will be populated by source generation, adding a brief XML documentation comment would help future maintainers understand its purpose.
Apply this diff to add documentation:
+ /// <summary> + /// Partial class placeholder for MessagePack source generator. + /// The implementation is auto-generated at build time. + /// </summary> [GeneratedMessagePackResolver] internal partial class AblyGeneratedResolver { }src/IO.Ably.Tests.Shared/MsgPack/MsgPackHelperTests.cs (1)
120-125: Prefer the genericMsgPackHelper.Deserialise<T>overload for TokenResponse setupYou can simplify this test and keep compile-time typing by using the generic overload instead of passing
typeof(Dictionary<string, object>).- // MessagePackObject doesn't exist in MessagePack-CSharp v3.x - // Deserialize to a dictionary instead and convert to JSON - var decodedMessagePack = MsgPackHelper.Deserialise(value.FromBase64(), typeof(System.Collections.Generic.Dictionary<string, object>)); + // MessagePackObject doesn't exist in MessagePack-CSharp v3.x + // Deserialize to a dictionary instead and convert to JSON + var decodedMessagePack = MsgPackHelper.Deserialise<Dictionary<string, object>>(value.FromBase64()); var jsonString = JsonHelper.Serialize(decodedMessagePack); var response = JsonHelper.Deserialize<TokenResponse>(jsonString);src/IO.Ably.Shared/MsgPack/CustomSerialisers/CapabilityMessagePackSerializer.cs (1)
11-32: Confirm nil ↔ Capability mapping semanticsSerialization writes
nilwhenCapabilityis null, but deserialization returnsnew Capability()when a nil is read. That changesnullinto an “empty” capability instance, which may be fine, but it’s worth double‑checking against existing behavior and callers’ expectations (do they distinguishnullvs. empty capability?).If previous code surfaced
nullin this case, consider either preservingnullor documenting this behavioral change.src/IO.Ably.Tests.Shared/MsgPack/MsgPackMessageSerializerTests.cs (1)
602-608: SerializeString only handles fixstr (≤31 bytes) – fragile for longer keys
SerializeStringemits0xa0 + str.Length, which is valid only for strings up to 31 bytes (fixstr). It works for the current set of field names and test values, but if future tests pass longer strings this helper will generate invalid MessagePack.If you expect to reuse it more broadly, consider extending it to emit
str8/str16whenstr.Lengthexceeds 31.src/IO.Ably.Shared/MsgPack/CustomSerialisers/JObjectMessagePackSerializer.cs (1)
12-51: HonorMessagePackSerializerOptionswhen converting to/from JSONThe serializer currently ignores the
optionsparameter and always uses the defaultMessagePackSerializersettings when callingConvertFromJsonandConvertToJson. That means custom options (compression, resolver,OldSpec, etc.) used for the surrounding graph are bypassed here.You can keep the existing behavior while respecting caller options by threading
optionsthrough:- var bytes = MessagePackSerializer.ConvertFromJson(value.ToJson()); + var bytes = MessagePackSerializer.ConvertFromJson(value.ToJson(), options); @@ - var jsonString = MessagePackSerializer.ConvertToJson(bytes); + var jsonString = MessagePackSerializer.ConvertToJson(bytes, options);This keeps the formatter consistent with the rest of the MsgPack pipeline and avoids subtle mismatches if non-standard options are in play.
src/IO.Ably.Shared/MsgPack/CustomSerialisers/MessageExtrasMessagePackSerializer.cs (1)
12-55: Pass serializer options through JSON conversion for consistencySimilar to the
JObjectMessagePackSerializer, this formatter ignores theoptionsargument when converting between JSON and MessagePack. If callers use non-defaultMessagePackSerializerOptions(custom resolver, compression, etc.), this path will silently diverge.Consider wiring
optionsthrough:- var jToken = value.ToJson(); + var jToken = value.ToJson(); @@ - var bytes = MessagePackSerializer.ConvertFromJson(jToken.ToString()); + var bytes = MessagePackSerializer.ConvertFromJson(jToken.ToString(), options); @@ - var jsonString = MessagePackSerializer.ConvertToJson(bytes); + var jsonString = MessagePackSerializer.ConvertToJson(bytes, options);This keeps
MessageExtrasserialization aligned with the global MsgPack configuration.src/IO.Ably.Tests.Shared/MsgPack/CustomSerializers/SimpleBufferWriter.cs (1)
10-50: Add basic safety checks toAdvanceto avoid corrupted internal stateAs-is,
Advanceblindly increments_index. If a caller ever passes a negativecountor a value larger than the space actually obtained fromGetMemory/GetSpan,_indexcan go out of range and laterArray.CopyinEnsureCapacitymay throw or copy incorrect data.Since this is a test helper, a simple defensive check keeps failures obvious and localized:
- public void Advance(int count) - { - _index += count; - } + public void Advance(int count) + { + if (count < 0) + { + throw new ArgumentOutOfRangeException(nameof(count)); + } + + if (_index + count > _buffer.Length) + { + throw new InvalidOperationException("Cannot advance past the end of the buffer."); + } + + _index += count; + }This mirrors the behavior of framework writers (fail fast on misuse) and makes test failures easier to diagnose.
src/IO.Ably.Tests.Shared/MsgPack/CustomSerializers/JObjectMessagePackSerializerTests.cs (1)
209-227: Add an assertion for thedoublefield in the large-number test
ShouldHandleLargeNumberssets a"double"property on the JObject but only asserts theint32Max,int64Max, andnegativevalues after deserialization. To fully validate the scenario, it would be good to assert the double as well:- deserialized["int32Max"].Value<int>().Should().Be(int.MaxValue); - deserialized["int64Max"].Value<long>().Should().Be(long.MaxValue); - deserialized["negative"].Value<int>().Should().Be(-999999999); + deserialized["int32Max"].Value<int>().Should().Be(int.MaxValue); + deserialized["int64Max"].Value<long>().Should().Be(long.MaxValue); + deserialized["double"].Value<double>().Should().Be(double.MaxValue); + deserialized["negative"].Value<int>().Should().Be(-999999999);This keeps the test aligned with its name and input data.
src/IO.Ably.Tests.Shared/MsgPack/CustomSerializers/MessageExtrasFormatterTests.cs (1)
210-222: Add explicit assertion or clarify test intent.This test lacks assertions and only includes a comment about graceful handling. If the intent is to verify no exception is thrown, consider either:
- Adding an explicit pass assertion (e.g.,
Assert.True(true)with a comment)- Using xUnit's
Record.Exception()pattern to verify no exception- Adding assertions about the reader's state after the operation
This makes the test's purpose clearer and ensures it's actually validating something.
src/IO.Ably.Tests.Shared/MsgPack/CustomSerializers/ChannelParamsFormatterTests.cs (1)
153-171: Test name is misleading.The test is named
ShouldMaintainOrderOfEntries, but the comment and assertion only verify that all keys are present, not their order. Since dictionaries don't guarantee order (as the comment correctly notes), the test name should reflect what it actually validates.Consider renaming to something like:
-public void ShouldMaintainOrderOfEntries() +public void ShouldPreserveAllKeys()src/IO.Ably.Tests.Shared/MsgPack/CustomSerializers/DateTimeOffsetFormatterTests.cs (1)
38-45: Test doesn't adequately verify timezone preservation.The test
ShouldPreserveTimezoneInformationusesTimeSpan.Zero(UTC) for the offset, which doesn't effectively test timezone preservation. To properly validate this, the test should:
- Use a non-zero offset (e.g.,
TimeSpan.FromHours(-5))- Verify that the offset is preserved after round-tripping
Example improvement:
-var originalDateTimeOffset = new TestClass(new DateTimeOffset(2014, 1, 1, 0, 0, 0, TimeSpan.Zero)); +var originalDateTimeOffset = new TestClass(new DateTimeOffset(2014, 1, 1, 0, 0, 0, TimeSpan.FromHours(-5))); var serialized = MsgPackHelper.Serialise(originalDateTimeOffset); var deserialized = MsgPackHelper.Deserialise<TestClass>(serialized); deserialized.DateTimeOffset.Should().Be(originalDateTimeOffset.DateTimeOffset); +deserialized.DateTimeOffset.Offset.Should().Be(TimeSpan.FromHours(-5));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (35)
src/IO.Ably.Android/IO.Ably.Android.csproj(4 hunks)src/IO.Ably.NETFramework/IO.Ably.NETFramework.csproj(4 hunks)src/IO.Ably.NETStandard20/IO.Ably.NETStandard20.csproj(2 hunks)src/IO.Ably.NetFramework.sln(0 hunks)src/IO.Ably.NetStandard.sln(4 hunks)src/IO.Ably.Package.sln(0 hunks)src/IO.Ably.PackagePush.sln(0 hunks)src/IO.Ably.Shared.MsgPack/IO.Ably.Shared.MsgPack.projitems(0 hunks)src/IO.Ably.Shared.MsgPack/IO.Ably.Shared.MsgPack.shproj(0 hunks)src/IO.Ably.Shared/CustomSerialisers/MessageDataConverter.cs(1 hunks)src/IO.Ably.Shared/IO.Ably.Shared.projitems(1 hunks)src/IO.Ably.Shared/MsgPack/CustomSerialisers/AblyGeneratedResolver.cs(1 hunks)src/IO.Ably.Shared/MsgPack/CustomSerialisers/AblyResolver.cs(1 hunks)src/IO.Ably.Shared/MsgPack/CustomSerialisers/CapabilityMessagePackSerializer.cs(1 hunks)src/IO.Ably.Shared/MsgPack/CustomSerialisers/ChannelParamsFormatter.cs(1 hunks)src/IO.Ably.Shared/MsgPack/CustomSerialisers/DateTimeOffsetMessagePackSerializer.cs(1 hunks)src/IO.Ably.Shared/MsgPack/CustomSerialisers/JObjectMessagePackSerializer.cs(1 hunks)src/IO.Ably.Shared/MsgPack/CustomSerialisers/MessageExtrasMessagePackSerializer.cs(1 hunks)src/IO.Ably.Shared/MsgPack/CustomSerialisers/TimespanMessagePackSerializer.cs(1 hunks)src/IO.Ably.Shared/MsgPack/MsgPackHelper.cs(1 hunks)src/IO.Ably.Shared/Types/Message.cs(4 hunks)src/IO.Ably.Tests.Shared/CustomSerializers/JsonSerializationTests.cs(1 hunks)src/IO.Ably.Tests.Shared/IO.Ably.Tests.Shared.projitems(3 hunks)src/IO.Ably.Tests.Shared/MsgPack/CustomSerializers/CapabilityFormatterTests.cs(1 hunks)src/IO.Ably.Tests.Shared/MsgPack/CustomSerializers/ChannelParamsFormatterTests.cs(1 hunks)src/IO.Ably.Tests.Shared/MsgPack/CustomSerializers/DateTimeOffsetFormatterTests.cs(1 hunks)src/IO.Ably.Tests.Shared/MsgPack/CustomSerializers/JObjectMessagePackSerializerTests.cs(1 hunks)src/IO.Ably.Tests.Shared/MsgPack/CustomSerializers/MessageExtrasFormatterTests.cs(1 hunks)src/IO.Ably.Tests.Shared/MsgPack/CustomSerializers/SimpleBufferWriter.cs(1 hunks)src/IO.Ably.Tests.Shared/MsgPack/CustomSerializers/TimespanFormatterTests.cs(1 hunks)src/IO.Ably.Tests.Shared/MsgPack/MsgPackHelperTests.cs(5 hunks)src/IO.Ably.Tests.Shared/MsgPack/MsgPackMessageSerializerTests.cs(1 hunks)src/IO.Ably.Xamarin.sln(0 hunks)src/IO.Ably.iOS/IO.Ably.iOS.csproj(4 hunks)src/IO.Ably.sln(0 hunks)
💤 Files with no reviewable changes (7)
- src/IO.Ably.Package.sln
- src/IO.Ably.NetFramework.sln
- src/IO.Ably.sln
- src/IO.Ably.Xamarin.sln
- src/IO.Ably.PackagePush.sln
- src/IO.Ably.Shared.MsgPack/IO.Ably.Shared.MsgPack.shproj
- src/IO.Ably.Shared.MsgPack/IO.Ably.Shared.MsgPack.projitems
🧰 Additional context used
🧬 Code graph analysis (18)
src/IO.Ably.Shared/MsgPack/CustomSerialisers/CapabilityMessagePackSerializer.cs (2)
src/IO.Ably.Shared/MsgPack/CustomSerialisers/JObjectMessagePackSerializer.cs (1)
IMessagePackFormatter(12-22)src/IO.Ably.Shared/Capability.cs (1)
ToJson(112-127)
src/IO.Ably.Shared/MsgPack/CustomSerialisers/DateTimeOffsetMessagePackSerializer.cs (3)
src/IO.Ably.Shared/MsgPack/CustomSerialisers/AblyResolver.cs (1)
IMessagePackFormatter(23-26)src/IO.Ably.Shared/MsgPack/CustomSerialisers/TimespanMessagePackSerializer.cs (1)
Serialize(12-15)src/IO.Ably.Shared/Extensions/DateExtensions.cs (1)
ToUnixTimeInMilliseconds(30-33)
src/IO.Ably.Shared/MsgPack/CustomSerialisers/MessageExtrasMessagePackSerializer.cs (1)
src/IO.Ably.Shared/MsgPack/CustomSerialisers/JObjectMessagePackSerializer.cs (1)
IMessagePackFormatter(12-22)
src/IO.Ably.Shared/MsgPack/CustomSerialisers/JObjectMessagePackSerializer.cs (2)
src/IO.Ably.Shared/MsgPack/CustomSerialisers/MessageExtrasMessagePackSerializer.cs (1)
Serialize(13-31)src/IO.Ably.Shared/Capability.cs (1)
ToJson(112-127)
src/IO.Ably.Tests.Shared/MsgPack/CustomSerializers/MessageExtrasFormatterTests.cs (3)
src/IO.Ably.Shared/MsgPack/CustomSerialisers/MessageExtrasMessagePackSerializer.cs (3)
MessageExtras(34-55)MessageExtrasFormatter(10-56)Serialize(13-31)src/IO.Ably.Shared/MsgPack/MsgPackHelper.cs (3)
MsgPackHelper(12-98)Serialise(21-34)Deserialise(36-50)src/IO.Ably.Tests.Shared/Helpers/JAssert.cs (2)
JAssert(6-22)DeepEquals(11-21)
src/IO.Ably.Tests.Shared/MsgPack/MsgPackHelperTests.cs (4)
src/IO.Ably.Shared/Types/Message.cs (5)
Message(34-44)Message(178-181)Message(189-192)Message(201-213)Message(222-234)src/IO.Ably.Shared/MsgPack/MsgPackHelper.cs (3)
MsgPackHelper(12-98)Serialise(21-34)Deserialise(36-50)src/IO.Ably.Shared/Utils/StringUtils.cs (1)
FromBase64(42-63)src/IO.Ably.Shared/JsonHelper.cs (2)
JsonHelper(16-115)Deserialize(78-81)
src/IO.Ably.Tests.Shared/MsgPack/CustomSerializers/DateTimeOffsetFormatterTests.cs (3)
src/IO.Ably.Shared/MsgPack/CustomSerialisers/DateTimeOffsetMessagePackSerializer.cs (3)
DateTimeOffset(18-48)DateTimeOffsetFormatter(9-49)Serialize(12-15)src/IO.Ably.Shared/MsgPack/MsgPackHelper.cs (3)
MsgPackHelper(12-98)Serialise(21-34)Deserialise(36-50)src/IO.Ably.Shared/Extensions/DateExtensions.cs (1)
ToUnixTimeInMilliseconds(30-33)
src/IO.Ably.Shared/MsgPack/CustomSerialisers/TimespanMessagePackSerializer.cs (2)
src/IO.Ably.Shared/MsgPack/CustomSerialisers/AblyResolver.cs (1)
IMessagePackFormatter(23-26)src/IO.Ably.Shared/MsgPack/CustomSerialisers/DateTimeOffsetMessagePackSerializer.cs (1)
Serialize(12-15)
src/IO.Ably.Tests.Shared/MsgPack/CustomSerializers/JObjectMessagePackSerializerTests.cs (4)
src/IO.Ably.Shared/MsgPack/CustomSerialisers/JObjectMessagePackSerializer.cs (3)
JObject(24-51)JObjectMessagePackSerializer(10-52)IMessagePackFormatter(12-22)src/IO.Ably.Shared/MsgPack/MsgPackHelper.cs (3)
MsgPackHelper(12-98)Serialise(21-34)Deserialise(36-50)src/IO.Ably.Tests.Shared/Helpers/JAssert.cs (2)
JAssert(6-22)DeepEquals(11-21)src/IO.Ably.Tests.Shared/MsgPack/CustomSerializers/SimpleBufferWriter.cs (1)
SimpleBufferWriter(10-51)
src/IO.Ably.Tests.Shared/MsgPack/CustomSerializers/TimespanFormatterTests.cs (2)
src/IO.Ably.Shared/MsgPack/CustomSerialisers/TimespanMessagePackSerializer.cs (3)
TimeSpan(18-48)TimespanFormatter(9-49)Serialize(12-15)src/IO.Ably.Shared/MsgPack/MsgPackHelper.cs (3)
MsgPackHelper(12-98)Serialise(21-34)Deserialise(36-50)
src/IO.Ably.Shared/MsgPack/MsgPackHelper.cs (2)
src/IO.Ably.Shared/MsgPack/CustomSerialisers/AblyResolver.cs (2)
AblyResolver(15-82)AblyResolver(19-21)src/IO.Ably.Shared/MsgPack/CustomSerialisers/MessageExtrasMessagePackSerializer.cs (1)
Serialize(13-31)
src/IO.Ably.Tests.Shared/CustomSerializers/JsonSerializationTests.cs (2)
src/IO.Ably.Shared/JsonHelper.cs (2)
JsonHelper(16-115)Deserialize(78-81)src/IO.Ably.Shared/Capability.cs (1)
ToJson(112-127)
src/IO.Ably.Shared/Types/Message.cs (3)
src/IO.Ably.Shared/Types/ErrorInfo.cs (1)
MessagePackObject(14-268)src/IO.Ably.Shared/Types/MessageExtras.cs (1)
JsonConverter(10-114)src/IO.Ably.Shared/CustomSerialisers/MessageDataConverter.cs (1)
MessageDataConverter(8-30)
src/IO.Ably.Tests.Shared/MsgPack/CustomSerializers/ChannelParamsFormatterTests.cs (2)
src/IO.Ably.Shared/MsgPack/CustomSerialisers/ChannelParamsFormatter.cs (3)
ChannelParams(30-48)ChannelParamsFormatter(10-49)Serialize(13-27)src/IO.Ably.Shared/MsgPack/MsgPackHelper.cs (3)
MsgPackHelper(12-98)Serialise(21-34)Deserialise(36-50)
src/IO.Ably.Shared/MsgPack/CustomSerialisers/ChannelParamsFormatter.cs (4)
src/IO.Ably.Shared/MsgPack/CustomSerialisers/AblyResolver.cs (1)
IMessagePackFormatter(23-26)src/IO.Ably.Shared/MsgPack/CustomSerialisers/JObjectMessagePackSerializer.cs (1)
IMessagePackFormatter(12-22)src/IO.Ably.Shared/MsgPack/CustomSerialisers/CapabilityMessagePackSerializer.cs (1)
Serialize(11-20)src/IO.Ably.Shared/MsgPack/CustomSerialisers/MessageExtrasMessagePackSerializer.cs (1)
Serialize(13-31)
src/IO.Ably.Shared/MsgPack/CustomSerialisers/AblyResolver.cs (6)
src/IO.Ably.Shared/MsgPack/CustomSerialisers/JObjectMessagePackSerializer.cs (3)
IMessagePackFormatter(12-22)JObject(24-51)JObjectMessagePackSerializer(10-52)src/IO.Ably.Shared/MsgPack/CustomSerialisers/DateTimeOffsetMessagePackSerializer.cs (2)
DateTimeOffset(18-48)DateTimeOffsetFormatter(9-49)src/IO.Ably.Shared/MsgPack/CustomSerialisers/TimespanMessagePackSerializer.cs (2)
TimeSpan(18-48)TimespanFormatter(9-49)src/IO.Ably.Shared/MsgPack/CustomSerialisers/CapabilityMessagePackSerializer.cs (2)
Capability(23-32)CapabilityFormatter(8-33)src/IO.Ably.Shared/MsgPack/CustomSerialisers/MessageExtrasMessagePackSerializer.cs (2)
MessageExtras(34-55)MessageExtrasFormatter(10-56)src/IO.Ably.Shared/MsgPack/CustomSerialisers/ChannelParamsFormatter.cs (2)
ChannelParams(30-48)ChannelParamsFormatter(10-49)
src/IO.Ably.Tests.Shared/MsgPack/MsgPackMessageSerializerTests.cs (4)
src/IO.Ably.Shared/Types/Message.cs (6)
Message(34-44)Message(178-181)Message(189-192)Message(201-213)Message(222-234)ToString(99-108)src/IO.Ably.Shared/MsgPack/MsgPackHelper.cs (3)
MsgPackHelper(12-98)Serialise(21-34)Deserialise(36-50)src/IO.Ably.Tests.Shared/Infrastructure/ResourceHelper.cs (2)
ResourceHelper(8-41)GetResource(10-23)src/IO.Ably.Tests.Shared/Helpers/JAssert.cs (2)
JAssert(6-22)DeepEquals(11-21)
src/IO.Ably.Tests.Shared/MsgPack/CustomSerializers/CapabilityFormatterTests.cs (5)
src/IO.Ably.Shared/MsgPack/CustomSerialisers/CapabilityMessagePackSerializer.cs (3)
Capability(23-32)CapabilityFormatter(8-33)Serialize(11-20)src/IO.Ably.Shared/CapabilityResource.cs (1)
AllowAll(48-52)src/IO.Ably.Shared/MsgPack/MsgPackHelper.cs (3)
MsgPackHelper(12-98)Serialise(21-34)Deserialise(36-50)src/IO.Ably.Shared/Capability.cs (1)
ToJson(112-127)src/IO.Ably.Tests.Shared/MsgPack/CustomSerializers/SimpleBufferWriter.cs (1)
SimpleBufferWriter(10-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: check (net8.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net7.0)
- GitHub Check: check
🔇 Additional comments (36)
src/IO.Ably.Shared/IO.Ably.Shared.projitems (1)
31-39: All new MessagePack files verified to exist.Verification confirms that all 9 new MessagePack-related files referenced in the project file exist at their specified paths. The project file entries are valid and will not cause build failures due to missing file references.
src/IO.Ably.Tests.Shared/IO.Ably.Tests.Shared.projitems (3)
26-26: LGTM - Test file addition is well-organized.The addition of
JsonSerializationTests.csto the CustomSerializers group is consistent with the existing test structure.
76-84: Good organization of MsgPack test coverage.The new test files provide comprehensive coverage for the MessagePack formatter implementations. The file structure logically groups related tests under
MsgPack\CustomSerializers\.
168-170: LGTM - Folder declaration is appropriate.The explicit
Folder IncludeforMsgPack\CustomSerializers\is correct for shared project items files and ensures proper IDE integration.src/IO.Ably.iOS/IO.Ably.iOS.csproj (2)
116-119: Analyzer configurations are consistent across platform projects and properly aligned.The script output confirms that MessagePackAnalyzer is automatically installed when you install MessagePack via NuGet and supports Xamarin platforms. All three platform projects (iOS, Android, NETFramework) use identical configuration: MessagePackAnalyzer.3.1.4 with roslyn4.3/cs analyzers. This consistency is correct—analyzer packages include intermediate roslyn version folders with different versions to support multiple SDK versions, such as roslyn3.11 and roslyn4.0, and roslyn4.3 follows this standard pattern. No actionable issues were found.
97-98: Verified: Version bump is justified and consistent. The update from 4.2.0.0 to 4.2.0.1 is the correct assembly version required for MessagePack v3.1.4 compatibility (which depends on System.Threading.Tasks.Extensions NuGet package ≥ 4.5.4). All platform projects (iOS, Android, NETFramework) consistently use 4.2.0.1, and NETStandard20 correctly references the NuGet package directly at v4.5.4. No issues identified.src/IO.Ably.Shared/MsgPack/CustomSerialisers/AblyGeneratedResolver.cs (2)
1-1: LGTM!The MessagePack namespace import is correct and necessary for the attribute used below.
5-8: No issues found — verification confirms correct implementation.The
[GeneratedMessagePackResolver]attribute is the correct source-generator attribute for MessagePack-CSharp v3.1.4, and the implementation is sound:
- The empty partial class is intentional; the source generator populates it with resolver members.
- The generator produces a resolver type accessible via
AblyGeneratedResolver.Instance, which is correctly registered inMsgPackHelper.csline 18 as part of theCompositeResolver.- The
MessagePackAnalyzer.3.1.4source generator is properly configured in the project files.src/IO.Ably.Shared/CustomSerialisers/MessageDataConverter.cs (2)
4-4: Namespace change has been properly migrated and no references remain to the old namespace.The verification confirms that all references to the old namespace
IO.Ably.Shared.CustomSerialisershave been updated. The[JsonConverter]attribute resolution for types in the same assembly works with fully qualified type names, so the usage inMessage.csline 79 ([JsonConverter(typeof(MessageDataConverter))]) will resolve correctly to the new namespace at compile time without requiring any using statement updates.
26-29:CanConvertreturning true is correct for this converter—no changes needed.This converter is a serialization behavior modifier applied to the polymorphic
Message.Dataproperty (typed asobject). Unlike the other converters in the codebase (CapabilityJsonConverter, DateTimeOffsetJsonConverter, etc.) which convert to specific types,MessageDataConverteris designed to handle any data type by delegating to the underlying serializer while enforcingNullValueHandling.Include. The test confirms it works with arbitrary object types. TheCanConvertimplementation correctly reflects this design.Likely an incorrect or invalid review comment.
src/IO.Ably.Shared/Types/Message.cs (5)
5-7: LGTM!The additional using directives are necessary for MessagePack serialization support.
14-14: LGTM!Using
keyAsPropertyName: truemaintains consistency with the JSON serialization format and ensures interoperability.
47-96: LGTM!All serializable properties are correctly annotated with
[Key("...")]attributes that match their correspondingJsonPropertynames, ensuring consistent serialization behavior between JSON and MessagePack formats.
113-115: LGTM!Correctly marking the computed
IsEmptyproperty with[IgnoreMember]to exclude it from MessagePack serialization, consistent with the existing[JsonIgnore]attribute.
77-87: I need to verify one more detail about theDataproperty (typeobject) serialization:All MessagePack formatter support for complex property types is correctly configured.
Verification confirms:
- MessagePack handles
objecttype through PrimitiveObjectResolver, which supports bool, byte, int, string, byte[], ICollection, and IDictionary, with additional support through the resolver chainMessageExtrashas a customMessageExtrasFormatterimplementingIMessagePackFormatter<MessageExtras>registered inAblyResolverDateTimeOffsethas a customDateTimeOffsetFormatterimplementingIMessagePackFormatter<DateTimeOffset>registered inAblyResolverMsgPackHelper.csconfiguresMessagePackSerializerOptionswithCompositeResolver.Create(AblyResolver.Instance, AblyGeneratedResolver.Instance, StandardResolver.Instance)The code properly handles all three complex property types through the configured resolver chain.
src/IO.Ably.Android/IO.Ably.Android.csproj (3)
20-21: Good: Compiler-generated files enabled for source generation.Note: The hardcoded path
obj/Generateddiffers from other projects that use$(BaseIntermediateOutputPath)Generated. If this is intentional for Xamarin.Android compatibility, document why. Otherwise, align with the other projects for consistency.
116-119: Good: MessagePackAnalyzer included for compile-time code generation.Verify that MessagePackAnalyzer version 3.1.4 roslyn4.3 artifacts are available in the package and compatible with your build environment.
96-97: Incorrect review comment: Version comparison claim is factually wrong.The original review asserts that Android uses System.Threading.Tasks.Extensions 4.2.0.1 while NETFramework uses 4.5.4. However, the actual codebase shows:
- Android: Reference Include Version 4.2.0.1 with HintPath to 4.5.4
- NETFramework: Reference Include Version 4.2.0.1 with HintPath to 4.5.4 (identical to Android)
- NETStandard20: PackageReference Version 4.5.4 (different mechanism, not a direct comparison)
- iOS: Reference Include Version 4.2.0.0 with HintPath to 4.5.0
Android and NETFramework reference the same version (4.2.0.1) with identical HintPaths. The claimed discrepancy between these two projects does not exist. The actual variation is between iOS (4.2.0.0/4.5.0) and the others, which the review did not mention.
Likely an incorrect or invalid review comment.
src/IO.Ably.NETStandard20/IO.Ably.NETStandard20.csproj (1)
10-11: Good: MessagePack 3.1.4 properly configured with source generation.The use of
$(BaseIntermediateOutputPath)Generatedis cleaner than hardcoded paths. Versions are consistent with other projects targeting .NET Standard 2.0 and above.Also applies to: 48-48, 55-55
src/IO.Ably.NETFramework/IO.Ably.NETFramework.csproj (3)
18-19: Good: Compiler-generated files enabled.Consistent with other projects in the solution.
129-130: Good: MessagePackAnalyzer build targets properly configured with error handling.The conditional import with error check ensures that the source generator runs correctly and fails fast if the analyzer package is missing.
Also applies to: 145-145, 147-147
26-26: No issues found: EXCLUDE_MSGPACK removal is complete and clean.Verification confirms:
- EXCLUDE_MSGPACK has been entirely removed from the codebase (zero matches)
- No remaining
#if EXCLUDE_MSGPACKor conditional guards exist- MessagePack code is unconditionally integrated across the library and properly tested
- Debug builds will function correctly with MessagePack always enabled
src/IO.Ably.NetStandard.sln (4)
3-4: LGTM! Visual Studio version metadata updated.The Visual Studio version update from 16.x to 17.x (Visual Studio 2022) is a standard metadata change with no functional impact on the solution.
17-17: TheIO.Ably.Tests.rulesetfile exists at the correct location (src/IO.Ably.Tests.ruleset), confirming the solution reference on line 17 is valid. No issues identified.
34-34: The DeltaCodec project type GUID change is correctly reflected in the solution file.The solution file line 34 shows
IO.Ably.DeltaCodecis referenced with the standard C# project type GUID (9A19103F-16F7-4668-BE54-9A1E7A4F7556) and explicitly points to a.csprojfile. Thelib/delta-codecdirectory is configured as a git submodule (https://github.com/ably/delta-codec-dotnet.git), but it's not initialized in this environment, preventing direct verification of the actual project file. However, the solution file configuration is consistent with treating DeltaCodec as a standard C# project, not a shared project.
277-282: Verification complete—no issues found.The changes to
SharedMSBuildProjectFilesare correct and complete:
- ✅ No MsgPack references remain in the solution file
- ✅ All projects that import shared files in this solution are properly listed with correct GUIDs and SharedItemsImports values:
- IO.Ably.Tests.Shared self-import (13) and imported by Tests.DotNET (5)
- IO.Ably.Shared self-import (13) and imported by NETStandard20 (5)
- ✅ Section is properly formatted and complete
src/IO.Ably.Shared/MsgPack/CustomSerialisers/AblyResolver.cs (1)
15-81: Centralised resolver implementation looks correct and efficientThe singleton resolver plus
FormatterCache<T>pattern gives you one formatter instance per handled type, and the mappings line up with your custom formatters (DateTimeOffset/TimeSpan, Capability, MessageExtras, ChannelParams, JObject). Returningnullfor unsupported types is appropriate given your CompositeResolver setup. I don’t see any correctness or performance concerns here.src/IO.Ably.Shared/MsgPack/CustomSerialisers/ChannelParamsFormatter.cs (1)
7-48: ChannelParams formatter looks correct and symmetricSerialization/deserialization are straightforward, null-safe, and symmetric over the
ChannelParamskey/value set. GivenChannelParamsis effectively a string-to-string bag today, writing keys and values as strings with a map header matches expectations, including support for null values viawriter.Write(string)andreader.ReadString().src/IO.Ably.Tests.Shared/MsgPack/CustomSerializers/JObjectMessagePackSerializerTests.cs (3)
22-279: Comprehensive JObject round-trip coverageThis suite does a good job exercising JObject round-trips (simple, nested, arrays, mixed types, special characters, nulls, empty objects, and JSON-string-based payloads) through
MsgPackHelper, and usesJAssert.DeepEqualsfor structural equality. Combined with the dedicated tests that hit the formatter directly, this gives solid confidence in the MsgPack integration around JToken.
281-297: Empty raw buffer handling test nicely exercises the formatter edge case
ShouldHandleEmptyByteArrayDeserializationdirectly targets theReadRaw/empty-buffer path inJObjectMessagePackSerializer.Deserializeand verifies that an empty payload returns null instead of throwing. This is a helpful regression test around theremainingSequence.Lengthandbytes.Lengthguards.
300-347: Good coverage for JObject embedded in a larger MessagePack object
ShouldDeserializeJObjectAsPartOfLargerStructureis an important integration test: it verifies that a JObject can be serialized/deserialized as a property of a[MessagePackObject]container using the configured resolvers, and that the result is still a JObject with the expected structure. This directly exercises the resolver wiring forJObjectMessagePackSerializerin a realistic scenario.src/IO.Ably.Tests.Shared/MsgPack/CustomSerializers/CapabilityFormatterTests.cs (1)
1-149: LGTM! Comprehensive test coverage for CapabilityFormatter.The test suite provides excellent coverage of the CapabilityFormatter including serialization/deserialization for various capability states (AllowAll, single/multiple resources, empty), null/nil handling, JSON round-tripping, and complex scenarios with wildcards. The tests are well-structured and validate both object equality and JSON equivalence.
src/IO.Ably.Tests.Shared/MsgPack/CustomSerializers/TimespanFormatterTests.cs (1)
1-156: LGTM! Thorough test coverage for TimespanFormatter.The test suite comprehensively validates TimespanFormatter behavior including serialization to milliseconds, deserialization from multiple data types (integer, float, string), edge cases (zero, negative values), nullable handling, and invalid input handling. The nested test classes are appropriately scoped for the test context.
src/IO.Ably.Tests.Shared/MsgPack/CustomSerializers/MessageExtrasFormatterTests.cs (1)
1-209: LGTM! Comprehensive test coverage for MessageExtrasFormatter.The test suite provides excellent coverage of MessageExtrasFormatter including delta handling, arbitrary properties, nested structures, null/nil handling, empty scenarios, and various data types. The use of JAssert.DeepEquals for JSON comparison is appropriate for validating complex structures.
Also applies to: 223-248
src/IO.Ably.Tests.Shared/MsgPack/CustomSerializers/ChannelParamsFormatterTests.cs (1)
1-152: LGTM! Comprehensive test coverage for ChannelParamsFormatter.The test suite provides thorough coverage of ChannelParams serialization/deserialization including empty/single/multiple entries, null/nil handling, map format verification, special characters, and empty string values. The tests are well-structured and validate both the serialization format and content preservation.
src/IO.Ably.Tests.Shared/MsgPack/CustomSerializers/DateTimeOffsetFormatterTests.cs (1)
1-37: LGTM! Thorough test coverage for DateTimeOffsetFormatter.The test suite comprehensively validates DateTimeOffsetFormatter behavior including serialization to Unix milliseconds, deserialization from multiple data types (integer, float, string), invalid input handling, and nullable scenarios. The nested test classes are appropriately scoped for the test context.
Also applies to: 46-138
| <EmitCompilerGeneratedFiles>true</EmitCompilerGeneratedFiles> | ||
| <CompilerGeneratedFilesOutputPath>obj/Generated</CompilerGeneratedFilesOutputPath> |
There was a problem hiding this comment.
🧩 Analysis chain
Verify compiler-generated files configuration is needed for MessagePack source generation.
The added EmitCompilerGeneratedFiles and CompilerGeneratedFilesOutputPath properties enable source generation for the MessagePack analyzer. Confirm this configuration is required by MessagePack v3.1.4 and is consistently applied across all platform-specific projects.
🏁 Script executed:
#!/bin/bash
# Description: Verify compiler-generated files configuration consistency across platform projects
# Expected: All platform .csproj files should have matching EmitCompilerGeneratedFiles settings
echo "=== Checking EmitCompilerGeneratedFiles in platform .csproj files ==="
fd -e csproj '(iOS|Android|NETFramework|NETStandard)' src/ | xargs -I {} grep -l "EmitCompilerGeneratedFiles" {}
echo ""
echo "=== Checking CompilerGeneratedFilesOutputPath consistency ==="
fd -e csproj '(iOS|Android|NETFramework|NETStandard)' src/ | xargs -I {} sh -c 'echo "File: {}"; grep "CompilerGeneratedFilesOutputPath" {} || echo "NOT FOUND"'Length of output: 1137
Standardize CompilerGeneratedFilesOutputPath across all platform projects.
MessagePack v3 uses AOT source generation of formatters by default using roslyn source generators, making the EmitCompilerGeneratedFiles configuration necessary. However, the iOS project uses a hardcoded path (obj/Generated) while NETFramework and NETStandard20 platforms use the more maintainable MSBuild property approach ($(BaseIntermediateOutputPath)Generated). Update src/IO.Ably.iOS/IO.Ably.iOS.csproj lines 16-17 to use $(BaseIntermediateOutputPath)Generated for consistency and maintainability.
🤖 Prompt for AI Agents
In src/IO.Ably.iOS/IO.Ably.iOS.csproj around lines 16 to 17, the project
currently hardcodes CompilerGeneratedFilesOutputPath to "obj/Generated"; change
it to use the MSBuild property by setting CompilerGeneratedFilesOutputPath to
"$(BaseIntermediateOutputPath)Generated" (keep EmitCompilerGeneratedFiles as
true) so the AOT-generated sources follow the same maintainable intermediate
output location as other platform projects.
There was a problem hiding this comment.
Somehow (BaseIntermediateOutputPath)Generated doesn't work for ios project.
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Xamarin.iOS BaseIntermediateOutputPath MSBuild property not working
💡 Result:
Short answer: Xamarin.iOS/MSBuild is likely overriding/deriving the full intermediate path, so setting BaseIntermediateOutputPath late in the project has no effect. Fix by setting the path early (or set IntermediateOutputPath directly) or pass it on the MSBuild command line.
What to try
- Put your BaseIntermediateOutputPath (or IntermediateOutputPath) in a Directory.Build.props (or in the first PropertyGroup at the top of the .csproj) so it is evaluated before SDK/targets are imported. This makes it take effect for targets that derive IntermediateOutputPath from BaseIntermediateOutputPath. [1][2]
- If a target or SDK explicitly sets IntermediateOutputPath, set IntermediateOutputPath itself (not just BaseIntermediateOutputPath), or pass it on the command line: msbuild /p:IntermediateOutputPath=path\obj\ . (If IntermediateOutputPath is set, BaseIntermediateOutputPath is ignored.) [1][2]
- For Xamarin.iOS-specific outputs (IPA, .app bundle) it’s common to use a post-build Target to copy the IPA/IpaPackagePath to your desired folder instead of trying to change internal target paths. [3]
References
- Explanation of IntermediateOutputPath vs BaseIntermediateOutputPath and ordering (why BaseIntermediateOutputPath can be ignored). [1]
- MSBuild common-targets notes and examples showing BaseIntermediateOutputPath / IntermediateOutputPath behavior. [2]
- Example: copy IPA to custom location with an MSBuild target. [3]
If you want, paste the top of your .csproj (first PropertyGroup) or your Directory.Build.props and I’ll suggest the exact XML to add.
@sacOO7, thank you for testing that! You're correct—Xamarin.iOS/MSBuild is likely overriding/deriving the full intermediate path, so setting BaseIntermediateOutputPath late in the project has no effect.
This happens because the Xamarin.iOS targets (imported at the end of your .csproj) may explicitly set IntermediateOutputPath, which causes BaseIntermediateOutputPath to be ignored due to MSBuild property evaluation order.
The hardcoded path obj/Generated is the appropriate solution for the iOS project. While it's inconsistent with the other platform projects (NETFramework, NETStandard20), this is acceptable given the platform-specific MSBuild behavior of Xamarin.iOS. My previous suggestion to standardize it was incorrect for this platform.
The current configuration is correct and should remain as-is. ✓
| <Reference Include="MessagePack, Version=3.1.4.0, Culture=neutral, PublicKeyToken=b4a0369545f0a1be"> | ||
| <HintPath>..\packages\MessagePack.3.1.4\lib\netstandard2.0\MessagePack.dll</HintPath> | ||
| <Private>True</Private> | ||
| </Reference> | ||
| <Reference Include="MessagePack.Annotations, Version=3.1.4.0, Culture=neutral, PublicKeyToken=b4a0369545f0a1be, processorArchitecture=MSIL"> | ||
| <HintPath>..\packages\MessagePack.Annotations.3.1.4\lib\netstandard2.0\MessagePack.Annotations.dll</HintPath> | ||
| </Reference> |
There was a problem hiding this comment.
🧩 Analysis chain
Verify MessagePack package versions and hint paths are correct.
The added MessagePack and MessagePack.Annotations v3.1.4 references use explicit HintPath entries pointing to netstandard2.0. Ensure these paths are consistent with the actual package structure and that the versions are aligned across all platform projects.
🏁 Script executed:
#!/bin/bash
# Description: Verify MessagePack package references consistency across all platform projects
# Expected: All platforms should reference MessagePack v3.1.4 with consistent HintPath patterns
echo "=== MessagePack references across platform projects ==="
fd -e csproj '(iOS|Android|NETFramework|NETStandard)' src/ | while read f; do
echo "File: $f"
grep -A 2 'Include="MessagePack' "$f" | head -5
echo ""
done
echo "=== Verifying HintPath patterns ==="
rg -A 2 'Include="MessagePack' --type=xml | grep -E '(HintPath|Version)' | head -20Length of output: 1915
---
🌐 Web query:
```web
MessagePack v3.1.4 NuGet package supported frameworks iOS Android
💡 Result:
MessagePack 3.1.4 supports the following mobile-related TFMs (per the NuGet package page): net6.0-android, net6.0-ios, net7.0-android, net7.0-ios, net8.0-android, net8.0-ios, net9.0-android, net9.0-ios, net10.0-android, net10.0-ios, plus legacy Xamarin targets (MonoAndroid, Xamarin.iOS/MonoTouch). [1]
Source: NuGet package details for MessagePack 3.1.4. [1] https://www.nuget.org/packages/MessagePack/3.1.4
---
🏁 Script executed:
```shell
#!/bin/bash
# Verify target frameworks for each project
echo "=== Project target frameworks ==="
for f in src/IO.Ably.iOS/IO.Ably.iOS.csproj src/IO.Ably.Android/IO.Ably.Android.csproj src/IO.Ably.NETFramework/IO.Ably.NETFramework.csproj src/IO.Ably.NETStandard20/IO.Ably.NETStandard20.csproj; do
if [ -f "$f" ]; then
echo "File: $f"
grep -E 'TargetFramework[s]?' "$f" | head -3
echo ""
fi
done
Length of output: 518
Critical framework mismatch: MessagePack v3.1.4 HintPath references are incompatible with project target frameworks.
The iOS, Android, and NETFramework projects are configured to use netstandard2.0 binaries via HintPath, but this framework is not supported by MessagePack v3.1.4 for these platforms:
- Android (TargetFrameworkVersion v11.0): Should use legacy Xamarin target (MonoAndroid) or modern mobile target (net6.0-android/net7.0-android, etc.), not netstandard2.0
- NETFramework (TargetFrameworkVersion v4.6.2): Should use net462 or net461, not netstandard2.0
- iOS: Using netstandard2.0 HintPath for a Xamarin.iOS project is incorrect; should use legacy Xamarin or modern mobile targets
MessagePack 3.1.4 supports modern mobile TFMs (net6.0-android, net6.0-ios, net7.0+), legacy Xamarin targets (MonoAndroid, Xamarin.iOS/MonoTouch), but not netstandard2.0 binaries for mobile platforms. These mismatches will cause assembly resolution failures at runtime.
Update the HintPath entries to reference the correct framework-specific binaries, or migrate to modern SDK-style projects using PackageReference.
🤖 Prompt for AI Agents
In src/IO.Ably.iOS/IO.Ably.iOS.csproj around lines 63 to 69, the HintPath
entries reference netstandard2.0 MessagePack assemblies which are incompatible
with the iOS/Xamarin mobile target; update the project to either (1) switch to
PackageReference for MessagePack 3.1.4 and upgrade the project TFM to a
supported modern mobile TFM (e.g., net6.0-ios/net6.0-android) or legacy Xamarin
target (Xamarin.iOS/MonoAndroid), or (2) if staying with old csproj style,
replace the HintPath targets with the platform-specific MessagePack binaries
shipped in the package (use the package's lib folder for the matching TFM such
as lib/monoandroid*/MessagePack.dll or lib/xamarin.ios*/MessagePack.dll or
lib/net462 for .NET Framework), and ensure Reference Include entries match the
correct assembly versions and Private metadata; do not reference netstandard2.0
assemblies for iOS/Android/NETFramework projects.
There was a problem hiding this comment.
MessagePack v3.1.4 supports netframework 4.7.2+, can you double check. To support it in netframework4.6, we are using dll from netstandard2.0
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
MessagePack v3.1.4 NuGet package official supported target frameworks
💡 Result:
MessagePack 3.1.4 (NuGet) — included/official target frameworks:
- .NET 8.0 (net8.0).
- .NET Standard 2.0 (netstandard2.0).
- .NET Framework 4.7.2 (net472).
(Computed compatible frameworks listed on the NuGet page include many earlier and later .NET/.NET Core/.NET Standard/.NET Framework variants.) [1]
Sources:
[1] NuGet package: MessagePack 3.1.4.
🌐 Web query:
.NET Standard 2.0 compatibility .NET Framework 4.6.1 4.6.2
💡 Result:
Short answer: .NET Standard 2.0 is supported on .NET Framework 4.6.1 and later — but with tooling/runtime caveats. [1][2][3]
Key points (concise)
- .NET Framework 4.6.1 (and 4.6.2, 4.7, 4.7.1, 4.8, etc.) implements .NET Standard 2.0, so libraries targeting netstandard2.0 can run on those frameworks. [1][3]
- Microsoft added a “.NET Framework compatibility mode” for netstandard2.0 so many .NET Framework packages can be referenced from netstandard2.0 projects; some platform-specific APIs (WPF, etc.) still won’t work. [1][3]
- Practical/tooling notes:
- Visual Studio 2017 15.3+ (and later VS) handles the netstandard2.0 support files for apps running on .NET Framework 4.6.1–4.7; those support files are automatically copied to output. For older toolsets you may need the compatibility support package. [4]
- Starting with .NET Framework 4.7.1 the support files are built into the framework (fewer deployment/binding-redirect issues). [4]
- Additional caution: some newer Microsoft NuGet core libraries dropped support for the oldest frameworks in later .NET releases, so when upgrading packages you may run into packages that no longer target net461 — prefer at least net462/net47+ if you need ongoing package compatibility. [5]
Sources
[1] Announcing .NET Standard 2.0 (.NET blog).
[2] Introducing .NET Standard (table & explanation).
[3] .NET Standard documentation (support matrix & guidance).
[4] .NET Framework 4.7.1 runtime/compiler features (notes on support files).
[5] .NET compatibility notes on older framework support (packages dropped).
@sacOO7, you're absolutely correct, and I apologize for the inaccurate previous comment.
MessagePack v3.1.4 officially includes .NET Standard 2.0 (netstandard2.0) as one of its target frameworks, along with net8.0 and net472.
Your approach is valid:
- .NET Standard 2.0 is supported on .NET Framework 4.6.1 and later, so using the netstandard2.0 binaries for .NET Framework 4.6.2 is a legitimate compatibility strategy.
- The same applies to the iOS (Xamarin.iOS) and Android (MonoAndroid) projects—both legacy Xamarin platforms support .NET Standard 2.0 libraries.
The HintPath references to netstandard2.0 assemblies are correct for these legacy project types. While MessagePack does officially target net472+ for .NET Framework, the netstandard2.0 target provides backward compatibility with earlier framework versions like 4.6.1 and 4.6.2.
Thank you for the clarification!
| <Reference Include="MessagePack, Version=3.1.4.0, Culture=neutral, PublicKeyToken=b4a0369545f0a1be"> | ||
| <HintPath>..\packages\MessagePack.3.1.4\lib\netstandard2.0\MessagePack.dll</HintPath> | ||
| <Private>True</Private> | ||
| </Reference> | ||
| <Reference Include="MessagePack.Annotations, Version=3.1.4.0, Culture=neutral, PublicKeyToken=b4a0369545f0a1be, processorArchitecture=MSIL"> | ||
| <HintPath>..\packages\MessagePack.Annotations.3.1.4\lib\netstandard2.0\MessagePack.Annotations.dll</HintPath> | ||
| </Reference> | ||
| <Reference Include="Microsoft.Bcl.AsyncInterfaces, Version=8.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51, processorArchitecture=MSIL"> | ||
| <HintPath>..\packages\Microsoft.Bcl.AsyncInterfaces.8.0.0\lib\net462\Microsoft.Bcl.AsyncInterfaces.dll</HintPath> | ||
| </Reference> | ||
| <Reference Include="Microsoft.NET.StringTools, Version=1.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a, processorArchitecture=MSIL"> | ||
| <HintPath>..\packages\Microsoft.NET.StringTools.17.11.4\lib\netstandard2.0\Microsoft.NET.StringTools.dll</HintPath> | ||
| </Reference> |
There was a problem hiding this comment.
🧩 Analysis chain
Verify: Major version bumps for framework compatibility.
The addition of System.Runtime.CompilerServices.Unsafe 6.0.0 (up from 4.5.0) and System.Collections.Immutable 8.0.0 represent significant version changes. Although these are dependencies of MessagePack 3.1.4, confirm:
- System.Runtime.CompilerServices.Unsafe 6.0.0 is compatible with .NET Framework 4.6.2
- No breaking API changes in these library versions that would affect the codebase
- All transitive dependencies resolve correctly for .NET Framework 4.6.2
Consider running target framework compatibility checks as part of testing.
Also applies to: 82-107
🌐 Web query:
System.Runtime.CompilerServices.Unsafe 6.0.0 .NET Framework 4.6.2 compatibility
💡 Result:
Yes — System.Runtime.CompilerServices.Unsafe 6.0.0 is compatible with .NET Framework 4.6.2. The NuGet package for 6.0.0 includes a net46.1 target (and netstandard2.0), which is compatible with 4.6.2 (4.6.2 >= 4.6.1). For later 6.1.x releases the package explicitly lists net462 as a target. [1][2]
Sources:
- System.Runtime.CompilerServices.Unsafe 6.0.0 (NuGet). [1]
- System.Runtime.CompilerServices.Unsafe 6.1.0 (NuGet) — shows net462 target. [2]
🌐 Web query:
System.Collections.Immutable 8.0.0 .NET Framework support
💡 Result:
Yes — System.Collections.Immutable 8.0.0 supports .NET Framework. The NuGet package lists .NETFramework 4.6.2 (and compatibility with 4.6.1/4.6.2+ / 4.7 / 4.7.1 / 4.7.2 / 4.8 / 4.8.1) and includes a net461 target; the .NETFramework build depends on System.Memory (>= 4.5.5) and System.Runtime.CompilerServices.Unsafe (>= 6.0.0). [1]
Sources:
[1] NuGet — System.Collections.Immutable 8.0.0 (package metadata).
🌐 Web query:
MessagePack 3.1.4 target frameworks requirements
💡 Result:
MessagePack 3.1.4 (NuGet) — target framework requirements (summary)
- Provided/targeted: .NET 8.0, .NETStandard 2.0, .NETStandard 2.1, and .NET Framework 4.7.2.
- Compatible (computed) frameworks include many .NET Core/.NET and framework TFMs (e.g., net5.0–net10.0, netcoreapp2.x–3.x, net6/net7/net8/net9, netcoreapp, netstandard, netframework variants).
- Included package asset TFMs and package dependencies by TFM (examples):
- .NETFramework 4.7.2 (assets include MessagePack.Annotations, MessagePackAnalyzer, Microsoft.Bcl.AsyncInterfaces, System.Memory, System.Runtime.CompilerServices.Unsafe, etc.).
- .NETStandard 2.0 (assets include MessagePack.Annotations, MessagePackAnalyzer, Microsoft.Bcl.AsyncInterfaces, System.Reflection.Emit, System.Memory, Unsafe, etc.).
- .NETStandard 2.1 (assets include MessagePack.Annotations, MessagePackAnalyzer, Microsoft.NET.StringTools, System.Collections.Immutable).
- net8.0 / net9.0 (assets include MessagePack.Annotations, MessagePackAnalyzer, Microsoft.NET.StringTools).
Source: NuGet package metadata for MessagePack 3.1.4. [1]
References
[1] NuGet — MessagePack 3.1.4 package metadata (target frameworks, included assets and dependencies). [NuGet package page].
🏁 Script executed:
#!/bin/bash
# Check the TargetFramework property in the .csproj file
grep -n "TargetFramework" src/IO.Ably.NETFramework/IO.Ably.NETFramework.csproj | head -5Length of output: 96
🏁 Script executed:
#!/bin/bash
# Also check for any PropertyGroup with framework version
grep -n -A2 "PropertyGroup" src/IO.Ably.NETFramework/IO.Ably.NETFramework.csproj | grep -i "target\|framework" | head -10Length of output: 237
CRITICAL: MessagePack 3.1.4 targets .NET Framework 4.7.2, incompatible with project's 4.6.2 target.
MessagePack 3.1.4 explicitly targets .NET Framework 4.7.2, but this project targets .NET Framework 4.6.2 (line 13: <TargetFrameworkVersion>v4.6.2</TargetFrameworkVersion>). While individual transitive dependencies like System.Runtime.CompilerServices.Unsafe 6.0.0 and System.Collections.Immutable 8.0.0 are compatible with .NET Framework 4.6.2, the primary package is incompatible.
This change cannot proceed without either:
- Upgrading the project's target framework to 4.7.2 or higher
- Using a compatible version of MessagePack that supports 4.6.2
github-actions
bot
temporarily deployed
to
staging/pull/1311/features
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
nuget/io.ably.nuspec(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: check (net7.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net6.0)
- GitHub Check: check
- GitHub Check: check (net9.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net7.0)
| <dependencies> | ||
| <group targetFramework="net46"> | ||
| <dependency id="System.Threading.Channels" version="4.6.0" /> | ||
| <dependency id="MessagePack" version="3.1.4" /> |
There was a problem hiding this comment.
🧩 Analysis chain
Verify MessagePack version 3.1.4 validity, security, and compatibility; consider using a version range.
The hard-pinned version "3.1.4" prevents consumers from automatically receiving patch updates or security fixes. Additionally, compatibility with older frameworks (net46, monoandroid, monotouch) and mobile platforms should be verified.
Consider using a version range instead of an exact version, such as [3.1.4, 4.0.0) to allow patch updates while preventing breaking changes.
Run the following script to verify the package version and check for security advisories:
Additionally, search the web for the latest MessagePack-CSharp documentation and compatibility information:
Also applies to: 27-27, 31-31, 35-35, 39-39, 44-44, 49-49
🏁 Script executed:
#!/bin/bash
# Description: Verify MessagePack version 3.1.4 exists, check for latest version, and verify security advisories.
# Check NuGet for MessagePack versions
echo "=== Checking NuGet for MessagePack versions ==="
curl -s "https://api.nuget.org/v3-flatcontainer/messagepack/index.json" | jq '.versions' | grep -E '"3\.[0-9]+\.[0-9]+"' | tail -10
# Get package metadata for version 3.1.4
echo -e "\n=== Checking if version 3.1.4 exists ==="
curl -s "https://api.nuget.org/v3-flatcontainer/messagepack/3.1.4/messagepack.nuspec" | head -20
# Check supported frameworks for 3.1.4
echo -e "\n=== Checking supported frameworks ==="
curl -s "https://api.nuget.org/v3/registration5-gz-semver2/messagepack/3.1.4.json" | gunzip 2>/dev/null | jq -r '.catalogEntry.dependencyGroups[].targetFramework' 2>/dev/null || echo "Could not retrieve framework info"
# Check GitHub security advisories
echo -e "\n=== Checking for security advisories ==="
gh api graphql -f query='
{
securityVulnerabilities(first: 10, ecosystem: NUGET, package: "MessagePack") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Length of output: 2816
🌐 Web query:
MessagePack-CSharp 3.1.4 compatibility with .NET Framework 4.6 and Xamarin
💡 Result:
Short answer: MessagePack-CSharp 3.1.4 targets .NETStandard 2.0 (and explicitly .NETFramework 4.7.2) and is compatible with Xamarin platforms. That means it requires a runtime that supports .NET Standard 2.0 — i.e. .NET Framework 4.6.1 or later — so it will NOT work on plain .NET Framework 4.0–4.6.0 without upgrading. NuGet also lists Xamarin (iOS/Mac/Android/TVOS/WatchOS) as compatible. [1][2]
If you must stay on .NET Framework 4.6.0, either upgrade the project to ≥4.6.1 (recommended) or use an older MessagePack release that supports that runtime. [1][3]
Sources:
- MessagePack 3.1.4 NuGet details (targets: .NETStandard 2.0, .NETFramework 4.7.2; compatibility info includes Xamarin). [1]
- MessagePack 3.1.4 NuGet package page (framework/compatibility listing). [2]
- MessagePack-CSharp GitHub releases. [3]
Remove MessagePack 3.1.4 from the net46 target framework group; it requires .NET Framework 4.6.1+.
MessagePack 3.1.4 is valid and not affected by known security vulnerabilities. However, there is a critical compatibility issue: MessagePack 3.1.4 explicitly requires .NET Standard 2.0 support, which means .NET Framework 4.6.1 or later. The net46 target framework (4.6.0) falls below this minimum requirement and will fail to resolve this dependency.
Fix: Either remove the dependency from the net46 group (line 23) or upgrade that framework to net461. The dependency is compatible with Xamarin platforms and modern frameworks (net6.0, net7.0).
🤖 Prompt for AI Agents
In nuget/io.ably.nuspec around line 23, the net46 target group declares a
MessagePack 3.1.4 dependency which requires .NET Framework 4.6.1+; either remove
the <dependency id="MessagePack" version="3.1.4" /> entry from the net46 target
group or change that target framework to net461 so the dependency can be
resolved; update the nuspec accordingly and ensure the package metadata and CI
builds still pass after the change.
There was a problem hiding this comment.
Okay, since MessagePack 3.1.4 supports netstandard2.0, we should be able to use it here right?
Dependency resolution mechanism should automatically take care of resolving relevant dependencies based on netstandard2.0 right
There was a problem hiding this comment.
@sacOO7, unfortunately no—the dependency resolution mechanism will not automatically resolve this because .NET Framework 4.6.0 does not support .NET Standard 2.0.
Here's why:
The compatibility issue:
net46refers to .NET Framework 4.6.0- .NET Standard 2.0 support was introduced in .NET Framework 4.6.1
- MessagePack 3.1.4 targets .NET Standard 2.0, which means it requires .NET Framework 4.6.1 minimum
What will happen:
When users try to install your package targeting net46, NuGet will fail to resolve the MessagePack 3.1.4 dependency because the framework version is incompatible. The dependency resolver won't "fall back" to a compatible version automatically.
Solutions:
-
Upgrade the target framework (recommended):
<group targetFramework="net461"> <dependency id="System.Threading.Channels" version="4.6.0" /> <dependency id="MessagePack" version="3.1.4" /> </group>
-
Remove MessagePack from the net46 group (if you must support .NET Framework 4.6.0):
- Keep the
net46group without MessagePack - Add a separate
net461group with MessagePack - This means users on .NET Framework 4.6.0 won't have MsgPack support
- Keep the
Additional note:
The other target frameworks in your nuspec are fine:
monoandroid,Xamarin.iOS, andmonotouchall support .NET Standard 2.0netstandard2.0,net6.0, andnet7.0obviously support it
I recommend upgrading to net461 since .NET Framework 4.6.0 is quite old (released 2015) and most users should be on 4.6.1+ by now.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
unity/Assets/Tests/AblySandbox/AblySandboxFixture.cs (1)
17-17: Consider making the fieldreadonly.Since
sandboxJsonProtocolis initialized once and never reassigned, marking it asreadonlywould enforce immutability and make the intent clearer.Apply this diff:
- private static Protocol sandboxJsonProtocol = Protocol.Json; + private static readonly Protocol sandboxJsonProtocol = Protocol.Json;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/IO.Ably.Shared/Transport/MsWebSocketTransport.cs(1 hunks)unity/Assets/Tests/AblySandbox/AblySandboxFixture.cs(3 hunks)unity/Assets/Tests/EditMode/AblyRealtimeSpecs.cs(1 hunks)unity/Assets/Tests/EditMode/AuthSpecs.cs(11 hunks)unity/Assets/Tests/PlayMode/AblyRealtimeSpecs.cs(1 hunks)unity/Assets/Tests/PlayMode/AuthSpecs.cs(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/IO.Ably.Shared/Transport/MsWebSocketTransport.cs
🧰 Additional context used
🧬 Code graph analysis (3)
unity/Assets/Tests/AblySandbox/AblySandboxFixture.cs (1)
src/IO.Ably.Shared/Utils/StringUtils.cs (1)
GetBytes(16-19)
unity/Assets/Tests/EditMode/AuthSpecs.cs (1)
src/IO.Ably.Shared/Protocol.cs (1)
IsBinary(29-32)
unity/Assets/Tests/PlayMode/AuthSpecs.cs (1)
src/IO.Ably.Shared/Protocol.cs (1)
IsBinary(29-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: check (net8.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net7.0)
- GitHub Check: check
- GitHub Check: check (net6.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net9.0)
🔇 Additional comments (8)
unity/Assets/Tests/PlayMode/AblyRealtimeSpecs.cs (1)
220-244: Constructor update correctly propagates protocol into AblyRequestPassing the
protocolargument into the newAblyRequest("/test", HttpMethod.Get, protocol)signature keeps this test aligned with the API change and ensures future runs (when_protocolsincludes more than JSON) execute the request under the correct protocol without impacting the Ably-Agent header assertions.unity/Assets/Tests/EditMode/AuthSpecs.cs (2)
376-376: LGTM! Proper async/await pattern.The change from fire-and-forget to awaiting
SetServerTime()ensures the server time is fetched and set before the test proceeds to validate token expiry behavior. This makes the test more deterministic.
732-732: LGTM! Clean abstraction for protocol determination.The migration to
protocol.IsBinary()provides a cleaner, more maintainable abstraction than direct protocol comparison. The extension method clearly expresses intent and centralizes the binary protocol check logic.Also applies to: 757-757, 778-778, 803-803, 829-829, 855-855, 883-883, 908-908, 941-941, 967-967
unity/Assets/Tests/EditMode/AblyRealtimeSpecs.cs (2)
220-220: LGTM!The addition of the
protocolparameter to theAblyRequestconstructor correctly aligns with the protocol-aware request construction changes introduced in this PR.
43-43: ****All Unity test spec files consistently use
Protocol[] _protocols = { Protocol.Json }only—including EditMode and PlayMode variants of AblyRealtimeSpecs, AuthSpecs, and StatsSpecs. This is an intentional architectural pattern, not an oversight. MsgPack testing is handled conditionally at the transport layer (TestTransportFactory) based on theUseBinaryProtocolparameter, which aligns with Ably's spec guideline allowing protocol selection "if supported" in a given environment.Likely an incorrect or invalid review comment.
unity/Assets/Tests/AblySandbox/AblySandboxFixture.cs (1)
119-120: Same redundantProtocolproperty assignment as line 56.Similar to the issue at lines 52-56, the constructor at line 119 receives
sandboxJsonProtocolas a parameter, but line 120 sets theProtocolproperty again. Verify if this is necessary.unity/Assets/Tests/PlayMode/AuthSpecs.cs (2)
376-383: AwaitingSetServerTimeimproves test determinismAwaiting
((AblyAuth) mainClient.Auth).SetServerTime()ensures server time has actually been obtained beforeStatsAsync()runs, removing the previous fire‑and‑forget race and making this test much less flaky. No issues spotted with this change.
730-736: Usingprotocol.IsBinary()forUseBinaryProtocolkeeps tests aligned with runtime protocol selectionBasing
UseBinaryProtocolonprotocol.IsBinary()in all these client constructions cleanly ties the wire format to the protocol under test and matches the sharedProtocol.IsBinaryhelper semantics. This keeps tests in sync with the runtime protocol selection logic and avoids hard‑coding JSON vs MsgPack here.Also applies to: 755-761, 776-782, 801-807, 827-833, 853-859, 881-887, 940-946, 965-971
| var request = new AblyRequest("/apps", HttpMethod.Post, sandboxJsonProtocol); | ||
| request.Headers.Add("Accept", "application/json"); | ||
| request.Headers.Add("Content-Type", "application/json"); | ||
| request.RequestBody = testAppSpec["post_apps"].ToString().GetBytes(); | ||
| request.Protocol = Protocol.Json; | ||
| request.Protocol = sandboxJsonProtocol; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify if setting Protocol property is necessary after passing it to the constructor.
The AblyRequest constructor at line 52 receives sandboxJsonProtocol as a parameter, but line 56 sets the Protocol property again to the same value. This appears redundant if the constructor already assigns the protocol internally.
Run the following script to check the AblyRequest constructor implementation:
🏁 Script executed:
#!/bin/bash
# Description: Check if AblyRequest constructor sets the Protocol property from the parameter.
# Find AblyRequest class definition and constructor
ast-grep --pattern $'class AblyRequest {
$$$
}'
# Also search for the constructor signature
rg -nP --type=cs -A 10 'class AblyRequest' | rg -A 10 'AblyRequest\s*\([^)]*Protocol'Length of output: 617
Need to see the complete constructor body to confirm whether the Protocol property is actually assigned from the parameter.
Let me get the complete constructor implementation:
🏁 Script executed:
#!/bin/bash
# Get the full AblyRequest constructor implementation
rg -nP --type=cs -A 30 'public AblyRequest\(string path, HttpMethod method, Protocol protocol\)' src/IO.Ably.Shared/Http/AblyRequest.csLength of output: 78
Let me locate the AblyRequest.cs file first:
🏁 Script executed:
#!/bin/bash
# Find AblyRequest.cs file
fd -name "AblyRequest.cs" --type fLength of output: 230
Let me search within the repository with proper cloning:
</parameter>
</invoke>
🏁 Script executed:
#!/bin/bash
# Find and display the full AblyRequest constructor
fd -name "AblyRequest.cs" --type fLength of output: 230
Let me use the file path from the previous successful output:
</parameter>
</invoke>
🏁 Script executed:
#!/bin/bash
# Display the full AblyRequest class using the path from previous output
cat src/IO.Ably.Shared/Http/AblyRequest.cs | head -50Length of output: 1455
Remove redundant Protocol property assignment at line 56.
The AblyRequest constructor already assigns the Protocol property from the parameter (line 18 of AblyRequest.cs), so setting it again at line 56 is unnecessary. Delete the redundant line.
🤖 Prompt for AI Agents
In unity/Assets/Tests/AblySandbox/AblySandboxFixture.cs around lines 52 to 56,
remove the redundant assignment "request.Protocol = sandboxJsonProtocol;" (line
56) because the AblyRequest constructor already sets Protocol from its
parameter; delete that line and keep the rest of the request setup unchanged.
Summary by CodeRabbit
New Features
Improvements
Tests
Removed
✏️ Tip: You can customize this high-level summary in your review settings.