fix(duckdb): separate extension install from load with best-effort lifecycle#530
Merged
Conversation
Add DuckDBExtensionConfig.install and .required and DuckDBSecretConfig.required NotRequired keys to support the install/load split and best-effort failure model. Refs: sqlspec-oag1.1.1
TDD red phase. Spy connection records install/load calls without network. Behavior-change tests (name-only never installs, install once per pool, concurrent install once, load/install best-effort) fail against current code; version/repository-imply-install and required-raises pass as regression guards. Refs: sqlspec-oag1.1.2
Name-only extensions are now LOAD-only (restores v0.49.1). Explicit install is triggered by install=True, force_install, or version/repository/repository_url. Install and load failures are best-effort (WARNING) unless required=True raises. Install runs under the pool lock via _install_extension_once. Repurpose the obsolete surfaces-install-errors pool test into a name-only load-only regression guard. Tests A/D/E green; install-once dedup (B/C) follows in sqlspec-oag1.1.4. Refs: sqlspec-oag1.1.3
Track installed extensions by (name, version, repository, repository_url) signature in a lock-guarded set so explicit installs run once per pool across reconnects and concurrent threads. force_install bypasses the cache; failed best-effort installs are not recorded so they retry on the next connection. Tests B (install once across sessions) and C (concurrent install once) now green. Refs: sqlspec-oag1.1.4
Secret creation is now best-effort by default: validation, CREATE SECRET, and verification run inside a try; failure logs a WARNING unless required=True, which also re-enables post-creation verification. Identifier validation stays inside the try so malicious identifiers are never executed on either path. Move the three strict-by-default secret tests to required=True and add best-effort vs required coverage. Refs: sqlspec-oag1.1.5
Refs: sqlspec-oag1.1
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #530 +/- ##
==========================================
+ Coverage 74.90% 74.91% +0.01%
==========================================
Files 442 442
Lines 55120 55148 +28
Branches 8697 8697
==========================================
+ Hits 41287 41316 +29
- Misses 11149 11151 +2
+ Partials 2684 2681 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
cofin
changed the title
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
2 participants
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.
Summary
Fixes a regression where name-only DuckDB extensions were installed over the network on every physical connection (and raised on any failure). This separates install (one-time, network, on-disk) from load (per-connection, local) in
DuckDBConnectionPooland restores a best-effort failure model.Closes #529
What changed
{"name": X}is now LOAD-only — no network install on every connection.install=True,force_install, or any ofversion/repository/repository_url.WARNING;required=Truere-raises (and, for secrets, re-enables post-creation verification).(name, version, repository, repository_url), lock-guarded, so explicit installs run once across reconnects and concurrent threads.force_installbypasses the cache; failed best-effort installs are not cached (they retry).load_extension()runs per physical connection.requiredtoggle. Identifier validation stays inside the try block, so malicious identifiers are never executed on either path.install/requiredkeys onDuckDBExtensionConfigandrequiredonDuckDBSecretConfig.Tests
New
tests/unit/adapters/test_duckdb/test_extension_lifecycle.pywith call-count tests (spy connection, no network, no timing):version/repository/repository_urlimply install.required=True.Existing strict-by-default secret tests moved to
required=True; an obsolete "surfaces install errors" pool test was repurposed into a name-only load-only guard.Verification
pyright sqlspec/adapters/duckdb/cleanruff check+ruff formatclean