Replace google_api_storage with direct GCS JSON API calls - #5053
Merged
Conversation
Storage config is resolved at runtime from STORAGE_BACKEND and STORAGE_PATH, so the suite picked up whatever a developer had in .env. With STORAGE_BACKEND=gcs set locally, four tests in export_worker_test.exs and projects_test.exs made live calls against a real bucket and failed. Stub Lightning.Config.storage/1 in the three case templates that already stub_with MockConfig, defaulting to the local backend and the "." path the env vars default to. Unknown keys fall through to Lightning.Config.API, and individual tests can still override.
Uploads to GCS have been broken since Tesla 1.18.3 (#4913). Tesla now requires multipart part names to be binaries, and google_gax labels its metadata part with an atom, so storage_objects_insert_simple raises before sending anything. Versions offer no way out: google_gax last shipped in 2021, googleapis/elixir-google-api is archived, and 1.18.2 reintroduces the CVEs 1.18.3 closed. store/2 now POSTs with uploadType=media, avoiding multipart entirely — the object name is the only metadata we set and it travels in the query string. The body is wrapped in Stream.map/2 so Tesla's adapters see a %Stream{} and send it chunked; a bare %File.Stream{} falls through. delete/1 becomes a plain DELETE. Both check the status explicitly, since Tesla returns {:ok, env} for a 403 or 500 and callers rely on non-2xx being an error tuple. delete/1 also applies prefix_storage_path/1, which it never did. On deployments with STORAGE_PATH set, every delete asked for a key that was never written, got a 404, and left the object in the bucket while Projects dropped its project_files row. bucket!/0 reads Lightning.Config.storage(:bucket) rather than the application env, making Config the single source of truth for storage config. A TokenSource behaviour stands in for Goth, which only starts when STORAGE_BACKEND=gcs and so never runs under test. Drops google_api_storage, taking google_gax and poison with it. Closes #5049
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release-2.18.0 #5053 +/- ##
================================================
+ Coverage 90.6% 90.7% +0.1%
================================================
Files 420 421 +1
Lines 19957 19972 +15
================================================
+ Hits 18078 18118 +40
+ Misses 1879 1854 -25 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
delete/1 now prefixes the object path with STORAGE_PATH, so it goes through
Path.safe_relative/1 and can fail where it never could before. Matching that
with {:ok, _} = raised a MatchError, and delete/1 runs inside the data
retention job's Enum.each over every expired file, so a single unusable path
would abort the cleanup for every project behind it.
prefix_storage_path/1 now returns {:error, {:unsafe_storage_path, path}} and
store/2 and delete/1 route through with/1. An absolute STORAGE_PATH is the
realistic way in: Path.safe_relative/1 rejects the joined path outright.
midigofrank
marked this pull request as ready for review
August 6, 2026 15:29
|
All PR changes are scoped and no S0/S1/S2 concerns emerge. Producing the pass-case output. Security Review ✅
|
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.
Description
Uploads to Google Cloud Storage have been broken since Tesla 1.18.3 landed in #4913. Tesla now requires multipart part names to be binaries, and
google_gaxlabels its metadata part with an atom, sostorage_objects_insert_simpleraises before a request is ever sent:Versions offer no way out:
google_gax's last release was 2021-12-28, googleapis/elixir-google-api is archived, and pinning Tesla back to 1.18.2 reintroduces the five CVEs 1.18.3 closed. Solib/lightning/storage/gcs.exnow calls the GCS JSON API directly and the dependency is gone.get_url/1already bypassed the library, so this only meant reimplementing two requests.store/2POSTs withuploadType=media, avoiding multipart entirely — the object name is the only metadata we set and it travels in the query string.delete/1becomes a plainDELETE, with the object path escaped as a single segment (/→%2F), and now appliesprefix_storage_path/1— see below.{:ok, env}for a 403 or 500, whereasgoogle_gaxfolded non-2xx into{:error, env}and callers rely on that. Without the check,ExportWorker.store_project_file/2would record a rejected upload as stored.bucket!/0readsLightning.Config.storage(:bucket)rather than the application env, makingLightning.Configthe single source of truth for storage config.Lightning.Storage.GCS.TokenSourcestands in for Goth, which only starts whenSTORAGE_BACKEND=gcsand so never runs under test. It's what makes the module testable — it had no tests before.google_api_storagedropped, takinggoogle_gaxandpoisonwith it.delete/1ignoringSTORAGE_PATHSTORAGE_PATHdefaults to".", whichPath.safe_relative/1normalises away, which is why this stayed hidden. On a deployment with a real prefix, every delete asked for a key that was never written and got a 404 — andProjects.remove_expired_files_for/1treats a 404 as "already gone" and drops the row anyway:Closes #5049
Validation steps
mix test test/lightning/storage/ test/lightning/storage_test.exs test/lightning/export_worker_test.exsSTORAGE_BACKEND=gcsin your.env,mix test test/lightning/export_worker_test.exs test/lightning/projects_test.exsshould pass rather than reaching for a real bucket.STORAGE_PATH: trigger a history export, confirm the object lands at the prefixed key, then confirm deletion removes it.Additional notes for the reviewer
%Stream{}wrapper instream_file/1is load-bearing, not stylistic.Tesla.Adapter.Finchmatches%Stream{}to send a chunked body;File.stream!/2returns a%File.Stream{}, which doesn't match and fails.AI Usage
Please disclose whether you've used AI anywhere in this PR (it's cool, we just
want to know!):
You can read more details in our
Responsible AI Policy
Pre-submission checklist
/reviewwith Claude Code)
(e.g.,
:owner,:admin,:editor,:viewer) — no authorizationsurface is touched by this PR