Fix connecting via SenderOptions.addr #68
Conversation
After the multi-url change, when configuring the address to connect to by setting SenderOptions.addr rather than by passing a config string, the configured value is ignored. Fix by making addr return the first value in addresses (as it is documented to do).
📝 WalkthroughWalkthroughThis PR fixes a regression bug where Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
| // duplicate (non-addr) keys are 'last writer wins' | ||
| Assert.That( | ||
| new SenderOptions("http::addr=localhost:9000;addr=localhost:9009;").addr, | ||
| Is.EqualTo("localhost:9009")); | ||
| new SenderOptions("http::auth_timeout=15000;auth_timeout=8000;").auth_timeout, |
There was a problem hiding this comment.
This test is at odds with how addr is documented ("When multiple addresses are configured, this returns the first one."). So either the test needs to change or addr does. Went with the former
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/net-questdb-client/Utils/SenderOptions.cs`:
- Around line 59-60: SenderOptions (record) has a mutable List<string>
_addresses which is shallow-copied by the compiler-generated copy constructor
and causes aliasing when using with-expressions (affecting addr setter and
WithClientCert); fix by adding a custom copy constructor for SenderOptions that
deep-clones _addresses (e.g., create a new List<string> from other._addresses)
so copies get independent lists, and ensure any methods/properties that mutate
_addresses (addr property setter) operate on the instance's private list;
alternatively, replace the field with an immutable collection type (e.g.,
ImmutableList<string>) and change addr setter to reassign a new immutable list
instead of calling _addresses.Clear(), then adjust WithClientCert to rely on the
safe copy semantics.
🧹 Nitpick comments (2)
src/net-questdb-client/Utils/SenderOptions.cs (2)
162-167:addrsetter does not guard againstnull.
_addresses.Add(value)will silently acceptnullsince the parameter type isstring(non-nullable reference type), but there's no runtime enforcement. Anulladdr would later causeNullReferenceExceptioninHost,Port, and anywhere else that dereferencesaddr. Consider adding anArgumentNullExceptionguard.🛡️ Proposed null guard
set { + ArgumentNullException.ThrowIfNull(value); _addresses.Clear(); _addresses.Add(value); }
600-611: Address values containing=are silently dropped.Line 607 splits on
=withRemoveEmptyEntries, so a param likeaddr=host=fooyields 3 parts, fails thekvp.Length == 2check, and the address is silently ignored. While unlikely for addresses, this is inconsistent with howDbConnectionStringBuilderhandles values (it only splits on the first=). Consider usingSplit('=', 2, ...)to limit the split count.♻️ Proposed fix
- var kvp = param.Split('=', StringSplitOptions.TrimEntries | StringSplitOptions.RemoveEmptyEntries); - if (kvp.Length == 2 && kvp[0].Equals("addr", StringComparison.OrdinalIgnoreCase)) + var kvp = param.Split('=', 2, StringSplitOptions.TrimEntries); + if (kvp.Length == 2 && kvp[0].Equals("addr", StringComparison.OrdinalIgnoreCase) && !string.IsNullOrEmpty(kvp[1]))
| private const string DefaultAddress = "localhost:9000"; | ||
| private readonly List<string> _addresses = new(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the SenderOptions.cs file
fd -t f "SenderOptions.cs" --type fRepository: questdb/net-questdb-client
Length of output: 115
🏁 Script executed:
# Get the file size and structure
wc -l src/net-questdb-client/Utils/SenderOptions.csRepository: questdb/net-questdb-client
Length of output: 119
🏁 Script executed:
# Check the record declaration and key lines
sed -n '1,70p' src/net-questdb-client/Utils/SenderOptions.csRepository: questdb/net-questdb-client
Length of output: 2817
🏁 Script executed:
# Check the addr property (around lines 162-167)
sed -n '160,175p' src/net-questdb-client/Utils/SenderOptions.csRepository: questdb/net-questdb-client
Length of output: 529
🏁 Script executed:
# Check the WithClientCert method (around line 731)
sed -n '725,740p' src/net-questdb-client/Utils/SenderOptions.csRepository: questdb/net-questdb-client
Length of output: 380
🏁 Script executed:
# Search for all 'with' expressions in C# files
rg -nP --type=cs '\bwith\s*\{' --glob '!**/obj/**' --glob '!**/bin/**'Repository: questdb/net-questdb-client
Length of output: 214
🏁 Script executed:
# Check how many lines the file has and look at the end for any relevant code
tail -20 src/net-questdb-client/Utils/SenderOptions.csRepository: questdb/net-questdb-client
Length of output: 525
🏁 Script executed:
# Verify field initializer behavior in records and check if there's any evidence of actual mutations
rg -nP --type=cs '_addresses' src/net-questdb-client/Utils/SenderOptions.cs -B2 -A2Repository: questdb/net-questdb-client
Length of output: 1477
🏁 Script executed:
# Check if there are usages of WithClientCert in tests or elsewhere that might expose the bug
rg -nP --type=cs 'WithClientCert' --glob '!**/obj/**' --glob '!**/bin/**' -B3 -A3Repository: questdb/net-questdb-client
Length of output: 1297
🏁 Script executed:
# Check what tests exist and if they might be hitting this aliasing issue
rg -nP --type=cs '\.addr\s*=' --glob '!**/obj/**' --glob '!**/bin/**' -B2 -A2Repository: questdb/net-questdb-client
Length of output: 520
Mutable List<string> field in record creates aliasing hazard with with expressions.
SenderOptions is a record type (line 43), so the compiler-synthesized copy constructor performs shallow copy of all fields. This means any instance created via with expression (e.g., WithClientCert at line 731) shares the same _addresses list reference. If the original or copy subsequently sets addr property, the setter's _addresses.Clear() call (line 165) mutates the shared list, corrupting the other instance's state.
Applies to lines 60 (field declaration), 162-167 (addr property), and 729-733 (WithClientCert).
Recommended fixes:
- Clone
_addressesin a custom copy constructor. - Replace
List<string>withImmutableList<string>and update the setter to reassign instead of mutate. - Change
SenderOptionsfromrecordtoclassif value semantics are not required.
🤖 Prompt for AI Agents
In `@src/net-questdb-client/Utils/SenderOptions.cs` around lines 59 - 60,
SenderOptions (record) has a mutable List<string> _addresses which is
shallow-copied by the compiler-generated copy constructor and causes aliasing
when using with-expressions (affecting addr setter and WithClientCert); fix by
adding a custom copy constructor for SenderOptions that deep-clones _addresses
(e.g., create a new List<string> from other._addresses) so copies get
independent lists, and ensure any methods/properties that mutate _addresses
(addr property setter) operate on the instance's private list; alternatively,
replace the field with an immutable collection type (e.g.,
ImmutableList<string>) and change addr setter to reassign a new immutable list
instead of calling _addresses.Clear(), then adjust WithClientCert to rely on the
safe copy semantics.
1 participant
After the multi-url change, when configuring the address to connect to by setting
SenderOptions.addrrather than by passing a config string, the configured value is ignored.Fix by making
addrreturn the first value inaddresses(as it is documented to do).closes #63
Summary by CodeRabbit
New Features
Bug Fixes