feat(node): support connection string syntax for nodejs client creation - #3917
feat(node): support connection string syntax for nodejs client creation#3917T1B0 wants to merge 1 commit into
Conversation
2afce42 to
e9ffac3
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3917 +/- ##
============================================
+ Coverage 84.11% 84.13% +0.01%
Complexity 1358 1358
============================================
Files 1215 1216 +1
Lines 170001 170223 +222
Branches 137611 137662 +51
============================================
+ Hits 142995 143213 +218
- Misses 23305 23308 +3
- Partials 3701 3702 +1
🚀 New features to boost your workflow:
|
6a3b7a1 to
44edc7c
Compare
github-actions
Bot
added
S-waiting-on-author
44edc7c to
be0d997
Compare
b78121c to
3745728
Compare
3745728 to
3de15b0
Compare
hubcio
left a comment
hubcio
left a comment
There was a problem hiding this comment.
a few notes from comparing against the rust parser. one i'd want fixed before merge - the credential echo in errors.
these landed outside the diff:
foreign/node/src/stream/consumer-stream.ts:81 and :113 - singleConsumerStream and groupConsumerStream still take ClientConfig, so passing a connection string fails to compile (TS2345). they call the widened getClient, and since getClient isn't exported they're the only place users can reach the widening.
foreign/node/src/client/client.connection.ts:86-90 - reconnect timer is never unref'd and the handle isn't kept, so unlimited means the process can't exit and destroy() can't cancel a wait already in flight. the heartbeat timer next door unrefs itself with a comment saying why. pre-existing, but this PR makes unlimited a one-word option and ships it in the new e2e.
foreign/node/src/client/client.config.ts:61 - error says heartbeatInterval in raw ms at someone who typed heartbeat_interval=1000h.
nothing tests the tls branch - the pre-merge action runs tls.system.e2e.ts, which builds from an object config, so the parser never gets hit.
|
|
||
| const protocolParts = connectionString.split('://'); | ||
| if (protocolParts.length !== 2) | ||
| throw new TypeError(`invalid connection string: "${connectionString}"`); |
There was a problem hiding this comment.
every parse failure echoes the whole connection string, which holds the password or PAT in plaintext. same at 63, 72, 77, 82, 86, 92, 96, 100, 104, 148, and worded differently at 186, 200, 212. rust returns an opaque error here for that reason - drop the interpolation, or echo scheme + host:port only.
| throw new TypeError(`invalid connection string: "${connectionString}"`); | ||
|
|
||
| const serverAddress = serverAndOptions[0]; | ||
| if (serverAddress.length === 0 || |
There was a problem hiding this comment.
lastIndexOf(':') guesses wrong on multi-colon authorities - 2001:db8::1 parses as {host:"2001:db8:", port:1}, and host:8090:9090 gives port 9090 where rust reads 8090. one regex replaces the whole block: /^(?:\[([^\]]+)\]|([^:\[\]]+)):(\d+)$/ with host = m[1] ?? m[2], keeping the > 65535 check.
| parsed.servername = value; | ||
| break; | ||
| case 'tls_ca_file': | ||
| parsed.ca = readFileSync(value); |
There was a problem hiding this comment.
reads the file during parsing even when the transport ends up plain TCP, and a bad path throws a raw ENOENT Error instead of the TypeError the test asserts. rust stores the path and reads it at connect.
| case 'nodelay': | ||
| parsed.noDelay = parseBoolean(name, value, connectionString); | ||
| break; | ||
| case 'tls_domain': |
There was a problem hiding this comment.
tls=true without tls_domain sends no SNI - node won't derive servername from host, rust falls back to it. fine against iggy's own server, breaks behind an SNI-routing terminator. default it to host when net.isIP(host) === 0.
|
|
||
| /** Parses a duration such as "500ms" or "5s" into milliseconds. */ | ||
| export const parseDuration = (value: string): number => { | ||
| const match = /^(\d+(?:\.\d+)?)(ms|s|m|h)$/.exec(value); |
There was a problem hiding this comment.
strict subset of IggyDuration::from_str - 0, unlimited, disabled, none, 1d, 5sec, 1h 30m all work in rust and throw here. heartbeat_interval=0 is the documented disable and only 0ms gets through. also 1.005s gives 1004.9999999999999, which normalizeClientConfig then rejects - Math.round the product.
| throw new TypeError(`invalid connection string: "${connectionString}"`); | ||
| const host = bracketed ? bracketed[1] : rawHost; | ||
|
|
||
| const options: ParsedConnectionOptions = serverAndOptions[1] |
There was a problem hiding this comment.
truthiness means a trailing ? parses while ?& throws. rust rejects both. serverAndOptions.length === 2.
| interval: parsed.reconnect?.interval ?? 5000, | ||
| maxRetries: value === 'unlimited' | ||
| ? Number.MAX_SAFE_INTEGER | ||
| : parseNumber(name, value, connectionString) |
There was a problem hiding this comment.
no upper bound - reconnection_retries=99999999999999 is accepted where rust errors on u32 overflow. second undocumented spelling of unlimited.
|
|
||
| export { Client, SimpleClient, SingleClient } from './client.js' | ||
| export * from './client.config.js'; | ||
| export * from './client.connection-string.js'; |
There was a problem hiding this comment.
export * makes parseDuration public api. it only needs the module export for its own test - export { parseConnectionString } from './client.connection-string.js';
|
|
||
| ### Connection strings | ||
|
|
||
| Every client constructor also accepts a connection string instead of a config |
There was a problem hiding this comment.
SimpleClient takes a RawClient, so not every constructor. worth also listing the reconnect and heartbeat defaults a connection string implies, and that ipv6 literals need brackets.
| import { Client } from '../client/client.js'; | ||
| import { getIggyAddress } from '../tcp.sm.utils.js'; | ||
|
|
||
| const dummyOpt = 'nodelay=true' + |
There was a problem hiding this comment.
reconnection_retries is set twice - last wins, so =1 is dead and this actually runs unlimited. only ping() is asserted so no option is checked, and destroy() on line 39 isn't awaited.
3 participants
Which issue does this PR address?
this PR add node sdk support for connection string syntax on client creation
accept common url like syntax
new Client("iggy://iggy:iggy@127.0.0.1:8090");note
node:urlmodule was not used since it would have introduced small differencez with rust and other sdk parsing, so connection string is manually parsed to match others sdk behaviors.