michael-s-molina commented on code in PR #39171: URL: https://github.com/apache/superset/pull/39171#discussion_r3561219232
########## superset-frontend/packages/superset-core/src/storage/index.ts: ########## @@ -0,0 +1,208 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** + * @fileoverview Shared types for extension storage APIs. + * + * These types are shared across all storage tiers (local, session, ephemeral, + * persistent) to ensure a consistent API pattern. + */ + +/** + * JSON-compatible value type. + * These are the only values that can be safely serialized/deserialized via JSON. + */ +export type JsonValue = + string | number | boolean | null | JsonValue[] | { [key: string]: JsonValue }; + +/** + * Base interface for a storage accessor. + * All storage tiers implement this interface for both user-scoped and shared access. + */ +export interface StorageAccessor { + /** + * Get a value from storage. + * + * @param key The key to retrieve. + * @returns The stored value, or null if not found. + */ + get<T = JsonValue>(key: string): Promise<T | null>; + + /** + * Set a value in storage. + * + * @param key The key to store. + * @param value The value to store (must be JSON-serializable). + */ + set<T = JsonValue>(key: string, value: T): Promise<void>; Review Comment: Not a bug — by design. `T` here is a generic default (`T = JsonValue`), not a constraint. It's intentionally unconstrained because `set()` supports non-JSON values via the `codec` option (`base64` for binary, `pickle` for arbitrary Python objects on the backend) — constraining `T extends JsonValue` would make those codec paths untypeable. See `PersistentSetOptions.codec`/`EphemeralSetOptions.codec`. ########## superset-frontend/src/core/storage/binaryCodec.ts: ########## @@ -0,0 +1,70 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** Convert bytes to a base64 string, for transport over the JSON REST API. */ +export function bytesToBase64(bytes: Uint8Array): string { + let binary = ''; + bytes.forEach(byte => { + binary += String.fromCharCode(byte); + }); + return btoa(binary); +} + +function isBinaryValue(value: unknown): value is Uint8Array | ArrayBuffer { + return value instanceof Uint8Array || value instanceof ArrayBuffer; +} + +/** + * Infer which codec to use for `value` when the caller didn't specify one. + * + * "json" names the serialization format applied to encode the value for + * storage/transport -- it is not a claim about the value's shape. Per + * RFC 8259, a JSON text may be any JSON value (object, array, string, + * number, boolean, or null), so scalars like `1` or `"foo"` are encoded + * with the "json" codec too, alongside objects and arrays. + * + * Binary values (Uint8Array/ArrayBuffer) are the one case JSON cannot + * represent, so they're base64-encoded instead. + */ +function inferDefaultCodec(value: unknown): 'json' | 'base64' { + return isBinaryValue(value) ? 'base64' : 'json'; +} + +/** + * Resolve the codec and request-body value for a `set()` call. + * + * If the caller explicitly set `options.codec`, it is always respected + * as-is and `value` is passed through untouched -- the caller is + * responsible for encoding it correctly (e.g. base64-encoding it + * themselves before calling `set()`). Auto base64-encoding only happens + * when no codec was specified and `value` is binary. + */ +export function resolveSetPayload<T>( + value: T, + explicitCodec: string | undefined, +): { value: unknown; codec: string } { + if (explicitCodec !== undefined) { + return { value, codec: explicitCodec }; Review Comment: Not a bug — this is the intended contract. `resolveSetPayload` only auto-encodes binary values when the caller leaves `codec` unset; once a caller passes an explicit `codec`, we respect it as-is and never reinterpret `value`, so the caller stays fully in control of encoding (including for `pickle`, which isn't JSON/base64-shaped at all). This is documented in the function's own docstring and covered by the test `'does not double-encode when the caller already base64-encoded a binary value'` in `binaryCodec.test.ts`. ########## superset-frontend/src/core/storage/ephemeralState.test.ts: ########## @@ -0,0 +1,144 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { SupersetClient } from '@superset-ui/core'; +import { createEphemeralState } from './ephemeralState'; + +jest.mock('@superset-ui/core', () => ({ + SupersetClient: { + get: jest.fn(), + put: jest.fn(), + delete: jest.fn(), + }, +})); + +const mockGet = SupersetClient.get as jest.Mock; +const mockPut = SupersetClient.put as jest.Mock; +const mockDelete = SupersetClient.delete as jest.Mock; + +beforeEach(() => { + jest.clearAllMocks(); + mockGet.mockResolvedValue({ json: { result: null } }); + mockPut.mockResolvedValue({}); + mockDelete.mockResolvedValue({}); +}); + +test('get calls correct URL with publisher/name pattern', async () => { + const store = createEphemeralState('myorg.myext'); + await store.get('job_progress'); + expect(mockGet).toHaveBeenCalledWith({ + endpoint: '/api/v1/extensions/myorg/myext/storage/ephemeral/job_progress', + }); +}); + +test('get returns result from response', async () => { + mockGet.mockResolvedValue({ json: { result: { pct: 42 } } }); + const store = createEphemeralState('myorg.myext'); + const result = await store.get('job_progress'); + expect(result).toEqual({ pct: 42 }); +}); + +test('get returns null when result is absent', async () => { + mockGet.mockResolvedValue({ json: {} }); + const store = createEphemeralState('myorg.myext'); + expect(await store.get('key')).toBeNull(); +}); + +test('set calls correct URL and includes value and ttl in body', async () => { + const store = createEphemeralState('myorg.myext'); + await store.set('job_progress', { pct: 42 }, { ttl: 300 }); + expect(mockPut).toHaveBeenCalledWith({ + endpoint: '/api/v1/extensions/myorg/myext/storage/ephemeral/job_progress', + body: JSON.stringify({ value: { pct: 42 }, ttl: 300, codec: 'json' }), + headers: { 'Content-Type': 'application/json' }, + }); +}); + +test('set passes codec from options', async () => { + const store = createEphemeralState('myorg.myext'); + await store.set('job_progress', 'sk-...', { ttl: 300, codec: 'pickle' }); + expect(mockPut).toHaveBeenCalledWith({ + endpoint: '/api/v1/extensions/myorg/myext/storage/ephemeral/job_progress', + body: JSON.stringify({ value: 'sk-...', ttl: 300, codec: 'pickle' }), + headers: { 'Content-Type': 'application/json' }, + }); Review Comment: Not a bug — this test exercises the SDK's codec-passthrough behavior (what gets sent in the request), not a live round-trip against the server's `SAFE_CODECS` allowlist. Asserting that an explicit `codec: 'pickle'` is forwarded as given is correct per the client's contract (see `binaryCodec.ts`'s "explicit codec always respected" design). Whether the server accepts `pickle` for a given call depends on which path is used — it's rejected over REST but valid for direct backend calls — which is a separate, already-covered concern on the server side (`SAFE_CODECS` in `superset/extensions/storage/codecs.py`). -- 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]
