feat(syntax): use SAM-provided schema instead of goformation #3163

nkomonen-amazon · web-flow · commit 9593551c4753 · 2023-03-07T16:29:04.000-08:00
Problem:
The http resource fetcher class did not provide an option to give an
eTag and attempt to fetch fresh content.

Solution:
Add a function that users can call with an eTag, it will then return the
response which would be new content with a new eTag, OR if the given
eTag is the same as the remote it will give back undefined content.

Problem:
For our doctype suport we use different schemas for cfn and sam. But now
there is an all-in-one sam schema that works for cfn as well.

Solution:
Have cfn and sam use the new sam schema. This requires us to update our
url content fetching + caching mechanisms to use e-tags to determine
stale content instead of using versions as we were previously doing.


* test: integ test for getDefaultSchemas() happy path

    The getDefaultSchemas() function downloads a schema
    if it doesn't exist yet in cache.

    This test uses the telemetry data to determine if
    the cache is used when a request for a schema is made,
    otherwise downloading it.

    - Additionally, this updates the schema assertion function
      to allow the user to choose the event occurence they want
      to assert against.

Signed-off-by: Nikolas Komonen &lt;nkomonen@amazon.com&gt;
diff --git a/src/integrationTest/schema/schema.test.ts b/src/integrationTest/schema/schema.test.ts
@@ -3,6 +3,8 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
+import { getDefaultSchemas, samAndCfnSchemaUrl } from '../../shared/schemas'
+import { FakeExtensionContext } from '../../test/fakeExtensionContext'
 import {
     getCITestSchemas,
     JSONObject,
@@ -12,6 +14,7 @@ import {
     assertRef,
     assertDefinition,
 } from '../../test/shared/schema/testUtils'
+import { assertTelemetry } from '../../test/testUtil'
 
 describe('Sam Schema Regression', function () {
     let samSchema: JSONObject
@@ -61,3 +64,48 @@ describe('Sam Schema Regression', function () {
         assertDefinitionProperty(samSchema, 'AWS::Serverless::Api.Auth', 'AddDefaultAuthorizerToCorsPreflight')
     })
 })
+
+describe('getDefaultSchemas()', () => {
+    let extensionContext: FakeExtensionContext
+
+    beforeEach(async () => {
+        extensionContext = await FakeExtensionContext.create()
+    })
+
+    it('uses cache on subsequent request for CFN/SAM schema', async () => {
+        await getDefaultSchemas(extensionContext)
+
+        // IMPORTANT: Since CFN and SAM use the same schema, the order their schema is retrieved is irrelevant
+
+        // Schema is downloaded on initial retrieval
+        assertTelemetry(
+            'toolkit_getExternalResource',
+            { url: samAndCfnSchemaUrl, passive: true, result: 'Succeeded' },
+            0
+        )
+        // Schema is retrieved from cache on subsequent retrieval
+        assertTelemetry(
+            'toolkit_getExternalResource',
+            { url: samAndCfnSchemaUrl, passive: true, result: 'Cancelled', reason: 'Cache hit' },
+            1
+        )
+    })
+
+    it('uses cache for all requests on second function call', async () => {
+        await getDefaultSchemas(extensionContext)
+        // Call a second time
+        await getDefaultSchemas(extensionContext)
+
+        // Only cache is used
+        assertTelemetry(
+            'toolkit_getExternalResource',
+            { url: samAndCfnSchemaUrl, passive: true, result: 'Cancelled', reason: 'Cache hit' },
+            2
+        )
+        assertTelemetry(
+            'toolkit_getExternalResource',
+            { url: samAndCfnSchemaUrl, passive: true, result: 'Cancelled', reason: 'Cache hit' },
+            3
+        )
+    })
+})
diff --git a/src/shared/resourcefetcher/httpResourceFetcher.ts b/src/shared/resourcefetcher/httpResourceFetcher.ts
@@ -15,6 +15,7 @@ import { getLogger, Logger } from '../logger'
 import { ResourceFetcher } from './resourcefetcher'
 import { Timeout, CancellationError, CancelEvent } from '../utilities/timeoutUtils'
 import { isCloud9 } from '../extensionUtilities'
+import { Headers } from 'got/dist/source/core'
 
 // XXX: patched Got module for compatability with older VS Code versions (e.g. Cloud9)
 // `got` has also deprecated `urlToOptions`
@@ -35,6 +36,8 @@ type FetcherResult = Promise<void> & {
     fsStream: fs.WriteStream
 }
 
+type RequestHeaders = { eTag?: string; gZip?: boolean }
+
 export class HttpResourceFetcher implements ResourceFetcher {
     private readonly logger: Logger = getLogger()
 
@@ -79,6 +82,40 @@ export class HttpResourceFetcher implements ResourceFetcher {
         return this.downloadRequest()
     }
 
+    /**
+     * Requests for new content but additionally uses the given E-Tag.
+     * If no E-Tag is given it behaves as a normal request.
+     *
+     * @param eTag
+     * @returns object with optional content. If content is undefined it implies the provided
+     *          E-Tag matched the server's, so no content was in response. E-Tag is the E-Tag
+     *          of the latest content
+     */
+    public async getNewETagContent(eTag?: string): Promise<{ content?: string; eTag: string }> {
+        const response = await this.getResponseFromGetRequest(this.params.timeout, { eTag, gZip: true })
+
+        const eTagResponse = response.headers.etag
+        if (!eTagResponse) {
+            throw new Error(`This URL does not support E-Tags. Cannot use this function for: ${this.url.toString()}`)
+        }
+
+        // NOTE: Even with use of `gzip` encoding header, the response content is uncompressed.
+        // Most likely due to the http request library uncompressing it for us.
+        let contents: string | undefined = response.body.toString()
+        if (response.statusCode === 304) {
+            // Explanation: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/If-None-Match
+            contents = undefined
+            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> {
         try {
             // HACK(?): receiving JSON as a string without `toString` makes it so we can't deserialize later
@@ -111,7 +148,7 @@ export class HttpResourceFetcher implements ResourceFetcher {
     // TODO: make pipeLocation a vscode.Uri
     private pipeGetRequest(pipeLocation: string, timeout?: Timeout): FetcherResult {
         const requester = isCloud9() ? patchedGot : got
-        const requestStream = requester.stream(this.url, { headers: { 'User-Agent': VSCODE_EXTENSION_ID.awstoolkit } })
+        const requestStream = requester.stream(this.url, { headers: this.buildRequestHeaders() })
         const fsStream = fs.createWriteStream(pipeLocation)
 
         const done = new Promise<void>((resolve, reject) => {
@@ -133,10 +170,10 @@ export class HttpResourceFetcher implements ResourceFetcher {
         return Object.assign(done, { requestStream, fsStream })
     }
 
-    private async getResponseFromGetRequest(timeout?: Timeout): Promise<Response<string>> {
+    private async getResponseFromGetRequest(timeout?: Timeout, headers?: RequestHeaders): Promise<Response<string>> {
         const requester = isCloud9() ? patchedGot : got
         const promise = requester(this.url, {
-            headers: { 'User-Agent': VSCODE_EXTENSION_ID.awstoolkit },
+            headers: this.buildRequestHeaders(headers),
         })
 
         const cancelListener = timeout?.token.onCancellationRequested(event => {
@@ -148,6 +185,22 @@ export class HttpResourceFetcher implements ResourceFetcher {
 
         return promise
     }
+
+    private buildRequestHeaders(requestHeaders?: RequestHeaders): Headers {
+        const headers: Headers = {}
+
+        headers['User-Agent'] = VSCODE_EXTENSION_ID.awstoolkit
+
+        if (requestHeaders?.eTag !== undefined) {
+            headers['If-None-Match'] = requestHeaders.eTag
+        }
+
+        if (requestHeaders?.gZip) {
+            headers['Accept-Encoding'] = 'gzip'
+        }
+
+        return headers
+    }
 }
 
 /**
diff --git a/src/shared/schemas.ts b/src/shared/schemas.ts
@@ -17,8 +17,10 @@ import { AWS_SCHEME } from './constants'
 import { writeFile } from 'fs-extra'
 import { SystemUtilities } from './systemUtilities'
 import { normalizeVSCodeUri } from './utilities/vsCodeUtils'
+import { telemetry } from './telemetry/telemetry'
 
-const goformationManifestUrl = 'https://api.github.com/repos/awslabs/goformation/releases/latest'
+// Note: this file is currently 12+ MB. When requesting it, specify compression/gzip. 
+export const samAndCfnSchemaUrl = 'https://raw.githubusercontent.com/aws/serverless-application-model/main/samtranslator/schema/schema.json'
 const devfileManifestUrl = 'https://api.github.com/repos/devfile/api/releases/latest'
 const schemaPrefix = `${AWS_SCHEME}://`
 
@@ -140,41 +142,41 @@ export class SchemaService {
  * @param extensionContext VSCode extension context
  */
 export async function getDefaultSchemas(extensionContext: vscode.ExtensionContext): Promise<Schemas | undefined> {
-    const cfnSchemaUri = vscode.Uri.joinPath(extensionContext.globalStorageUri, 'cloudformation.schema.json')
-    const samSchemaUri = vscode.Uri.joinPath(extensionContext.globalStorageUri, 'sam.schema.json')
     const devfileSchemaUri = vscode.Uri.joinPath(extensionContext.globalStorageUri, 'devfile.schema.json')
-
-    const goformationSchemaVersion = await getPropertyFromJsonUrl(goformationManifestUrl, 'tag_name')
     const devfileSchemaVersion = await getPropertyFromJsonUrl(devfileManifestUrl, 'tag_name')
 
+    // Sam schema is a superset of Cfn schema, so we can use it for both
+    const samAndCfnSchemaDestinationUri = vscode.Uri.joinPath(extensionContext.globalStorageUri, 'sam.schema.json')
+    const samAndCfnCacheKey = 'samAndCfnSchemaVersion'
+
     const schemas: Schemas = {}
 
     try {
-        await updateSchemaFromRemote({
-            destination: cfnSchemaUri,
-            version: goformationSchemaVersion,
-            url: `https://raw.githubusercontent.com/awslabs/goformation/${goformationSchemaVersion}/schema/cloudformation.schema.json`,
-            cacheKey: 'cfnSchemaVersion',
+        await updateSchemaFromRemoteETag({
+            destination: samAndCfnSchemaDestinationUri,
+            eTag: undefined,
+            url: samAndCfnSchemaUrl,
+            cacheKey: samAndCfnCacheKey,
             extensionContext,
             title: schemaPrefix + 'cloudformation.schema.json',
         })
-        schemas['cfn'] = cfnSchemaUri
+        schemas['cfn'] = samAndCfnSchemaDestinationUri
     } catch (e) {
-        getLogger().verbose('Could not download cfn schema: %s', (e as Error).message)
+        getLogger().verbose('Could not download sam/cfn schema: %s', (e as Error).message)
     }
 
     try {
-        await updateSchemaFromRemote({
-            destination: samSchemaUri,
-            version: goformationSchemaVersion,
-            url: `https://raw.githubusercontent.com/awslabs/goformation/${goformationSchemaVersion}/schema/sam.schema.json`,
-            cacheKey: 'samSchemaVersion',
+        await updateSchemaFromRemoteETag({
+            destination: samAndCfnSchemaDestinationUri,
+            eTag: undefined,
+            url: samAndCfnSchemaUrl,
+            cacheKey: samAndCfnCacheKey,
             extensionContext,
             title: schemaPrefix + 'sam.schema.json',
         })
-        schemas['sam'] = samSchemaUri
+        schemas['sam'] = samAndCfnSchemaDestinationUri
     } catch (e) {
-        getLogger().verbose('Could not download sam schema: %s', (e as Error).message)
+        getLogger().verbose('Could not download sam/cfn schema: %s', (e as Error).message)
     }
 
     try {
@@ -231,13 +233,7 @@ export async function updateSchemaFromRemote(params: {
             throw new Error(`failed to resolve schema: ${params.destination}`)
         }
 
-        const parsedFile = { ...JSON.parse(content), title: params.title }
-        const dir = vscode.Uri.joinPath(params.destination, '..')
-        await SystemUtilities.createDirectory(dir)
-        await writeFile(params.destination.fsPath, JSON.stringify(parsedFile))
-        await params.extensionContext.globalState.update(params.cacheKey, params.version).then(undefined, err => {
-            getLogger().warn(`schemas: failed to update cache key for "${params.title}": ${err?.message}`)
-        })
+        await doCacheContent(content, params)
     } catch (err) {
         if (cachedContent) {
             getLogger().warn(
@@ -251,6 +247,93 @@ export async function updateSchemaFromRemote(params: {
     }
 }
 
+/**
+ * Fetches url content and caches locally. Uses E-Tag to determine if cached
+ * content can be used.
+ * @param params.destination Path to local file
+ * @param params.eTag E-Tag to send with fetch request. If this matches the url's it means we can use our cache.
+ * @param params.url Url to fetch from
+ * @param params.cacheKey Cache key to check version against
+ * @param params.extensionContext VSCode extension context
+ */
+export async function updateSchemaFromRemoteETag(params: {
+    destination: vscode.Uri
+    eTag?: string
+    url: string
+    cacheKey: string
+    extensionContext: vscode.ExtensionContext
+    title: string
+}): Promise<void> {
+    const cachedETag = params.extensionContext.globalState.get<string>(params.cacheKey)
+
+    // Check that the cached file actually can be fetched. Else we might
+    // never update the cache.
+    const fileFetcher = new FileResourceFetcher(params.destination.fsPath)
+    const cachedContent = await fileFetcher.get()
+
+    // Determine if existing cached content is sufficient
+    const needsUpdate = cachedETag === undefined || cachedETag !== params.eTag
+    const hasCachedContent = cachedContent !== undefined
+    if (hasCachedContent && !needsUpdate) {
+        return
+    }
+
+    try {
+        // Only use our cached E-Tag if we have it + cached content
+        const eTagToRequest = !!cachedETag && cachedContent !== undefined ? cachedETag : params.eTag
+
+        const httpFetcher = new HttpResourceFetcher(params.url, { showUrl: true })
+        const response = await httpFetcher.getNewETagContent(eTagToRequest)
+        const { content, eTag: latestETag } = response
+        if (content === undefined) {
+            // Our cached content is the latest
+            telemetry.toolkit_getExternalResource.emit({
+                url: params.url,
+                passive: true,
+                result: 'Cancelled',
+                reason: 'Cache hit',
+            })
+            return
+        }
+        telemetry.toolkit_getExternalResource.emit({ url: params.url, passive: true, result: 'Succeeded' })
+        await doCacheContent(content, { ...params, version: latestETag })
+    } catch (err) {
+        if (cachedContent) {
+            getLogger().warn(
+                `schemas: Using cached schema instead since failed to fetch the latest version for "${params.title}": %s`,
+                err
+            )
+        } else {
+            throw err
+        }
+    }
+}
+
+/**
+ * Cache content to our extension context
+ * @param content
+ * @param params.version An identifier for a version of the resource. Can include an E-Tag value
+ */
+async function doCacheContent(
+    content: string,
+    params: {
+        destination: vscode.Uri
+        version?: string
+        url: string
+        cacheKey: string
+        extensionContext: vscode.ExtensionContext
+        title: string
+    }
+): Promise<void> {
+    const parsedFile = { ...JSON.parse(content), title: params.title }
+    const dir = vscode.Uri.joinPath(params.destination, '..')
+    await SystemUtilities.createDirectory(dir)
+    await writeFile(params.destination.fsPath, JSON.stringify(parsedFile))
+    await params.extensionContext.globalState.update(params.cacheKey, params.version).then(undefined, err => {
+        getLogger().warn(`schemas: failed to update cache key for "${params.title}": ${err?.message}`)
+    })
+}
+
 /**
  * Adds custom tags to the YAML extension's settings in order to hide error
  * notifications for SAM/CFN intrinsic functions if a user has the YAML extension.
diff --git a/src/test/testUtil.ts b/src/test/testUtil.ts
@@ -145,11 +145,18 @@ export function getMetrics<K extends MetricName>(
     return globals.telemetry.logger.query(query) as unknown as Partial<MetricShapes[K]>[]
 }
 
-/*
- * Finds the first emitted telemetry metric with the given name, then checks if the metadata fields
- * match the expected values.
+/**
+ * Finds the emitted telemetry metric with the given name, then checks if the metadata fields
+ * match the expected values. Defaults to checking against the first occurrence, but this
+ * can be overidden.
+ *
+ * @param eventNum The nth occurrence of the event you want to test against, defaults to the first.
  */
-export function assertTelemetry<K extends MetricName>(name: K, expected: MetricShapes[K]): void | never {
+export function assertTelemetry<K extends MetricName>(
+    name: K,
+    expected: MetricShapes[K],
+    eventNum: number = 0
+): void | never {
     const expectedCopy = { ...expected } as { -readonly [P in keyof MetricShapes[K]]: MetricShapes[K][P] }
     const passive = expectedCopy?.passive
     const query = { metricName: name, excludeKeys: ['awsAccount', 'duration'] }
@@ -164,7 +171,7 @@ export function assertTelemetry<K extends MetricName>(name: K, expected: MetricS
 
     const metadata = globals.telemetry.logger.query(query)
     assert.ok(metadata.length > 0, `Telemetry did not contain any metrics with the name "${name}"`)
-    assert.deepStrictEqual(metadata[0], expectedCopy)
+    assert.deepStrictEqual(metadata[eventNum], expectedCopy)
 
     if (passive !== undefined) {
         const metric = globals.telemetry.logger.queryFull(query)
