Skip to content

refactor(csharp): update integer properties to unsigned types for consistency - #4027

Merged
hubcio merged 13 commits into
masterfrom
dotnet-protocol
Sep 3, 2026
Merged

refactor(csharp): update integer properties to unsigned types for consistency#4027
hubcio merged 13 commits into
masterfrom
dotnet-protocol

Conversation

@lukaszzborek

Copy link
Copy Markdown
Contributor

No description provided.

@github-actions github-actions Bot added the S-waiting-on-review PR is waiting on a reviewer label Sep 1, 2026
@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.71779% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.10%. Comparing base (d3028f9) to head (0d1003f).

Files with missing lines Patch % Lines
foreign/csharp/Iggy_SDK/Mappers/BinaryMapper.cs 89.85% 16 Missing and 5 partials ⚠️
...n/csharp/Iggy_SDK/Consumers/IggyConsumer.Rented.cs 75.00% 1 Missing and 1 partial ⚠️
foreign/csharp/Iggy_SDK/Consumers/IggyConsumer.cs 84.61% 2 Missing ⚠️
...eign/csharp/Iggy_SDK/Contracts/Tcp/TcpContracts.cs 96.72% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #4027      +/-   ##
============================================
+ Coverage     85.05%   85.10%   +0.04%     
  Complexity     1402     1402              
============================================
  Files          1237     1236       -1     
  Lines        181722   181699      -23     
  Branches     148015   148003      -12     
============================================
+ Hits         154567   154629      +62     
+ Misses        23098    23047      -51     
+ Partials       4057     4023      -34     
Components Coverage Δ
Rust Core 86.11% <ø> (ø)
Java SDK 67.29% <ø> (ø)
C# SDK 76.33% <91.71%> (+0.93%) ⬆️
Python SDK 90.06% <ø> (ø)
PHP SDK 85.65% <ø> (ø)
Node SDK 94.50% <ø> (ø)
Go SDK 69.28% <ø> (ø)
Files with missing lines Coverage Δ
...ign/csharp/Iggy_SDK/Contracts/Auth/AuthResponse.cs 100.00% <100.00%> (ø)
foreign/csharp/Iggy_SDK/Contracts/ClientInfo.cs 100.00% <ø> (ø)
...reign/csharp/Iggy_SDK/Contracts/MessageResponse.cs 100.00% <100.00%> (ø)
...oreign/csharp/Iggy_SDK/Contracts/PolledMessages.cs 14.28% <100.00%> (+14.28%) ⬆️
.../csharp/Iggy_SDK/Contracts/PolledMessagesRental.cs 100.00% <ø> (ø)
...csharp/Iggy_SDK/Contracts/RentedMessageResponse.cs 100.00% <100.00%> (ø)
foreign/csharp/Iggy_SDK/Contracts/StatsResponse.cs 100.00% <ø> (ø)
...oreign/csharp/Iggy_SDK/Contracts/StreamResponse.cs 100.00% <ø> (ø)
foreign/csharp/Iggy_SDK/Identifier.cs 75.92% <100.00%> (-2.41%) ⬇️
...oreign/csharp/Iggy_SDK/IggyClient/IIggyConsumer.cs 50.00% <ø> (ø)
... and 9 more

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@hubcio hubcio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

outside the diff:

  • TcpContracts.cs:282 - stream.Topics != null has the same empty-dictionary bug you fixed for Streams: an empty Topics writes has_topics = 1 with no entries and the server rejects the request. same is { Count: > 0 } here and at line 347.
  • TcpContracts.cs:151 - UpdatePermissions(userId, null) sizes the buffer without the has-permissions byte, then writes it at line 167. index out of range on the documented default permissions = null.
  • BinaryFactory.cs:155 - CreatePartitionPayload has no callers and throws on first use (16-byte buffer, 8-byte writes at offsets 12 and 16). delete it.

