codeant-ai-for-open-source[bot] commented on code in PR #39171: URL: https://github.com/apache/superset/pull/39171#discussion_r3561095641
########## 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: **Suggestion:** The API contract says values must be JSON-serializable, but the unconstrained generic type accepts any value at compile time, so callers can pass unserializable inputs that fail at runtime in storage implementations. Constrain the generic to JSON-compatible (and explicitly supported binary types where applicable) to enforce the documented contract and prevent invalid writes. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Ephemeral storage writes fail for unserializable values. - ⚠️ Extensions see confusing 400s from storage API. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Extension authors use the public storage API exposed via `extensions.getContext()` in `superset-frontend/packages/superset-core/src/extensions/index.ts:80-98`, calling `ctx.storage.ephemeral.set('tempData', value, { ttl: 3600 })` as shown in the documented example. 2. The `ctx.storage.ephemeral` accessor is typed as `EphemeralStorageTier`, whose `set` method is declared generically as `set<T = JsonValue>(key: string, value: T, options: EphemeralSetOptions): Promise<void>` in `superset-frontend/packages/superset-core/src/storage/index.ts:132-138`, without constraining `T` to `JsonValue`. 3. Because `T` is unconstrained, TypeScript happily infers `T = undefined` (or another non-JSON type) when an extension calls `ctx.storage.ephemeral.set('tempData', undefined, { ttl: 3600 })`; this compiles even though the JSDoc comment at `superset-frontend/packages/superset-core/src/storage/index.ts:51-52` states the value “must be JSON-serializable”. 4. At runtime, the host implementation `createEphemeralState` in `superset-frontend/src/core/storage/ephemeralState.ts:72-82` passes the value into `resolveSetPayload` and then into `SupersetClient.put`, building the JSON body with `JSON.stringify({ value: payload.value, ttl: options.ttl, codec: payload.codec })`; when `payload.value` is `undefined`, `JSON.stringify` drops the `value` field entirely, and the backend endpoint `set_ephemeral` in `superset/extensions/storage/api.py:144-152` rejects the request with `400` “Request body must contain 'value' field”, causing a runtime failure despite the call type-checking. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f828cc10402740ad8df17a1a09a30937&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f828cc10402740ad8df17a1a09a30937&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/packages/superset-core/src/storage/index.ts **Line:** 53:53 **Comment:** *Api Mismatch: The API contract says values must be JSON-serializable, but the unconstrained generic type accepts any value at compile time, so callers can pass unserializable inputs that fail at runtime in storage implementations. Constrain the generic to JSON-compatible (and explicitly supported binary types where applicable) to enforce the documented contract and prevent invalid writes. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39171&comment_hash=dec0468de2ada36ee0ef57803fb2d1d085a48e2fa73f90b9c8e4f429c31ba505&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39171&comment_hash=dec0468de2ada36ee0ef57803fb2d1d085a48e2fa73f90b9c8e4f429c31ba505&reaction=dislike'>👎</a> ########## superset-frontend/src/core/storage/localState.ts: ########## @@ -0,0 +1,63 @@ +/** + * 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 type { JsonValue, StorageAccessor } from '@apache-superset/core/storage'; +import getBootstrapData from 'src/utils/getBootstrapData'; + +const KEY_PREFIX = 'superset-ext'; + +function getCurrentUserId(): number { + const bootstrapData = getBootstrapData(); + const userId = bootstrapData?.user?.userId; + if (userId === undefined) { + throw new Error('Storage APIs require an authenticated user.'); + } + return userId; +} + +function buildKey(...parts: (string | number)[]): string { + return [KEY_PREFIX, ...parts].join(':'); +} + +/** + * Create browser storage (localStorage/sessionStorage) bound to an extension ID. + */ +export function createBrowserStorage( + storage: Storage, + extensionId: string, +): StorageAccessor { + return { + async get<T = JsonValue>(key: string): Promise<T | null> { + const userId = getCurrentUserId(); + const storageKey = buildKey(extensionId, 'user', userId, key); + const value = storage.getItem(storageKey); + return value ? (JSON.parse(value) as T) : null; + }, + async set<T = JsonValue>(key: string, value: T): Promise<void> { + const userId = getCurrentUserId(); + const storageKey = buildKey(extensionId, 'user', userId, key); + storage.setItem(storageKey, JSON.stringify(value)); Review Comment: **Suggestion:** `set` serializes with `JSON.stringify` and writes the result directly, but `JSON.stringify(undefined)` (and some unsupported values) returns non-JSON output that later makes `get` crash at `JSON.parse`. Reject non-serializable values before storing (or coerce to a valid JSON sentinel) so reads cannot fail on values written by the same API. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Local/session storage reads crash on unparsable stored values. - ⚠️ Extensions may store undefined leading later runtime errors. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Extensions obtain per-extension browser storage via `getContext().storage.local` and `getContext().storage.session` as described in `superset-frontend/packages/superset-core/src/extensions/index.ts:80-98`, using these accessors to persist UI state and preferences. 2. The concrete implementation for these accessors is `createBrowserStorage` in `superset-frontend/src/core/storage/localState.ts:41-63`, which returns a `StorageAccessor` whose `set` method is declared generically as `async set<T = JsonValue>(key: string, value: T): Promise<void>` but does not enforce at runtime that `value` is actually JSON-serializable. 3. Because the generic `T` is unconstrained (definition in `superset-frontend/packages/superset-core/src/storage/index.ts:38-53`), extension code can successfully compile and call `await store.set('key', undefined)` or pass other unsupported values; `createBrowserStorage.set` then executes `storage.setItem(storageKey, JSON.stringify(value));` at `superset-frontend/src/core/storage/localState.ts:55`, which for `value === undefined` stores the literal string `"undefined"` under the namespaced key. 4. On a subsequent read, `createBrowserStorage.get` at `superset-frontend/src/core/storage/localState.ts:46-50` retrieves the stored string and calls `JSON.parse(value)`; when `value` is `"undefined"` (or any other non-JSON output resulting from unsupported inputs), `JSON.parse` throws a `SyntaxError`, causing the extension’s code path that uses `ctx.storage.local.get` or `session.get` to crash even though the values were written through the same API. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0820f1361b654c10ac8645a95a191bee&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0820f1361b654c10ac8645a95a191bee&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/core/storage/localState.ts **Line:** 55:55 **Comment:** *Logic Error: `set` serializes with `JSON.stringify` and writes the result directly, but `JSON.stringify(undefined)` (and some unsupported values) returns non-JSON output that later makes `get` crash at `JSON.parse`. Reject non-serializable values before storing (or coerce to a valid JSON sentinel) so reads cannot fail on values written by the same API. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39171&comment_hash=65638f726d8667ea2ea233f3df822117f0b06b45251b49c3d2c0b2bb030c452f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39171&comment_hash=65638f726d8667ea2ea233f3df822117f0b06b45251b49c3d2c0b2bb030c452f&reaction=dislike'>👎</a> -- 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]
