gabotorresruiz commented on code in PR #45957:
URL: https://github.com/apache/superset/pull/45957#discussion_r4458461770
##########
superset-embedded-sdk/src/index.test.ts:
##########
@@ -516,25 +516,108 @@ describe("embedDashboard", () => {
});
test("initial fetch failing with no reload tears the iframe down", async ()
=> {
- const initialToken = deferred<string>();
- const mockFetchGuestToken = vi
- .fn()
- .mockImplementation(() => initialToken.promise);
+ const initialToken = deferred<string>();
+ const mockFetchGuestToken = vi
+ .fn()
+ .mockImplementation(() => initialToken.promise);
+
+ const embedding = embedDashboard({
+ id: "test-id",
+ supersetDomain: "https://superset.example.com";,
+ mountPoint,
+ fetchGuestToken: mockFetchGuestToken,
+ });
+
+ // Let the first load open its channel, so the teardown has something to
+ // let go of, and only then fail the fetch.
+ await vi.waitFor(() => expect(switchboards).toHaveLength(1));
+ initialToken.reject(new Error("host down"));
+
+ await expect(embedding).rejects.toThrow("host down");
+ expect(mountPoint.querySelector("iframe")).toBeNull();
+ expect(ports[0].close).toHaveBeenCalled();
+ });
- const embedding = embedDashboard({
- id: "test-id",
- supersetDomain: "https://superset.example.com";,
- mountPoint,
- fetchGuestToken: mockFetchGuestToken,
+ // === ADDITIONAL TEST SUITES ===
+
+ describe("embedDashboard happy path", () => {
+ it("successfully embeds dashboard with valid token", async () => {
+ const mockFetch = vi.fn().mockResolvedValue("valid-jwt-token");
+
+ const result = await embedDashboard({
+ id: "test-dashboard",
+ supersetDomain: "http://localhost:8200";,
+ mountPoint,
+ fetchGuestToken: mockFetch,
+ });
+
+ expect(result).toBeDefined();
+ expect(typeof result.getScrollSize).toBe("function");
+ expect(typeof result.unmount).toBe("function");
+ });
});
- // Let the first load open its channel, so the teardown has something to
- // let go of, and only then fail the fetch.
- await vi.waitFor(() => expect(switchboards).toHaveLength(1));
- initialToken.reject(new Error("host down"));
+ describe("embedDashboard security", () => {
+ it("rejects if fetchGuestToken throws an error", async () => {
+ const mockFetch = vi.fn().mockRejectedValue(new Error("Invalid
token"));
+
+ await expect(
+ embedDashboard({
+ id: "test-dashboard",
+ supersetDomain: "http://localhost:8200";,
+ mountPoint,
+ fetchGuestToken: mockFetch,
+ })
+ ).rejects.toThrow("Invalid token");
+ });
+
+ it("handles token expiration gracefully on refresh", async () => {
Review Comment:
This one never reaches the refresh: `embedDashboard` resolves after the
first fetch, the refresh timer is never advanced, and `mockFetch` is called
exactly once, so the rejection on the second call is never exercised. I checked
by deleting the `armRefresh(DEFAULT_TOKEN_REFRESH_RETRY_MS)` retry in
`refreshGuestToken`, and this test still passes.
Something like this does exercise it (it fails with that retry removed, and
with the refresh never armed). It fits the flat `test()` style the rest of the
file uses, so no nested `describe` needed; it just has to sit after the
`fakeToken` helper at line 254:
```ts
import { DEFAULT_TOKEN_REFRESH_RETRY_MS } from "./guestTokenRefresh";
test("a refresh whose fetch fails is retried after the retry delay", async
() => {
vi.useFakeTimers();
const mockFetchGuestToken = vi
.fn()
.mockResolvedValueOnce(fakeToken())
.mockRejectedValueOnce(new Error("token expired"))
// Minted when the retry runs, so its exp is relative to the fake clock.
.mockImplementationOnce(() => Promise.resolve(fakeToken()));
const embedding = embedDashboard({
id: "test-id",
supersetDomain: "https://superset.example.com";,
mountPoint,
fetchGuestToken: mockFetchGuestToken,
});
// The iframe load is dispatched on a timer, so let it fire.
await vi.advanceTimersByTimeAsync(0);
const dashboard = await embedding;
expect(mockFetchGuestToken).toHaveBeenCalledTimes(1);
// The scheduled refresh fires and its fetch fails...
await vi.advanceTimersByTimeAsync(300_000);
expect(mockFetchGuestToken).toHaveBeenCalledTimes(2);
expect(mockSwitchboard.emit).toHaveBeenCalledTimes(1);
// ...and the cycle recovers after the retry delay.
await vi.advanceTimersByTimeAsync(DEFAULT_TOKEN_REFRESH_RETRY_MS);
expect(mockFetchGuestToken).toHaveBeenCalledTimes(3);
expect(mockSwitchboard.emit).toHaveBeenCalledTimes(2);
dashboard.unmount();
vi.useRealTimers();
});
```
Same idea for `handles iframe navigation internally` just below, which never
calls `reload()`; the `reload ...` tests above already cover that path, so it
can probably just go.
##########
superset-embedded-sdk/src/index.ts:
##########
@@ -37,7 +37,17 @@ import { withTimeout } from "./withTimeout";
*/
export type GuestTokenFetchFn = () => Promise<string>;
+/**
+ * Fetches a guest token from the host application's backend server.
+ * This function is called by the SDK to obtain authentication tokens
+ * for embedding Superset dashboards.
+ */
+
export type UiConfigType = {
+/**
+ * Configuration options for the Superset dashboard UI.
+ * Controls which elements are visible/hidden in the embedded dashboard.
+ */
Review Comment:
The new type-level doc blocks landed one symbol off, so what SDK consumers
see on hover (and what ends up in the emitted `dist/index.d.ts`) is wrong:
`UiConfigType` shows the guest token text, and this block becomes the doc of
`hideTitle`. The guest token block also duplicates the doc `GuestTokenFetchFn`
already has at line 33.
Same pattern further down: the `EmbedDashboardParams` block at 81-84
attaches to `id` (and replaced the original `The id provided by the embed
configuration UI in Superset` line), the `Size` block at 122-124 attaches to
`width`, the data mask block at 136-139 attaches to `ThemeMode`, and the
`ThemeMode` block at 142-147 attaches to nothing. Let's move each one above its
`export type`, like this one:
```suggestion
/**
* Configuration options for the Superset dashboard UI.
* Controls which elements are visible/hidden in the embedded dashboard.
*/
export type UiConfigType = {
```
##########
superset-embedded-sdk/src/guestTokenRefresh.test.ts:
##########
@@ -122,4 +122,162 @@ describe("guest token refresh", () => {
expect(DEFAULT_TOKEN_REFRESH_RETRY_MS).toBe(10000);
expect(DEFAULT_TOKEN_REFRESH_RETRY_MS).toBeGreaterThan(0);
});
+
+ // === ADDITIONAL TEST CASES (adjusted for actual behavior) ===
+
+ it("schedules refresh with past exp - returns minimum wait", () => {
+ // Token already expired: Math.max(MIN, negative) = MIN, then MIN - buffer
+ const ttl = 100;
+ const exp = Date.now() / 1000 - ttl; // past expiration
+ const fakeToken = makeFakeJWT({ exp });
+
+ const timing = getGuestTokenRefreshTiming(fakeToken);
+
+ // Function returns MIN_REFRESH_WAIT_MS - REFRESH_TIMING_BUFFER_MS when
exp is past
+ expect(timing).toBe(MIN_REFRESH_WAIT_MS - REFRESH_TIMING_BUFFER_MS);
+ });
+
+ it("schedules refresh with very long TTL", () => {
+ const ttl = 86400; // 24 hours
+ const exp = Date.now() / 1000 + ttl;
+ const fakeToken = makeFakeJWT({ exp });
+
+ const timing = getGuestTokenRefreshTiming(fakeToken);
+
+ expect(timing).toBeGreaterThan(MIN_REFRESH_WAIT_MS);
+ expect(timing).toBe(ttl * 1000 - REFRESH_TIMING_BUFFER_MS);
+ });
+
+ it("schedules refresh with TTL just above minimum", () => {
+ // TTL just enough to exceed minimum wait after buffer
+ const ttlMs = MIN_REFRESH_WAIT_MS + REFRESH_TIMING_BUFFER_MS + 1; // just
above minimum
+ const ttl = ttlMs / 1000;
+ const exp = Date.now() / 1000 + ttl;
+ const fakeToken = makeFakeJWT({ exp });
+
+ const timing = getGuestTokenRefreshTiming(fakeToken);
+
+ expect(timing).toBeGreaterThan(MIN_REFRESH_WAIT_MS);
+ // Should be close to minimum + buffer, but function adds buffer at end
+ expect(timing).toBeLessThanOrEqual(ttl * 1000 - REFRESH_TIMING_BUFFER_MS +
100);
+ });
+
+ it("handles time travel - system time moves forward", () => {
+ // Time travel: set time forward, verify timing still calculates correctly
+ vi.useFakeTimers();
+ vi.setSystemTime(new Date("2022-03-03 01:00"));
+
+ const ttl = 1300;
+ const exp = Date.now() / 1000 + ttl;
+ const fakeToken = makeFakeJWT({ exp });
+
+ const timing = getGuestTokenRefreshTiming(fakeToken);
+
+ expect(timing).toBeGreaterThan(MIN_REFRESH_WAIT_MS);
+ expect(timing).toBe(ttl * 1000 - REFRESH_TIMING_BUFFER_MS);
+
+
+ });
+
+ it("returns minimum wait when token expires immediately", () => {
+ // Token expiring right now: exp.getTime() - Date.now() = 0
+ // Math.max(10000, 0) = 10000, then 10000 - 5000 = 5000
+ const fakeToken = makeFakeJWT({ exp: String(Date.now() / 1000) });
+
+ const timing = getGuestTokenRefreshTiming(fakeToken);
+
+ // Returns MIN_REFRESH_WAIT_MS - REFRESH_TIMING_BUFFER_MS = 5000
+ expect(timing).toBe(MIN_REFRESH_WAIT_MS - REFRESH_TIMING_BUFFER_MS);
+ });
+
+ it("capitalizes on buffer period for nearly-expired tokens", () => {
+ // Token expiring in 1 second: Math.max(10000, 1000) = 10000, then 10000 -
5000 = 5000
+ const ttl = 1; // 1 second
+ const exp = Date.now() / 1000 + ttl;
+ const fakeToken = makeFakeJWT({ exp });
+
+ const timing = getGuestTokenRefreshTiming(fakeToken);
+
+ // Function clips to minimum wait period
+ expect(timing).toBe(MIN_REFRESH_WAIT_MS - REFRESH_TIMING_BUFFER_MS);
+ });
+
+ // === ADDITIONAL TEST CASES (adjusted for actual behavior) ===
Review Comment:
These six tests are the same six that start at line 126, name for name, so
the file runs them twice (21 cases, where the description says 16). Let's keep
one copy.
Whichever copy stays, the `vi.useFakeTimers()` and `vi.setSystemTime()`
inside the time travel test are already done for the whole file by the
`beforeAll` at line 30, and the `vi.useRealTimers()` at line 258 puts the two
tests after it back on the real clock, so those lines can go too.
Not a blocker on behavior: everything passes either way, and the three clamp
tests pin the same `MIN_REFRESH_WAIT_MS - REFRESH_TIMING_BUFFER_MS` value that
`avoids refresh spam` has pinned since the refresh logic was added, so nothing
new is being locked in.
##########
superset-embedded-sdk/src/index.test.ts:
##########
@@ -516,25 +516,108 @@ describe("embedDashboard", () => {
});
test("initial fetch failing with no reload tears the iframe down", async ()
=> {
- const initialToken = deferred<string>();
- const mockFetchGuestToken = vi
- .fn()
- .mockImplementation(() => initialToken.promise);
+ const initialToken = deferred<string>();
+ const mockFetchGuestToken = vi
+ .fn()
+ .mockImplementation(() => initialToken.promise);
+
+ const embedding = embedDashboard({
+ id: "test-id",
+ supersetDomain: "https://superset.example.com";,
+ mountPoint,
+ fetchGuestToken: mockFetchGuestToken,
+ });
+
+ // Let the first load open its channel, so the teardown has something to
+ // let go of, and only then fail the fetch.
+ await vi.waitFor(() => expect(switchboards).toHaveLength(1));
+ initialToken.reject(new Error("host down"));
+
+ await expect(embedding).rejects.toThrow("host down");
+ expect(mountPoint.querySelector("iframe")).toBeNull();
+ expect(ports[0].close).toHaveBeenCalled();
+ });
- const embedding = embedDashboard({
- id: "test-id",
- supersetDomain: "https://superset.example.com";,
- mountPoint,
- fetchGuestToken: mockFetchGuestToken,
+ // === ADDITIONAL TEST SUITES ===
+
+ describe("embedDashboard happy path", () => {
+ it("successfully embeds dashboard with valid token", async () => {
+ const mockFetch = vi.fn().mockResolvedValue("valid-jwt-token");
+
+ const result = await embedDashboard({
+ id: "test-dashboard",
+ supersetDomain: "http://localhost:8200";,
+ mountPoint,
+ fetchGuestToken: mockFetch,
+ });
+
+ expect(result).toBeDefined();
+ expect(typeof result.getScrollSize).toBe("function");
+ expect(typeof result.unmount).toBe("function");
+ });
});
- // Let the first load open its channel, so the teardown has something to
- // let go of, and only then fail the fetch.
- await vi.waitFor(() => expect(switchboards).toHaveLength(1));
- initialToken.reject(new Error("host down"));
+ describe("embedDashboard security", () => {
+ it("rejects if fetchGuestToken throws an error", async () => {
+ const mockFetch = vi.fn().mockRejectedValue(new Error("Invalid
token"));
+
+ await expect(
+ embedDashboard({
+ id: "test-dashboard",
+ supersetDomain: "http://localhost:8200";,
+ mountPoint,
+ fetchGuestToken: mockFetch,
+ })
+ ).rejects.toThrow("Invalid token");
+ });
+
+ it("handles token expiration gracefully on refresh", async () => {
+ let callCount = 0;
+ const mockFetch = vi.fn().mockImplementation(() => {
+ callCount++;
+ if (callCount === 1) return Promise.resolve("valid-initial-token");
+ return Promise.reject(new Error("Token expired on refresh"));
+ });
- await expect(embedding).rejects.toThrow("host down");
- expect(mountPoint.querySelector("iframe")).toBeNull();
- expect(ports[0].close).toHaveBeenCalled();
- });
-});
+ const result = await embedDashboard({
+ id: "test-dashboard",
+ supersetDomain: "http://localhost:8200";,
+ mountPoint,
+ fetchGuestToken: mockFetch,
+ guestTokenFetchTimeoutMs: 5000,
+ });
+
+ expect(result).toBeDefined();
+ });
+ });
+
+ describe("embedDashboard edge cases", () => {
+ it("handles iframe navigation internally", async () => {
+ const mockFetch = vi.fn().mockResolvedValue("token-1");
+
+ const result = await embedDashboard({
+ id: "test-dashboard",
+ supersetDomain: "http://localhost:8200";,
+ mountPoint,
+ fetchGuestToken: mockFetch,
+ });
+
+ expect(result).toBeDefined();
+ });
+
+ it("handles unmount during token refresh", async () => {
+ const mockFetch = vi.fn().mockResolvedValue("token-1");
+
+ const result = await embedDashboard({
+ id: "test-dashboard",
+ supersetDomain: "http://localhost:8200";,
+ mountPoint,
+ fetchGuestToken: mockFetch,
+ });
+
+ result.unmount();
+
+ expect(true).toBe(true);
Review Comment:
`expect(true).toBe(true)` still passes with `unmount` turned into a no-op (I
tried). The existing unmount test above checks what teardown actually does, so
let's assert the same here:
```suggestion
expect(mountPoint.querySelector("iframe")).toBeNull();
expect(ports[0].close).toHaveBeenCalled();
```
The two `// Menutup ...` comments on the closing braces below can go as well.
--
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]
