SNOW-3155971: Snowflake restful implementation by sfc-gh-turbaszek · Pull Request #638 · snowflakedb/universal-driver

sfc-gh-turbaszek · 2026-03-19T16:07:31Z

No description provided.

Copilot

Pull request overview

This PR extends the DatabaseDriver v1 surface to support a backward-compatible “REST” interface used by Snowflake CLI, by exposing master_token in connection info and adding new RPCs for “generic HTTP via core TLS client” and “token-request via master token”, with corresponding Python wrapper methods and integration tests.

Changes:

Add master_token to ConnectionGetInfoResponse and plumb it through Rust ↔ protobuf ↔ Python.
Add ConnectionSendHttp and ConnectionRequestToken RPCs and implement them in sf_core.
Implement SnowflakeRestful request/fetch/token-request methods in Python and add integration coverage; update pre-commit Python hook to run via uv.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
`sf_core/src/protobuf/apis/database_driver_v1.rs`	Maps Rust connection info (now including `master_token`) into protobuf responses; adds RPC handlers that forward into the driver implementation.
`sf_core/src/apis/database_driver_v1/connection.rs`	Adds core implementations for generic HTTP requests and token-request calls using the connection’s configured HTTP client/tokens.
`protobuf/database_driver_v1.proto`	Extends the proto with `master_token` plus new HTTP/token-request messages and RPCs.
`python/src/snowflake/connector/_internal/snowflake_restful.py`	Implements the Python-side REST compatibility wrapper by calling the new RPCs.
`python/tests/integ/test_snowflake_restful.py`	Adds integration tests that mirror Snowflake CLI call patterns against `connection.rest`.
`.pre-commit-config.yaml`	Switches Python pre-commit hook to run checks via `uv`.

You can also share your feedback on Copilot code review. Take the survey.

Copilot

Pull request overview

Implements a backward-compatible “RESTful” interface for Snowflake CLI usage by extending the DatabaseDriverV1 RPC surface to (a) proxy generic HTTP calls through sf_core’s TLS-configured client and (b) support session token ISSUE/RENEW via the Snowflake /session/token-request endpoint, then wiring these capabilities into the Python connector’s connection.rest.

Changes:

Add Snowflake token-request support in sf_core and expose it via a new ConnectionRequestToken RPC.
Add a generic ConnectionSendHttp RPC to send HTTP requests using the connection’s configured HTTP/TLS client.
Expose server_url, master_token, and REST-style request/fetch helpers in Python, with new integration tests covering CLI-like call patterns.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
`sf_core/src/rest/snowflake/mod.rs`	Adds `token_request()` and `TokenRequestResult` to call `/session/token-request` for ISSUE/RENEW.
`sf_core/src/apis/database_driver_v1/connection.rs`	Adds `connection_send_http_request()` and `connection_token_request()` driver APIs; includes returning `master_token` in connection info.
`sf_core/src/apis/database_driver_v1/error.rs`	Introduces `ApiError::HttpRequest` to preserve `reqwest::Error` context.
`sf_core/src/protobuf/apis/database_driver_v1.rs`	Wires new RPC methods and maps `master_token` into `ConnectionGetInfoResponse`.
`protobuf/database_driver_v1.proto`	Extends the proto API: `master_token`, `ConnectionSendHttp`, and `ConnectionRequestToken`.
`python/src/snowflake/connector/_internal/snowflake_restful.py`	Implements `server_url`, `master_token`, `request()`, `fetch()`, and `_token_request()` using the new RPCs.
`python/tests/integ/test_snowflake_restful.py`	Adds integration tests validating REST-compat properties and request/token flows.
`.pre-commit-config.yaml`	Updates Python pre-commit hook invocation to run via `uv`.

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.

Copilot · 2026-03-30T13:22:12Z

    @property
    def _port(self) -> int | None:
-        return urlparse(self._connection_info.server_url).port or 443
+        url = self._connection_info.server_url


_port is derived only from parsing connection_info.server_url, and falls back to 443 when server_url is unset. Since ConnectionGetInfoResponse also has a dedicated port field, consider preferring that field (when present) so _port stays accurate even if server_url is temporarily unavailable.

Suggested change

url = self._connection_info.server_url

info = self._connection_info

# Prefer explicit port from connection info when available

if getattr(info, "port", None):

return info.port

# Fall back to deriving port from server_url, defaulting to 443

url = info.server_url

Copilot · 2026-03-30T13:22:13Z

+    #[snafu(display("HTTP request failed: {context}"))]
+    HttpRequest {
+        context: String,
+        #[snafu(implicit)]
+        location: Location,
+        source: reqwest::Error,
+    },


ApiError::HttpRequest's display string only includes the high-level {context} and omits the underlying reqwest::Error. This can make the top-level error message much less actionable (e.g., missing "connection refused" / "timeout" details) unless callers explicitly inspect the root cause chain. Consider including {source} in the display format so the primary error message carries the concrete failure reason.

^ valid comment

Done — changed display to "HTTP request failed: {context}: {source}" so the underlying reqwest error (timeout, connection refused, etc.) is included.

Copilot · 2026-03-30T13:22:13Z

+        host = self._host
+        if not host:
+            raise ValueError("server_url and host are both unavailable from connection info")
+        return f"https://{host}:443"


server_url fallback hard-codes https://{host}:443 when connection_info.server_url is unset, ignoring the connection's configured port/protocol (ConnectionGetInfoResponse.port / _protocol). This can produce an incorrect URL for non-443 ports or non-https setups. Consider composing the fallback from the available host + port fields (and protocol if available), rather than hard-coding 443/https.

