michael-s-molina commented on issue #41854: URL: https://github.com/apache/superset/issues/41854#issuecomment-4958040149
@villebro thanks for the close read here — you're right, and looking at the actual implementation confirmed it. **We're already storing the data as raw binary** — the DB column is `LargeBinary`, and what the `base64` codec's `encode()`/`decode()` do is unwrap the wire string to bytes *before* storage and re-wrap stored bytes to a string only for the response. No base64 text is ever persisted today. So this isn't a storage change at all — it's that the codec is misnamed: calling the persisted format `base64` bakes a REST-transport detail into what's really just "the stored value is raw bytes," and that name leaks into the backend API too — backend extension code calling `ctx.storage.persistent.set()`/`get()` directly has no JSON-transport constraint, so it shouldn't have to hand over/receive a base64 string just because that's what the codec happens to be called. Taking your suggestions 1 and 2 together, but landing on a slightly different shape than an explicit `isBase64` flag: - Renamed the codec to `binary` — an identity codec over raw `bytes`; `encode()`/`decode()` apply no transformation, matching what's already stored. - Base64 handling stays purely at the REST layer, but it's derived from the codec name rather than a separate request field: since JSON has no byte type, `binary` is the only codec whose wire value is base64 rather than a native JSON value, so the API layer knows to base64-decode/-encode around it just by checking `codec == "binary"`. The frontend SDK already always base64-encodes whenever it selects `binary` (whether auto-detected from `Uint8Array`/`ArrayBuffer` or requested explicitly), so there's no scenario where a caller would need to say "codec is binary, but the wire value isn't base64" — an independent flag would just be redundant with the codec choice, not add a real degree of freedom. - Backend code working with `bytes` directly (no REST hop) never sees base64 at all under this codec. Agreed on deferring `codecParams` — the codec registry is a plain string→codec lookup today, so adding a params mechanism later for something like a Marshmallow-schema or lossy-audio codec doesn't require reworking this. **Updated the SIP text** to reflect `binary` instead of `base64` in the codec table and surrounding prose. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
