[SEA-NodeJS] Kernel-parity batch: mTLS + custom headers/UA + retry/backoff + operation-status fields#420
Open
msrathore-db wants to merge 12 commits into
Open
[SEA-NodeJS] Kernel-parity batch: mTLS + custom headers/UA + retry/backoff + operation-status fields#420msrathore-db wants to merge 12 commits into
msrathore-db wants to merge 12 commits into
Conversation
msrathore-db
force-pushed
the
msrathore/sea-mtls-headers-useragent
branch
from
68dbb3d to
1607ce0
Compare
msrathore-db
force-pushed
the
msrathore/sea-mtls-headers-useragent
branch
from
1607ce0 to
b0cf092
Compare
msrathore-db
force-pushed
the
msrathore/sea-mtls-headers-useragent
branch
from
b0cf092 to
cdcf766
Compare
msrathore-db
force-pushed
the
msrathore/sea-mtls-headers-useragent
branch
from
cdcf766 to
84924c9
Compare
Wire the SEA/kernel path's remaining TLS-adjacent connection options
through to the napi binding, matching the Python connector's use_kernel
path (session.py + backend/kernel/client.py):
- mTLS client identity: `clientCertPem` / `clientKeyPem` (PEM string or
Buffer), normalised to Buffers and routed to the kernel
`TlsConfig::client_cert_pem` / `client_key_pem`. Both-or-neither
enforced up front with an actionable error.
- Independent hostname-verify toggle: `checkServerCertificateHostname`
(kernel `skip_hostname_verification`) for full parity with Python's
`tls_verify_hostname` — skip only the hostname check while still
validating the chain. The master `checkServerCertificate=false` still
subsumes it.
- Custom HTTP headers + User-Agent: headers cross the FFI as an ordered
list (`Array<{name,value}>`, the napi `HeaderEntry` shape matching the
kernel core `Vec<(String,String)>` and Python's `List[Tuple]`): caller
`customHeaders` first, then the connector's composed `User-Agent`
appended last (always emitted; the kernel folds the last User-Agent into
its base `DatabricksJDBCDriverOSS/...` UA). Kernel-managed reserved names
`Authorization` / `x-databricks-org-id` are dropped before the FFI hop,
matching Python's `_KERNEL_MANAGED_HEADERS` double-wall.
Adds `buildSeaHttpOptions`, extends `buildSeaTlsOptions`/`SeaTlsOptions`,
and factors PEM normalisation into a shared helper. Bumps KERNEL_REV and
regenerates `native/sea/index.d.ts`. Unit tests cover mTLS
pairing/validation, the hostname toggle, ordered header pass-through,
reserved-name dropping, and User-Agent composition/ordering; verified the
real native binding marshals every new field across the FFI and rejects a
wrong header shape.
Depends on the kernel napi change exposing clientCertPem / clientKeyPem /
customHeaders / checkServerCertificateHostname; KERNEL_REV must be
repointed to that commit once merged.
Co-authored-by: Isaac
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
msrathore-db
force-pushed
the
msrathore/sea-mtls-headers-useragent
branch
from
84924c9 to
cfe3b3f
Compare
Kernel #126 (logging bridge) and #127 (mTLS identity + custom HTTP headers) are both merged to kernel main. Pin KERNEL_REV to the unified main SHA 80b68e1eef3b613910183a50dfa4dace854d50dd and regenerate native/sea/index.* from it. The contract now carries both feature surfaces (gains the logging exports from #126). Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
Same alignment as the logging PR: kernel #131/#135 renamed the published napi package @databricks/sql-kernel -> @databricks/databricks-sql-kernel. Update the packaging test, version-test hint, SeaNativeLoader install hint, and README to match the regenerated router, fixing the native-packaging unit tests under the KERNEL_REV bump to kernel main 80b68e1. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
The kernel owns the retry loop on the SEA/use_kernel path, so forward the driver's existing ClientConfig retry knobs (the same ones the Thrift HttpRetryPolicy reads) onto the napi ConnectionOptions retry kwargs — keeping SEA and Thrift governed by one retry config. Mirrors Python connector #820. - buildSeaRetryOptions(config): ms -> whole seconds, clamped to napi u32. retryDelayMin->retryMinWaitSecs, retryDelayMax->retryMaxWaitSecs, retriesTimeout->retryOverallTimeoutSecs, retryMaxAttempts passes through as a TOTAL attempt count (the kernel converts to retries-after-first). - SeaBackend.connect() merges it into the native options from the client config. - Adds SeaSessionDefaults retry fields + unit tests (mapping, rounding, clamp). Requires kernel napi retry kwargs (databricks-sql-kernel #141). KERNEL_REV is pinned to #141's branch HEAD as a placeholder — MUST be re-pinned to #141's squash-merge SHA before this merges (orphan-SHA risk otherwise). Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
…o msrathore/sea-mtls-headers-useragent Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com> # Conflicts: # tests/unit/sea/connectionOptions.test.ts
…essage, diagnosticInfo, errorDetailsJson) Ports the async rich-status work (was #422) onto the consolidated branch: the napi Statement.status() fields the kernel already exposes are now surfaced through getOperationStatus instead of a flat Succeeded (M1 item). Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
Consolidation fixups: - buildSeaRetryOptions now OMITS any knob the client config didn't set to a finite number (was emitting NaN across the FFI when getConfig() lacked retry fields, e.g. the fake test context). Finite negatives still clamp to 0. - Repair the brace balance in connectionOptions.test.ts after merging the mTLS and retry test blocks. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
msrathore-db
changed the title
…Info/errorDetailsJson The napi error envelope already emits displayMessage / diagnosticInfo / errorDetailsJson, but SeaErrorMapping.buildKernelMetadata dropped them. Surface them under kernelMetadata so SEA stays at parity with: - Thrift, which carries the same data via OperationStateError.response (TGetOperationStatusResp.displayMessage / .errorMessage / .errorDetailsJson) - the Python use_kernel connector, which forwards all of them onto its exceptions Also make the kernelMetadata attach-guard a key-count check so future fields are covered automatically. Adds unit tests incl. explicit Thrift-parity assertions. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
…tionOptions test Fixes the failing lint job (prettier --check) on the two files it flagged. Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
Two driver-side parameter-binding bugs surfaced by the SEA parity suite
(the kernel binds (value_str, sql_type) faithfully — the defects are in
how DBSQLParameter stringifies/types the value):
- A JS `number` that is whole but outside the INT (i32) range was typed
INTEGER, so e.g. `1e30` was rejected by the server as
`invalid INT literal "1e+30"`. inferNumberType now picks the narrowest
fitting type: INTEGER within i32, BIGINT within the safe-integer range,
DOUBLE otherwise (and for non-integers).
- A JS `Date` bound with an explicit DATE type was stringified with the
full ISO-8601 timestamp (`...T00:00:00.000Z`), which the SEA wire
rejects as a DATE literal ("trailing input"). It now projects the
calendar date (`yyyy-mm-dd`); the no-explicit-type path still binds a
Date as a full TIMESTAMP.
Both paths are shared by the Thrift and SEA backends; the changes only
affect values that the previous logic mis-typed (and which the server
rejected), so existing behaviour is preserved. Unit tests added for the
magnitude-based integer inference and the DATE projection.
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
The shared ArrowResultConverter coerces native Arrow Decimal128 to an IEEE-754 double (`Number(unscaled)/10**scale`) and Int64 to a JS `number`. On the Thrift backend that loss is avoidable: with `useArrowNativeTypes=false` the server ships DECIMAL as a Utf8 string (passed through untouched), so Thrift returns the exact value by default. SEA has no such escape hatch — the kernel always delivers native Arrow Decimal128 / Int64 — so high-precision DECIMALs were truncated (e.g. `123456789012345.6789` → `123456789012345.67`) and 64-bit integers past 2^53 lost their low digits. Add an opt-in `preserveBigNumericPrecision` flag on ArrowResultConverter that renders DECIMAL as an exact string (via `bigNumDecimalToString`) and keeps BIGINT as a `bigint`. The flag also short-circuits the second-pass `convertThriftValue` narrowing (DECIMAL_TYPE → `Number`, BIGINT_TYPE → `convertBigInt`) for those columns. The SEA operation backend enables it; the Thrift backend leaves it off, so its long-standing `number` representation is unchanged. This brings SEA to parity with Thrift's default precise output. Unit tests: exact decimal-string formatting (incl. negatives, leading zero, scale 0) and the BIGINT preserve-vs-default behaviour. Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Consolidated PR landing every kernel-supported capability that was missing on the Node SEA path — the items the kernel core (and the Python connector) already support but the Node driver did not yet wire. Folds in the previously-separate #427 and #422.
Features
clientCertPem/clientKeyPem,customHeaders,userAgentEntry→ kernelTlsConfig/HttpConfig.custom_headers. Kernel: ✅ · Python: ✅ #819/#823.buildSeaRetryOptionsmaps the driver'sClientConfigretry knobs (ms) → kernelretry{Min,Max}WaitSecs/retryMaxAttempts/retryOverallTimeoutSecs(whole secs), merged inSeaBackend.connect(). Unset knobs are omitted (kernel default stands; no NaN across the FFI). Kernel: ✅ (napi kwargs added in kernel Patch-Package security vulnerability #141) · Python: ✅ #820.numModifiedRows/displayMessage/diagnosticInfo/errorDetailsJsonsurfaced throughgetOperationStatusinstead of a flatSucceeded(the M1 item inSeaOperationLifecycle). Kernel: ✅ · Python: ✅ #825.Plus the napi package-name alignment to
@databricks/databricks-sql-kernel-*(kernel #131/#135).Verification
KERNEL_REVregenerated against the unified kernel SHA carrying mTLS + retry.KERNEL_REVis pinned to #141's branch HEAD (fcc459b) as a placeholder — it carries the napi retry kwargs. Re-pin to #141's squash-merge SHA before merge (orphan-SHA risk otherwise). Kernel #141 is armed for auto-merge.Supersedes
This pull request and its description were written by Isaac.