From 21c7f12132dc4cc72724e1946dc0ce1ea627edc0 Mon Sep 17 00:00:00 2001 From: Avi Alpert <131792194+avi-alpert@users.noreply.github.com> Date: Fri, 31 Jan 2025 15:31:03 -0500 Subject: [PATCH] feat(amazonq): migrate /doc and /dev to zipStream (#6472) ## Problem Users see a `Sorry, I ran into an issue while trying to upload your code` error when using /dev or /doc if files in the repo have a modified or created time before the MSDOS epoch. This affects some users who Remote SSH into a Linux machine. Logs show an out of range exception thrown from the adm-zip package. Adm-zip issue reported [here](https://github.com/cthackers/adm-zip/issues/485) ## Solution Switch to the internal [zipStream module](https://github.com/aws/aws-toolkit-vscode/blob/master/packages/core/src/shared/utilities/zipStream.ts) --- - Treat all work as PUBLIC. Private `feature/x` branches will not be squash-merged at release time. - Your code changes must meet the guidelines in [CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines). - License: I confirm that my contribution is made under the terms of the Apache 2.0 license. --- ...-ca03bdf5-a707-479e-9405-e9ac3b73e30a.json | 4 ++++ .../unit/amazonqFeatureDev/util/files.test.ts | 14 ++++++-------- .../core/src/amazonqFeatureDev/util/files.ts | 19 +++++++++---------- packages/core/src/shared/index.ts | 1 + .../core/src/shared/utilities/zipStream.ts | 15 ++++++++++++++- .../test/shared/utilities/zipStream.test.ts | 11 +++++++++++ .../testInteg/perf/prepareRepoData.test.ts | 5 ++--- 7 files changed, 47 insertions(+), 22 deletions(-) create mode 100644 packages/amazonq/.changes/next-release/Bug Fix-ca03bdf5-a707-479e-9405-e9ac3b73e30a.json diff --git a/packages/amazonq/.changes/next-release/Bug Fix-ca03bdf5-a707-479e-9405-e9ac3b73e30a.json b/packages/amazonq/.changes/next-release/Bug Fix-ca03bdf5-a707-479e-9405-e9ac3b73e30a.json new file mode 100644 index 00000000000..4399da617e7 --- /dev/null +++ b/packages/amazonq/.changes/next-release/Bug Fix-ca03bdf5-a707-479e-9405-e9ac3b73e30a.json @@ -0,0 +1,4 @@ +{ + "type": "Bug Fix", + "description": "Amazon Q: Fix code upload error when using /dev or /doc on Remote SSH" +} diff --git a/packages/amazonq/test/unit/amazonqFeatureDev/util/files.test.ts b/packages/amazonq/test/unit/amazonqFeatureDev/util/files.test.ts index 0bf7df006b2..754558fd9cf 100644 --- a/packages/amazonq/test/unit/amazonqFeatureDev/util/files.test.ts +++ b/packages/amazonq/test/unit/amazonqFeatureDev/util/files.test.ts @@ -11,13 +11,11 @@ import { maxRepoSizeBytes, } from 'aws-core-vscode/amazonqFeatureDev' import { assertTelemetry, getWorkspaceFolder, TestFolder } from 'aws-core-vscode/test' -import { fs, AmazonqCreateUpload } from 'aws-core-vscode/shared' +import { fs, AmazonqCreateUpload, ZipStream } from 'aws-core-vscode/shared' import { MetricName, Span } from 'aws-core-vscode/telemetry' import sinon from 'sinon' import { CodeWhispererSettings } from 'aws-core-vscode/codewhisperer' -import AdmZip from 'adm-zip' - const testDevfilePrepareRepo = async (devfileEnabled: boolean) => { const files: Record = { 'file.md': 'test content', @@ -38,8 +36,8 @@ const testDevfilePrepareRepo = async (devfileEnabled: boolean) => { } const expectedFiles = !devfileEnabled - ? ['./file.md', './.gitignore'] - : ['./devfile.yaml', './file.md', './.gitignore', './abc.jar', 'data/logo.ico'] + ? ['file.md', '.gitignore'] + : ['devfile.yaml', 'file.md', '.gitignore', 'abc.jar', 'data/logo.ico'] const workspace = getWorkspaceFolder(folder.path) sinon @@ -71,8 +69,8 @@ const testPrepareRepoData = async ( } // Unzip the buffer and compare the entry names - const zip = new AdmZip(result.zipFileBuffer) - const actualZipEntries = zip.getEntries().map((entry) => entry.entryName) + const zipEntries = await ZipStream.unzip(result.zipFileBuffer) + const actualZipEntries = zipEntries.map((entry) => entry.filename) actualZipEntries.sort((a, b) => a.localeCompare(b)) assert.deepStrictEqual(actualZipEntries, expectedFiles) } @@ -89,7 +87,7 @@ describe('file utils', () => { await folder.write('file2.md', 'test content') const workspace = getWorkspaceFolder(folder.path) - await testPrepareRepoData(workspace, ['./file1.md', './file2.md']) + await testPrepareRepoData(workspace, ['file1.md', 'file2.md']) }) it('prepareRepoData ignores denied file extensions', async function () { diff --git a/packages/core/src/amazonqFeatureDev/util/files.ts b/packages/core/src/amazonqFeatureDev/util/files.ts index 7e93cf0f9ce..b0949edeca1 100644 --- a/packages/core/src/amazonqFeatureDev/util/files.ts +++ b/packages/core/src/amazonqFeatureDev/util/files.ts @@ -7,11 +7,9 @@ import * as vscode from 'vscode' import * as path from 'path' import { collectFiles } from '../../shared/utilities/workspaceUtils' -import AdmZip from 'adm-zip' import { ContentLengthError, PrepareRepoFailedError } from '../errors' import { getLogger } from '../../shared/logger/logger' import { maxFileSizeBytes } from '../limits' -import { createHash } from 'crypto' import { CurrentWsFolders } from '../types' import { hasCode, ToolkitError } from '../../shared/errors' import { AmazonqCreateUpload, Span, telemetry as amznTelemetry } from '../../shared/telemetry/telemetry' @@ -20,8 +18,7 @@ import { maxRepoSizeBytes } from '../constants' import { isCodeFile } from '../../shared/filetypes' import { fs } from '../../shared' import { CodeWhispererSettings } from '../../codewhisperer' - -const getSha256 = (file: Buffer) => createHash('sha256').update(file).digest('base64') +import { ZipStream } from '../../shared/utilities/zipStream' export async function checkForDevFile(root: string) { const devFilePath = root + '/devfile.yaml' @@ -37,7 +34,7 @@ export async function prepareRepoData( workspaceFolders: CurrentWsFolders, telemetry: TelemetryHelper, span: Span, - zip: AdmZip = new AdmZip() + zip: ZipStream = new ZipStream() ) { try { const autoBuildSetting = CodeWhispererSettings.instance.getAutoBuildSetting() @@ -77,11 +74,12 @@ export async function prepareRepoData( continue } totalBytes += fileSize - - const zipFolderPath = path.dirname(file.zipFilePath) + // Paths in zip should be POSIX compliant regardless of OS + // Reference: https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT + const posixPath = file.zipFilePath.split(path.sep).join(path.posix.sep) try { - zip.addLocalFile(file.fileUri.fsPath, zipFolderPath) + zip.writeFile(file.fileUri.fsPath, posixPath) } catch (error) { if (error instanceof Error && error.message.includes('File not found')) { // No-op: Skip if file was deleted or does not exist @@ -111,11 +109,12 @@ export async function prepareRepoData( telemetry.setRepositorySize(totalBytes) span.record({ amazonqRepositorySize: totalBytes }) + const zipResult = await zip.finalize() - const zipFileBuffer = zip.toBuffer() + const zipFileBuffer = zipResult.streamBuffer.getContents() || Buffer.from('') return { zipFileBuffer, - zipFileChecksum: getSha256(zipFileBuffer), + zipFileChecksum: zipResult.hash, } } catch (error) { getLogger().debug(`featureDev: Failed to prepare repo: ${error}`) diff --git a/packages/core/src/shared/index.ts b/packages/core/src/shared/index.ts index 157b2ead6b0..dffa40df3ee 100644 --- a/packages/core/src/shared/index.ts +++ b/packages/core/src/shared/index.ts @@ -48,6 +48,7 @@ export * from './localizedText' export * as env from './vscode/env' export * from './vscode/commands2' export * from './utilities/pathUtils' +export * from './utilities/zipStream' export * from './errors' export * as messages from './utilities/messages' export * as errors from './errors' diff --git a/packages/core/src/shared/utilities/zipStream.ts b/packages/core/src/shared/utilities/zipStream.ts index a1389676b75..e03e71cd7ff 100644 --- a/packages/core/src/shared/utilities/zipStream.ts +++ b/packages/core/src/shared/utilities/zipStream.ts @@ -6,7 +6,7 @@ import { WritableStreamBuffer } from 'stream-buffers' import crypto from 'crypto' import { readFileAsString } from '../filesystemUtilities' // Use require instead of import since this package doesn't support commonjs -const { ZipWriter, TextReader } = require('@zip.js/zip.js') +const { ZipWriter, TextReader, ZipReader, Uint8ArrayReader } = require('@zip.js/zip.js') import { getLogger } from '../logger/logger' export interface ZipStreamResult { @@ -15,6 +15,10 @@ export interface ZipStreamResult { streamBuffer: WritableStreamBuffer } +export type ZipReaderResult = { + filename: string +} + export type ZipStreamProps = { hashAlgorithm: 'md5' | 'sha256' maxNumberOfFileStreams: number @@ -150,4 +154,13 @@ export class ZipStream { streamBuffer: this._streamBuffer, } } + + public static async unzip(zipBuffer: Buffer): Promise { + const reader = new ZipReader(new Uint8ArrayReader(new Uint8Array(zipBuffer))) + try { + return await reader.getEntries() + } finally { + await reader.close() + } + } } diff --git a/packages/core/src/test/shared/utilities/zipStream.test.ts b/packages/core/src/test/shared/utilities/zipStream.test.ts index 96ae3c73509..ada48c79ccf 100644 --- a/packages/core/src/test/shared/utilities/zipStream.test.ts +++ b/packages/core/src/test/shared/utilities/zipStream.test.ts @@ -60,4 +60,15 @@ describe('zipStream', function () { assert.strictEqual(result.hash, expectedMd5) assert.strictEqual(result.sizeInBytes, (await fs.stat(zipPath)).size) }) + + it('should unzip from a buffer', async function () { + const zipStream = new ZipStream() + await zipStream.writeString('foo bar', 'file.txt') + const result = await zipStream.finalize() + + const zipBuffer = result.streamBuffer.getContents() + assert.ok(zipBuffer) + const zipEntries = await ZipStream.unzip(zipBuffer) + assert.strictEqual(zipEntries[0].filename, 'file.txt') + }) }) diff --git a/packages/core/src/testInteg/perf/prepareRepoData.test.ts b/packages/core/src/testInteg/perf/prepareRepoData.test.ts index 18a8481954a..eb11f183ae8 100644 --- a/packages/core/src/testInteg/perf/prepareRepoData.test.ts +++ b/packages/core/src/testInteg/perf/prepareRepoData.test.ts @@ -30,7 +30,7 @@ function performanceTestWrapper(numFiles: number, fileSize: number) { getEqualOSTestOptions({ userCpuUsage: 200, systemCpuUsage: 35, - heapTotal: 4, + heapTotal: 20, }), `handles ${numFiles} files of size ${fileSize} bytes`, function () { @@ -63,8 +63,7 @@ function verifyResult(setup: setupResult, result: resultType, telemetry: Telemet assert.strictEqual(Buffer.isBuffer(result.zipFileBuffer), true) assert.strictEqual(telemetry.repositorySize, expectedSize) assert.strictEqual(result.zipFileChecksum.length, 44) - - assert.ok(getFsCallsUpperBound(setup.fsSpy) <= setup.numFiles * 4, 'total system calls should be under 4 per file') + assert.ok(getFsCallsUpperBound(setup.fsSpy) <= setup.numFiles * 8, 'total system calls should be under 8 per file') } describe('prepareRepoData', function () {