Suggested change

return f"https://{host}:443"

protocol = self._protocol or "https"

port = self._port or 443

return f"{protocol}://{host}:{port}"

Copilot · 2026-03-30T13:22:13Z

+    let refresh_response =
+        response
+            .json::<TokenRequestResponse>()
+            .await
+            .context(CommunicationSnafu {
+                context: "Failed to parse token request response",
+            })?;


In token_request, the local variable is named refresh_response, but this function is parsing the token request response. Renaming it (e.g., token_response) would avoid confusion with refresh_session and make the control flow easier to follow.

Copilot · 2026-03-30T13:22:13Z

+        parsed = urlparse(connection.rest.server_url)
+        assert parsed.scheme in ("http", "https")
+        assert parsed.hostname is not None
+        assert len(parsed.hostname) > 0


test_server_url_contains_host_and_port claims server_url should be protocol://host:port, but the assertions only check scheme and hostname (no assertion for parsed.port). Either assert parsed.port is not None / equals the expected value, or adjust the test name/docstring to match the actual contract being verified.

Suggested change

assert len(parsed.hostname) > 0

assert len(parsed.hostname) > 0

assert parsed.port is not None

sfc-gh-asolarski · 2026-03-30T13:27:26Z

-        return urlparse(self._connection_info.server_url).scheme
+        url = self._connection_info.server_url
+        if not url:
+            return "https"


it looks like "https" could be constant, something like DEFAULT_PROTOCOL

same for port?

Done — extracted DEFAULT_PROTOCOL = "https" and DEFAULT_PORT = 443 as module-level constants.

sfc-gh-asolarski · 2026-03-30T13:37:57Z

+            raise ValueError(f"Unsupported HTTP method '{method}'. Only 'get' and 'post' are supported.")
+
+        headers = {
+            "Content-Type": "application/json",


for "application/json" we could utilize some constants

Done — extracted as APPLICATION_JSON constant.

sfc-gh-asolarski · 2026-03-30T13:41:21Z

+        self,
+        url: str,
+        body: dict | None = None,
+        method: str = "post",


it seems that these would be cleaner as enums, but I guess it must stay for backward compatibility, still could be extracted to some constants at least

Introduced a TokenRequestType proto enum with ISSUE and RENEW values. The Python side still accepts strings for backward compatibility but maps them to the proto enum via _TOKEN_REQUEST_TYPE_MAP.

sfc-gh-asolarski · 2026-03-30T13:47:19Z

in snowflake_restful.py we claim

class SnowflakeRestful: """Internal only iterface to underlying v1 API"""

but here we're testing with v2 API, it looks like it needs clarification

Updated the class docstring to clarify that SnowflakeRestful wraps the driver's internal gRPC API but is used by CLI to issue Snowflake REST API calls (both v1 and v2).

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

sfc-gh-asolarski

BD number requires change in tests, besides that only nitpicks

Expose master_token via ConnectionGetInfoResponse and implement the three REST methods (request, fetch, _token_request) that Snowflake CLI depends on. Uses the requests library for HTTP calls with session/master token authentication. Adds integration tests covering all properties and request patterns. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Copilot · 2026-04-07T13:16:10Z

+pub async fn token_request(
+    client: &reqwest::Client,
+    server_url: &str,
+    client_info: &ClientInfo,
+    tokens: &SessionTokens,
+    request_type: &str,
+) -> Result<TokenRequestResult, RestError> {
+    let token_url = Url::parse(server_url)
+        .and_then(|base| base.join(TOKEN_REQUEST_PATH))
+        .context(UrlJoinSnafu {
+            path: TOKEN_REQUEST_PATH,
+        })?;
+
+    let body = serde_json::json!({
+        "oldSessionToken": tokens.session_token.reveal(),
+        "requestType": request_type,
+    });


token_request() takes request_type: &str and forwards it directly into the request body and error variants. Since this is a public API, callers can pass arbitrary values and only discover the mistake via a server-side failure (potentially with a confusing error message). Consider validating request_type up-front (e.g., only allow "ISSUE"/"RENEW") or changing the parameter type to a dedicated enum to make invalid request types unrepresentable.

Copilot · 2026-04-07T13:16:10Z

+                    .build()
+                    .unwrap(),
+            );
+            conn.server_url = Some("https://192.0.2.1:9".into());


The new HTTP tests use server_url = "https://192.0.2.1:9" and rely on a network-layer failure to proceed. Even with a 100ms client timeout, connecting to a TEST-NET address can be slower/flaky depending on CI networking (timeouts vs immediate refusal), which can make these unit tests intermittently slow or fail. Consider using a deterministic local unreachable target (e.g., https://127.0.0.1:9 where connection refusal is immediate) or a lightweight mock HTTP server to avoid dependence on external routing behavior.

Suggested change

conn.server_url = Some("https://192.0.2.1:9".into());

conn.server_url = Some("https://127.0.0.1:9".into());

Copilot AI review requested due to automatic review settings March 19, 2026 16:07

Copilot AI reviewed Mar 19, 2026

View reviewed changes

sfc-gh-turbaszek changed the title ~~No snow snowflake restful further implementation~~ SNOW-3155971: Snowflake restful implementation Mar 24, 2026

Copilot AI review requested due to automatic review settings March 24, 2026 09:28

Copilot started reviewing on behalf of sfc-gh-turbaszek March 24, 2026 09:29 View session

Copilot AI reviewed Mar 24, 2026

View reviewed changes

sfc-gh-turbaszek commented Mar 24, 2026

View reviewed changes

Comment thread python/src/snowflake/connector/_internal/snowflake_restful.py