codeant-ai-for-open-source[bot] commented on code in PR #42811: URL: https://github.com/apache/superset/pull/42811#discussion_r3723358464
########## superset-frontend/tools/webpack.proxy-config.test.js: ########## @@ -0,0 +1,207 @@ +/** + * 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. + */ +const http = require('http'); +const zlib = require('zlib'); +const { compressBuffer } = require('simple-zstd'); +const { createProxyMiddleware } = require('http-proxy-middleware'); + +// yargs ships ESM-only and jest's default transform doesn't cover +// node_modules; webpack.proxy-config.js only uses it to parse a `--env` +// CLI flag we don't exercise here (the target port is set via +// process.env.supersetPort below), so stub it out rather than teaching +// the whole suite's transformIgnorePatterns about it. +jest.mock('yargs', () => jest.fn(() => ({ parse: () => ({}) }))); +jest.mock('yargs/helpers', () => ({ hideBin: argv => argv })); + +const HANG_GUARD_MS = 2000; + +/** + * Wires the real dev proxy config to a real HTTP server, exactly the way + * webpack-dev-server does (`devServer.proxy: [() => proxyConfig]`), and + * points it at a caller-supplied backend. Both servers are ephemeral + * (port 0) so tests can run in parallel. + */ +async function startProxy(backendPort) { + const previousPort = process.env.supersetPort; + // webpack.proxy-config.js resolves its target port from process.env at + // require()-time, so the module must be (re-)required after this is set. + process.env.supersetPort = String(backendPort); + jest.resetModules(); + // eslint-disable-next-line global-require + const getProxyConfig = require('../webpack.proxy-config'); + process.env.supersetPort = previousPort; + + const proxyMiddleware = createProxyMiddleware(getProxyConfig(undefined)); + const server = http.createServer((req, res) => proxyMiddleware(req, res)); + await new Promise(resolve => server.listen(0, resolve)); + return server; +} + +async function startBackend(handler) { + const server = http.createServer(handler); + await new Promise(resolve => server.listen(0, resolve)); + return server; +} + +function get(port) { + return new Promise((resolve, reject) => { + const req = http.get( + { hostname: 'localhost', port, path: '/dashboard/list/' }, + res => { + const chunks = []; + res.on('data', chunk => chunks.push(chunk)); + res.on('end', () => + resolve({ + statusCode: res.statusCode, + body: Buffer.concat(chunks).toString(), + }), + ); + res.on('error', reject); + }, + ); + req.on('error', reject); + }); +} + +async function closeAll(...servers) { + await Promise.all( + servers.map(server => new Promise(resolve => server.close(resolve))), + ); +} + +describe('webpack.proxy-config zstd/gzip HTML decompression', () => { + test('decompresses a complete zstd-encoded HTML response and injects the [DEV] title', async () => { + const html = + '<html><head><title>Superset</title></head><body>hi</body></html>'; + const backend = await startBackend(async (req, res) => { + const compressed = await compressBuffer(Buffer.from(html), 3); + res.writeHead(200, { + 'content-type': 'text/html; charset=utf-8', + 'content-encoding': 'zstd', + }); + res.end(compressed); + }); + const proxy = await startProxy(backend.address().port); + + try { + const { statusCode, body } = await get(proxy.address().port); + expect(statusCode).toBe(200); + expect(body).toContain('[DEV] Superset'); + expect(body).toContain('<body>hi</body>'); + } finally { + await closeAll(proxy, backend); + } + }); + + test( + 'fails fast instead of hanging when the backend connection drops mid-response (zstd)', + async () => { + const html = `<html><head><title>Superset</title></head><body>${'x'.repeat(20000)}</body></html>`; + const backend = await startBackend(async (req, res) => { + const compressed = await compressBuffer(Buffer.from(html), 3); + res.writeHead(200, { + 'content-type': 'text/html; charset=utf-8', + 'content-encoding': 'zstd', + }); + // Simulate the backend dying mid-response -- e.g. the Flask dev + // server's reloader restarting on a file save -- by writing only + // half the compressed body and then hard-destroying the socket. + // The short delay lets the proxy fully receive the response headers + // first, so this exercises the body-stream-level failure inside + // processHTML rather than a connection-level error that + // http-proxy-middleware's own error handler would intercept first. + res.write(compressed.subarray(0, Math.floor(compressed.length / 2))); + setTimeout(() => res.socket.destroy(), 20); + }); + const proxy = await startProxy(backend.address().port); + + try { + const hangGuard = new Promise((_resolve, reject) => { + setTimeout( + () => + reject( + new Error( + 'request never resolved -- the client-facing response hung ' + + 'instead of the proxy propagating the backend disconnect', + ), + ), + HANG_GUARD_MS, + ); Review Comment: **Suggestion:** Each failure test leaves its `setTimeout` active after `get` resolves successfully. The timer still fires two seconds later and rejects the losing `hangGuard` promise, keeping an unnecessary event-loop handle alive and potentially producing open-handle or delayed-test noise; retain the timer handle and clear it in a `finally` block after the race completes. [resource leak] <details> <summary><b>Severity Level:</b> Minor ๐งน</summary> ```mdx - โ ๏ธ Regression tests retain unnecessary timer handles. - โ ๏ธ Jest runs may report delayed or open-handle noise. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e48826694bb2404ab1ed1a9df732b1ea&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=e48826694bb2404ab1ed1a9df732b1ea&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/tools/webpack.proxy-config.test.js **Line:** 135:145 **Comment:** *Resource Leak: Each failure test leaves its `setTimeout` active after `get` resolves successfully. The timer still fires two seconds later and rejects the losing `hangGuard` promise, keeping an unnecessary event-loop handle alive and potentially producing open-handle or delayed-test noise; retain the timer handle and clear it in a `finally` block after the race completes. 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%2F42811&comment_hash=8224d7e27ff053748d69350eaf7127ee2255609dc4da86af89996fc547d5459f&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42811&comment_hash=8224d7e27ff053748d69350eaf7127ee2255609dc4da86af89996fc547d5459f&reaction=dislike'>๐</a> ########## superset-frontend/webpack.proxy-config.js: ########## @@ -133,23 +133,30 @@ async function processHTML(proxyResponse, response) { } else if (responseEncoding === 'zstd') { uncompress = await zstdDecompress(); } - if (uncompress) { - originalResponse.pipe(uncompress); - originalResponse = uncompress; - } - originalResponse - .on('data', data => { - body = Buffer.concat([body, data]); - }) - .on('error', error => { - // eslint-disable-next-line no-console - console.error(error); - response.end(`Error fetching proxied request: ${error.message}`); - }) - .on('end', () => { - response.end(toDevHTML(body.toString())); - }); + const chunks = []; + const collector = new Writable({ + write(chunk, encoding, callback) { + chunks.push(chunk); + callback(); + }, + }); + + // `pipeline` (unlike `.pipe()`) destroys every stream in the chain -- and + // rejects -- as soon as any one of them errors or closes prematurely. A + // proxied backend connection dying mid-response (e.g. the Flask dev + // server's reloader restarting on a file save) is exactly that case: + // plain `.pipe()` never forwards the upstream error/close to `uncompress`, + // so `uncompress` (and, for `zstd`, the child process backing it) sits + // waiting for input that will never arrive, `end`/`error` never fire, and + // the client-facing response hangs forever instead of failing fast. + await pipeline( + ...(uncompress + ? [proxyResponse, uncompress, collector] + : [proxyResponse, collector]), + ); + + response.end(toDevHTML(Buffer.concat(chunks).toString())); Review Comment: **Suggestion:** When `pipeline` rejects after `response.flushHeaders()` has already sent the backend's successful status, this newly reachable error path writes an error message while retaining the original 2xx status and HTML content type. Clients can therefore treat a truncated backend response as a successful page instead of detecting a failed request; destroy the client response or otherwise preserve an HTTP failure signal when the stream fails after headers are sent. [api mismatch] <details> <summary><b>Severity Level:</b> Major โ ๏ธ</summary> ```mdx - โ Dashboard HTML failures are reported with successful HTTP status. - โ ๏ธ Browser retry and error handling receive misleading responses. - โ ๏ธ Dev proxy clients may treat error bodies as valid HTML. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c888d42bce274cf9821c65c0a3eb07e4&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=c888d42bce274cf9821c65c0a3eb07e4&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/webpack.proxy-config.js **Line:** 153:159 **Comment:** *Api Mismatch: When `pipeline` rejects after `response.flushHeaders()` has already sent the backend's successful status, this newly reachable error path writes an error message while retaining the original 2xx status and HTML content type. Clients can therefore treat a truncated backend response as a successful page instead of detecting a failed request; destroy the client response or otherwise preserve an HTTP failure signal when the stream fails after headers are sent. 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%2F42811&comment_hash=553b64b7187440d40b923c62097c014072cae6a5fbc8298549216404461fa4fc&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42811&comment_hash=553b64b7187440d40b923c62097c014072cae6a5fbc8298549216404461fa4fc&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]
