fix: X-API-Key guard, ADBC connection lifetime and mTLS, and 64-bit result precision - #77
Open
lukekim wants to merge 9 commits into
Open
fix: X-API-Key guard, ADBC connection lifetime and mTLS, and 64-bit result precision#77lukekim wants to merge 9 commits into
lukekim wants to merge 9 commits into
Conversation
Wraps POST /v1/search, which was previously unreachable from Go without hand-rolling the HTTP call. spice.js is the only other SDK that exposes it. Only Text is required; Datasets, Limit, Where, AdditionalColumns and Keywords are optional. Supplying Keywords pre-filters the embedding column with a lexical search before the vector search, making the search hybrid. Decodes the runtime's wire format directly, including the `_score` field name and the data/primary_key/metadata objects the runtime omits when empty.
The error path only unpacked a JSON {"error": ...} body and otherwise reported
a bare "failed with status=400", dropping the part that says what to fix — for
example "Search cannot be run on X because it has no embeddings or full text
search indexes", which the runtime returns as plain text.
Caught by running the new search path against a live runtime.
There was a problem hiding this comment.
Pull request overview
Adds first-class Go SDK support for the runtime’s POST /v1/search endpoint by introducing SpiceClient.Search plus request/response types, tests, and README documentation so Go users can perform vector, keyword, and hybrid search without hand-rolling HTTP.
Changes:
- Introduces
SearchRequest,SearchResponse, andSearchMatchwire-format types andSpiceClient.Searchimplementation. - Adds unit tests covering request encoding, method/path, response decoding (including omitted objects), validation, and runtime error surfacing.
- Documents
Searchusage and behavior in the README, including hybrid search viaKeywords.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| search.go | Adds SpiceClient.Search and associated request/response types plus runtime error-message handling. |
| search_test.go | Adds httptest-based unit tests for encoding/decoding, validation, and error surfacing. |
| README.md | Documents the new Search API with examples and field behavior notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…gistry `spice add spiceai/quickstart` makes every run depend on the Spicepod registry. That service is currently returning a body the CLI cannot unpack, so every job fails at: Invalid argument: Failed to extract Spicepod archive: invalid Zip archive: Could not find EOCD The runtime and the SDK are fine; only the fetch is broken. Declaring the same dataset spiceai/quickstart provides removes the dependency. Applies to both the Unix and WSL setup steps. Verified locally: taxi_trips loads and is queryable within seconds (2,964,624 rows). Mirrors spicepy#176.
…' into merge/ci-fix
An empty X-API-Key reads as a supplied-but-invalid credential to auth middleware, which behaves differently from omitting the header. IsSpiceReady already guards this; Search now matches. Addresses review feedback on #77.
3 tasks
Contributor
|
Heads-up: this PR looks superseded, and that is very likely the source of its conflict. #79 ("feat: add Search for the runtime's /v1/search endpoint") merged on 2026-08-01 and changed exactly the same four files this PR does:
So That duplication is our fault, not yours — #79 was opened by this bot on 2026-08-01 without checking for an existing open PR implementing the same feature, and yours predates it by five days. The same pair is still open in two other SDK repos (spice-rs#82 vs #84, spicepy#174 vs #177); I have flagged both there. Suggestion: this PR can probably be closed — but it is worth diffing against |
krinart
added a commit
to claudespice/gospice
that referenced
this pull request
Aug 19, 2026
An empty X-API-Key reads as a supplied-but-invalid credential to auth middleware, which behaves differently from omitting the header. Matches the fix already applied to Search in spiceai#77.
krinart
added a commit
that referenced
this pull request
Aug 19, 2026
#82) * feat: add Nsql and NsqlGenerateSQL for the runtime's /v1/nsql endpoint Text-to-SQL was reachable from spice.js but from no other SDK, so Go callers had to hand-roll the HTTP call - including knowing to ask for application/vnd.spiceai.nsql.v1+json, without which the runtime returns a bare array of rows and drops the generated SQL. Nsql runs the generated query and returns the rows alongside the SQL. NsqlGenerateSQL stops after generation, so the query can be inspected, edited, or run through Sql to get Arrow-typed results instead of decoded JSON. Adds the new tests to the CI job that runs without a live runtime. * fix: only send X-API-Key when a key is configured An empty X-API-Key reads as a supplied-but-invalid credential to auth middleware, which behaves differently from omitting the header. Matches the fix already applied to Search in #77. --------- Co-authored-by: claudespice <270518434+claudespice@users.noreply.github.com> Co-authored-by: Viktor Yershov <viktor@spice.ai>
#79 landed the same Search feature on trunk five days after this branch opened, touching the same four files, so this was an add/add conflict on search.go and search_test.go rather than a divergence to reconcile. Resolved in favour of trunk's implementation, which is the superset: it takes *int / *string for Limit and Where so an explicit zero is expressible, validates Limit >= 1, and carries a malformed-response test this branch lacked. README.md and .github/workflows/go.yml are restored to trunk exactly — the automatic merge had appended this branch's Search section as a second "## Search" heading documenting a Limit signature that no longer compiles, and had reverted the Windows CI allowlist to its pre-#79 form, dropping TestSearch, TestNsql, TestListActiveQueries and TestCancelActiveQuery. Two fixes from this branch are kept, because trunk does not have them: - Search sent X-API-Key unconditionally, so a client with no key configured sent an empty one. Auth middleware reads that as a supplied-but-invalid credential rather than as an absent one, which is the bug Copilot flagged here. client.go, nsql.go, status.go and active_queries.go all guard the header; search.go now matches them. - The runtime reports some search failures as JSON {"error": "..."} and others as plain text. Trunk printed the raw body, so a JSON failure reached the caller as an envelope instead of a message. Both are covered by new tests: TestSearchApiKeyHeader asserts the header is absent, not empty, when no key is set, and TestSearchErrorSurfacesJSONMessage covers the JSON and empty-body shapes.
RefreshDataset set the header unconditionally, so a client with no key configured sent an empty X-API-Key. Auth middleware reads that as a supplied-but-invalid credential rather than as an absent one, which is the same bug just fixed in Search: client.go, nsql.go, status.go and active_queries.go all guard the header, and these were the two callers that did not. TestRefreshDatasetApiKeyHeader asserts the header is absent, not empty, when no key is set. It needs no live runtime, so it joins the allowlist the Windows CI job runs natively.
lukekim
changed the title
Contributor
Author
|
Confirmed and resolved — thanks for catching this. Diffed against The diff was worth doing — two things this branch had did not make it into #79, so they were live gaps in
Also had to undo two things the automatic merge did quietly: it appended this branch's README section as a second Retitled and rewrote the description to match what the PR actually does now. Net diff is 5 files, +168/-6. Worth checking whether spice-rs#82 and spicepy#174 carry the same API-key guard their merged counterparts dropped — if the SDKs were written from the same template, that bug is likely live in all three. |
…ults Addresses four findings from an adversarial review. None originate in this PR — all four are live in trunk — but they are fixed here rather than left for a release to carry. Re-authentication no longer disrupts concurrent work. SqlWithParams read the connection pointer under adbcMu and then used it after releasing the lock, while reinitADBC closed that same connection under another request's failure path. One expired session could therefore kill an unrelated in-flight query, and a RecordReader already returned to a caller keeps streaming from the connection long after the call returned, so it could fail mid-iteration with a use-after-close. Connections are now leased: a replaced connection is retired rather than closed, and closes when the last query and reader using it releases. The lease travels with the returned reader, whose Retain/Release the wrapper mirrors. Covered by tests under -race, which CI now runs for them. Re-authentication no longer triggers on StatusUnauthorized. That status means the credential is recognised but lacks permission, which a fresh handshake cannot fix; retrying on it churned the connection, and after the change above would retire one, only to fail with the same error. ADBC now receives the TLS material. The FlightSQL options carried only URI, credentials and user agent, omitting the root CA and client certificate that WithTLSRootCertificate and WithTLSClientCertificate configure — so Init and every HTTP method could reach an mTLS or private-CA runtime while SqlWithParams alone failed verification. Configuring either now also forces grpc+tls://, since the material is meaningless over a plaintext connection. Option building is split out into adbcOptions so it is testable without dialing. Search and Nsql no longer round 64-bit values. Their results reach callers through interface-backed maps, and encoding/json decodes an untyped number into float64 — 53 bits of integer precision. A primary key of 9007199254740993 came back as 9007199254740992: not an error, just a plausible-looking ID naming the wrong row. Both now decode with UseNumber, so those values arrive as json.Number with their original text intact. This is a breaking change for callers asserting float64 on them, recorded in the release notes; typed fields such as Score and RowCount are unchanged. The fourth finding — that ListActiveQueries and CancelActiveQuery are unscoped on runtimes up to v2.1.5 — is a runtime gap this SDK cannot close. A client-side guard would not be a security control, since the endpoints are open to any HTTP caller regardless of what this SDK declines to do, and there is no version endpoint to gate on. The doc comments now lead with the caveat instead of trailing a sentence that says the opposite.
lukekim
changed the title
TestIsADBCAuthError still asserted that adbc.StatusUnauthorized triggers re-authentication, which the previous commit deliberately changed. The status means the credential is recognised but is not permitted to perform the operation, so a fresh handshake with the same credential cannot fix it. #73, which introduced the retry, describes only the Unauthenticated session-expiry path in its title and its rationale; Unauthorized was included alongside it without one. Now that a reconnect retires the connection for every other user of it, retrying on a permission error costs a live connection to arrive at the same failure. Also covers a PermissionDenied message reaching the string fallback.
lukekim
enabled auto-merge (squash)
August 21, 2026 08:19
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
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.
#79landed Search ontrunkon 2026-08-01, five days after this PR opened and touching the same four files, so the feature work here is superseded and has been dropped in favour of trunk's implementation. What remains are fixes — the two this branch already carried, plus four found by an adversarial review of the surrounding code.None of the four originate in this PR. All are live in
trunktoday, so they are fixed here rather than left for a release to carry.Empty
X-API-Keyis not an absent oneSearchandRefreshDatasetsetX-API-Keyunconditionally, so a client with no key configured sent an empty one. Auth middleware reads that as a supplied-but-invalid credential rather than an absent one — the behaviour gospice#70 exists to allow.client.go,nsql.go,status.goandactive_queries.goalready guard the header; these were the only two callers that did not.Re-authentication could kill an unrelated in-flight query
SqlWithParamsread the ADBC connection pointer underadbcMuand then used it after releasing the lock, whilereinitADBCclosed that same connection under a different request's failure path. One expired session could therefore terminate a concurrent query — and because aRecordReadergoes on streaming from the connection long afterSqlWithParamsreturns, an already-returned reader could fail mid-iteration with a use-after-close.Connections are now leased. A replaced connection is retired rather than closed, and closes only once the last query and reader using it has released. The lease travels with the returned reader, whose
Retain/Releasethe wrapper mirrors.Re-authentication also no longer triggers on
StatusUnauthorized. That status means the credential is recognised but lacks permission, which a fresh handshake cannot fix — retrying on it churned the connection for nothing, and after the change above would retire one only to fail with the same error.mTLS never reached
SqlWithParamsThe FlightSQL driver options carried only URI, credentials and user agent, omitting the root CA and client certificate that
WithTLSRootCertificateandWithTLSClientCertificateconfigure.Initand every HTTP method could reach an mTLS or private-CA runtime whileSqlWithParamsalone failed verification. Configuring either now also forcesgrpc+tls://, since the material is meaningless over a plaintext connection.SearchandNsqlsilently rounded 64-bit valuesTheir results reach callers through interface-backed maps, and
encoding/jsondecodes an untyped JSON number intofloat64— 53 bits of integer precision. A primary key of9007199254740993came back as9007199254740992: not an error, just a plausible-looking ID naming the wrong row. Both now decode withUseNumber.Not fixed: unscoped active-query endpoints
The review also flagged that
ListActiveQueriesandCancelActiveQueryare unscoped on every runtime release up to and includingv2.1.5, so any caller with write access sees and can cancel every principal's queries.That is real, but it is a runtime gap this SDK cannot close. A client-side guard would not be a security control — the endpoints are open to any HTTP caller regardless of what this SDK declines to do — and there is no version endpoint to gate on. Scoping landed upstream in spiceai/spiceai#12841 and needs a runtime release. The doc comments now lead with the caveat instead of trailing a sentence that says the opposite.
Tests
TestSearchApiKeyHeader/TestRefreshDatasetApiKeyHeader— header absent, not empty, with no key.TestSearchErrorSurfacesJSONMessage— JSON and empty-body error shapes.TestADBCRetireClosesOnlyWhenUnused,TestADBCLeaseIsRaceFree,TestLeasedRecordReaderHoldsConnectionOpen,TestLeasedRecordReaderBalancesRetain— connection closes only when nothing is using it.TestIsADBCAuthStatus—Unauthenticatedreconnects,Unauthorizeddoes not.TestADBCOptionsCarryTLSMaterial— CA and client cert reach the driver, TLS scheme forced.TestSearchPreservesLargeIntegers/TestNsqlPreservesLargeIntegers— 2^53+1, int64 max, uint64 max.All are runtime-free and run in the Windows CI job. CI now also runs the lease tests under
-race, which is the only thing that catches the connection-lifetime class of bug.