Comment thread foreign/csharp/Iggy_SDK/Mappers/BinaryMapper.cs Outdated
Comment thread foreign/csharp/Iggy_SDK/Mappers/BinaryMapper.cs
Comment thread foreign/csharp/Iggy_SDK_Tests/MapperTests/BinaryMapper.cs Outdated
Comment thread foreign/csharp/Iggy_SDK/Mappers/BinaryMapper.cs
Comment thread foreign/csharp/Iggy_SDK/Mappers/BinaryMapper.cs Outdated
Comment thread foreign/csharp/Iggy_SDK/Mappers/BinaryMapper.cs Outdated
Comment thread foreign/csharp/Iggy_SDK/Kinds/Partitioning.cs
Comment thread foreign/csharp/Iggy_SDK/Mappers/BinaryMapper.cs Outdated
Comment thread foreign/csharp/Iggy_SDK/Contracts/Tcp/TcpContracts.cs Outdated
Comment thread foreign/csharp/Iggy_SDK_Tests/Utils/BinaryFactory.cs Outdated
@github-actions github-actions Bot added S-waiting-on-author PR is waiting on author response and removed S-waiting-on-review PR is waiting on a reviewer labels Sep 1, 2026
spetz
spetz previously approved these changes Sep 2, 2026

@spetz spetz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, here are 2 extra things to improve outisde the scope of this PR, so feel free to either update this PR or we can add these to the next one.

foreign/csharp/Iggy_SDK/Vsr/ConsumerGroupClientState.cs:46 - an empty cached assignment is treated as missing, causing repeated synchronization and a possible hot loop when PollingIntervalMs is 0. Consider preserving empty assignments and returning the 0xFFFF_FFFE no-assignment sentinel with backoff.

foreign/csharp/Iggy_SDK/Mappers/BinaryMapper.cs:803 - the options-length prefix is read without confirming four bytes remain, so truncated input throws ArgumentOutOfRangeException instead of MalformedResponseException. Consider reusing ReadLength.

numinnex
numinnex previously approved these changes Sep 2, 2026
@lukaszzborek
lukaszzborek dismissed stale reviews from spetz and numinnex via 3f48365 September 2, 2026 14:29
@lukaszzborek lukaszzborek added S-waiting-on-review PR is waiting on a reviewer and removed S-waiting-on-author PR is waiting on author response labels Sep 2, 2026
@lukaszzborek

Copy link
Copy Markdown
Contributor Author

2 extra comments from @spetz was also done in this PR

@hubcio hubcio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

went through all 21 comments from the last round against the code - every one is genuinely fixed, and the unit suite is green locally (1028 tests across net8.0 and net10.0, 0 build warnings). the MapHeaders rewrite onto shared ReadHeaderKind / ReadHeaderField and the ArrayBufferWriter permissions pass came out cleaner than what I asked for.

two things live outside the diff so I could not anchor them:

Iggy_SDK_Tests/VsrTests/SyncConsumerGroupTests.cs - nothing covers the sentinel this fix is built on. neither the PollGroupMessagesRentedAsync return nor the consumer backoff has a test, while go covers both in tcp_group_polling_test.go:262 and :279. the assert that actually guards the HasAssignment change is that a second poll issues no new SYNC_CONSUMER_GROUP.

Iggy_SDK/IggyClient/IIggyConsumer.cs:45 and :57 - PollMessagesAsync and PollMessagesRentedAsync are public and do not say PartitionId can come back as NO_ASSIGNED_PARTITION with an empty batch. go states it in the interface contract (contracts/client.go:128). anyone using the raw poll instead of IggyConsumer would key offsets on partition 4294967294.

Comment thread foreign/csharp/Iggy_SDK/Mappers/BinaryMapper.cs
Comment thread foreign/csharp/Iggy_SDK/Vsr/ConsumerGroupClientState.cs
Comment thread foreign/csharp/Iggy_SDK/Consumers/IggyConsumer.cs
Comment thread foreign/csharp/Iggy_SDK/Contracts/PolledMessages.cs Outdated
@github-actions github-actions Bot added S-waiting-on-author PR is waiting on author response and removed S-waiting-on-review PR is waiting on a reviewer labels Sep 2, 2026
@lukaszzborek lukaszzborek added S-waiting-on-review PR is waiting on a reviewer and removed S-waiting-on-author PR is waiting on author response labels Sep 2, 2026
@hubcio
hubcio merged commit e77ee39 into master Sep 3, 2026
57 checks passed
@hubcio
hubcio deleted the dotnet-protocol branch September 3, 2026 07:44
@github-actions github-actions Bot removed the S-waiting-on-review PR is waiting on a reviewer label Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants