Skip to content

fix: return raw bytes from S3 and AzureBlob downloads - #26

Merged
zachdaniel merged 2 commits into
ash-project:mainfrom
vasspilka:fix/raw-binary-downloads
May 24, 2026
Merged

fix: return raw bytes from S3 and AzureBlob downloads#26
zachdaniel merged 2 commits into
ash-project:mainfrom
vasspilka:fix/raw-binary-downloads

Conversation

@vasspilka

@vasspilka vasspilka commented May 22, 2026

Copy link
Copy Markdown
Contributor

Summary

AshStorage.Service.AzureBlob.download/2 and AshStorage.Service.S3.download/2 are typed {:ok, binary()} | {:error, term()}, but they delegate to Req.get/2 without disabling Req's default decode_body step. That step deserialises response bodies based on the response's Content-Type header:

Format Decoder
json Jason.decode/2
csv NimbleCSV.RFC4180.parse_string/2 (if loaded)
tar, tgz :erl_tar.extract/2
zip :zip.unzip/2
gzip :zlib.gunzip/1
zst :ezstd.decompress/1 (if loaded)

So a blob stored with Content-Type: application/zip comes back already unzipped; text/csv comes back as a list of lists; JSON comes back as a map. The typespec claims binary(); the runtime returns whatever Req decided to decode.

This was discovered in the wild when a CSV uploaded to chat became unreadable to the downstream parser. The Disk service uses File.read/1, so unit tests never hit it; only the Azure/S3 paths do.

There's also a latent verify_md5/2 interaction: when body is an auto-decoded term, MD5 is computed over the iodata flattening of that term, not the original bytes — silently inconsistent with the digest the server stored.

Change

The runtime behaviour is intentional and used by real callers, so the fix is in the typespec, not the code path. The behaviour callback now reflects what services actually return:

- @callback download(key(), Context.t()) :: {:ok, binary()} | {:error, term()}
+ @callback download(key(), Context.t()) :: {:ok, term()} | {:error, term()}

The AshStorage.Service.download/2 callback doc explains the contract: auto-decoding by default on the Req-based services, raw bytes on Disk/Mirror.

For callers that want the raw uploaded bytes — writing to disk, streaming to a client, verifying a checksum — both Req-based services accept a new :decode_body service option (defaults to true):

AshStorage.Service.AzureBlob.download(key, Context.new(account: ..., container: ..., decode_body: false))
AshStorage.Service.S3.download(key, Context.new(bucket: ..., decode_body: false))

The option is registered in both adapters' service_opts_fields/0 so consumers that introspect the schema see it.

Tests

New integration tests in both suites:

  • test/ash_storage/service/azure_blob_integration_test.exs — JSON auto-decodes to a map by default; decode_body: false returns the raw CSV and JSON bytes.
  • test/ash_storage/service/s3_integration_test.exs — same, with a small raw_put_with_content_type/3 helper that sets Content-Type via a signed PUT (since S3.upload/3 doesn't currently expose the header).

The new tests are tagged :azure_integration / :s3_integration like the existing ones, so they only run when Docker is available.

Acceptance Criteria

  • I accept the AI Policy, or AI was not used in the creation of this PR.
  • Bug fixes include regression tests
  • Documentation changes
  • Features include unit/acceptance tests

🤖 Generated with Claude Code

@vasspilka
vasspilka force-pushed the fix/raw-binary-downloads branch from 48b25b1 to 0830cfb Compare May 22, 2026 16:01
@vasspilka
vasspilka marked this pull request as draft May 22, 2026 16:16
Req's default `decode_body` step deserialises response bodies based on
the `Content-Type` header — JSON to maps, CSV to lists of lists, tar/zip
archives transparently extracted, etc. Both `AshStorage.Service.AzureBlob.download/2`
and `AshStorage.Service.S3.download/2` are documented to return
`{:ok, binary()}`, but they call `Req.get/2` without opting out of the
decoder. Any blob the cloud returns with a recognised Content-Type
(text/csv, application/json, application/zip, ...) comes back to the
caller pre-parsed instead of as bytes.

This also silently breaks the `verify_md5/2` check that runs right after
the download: when the body is decoded, MD5 is computed over the iodata
flattening, not the original bytes.

The fix passes `decode_body: false` to `Req.get/2` so the body comes back
as-is, matching the typespec. `decode_body: false` is preferred over
`raw: true` because the latter also disables `decompress_body`, which we
want to keep for transparent gzip/br handling.

Callers that intentionally want Req's content-type-based decoding can
opt back in via `service_opts: [decode_body: true]` on either service.
The new option is documented in `service_opts_fields/0`.

Integration tests cover both directions for AzureBlob and S3.
@vasspilka
vasspilka force-pushed the fix/raw-binary-downloads branch from 0830cfb to 6bb3eab Compare May 22, 2026 17:12
@vasspilka
vasspilka marked this pull request as ready for review May 22, 2026 17:12
@zachdaniel

Copy link
Copy Markdown
Contributor

Yeah that's just missing type spec updates. Lets do what you said where the current behavior becomes the default with an option to change.

@vasspilka

Copy link
Copy Markdown
Contributor Author

Hey @zachdaniel , did as you said.

Note however because of the decoding there is also a bug in main right now. I created an issue #27 for it.

Basically with automatic decoding the checksums will fail as they need raw binary to execute. We can address this in a future PR.

@vasspilka

Copy link
Copy Markdown
Contributor Author

@zachdaniel If that bug changes your mind regarding the default will be happy to switch it back to decode_body: false

@zachdaniel
zachdaniel merged commit f93e6b6 into ash-project:main May 24, 2026
22 of 24 checks passed
@zachdaniel

Copy link
Copy Markdown
Contributor

🚀 Thank you for your contribution! 🚀

@zachdaniel

Copy link
Copy Markdown
Contributor

Ah, I didn't see this last comment until it was too late 🤔.

Yeah I guess you're right we should adjust the contract and not decode the body. We wouldn't even really want to have the option to decode if it can break dependents right?

@zachdaniel

Copy link
Copy Markdown
Contributor

I can revert this PR or if you want to just open a new one with your proposed proper fix. Up to you 🙇‍♂️

@vasspilka

Copy link
Copy Markdown
Contributor Author

@zachdaniel Yes please, you can revert and I'll reopen the PR with decode_body: false :)

Cheers

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