Merge pull request aws#6390 from jpinkney-aws/fetcher

refactor(core): Improve httpResourceFetcher API

Author: jpinkney-aws

packages/core/src/lambda/utils.ts

@@ -13,8 +13,6 @@ import { CloudFormationClient } from '../shared/clients/cloudFormationClient'
 import { LambdaClient } from '../shared/clients/lambdaClient'
 import { getFamily, getNodeMajorVersion, RuntimeFamily } from './models/samLambdaRuntime'
 import { getLogger } from '../shared/logger'
-import { ResourceFetcher } from '../shared/resourcefetcher/resourcefetcher'
-import { CompositeResourceFetcher } from '../shared/resourcefetcher/compositeResourceFetcher'
 import { HttpResourceFetcher } from '../shared/resourcefetcher/httpResourceFetcher'
 import { FileResourceFetcher } from '../shared/resourcefetcher/fileResourceFetcher'
 import { sampleRequestManifestPath } from './constants'
@@ -99,7 +97,7 @@ interface SampleRequestManifest {
 
 export async function getSampleLambdaPayloads(): Promise<SampleRequest[]> {
     const logger = getLogger()
-    const sampleInput = await makeSampleRequestManifestResourceFetcher().get()
+    const sampleInput = await getSampleRequestManifest()
 
     if (!sampleInput) {
         throw new Error('Unable to retrieve Sample Request manifest')
@@ -120,9 +118,11 @@ export async function getSampleLambdaPayloads(): Promise<SampleRequest[]> {
     return inputs
 }
 
-function makeSampleRequestManifestResourceFetcher(): ResourceFetcher {
-    return new CompositeResourceFetcher(
-        new HttpResourceFetcher(sampleRequestManifestPath, { showUrl: true }),
-        new FileResourceFetcher(globals.manifestPaths.lambdaSampleRequests)
-    )
+async function getSampleRequestManifest(): Promise<string | undefined> {
+    const httpResp = await new HttpResourceFetcher(sampleRequestManifestPath, { showUrl: true }).get()
+    if (!httpResp) {
+        const fileResp = new FileResourceFetcher(globals.manifestPaths.lambdaSampleRequests)
+        return fileResp.get()
+    }
+    return httpResp.text()
 }

packages/core/src/lambda/vue/configEditor/samInvokeBackend.ts

@@ -170,7 +170,8 @@ export class SamInvokeWebview extends VueWebview {
                 return
             }
             const sampleUrl = `${sampleRequestPath}${pickerResponse.filename}`
-            const sample = (await new HttpResourceFetcher(sampleUrl, { showUrl: true }).get()) ?? ''
+            const resp = await new HttpResourceFetcher(sampleUrl, { showUrl: true }).get()
+            const sample = (await resp?.text()) ?? ''
 
             return sample
         } catch (err) {

packages/core/src/lambda/vue/remoteInvoke/invokeLambda.ts

@@ -219,7 +219,8 @@ export class RemoteInvokeWebview extends VueWebview {
                 return
             }
             const sampleUrl = `${sampleRequestPath}${pickerResponse.filename}`
-            const sample = (await new HttpResourceFetcher(sampleUrl, { showUrl: true }).get()) ?? ''
+            const resp = await new HttpResourceFetcher(sampleUrl, { showUrl: true }).get()
+            const sample = (await resp?.text()) ?? ''
 
             return sample
         } catch (err) {

packages/core/src/shared/resourcefetcher/compositeResourceFetcher.ts

@@ -8,10 +8,11 @@ import { getLogger, Logger } from '../logger'
 import { ResourceFetcher } from './resourcefetcher'
 import { Timeout, CancelEvent } from '../utilities/timeoutUtils'
 import request, { RequestError } from '../request'
+import { withRetries } from '../utilities/functionUtils'
 
 type RequestHeaders = { eTag?: string; gZip?: boolean }
 
-export class HttpResourceFetcher implements ResourceFetcher {
+export class HttpResourceFetcher implements ResourceFetcher<Response> {
     private readonly logger: Logger = getLogger()
 
     /**
@@ -20,27 +21,27 @@ export class HttpResourceFetcher implements ResourceFetcher {
      * @param params Additional params for the fetcher
      * @param {boolean} params.showUrl Whether or not to the URL in log statements.
      * @param {string} params.friendlyName If URL is not shown, replaces the URL with this text.
-     * @param {function} params.onSuccess Function to execute on successful request. No effect if piping to a location.
      * @param {Timeout} params.timeout Timeout token to abort/cancel the request. Similar to `AbortSignal`.
+     * @param {number} params.retries The number of retries a get request should make if one fails
      */
     public constructor(
         private readonly url: string,
         private readonly params: {
             showUrl: boolean
             friendlyName?: string
-            onSuccess?(contents: string): void
             timeout?: Timeout
+            retries?: number
         }
     ) {}
 
     /**
-     * Returns the contents of the resource, or undefined if the resource could not be retrieved.
-     *
-     * @param pipeLocation Optionally pipe the download to a file system location
+     * Returns the response of the resource, or undefined if the response failed could not be retrieved.
      */
-    public get(): Promise<string | undefined> {
+    public get(): Promise<Response | undefined> {
         this.logger.verbose(`downloading: ${this.logText()}`)
-        return this.downloadRequest()
+        return withRetries(() => this.downloadRequest(), {
+            maxRetries: this.params.retries ?? 1,
+        })
     }
 
     /**
@@ -69,26 +70,16 @@ export class HttpResourceFetcher implements ResourceFetcher {
             this.logger.verbose(`E-Tag, ${eTagResponse}, matched. No content downloaded from: ${this.url}`)
         } else {
             this.logger.verbose(`No E-Tag match. Downloaded content from: ${this.logText()}`)
-            if (this.params.onSuccess) {
-                this.params.onSuccess(contents)
-            }
         }
 
         return { content: contents, eTag: eTagResponse }
     }
 
-    private async downloadRequest(): Promise<string | undefined> {
+    private async downloadRequest(): Promise<Response | undefined> {
         try {
-            // HACK(?): receiving JSON as a string without `toString` makes it so we can't deserialize later
             const resp = await this.getResponseFromGetRequest(this.params.timeout)
-            const contents = (await resp.text()).toString()
-            if (this.params.onSuccess) {
-                this.params.onSuccess(contents)
-            }
-
             this.logger.verbose(`downloaded: ${this.logText()}`)
-
-            return contents
+            return resp
         } catch (err) {
             const error = err as RequestError
             this.logger.verbose(
@@ -150,7 +141,8 @@ export async function getPropertyFromJsonUrl(
     fetcher?: HttpResourceFetcher
 ): Promise<any | undefined> {
     const resourceFetcher = fetcher ?? new HttpResourceFetcher(url, { showUrl: true })
-    const result = await resourceFetcher.get()
+    const resp = await resourceFetcher.get()
+    const result = await resp?.text()
     if (result) {
         try {
             const json = JSON.parse(result)

packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts

@@ -4,10 +4,10 @@
  */
 
 // TODO: this is just a "thunk". Replace it with something more generic.
-export interface ResourceFetcher {
+export interface ResourceFetcher<T = string> {
     /**
      * Returns the contents of the resource, or undefined if the resource could not be retrieved.
      * Implementations are expected to handle Errors.
      */
-    get(): Promise<string | undefined>
+    get(): Promise<T | undefined>
 }

packages/core/src/shared/resourcefetcher/resourcefetcher.ts

@@ -196,8 +196,8 @@ async function downloadInstallScript(debuggerPath: string): Promise<string> {
         installScriptPath = path.join(debuggerPath, 'installVsdbgScript.sh')
     }
 
-    const installScriptFetcher = new HttpResourceFetcher(installScriptUrl, { showUrl: true })
-    const installScript = await installScriptFetcher.get()
+    const installScriptFetcher = await new HttpResourceFetcher(installScriptUrl, { showUrl: true }).get()
+    const installScript = await installScriptFetcher?.text()
     if (!installScript) {
         throw Error(`Failed to download ${installScriptUrl}`)
     }

packages/core/src/shared/sam/debugger/csharpSamDebug.ts

@@ -225,7 +225,8 @@ export async function updateSchemaFromRemote(params: {
 
     try {
         const httpFetcher = new HttpResourceFetcher(params.url, { showUrl: true })
-        const content = await httpFetcher.get()
+        const resp = await httpFetcher.get()
+        const content = await resp?.text()
 
         if (!content) {
             throw new Error(`failed to resolve schema: ${params.destination}`)

packages/core/src/shared/schemas.ts

@@ -179,8 +179,8 @@ async function httpsGetRequestWrapper(url: string): Promise<string> {
     const logger = getLogger()
     logger.verbose('Step Functions is getting content...')
 
-    const fetcher = new HttpResourceFetcher(url, { showUrl: true })
-    const val = await fetcher.get()
+    const fetcher = await new HttpResourceFetcher(url, { showUrl: true }).get()
+    const val = await fetcher?.text()
 
     if (!val) {
         const message = 'Step Functions was unable to get content.'

packages/core/src/stepFunctions/utils.ts

@@ -21,6 +21,7 @@ import { FakeExtensionContext } from '../../../fakeExtensionContext'
 import * as samCliRemoteTestEvent from '../../../../shared/sam/cli/samCliRemoteTestEvent'
 import { TestEventsOperation, SamCliRemoteTestEventsParameters } from '../../../../shared/sam/cli/samCliRemoteTestEvent'
 import { assertLogsContain } from '../../../globalSetup.test'
+import { createResponse } from '../../../testUtil'
 
 describe('RemoteInvokeWebview', () => {
     let outputChannel: vscode.OutputChannel
@@ -394,7 +395,7 @@ describe('RemoteInvokeWebview', () => {
             createQuickPickStub.returns({})
             promptUserStub.resolves([{ label: 'testEvent', filename: 'testEvent.json' }])
             verifySinglePickerOutputStub.returns({ label: 'testEvent', filename: 'testEvent.json' })
-            httpFetcherStub.resolves(mockSampleContent)
+            httpFetcherStub.resolves(createResponse(mockSampleContent))
 
             const result = await remoteInvokeWebview.getSamplePayload()
 

packages/core/src/test/lambda/vue/remoteInvoke/invokeLambda.test.ts

@@ -28,6 +28,7 @@ import { SamDebugConfigProvider } from '../../../shared/sam/debugger/awsSamDebug
 import sinon from 'sinon'
 import * as nls from 'vscode-nls'
 import { assertLogsContain } from '../../../test/globalSetup.test'
+import { createResponse } from '../../testUtil'
 
 const localize = nls.loadMessageBundle()
 
@@ -166,7 +167,7 @@ describe('SamInvokeWebview', () => {
             createQuickPickStub.returns({})
             promptUserStub.resolves([{ label: 'testEvent', filename: 'testEvent.json' }])
             verifySinglePickerOutputStub.returns({ label: 'testEvent', filename: 'testEvent.json' })
-            httpFetcherStub.resolves(mockSampleContent)
+            httpFetcherStub.resolves(createResponse(mockSampleContent))
 
             const result = await samInvokeWebview.getSamplePayload()
 

packages/core/src/test/lambda/vue/samInvokeBackend.test.ts

@@ -6,6 +6,7 @@
 import assert from 'assert'
 import { HttpResourceFetcher, getPropertyFromJsonUrl } from '../../../shared/resourcefetcher/httpResourceFetcher'
 import { stub } from '../../utilities/stubber'
+import { createResponse } from '../../testUtil'
 
 describe('getPropertyFromJsonUrl', function () {
     const dummyUrl = 'url'
@@ -19,18 +20,18 @@ describe('getPropertyFromJsonUrl', function () {
 
     it('undefined if resource is not JSON', async function () {
         const mockFetcher = stub(HttpResourceFetcher)
-        mockFetcher.get.resolves('foo' as any) // horrible hack: this works without the declaration but the language server latches onto this using a FetcherResult return type
+        mockFetcher.get.resolves(createResponse('foo'))
         assert.strictEqual(await getPropertyFromJsonUrl(dummyUrl, dummyProperty, mockFetcher), undefined)
     })
 
     it('undefined if property is not present', async function () {
         const mockFetcher = stub(HttpResourceFetcher)
-        mockFetcher.get.resolves('{"foo": "bar"}' as any)
+        mockFetcher.get.resolves(createResponse('{"foo": "bar"}'))
         assert.strictEqual(await getPropertyFromJsonUrl(dummyUrl, dummyProperty, mockFetcher), undefined)
     })
     it('returns value if property is present', async function () {
         const mockFetcher = stub(HttpResourceFetcher)
-        mockFetcher.get.resolves('{"property": "111"}' as any)
+        mockFetcher.get.resolves(createResponse('{"property": "111"}'))
         mockFetcher.getNewETagContent.resolves({ content: 'foo', eTag: '' })
         const value = await getPropertyFromJsonUrl(dummyUrl, dummyProperty, mockFetcher)
         assert.strictEqual(value, '111')

packages/core/src/test/shared/resourceFetcher/compositeResourceFetcher.test.ts

@@ -18,7 +18,7 @@ import { DeclaredCommand } from '../shared/vscode/commands2'
 import { mkdirSync, existsSync } from 'fs' // eslint-disable-line no-restricted-imports
 import { randomBytes } from 'crypto'
 import request from '../shared/request'
-import { stub } from 'sinon'
+import { createStubInstance, stub } from 'sinon'
 
 const testTempDirs: string[] = []
 
@@ -634,3 +634,10 @@ export function getFetchStubWithResponse(response: Partial<Response>) {
 export function copyEnv(): NodeJS.ProcessEnv {
     return { ...process.env }
 }
+
+// Returns a stubbed response object
+export function createResponse(text: string): Response {
+    const responseStub = createStubInstance(Response)
+    responseStub.text.resolves(text)
+    return responseStub
+}