Skip to content

Conversation

@carlopi
Copy link

@carlopi carlopi commented Sep 22, 2025

Hello!
This is basically implementing #35.

There are few logical part of this PRs:

  1. passing ClientContext around, so that it's reachable when calling `perform_https_request
    This should be a NOP, does changes a bunch of signature and insulate the code a bit less, but should be safe to do
  2. swap from custom HTTP call performed via openssl to using the HTTPUtil functionality (see Generalize HTTP interface and use the new HTTP interface in httpfs duckdb/duckdb#17464 for initial PR to duckdb/duckdb)
    This is the most fragile part, that might be worth some extra tests, since duckdb_gsheets might be using some Openssl [implicit] parameters. I think I covered basics, but extra pair of eyes to review changes in perform_https_request OR changes in behaviour are welcome.
    There are two big advantage in using HTTPUtils: better integration with the rest of the system, so that for example logging would come out of the box (I think CALL enable_logging(level='trace', storage='stdout'); should show all API calls after this PR, and second that one reuse existing infrastructure, so that improvements are shared everywhere. As part of this second bit, HTTPUtil has a Wasm implementation, so this makes possible to use ghsees from a browser.
  3. add an option to override the default sheets endpoint. Some form of this (name / behaviour can change) is necessary to provide a CORS-enabled alternative
  4. fixup some bits on linking extensions for Wasm (this should have no impact if not for wasm platform)

1 and 3 should be OK, 2 is where there are risks and rewards, so needs to be checked with care.

All put together you should then be able to do, within duckdb-wasm currently deployed at https://shell.duckdb.org (once you disable signed extension access, that do require to serve it yourself):

SET builtin_httpfs = false; --- this is needed at the moment, but will become the default behaviour
INSTALL gsheets FROM '<REPOSITORY>';
LOAD gsheets;
CREATE SECRET (TYPE gsheet, PROVIDER 'autorization_token', TOKEN '<TOKEN>);
FROM read_gsheet('https://docs.google.com/spreadsheets/d/<SHEET_ID>');
---- Invalid Error: No JSON object found in the response
---- (error here could improve, it's a CORS error)
SET gsheets_endpoint = 'cors-proxy.duckdb.org/cors_proxy/sheets.googleapis.com';
FROM read_gsheet('https://docs.google.com/spreadsheets/d/<SHEET_ID>');
---- works!

I have not looked at automatic secret (that currently succeed at SQL level but has no token), so maybe it would be handy to add a block like:

#ifdef EMSCRIPTEN
throw UnsupportedException("...");
#endif 

near https://github.com/evidence-dev/duckdb_gsheets/blob/main/src/gsheets_auth.cpp#L171

}
case HttpMethod::POST:
case HttpMethod::PUT:
throw IOException("POST or PUT not implemented");
Copy link
Member

@archiewood archiewood Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

presume still have to implement these?

GET=read_gsheet()
POST=COPY TO

SSL_library_init();
SSL_load_error_strings();
OpenSSL_add_all_algorithms();
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need SSL at all now if we are using the duckdb util instead?

#include "duckdb/main/database.hpp"
#include <chrono>
#include <openssl/ssl.h>
#include <openssl/err.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we remove SSL?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants