Updates to @actions/artifact package (#367)

* GZip implementation

* Optimizations and cleanup

* Update tests

* More test updates

* Update packages/artifact/src/internal-utils.ts

Co-Authored-By: Josh Gross <joshmgross@github.com>

* Clarification around Upload Paths

* Refactor to make http clients classes

* GZip fixes

* Documentation around compression

* More detailed status information during large uploads

* Pretty format

* Percentage updates without rounding

* Fix edge cases with formatting numbers

* Update packages/artifact/src/internal-utils.ts

Co-Authored-By: Josh Gross <joshmgross@github.com>

* Cleanup

* Small reorg with status reporter

* PR Feedback

* Cleanup + Simplification

* Test Cleanup

* Mock updates

* More cleanup

* Format fixes

* Overhaul to the http-manager

* Fix tests

* Promisify stats

* Documentation around implementation

* Improvements to documentation

* PR Feedback

* Remove Downloading multiple artifacts concurrently

Co-authored-by: Josh Gross <joshmgross@github.com>

packages/artifact/__tests__/download-specification.test.ts

@@ -1,8 +1,8 @@
 import * as path from 'path'
 import * as core from '@actions/core'
 import {URL} from 'url'
-import {getDownloadSpecification} from '../src/internal-download-specification'
-import {ContainerEntry} from '../src/internal-contracts'
+import {getDownloadSpecification} from '../src/internal/download-specification'
+import {ContainerEntry} from '../src/internal/contracts'
 
 const artifact1Name = 'my-artifact'
 const artifact2Name = 'my-artifact-extra'

packages/artifact/__tests__/download.test.ts

@@ -3,17 +3,17 @@ import * as http from 'http'
 import * as io from '../../io/src/io'
 import * as net from 'net'
 import * as path from 'path'
-import * as configVariables from '../src/internal-config-variables'
+import * as configVariables from '../src/internal/config-variables'
 import {HttpClient, HttpClientResponse} from '@actions/http-client'
-import * as downloadClient from '../src/internal-download-http-client'
+import {DownloadHttpClient} from '../src/internal/download-http-client'
 import {
   ListArtifactsResponse,
   QueryArtifactResponse
-} from '../src/internal-contracts'
+} from '../src/internal/contracts'
 
 const root = path.join(__dirname, '_temp', 'artifact-download')
 
-jest.mock('../src/internal-config-variables')
+jest.mock('../src/internal/config-variables')
 jest.mock('@actions/http-client')
 
 describe('Download Tests', () => {
@@ -32,7 +32,8 @@ describe('Download Tests', () => {
    */
   it('List Artifacts - Success', async () => {
     setupSuccessfulListArtifactsResponse()
-    const artifacts = await downloadClient.listArtifacts()
+    const downloadHttpClient = new DownloadHttpClient()
+    const artifacts = await downloadHttpClient.listArtifacts()
     expect(artifacts.count).toEqual(2)
 
     const artifactNames = artifacts.value.map(item => item.name)
@@ -58,7 +59,8 @@ describe('Download Tests', () => {
 
   it('List Artifacts - Failure', async () => {
     setupFailedResponse()
-    expect(downloadClient.listArtifacts()).rejects.toThrow(
+    const downloadHttpClient = new DownloadHttpClient()
+    expect(downloadHttpClient.listArtifacts()).rejects.toThrow(
       'Unable to list artifacts for the run'
     )
   })
@@ -68,7 +70,8 @@ describe('Download Tests', () => {
    */
   it('Container Items - Success', async () => {
     setupSuccessfulContainerItemsResponse()
-    const response = await downloadClient.getContainerItems(
+    const downloadHttpClient = new DownloadHttpClient()
+    const response = await downloadHttpClient.getContainerItems(
       'artifact-name',
       configVariables.getRuntimeUrl()
     )
@@ -93,8 +96,9 @@ describe('Download Tests', () => {
 
   it('Container Items - Failure', async () => {
     setupFailedResponse()
+    const downloadHttpClient = new DownloadHttpClient()
     expect(
-      downloadClient.getContainerItems(
+      downloadHttpClient.getContainerItems(
         'artifact-name',
         configVariables.getRuntimeUrl()
       )

packages/artifact/__tests__/upload-specification.test.ts

@@ -2,7 +2,7 @@ import * as io from '../../io/src/io'
 import * as path from 'path'
 import {promises as fs} from 'fs'
 import * as core from '@actions/core'
-import {getUploadSpecification} from '../src/internal-upload-specification'
+import {getUploadSpecification} from '../src/internal/upload-specification'
 
 const artifactName = 'my-artifact'
 const root = path.join(__dirname, '_temp', 'upload-specification')

packages/artifact/__tests__/upload.test.ts

@@ -2,16 +2,16 @@ import * as http from 'http'
 import * as io from '../../io/src/io'
 import * as net from 'net'
 import * as path from 'path'
-import * as uploadHttpClient from '../src/internal-upload-http-client'
+import {UploadHttpClient} from '../src/internal/upload-http-client'
 import * as core from '@actions/core'
 import {promises as fs} from 'fs'
-import {getRuntimeUrl} from '../src/internal-config-variables'
+import {getRuntimeUrl} from '../src/internal/config-variables'
 import {HttpClient, HttpClientResponse} from '@actions/http-client'
 import {
   ArtifactResponse,
   PatchArtifactSizeSuccessResponse
-} from '../src/internal-contracts'
-import {UploadSpecification} from '../src/internal-upload-specification'
+} from '../src/internal/contracts'
+import {UploadSpecification} from '../src/internal/upload-specification'
 
 const root = path.join(__dirname, '_temp', 'artifact-upload')
 const file1Path = path.join(root, 'file1.txt')
@@ -26,7 +26,7 @@ let file3Size = 0
 let file4Size = 0
 let file5Size = 0
 
-jest.mock('../src/internal-config-variables')
+jest.mock('../src/internal/config-variables')
 jest.mock('@actions/http-client')
 
 describe('Upload Tests', () => {
@@ -75,6 +75,7 @@ describe('Upload Tests', () => {
    */
   it('Create Artifact - Success', async () => {
     const artifactName = 'valid-artifact-name'
+    const uploadHttpClient = new UploadHttpClient()
     const response = await uploadHttpClient.createArtifactInFileContainer(
       artifactName
     )
@@ -93,6 +94,7 @@ describe('Upload Tests', () => {
 
   it('Create Artifact - Failure', async () => {
     const artifactName = 'invalid-artifact-name'
+    const uploadHttpClient = new UploadHttpClient()
     expect(
       uploadHttpClient.createArtifactInFileContainer(artifactName)
     ).rejects.toEqual(
@@ -137,12 +139,13 @@ describe('Upload Tests', () => {
     const expectedTotalSize =
       file1Size + file2Size + file3Size + file4Size + file5Size
     const uploadUrl = `${getRuntimeUrl()}_apis/resources/Containers/13`
+    const uploadHttpClient = new UploadHttpClient()
     const uploadResult = await uploadHttpClient.uploadArtifactToFileContainer(
       uploadUrl,
       uploadSpecification
     )
     expect(uploadResult.failedItems.length).toEqual(0)
-    expect(uploadResult.size).toEqual(expectedTotalSize)
+    expect(uploadResult.uploadSize).toEqual(expectedTotalSize)
   })
 
   it('Upload Artifact - Failed Single File Upload', async () => {
@@ -154,12 +157,13 @@ describe('Upload Tests', () => {
     ]
 
     const uploadUrl = `${getRuntimeUrl()}_apis/resources/Containers/13`
+    const uploadHttpClient = new UploadHttpClient()
     const uploadResult = await uploadHttpClient.uploadArtifactToFileContainer(
       uploadUrl,
       uploadSpecification
     )
     expect(uploadResult.failedItems.length).toEqual(1)
-    expect(uploadResult.size).toEqual(0)
+    expect(uploadResult.uploadSize).toEqual(0)
   })
 
   it('Upload Artifact - Partial Upload Continue On Error', async () => {
@@ -189,13 +193,14 @@ describe('Upload Tests', () => {
 
     const expectedPartialSize = file1Size + file2Size + file4Size + file5Size
     const uploadUrl = `${getRuntimeUrl()}_apis/resources/Containers/13`
+    const uploadHttpClient = new UploadHttpClient()
     const uploadResult = await uploadHttpClient.uploadArtifactToFileContainer(
       uploadUrl,
       uploadSpecification,
       {continueOnError: true}
     )
     expect(uploadResult.failedItems.length).toEqual(1)
-    expect(uploadResult.size).toEqual(expectedPartialSize)
+    expect(uploadResult.uploadSize).toEqual(expectedPartialSize)
   })
 
   it('Upload Artifact - Partial Upload Fail Fast', async () => {
@@ -225,13 +230,14 @@ describe('Upload Tests', () => {
 
     const expectedPartialSize = file1Size + file2Size + file3Size
     const uploadUrl = `${getRuntimeUrl()}_apis/resources/Containers/13`
+    const uploadHttpClient = new UploadHttpClient()
     const uploadResult = await uploadHttpClient.uploadArtifactToFileContainer(
       uploadUrl,
       uploadSpecification,
       {continueOnError: false}
     )
     expect(uploadResult.failedItems.length).toEqual(2)
-    expect(uploadResult.size).toEqual(expectedPartialSize)
+    expect(uploadResult.uploadSize).toEqual(expectedPartialSize)
   })
 
   it('Upload Artifact - Failed upload with no options', async () => {
@@ -261,12 +267,13 @@ describe('Upload Tests', () => {
 
     const expectedPartialSize = file1Size + file2Size + file3Size + file5Size
     const uploadUrl = `${getRuntimeUrl()}_apis/resources/Containers/13`
+    const uploadHttpClient = new UploadHttpClient()
     const uploadResult = await uploadHttpClient.uploadArtifactToFileContainer(
       uploadUrl,
       uploadSpecification
     )
     expect(uploadResult.failedItems.length).toEqual(1)
-    expect(uploadResult.size).toEqual(expectedPartialSize)
+    expect(uploadResult.uploadSize).toEqual(expectedPartialSize)
   })
 
   it('Upload Artifact - Failed upload with empty options', async () => {
@@ -296,25 +303,28 @@ describe('Upload Tests', () => {
 
     const expectedPartialSize = file1Size + file2Size + file3Size + file5Size
     const uploadUrl = `${getRuntimeUrl()}_apis/resources/Containers/13`
+    const uploadHttpClient = new UploadHttpClient()
     const uploadResult = await uploadHttpClient.uploadArtifactToFileContainer(
       uploadUrl,
       uploadSpecification,
       {}
     )
     expect(uploadResult.failedItems.length).toEqual(1)
-    expect(uploadResult.size).toEqual(expectedPartialSize)
+    expect(uploadResult.uploadSize).toEqual(expectedPartialSize)
   })
 
   /**
    * Artifact Association Tests
    */
   it('Associate Artifact - Success', async () => {
+    const uploadHttpClient = new UploadHttpClient()
     expect(async () => {
       uploadHttpClient.patchArtifactSize(130, 'my-artifact')
     }).not.toThrow()
   })
 
   it('Associate Artifact - Not Found', async () => {
+    const uploadHttpClient = new UploadHttpClient()
     expect(
       uploadHttpClient.patchArtifactSize(100, 'non-existent-artifact')
     ).rejects.toThrow(
@@ -323,6 +333,7 @@ describe('Upload Tests', () => {
   })
 
   it('Associate Artifact - Error', async () => {
+    const uploadHttpClient = new UploadHttpClient()
     expect(
       uploadHttpClient.patchArtifactSize(-2, 'my-artifact')
     ).rejects.toThrow('Unable to finish uploading artifact my-artifact')

packages/artifact/__tests__/util.test.ts

@@ -1,12 +1,12 @@
 import * as fs from 'fs'
 import * as io from '../../io/src/io'
 import * as path from 'path'
-import * as utils from '../src/internal-utils'
+import * as utils from '../src/internal/utils'
 import * as core from '@actions/core'
 import {HttpCodes} from '@actions/http-client'
-import {getRuntimeUrl, getWorkFlowRunId} from '../src/internal-config-variables'
+import {getRuntimeUrl, getWorkFlowRunId} from '../src/internal/config-variables'
 
-jest.mock('../src/internal-config-variables')
+jest.mock('../src/internal/config-variables')
 
 describe('Utils', () => {
   beforeAll(() => {
@@ -49,6 +49,36 @@ describe('Utils', () => {
     }
   })
 
+  it('Check Artifact File Path for any invalid characters', () => {
+    const invalidNames = [
+      'some/invalid"artifact/path',
+      'some/invalid:artifact/path',
+      'some/invalid<artifact/path',
+      'some/invalid>artifact/path',
+      'some/invalid|artifact/path',
+      'some/invalid*artifact/path',
+      'some/invalid?artifact/path',
+      'some/invalid artifact/path',
+      ''
+    ]
+    for (const invalidName of invalidNames) {
+      expect(() => {
+        utils.checkArtifactFilePath(invalidName)
+      }).toThrow()
+    }
+
+    const validNames = [
+      'my/perfectly-normal/artifact-path',
+      'my/perfectly\\Normal/Artifact-path',
+      'm¥/ñðrmål/Är†ï£å¢†'
+    ]
+    for (const validName of validNames) {
+      expect(() => {
+        utils.checkArtifactFilePath(validName)
+      }).not.toThrow()
+    }
+  })
+
   it('Test constructing artifact URL', () => {
     const runtimeUrl = getRuntimeUrl()
     const runId = getWorkFlowRunId()
@@ -61,13 +91,25 @@ describe('Utils', () => {
   it('Test constructing headers with all optional parameters', () => {
     const type = 'application/json'
     const size = 24
+    const uncompressedLength = 100
     const range = 'bytes 0-199/200'
-    const options = utils.getRequestOptions(type, size, range)
-    expect(Object.keys(options).length).toEqual(4)
+    const options = utils.getRequestOptions(
+      type,
+      true,
+      true,
+      uncompressedLength,
+      size,
+      range
+    )
+    expect(Object.keys(options).length).toEqual(8)
     expect(options['Accept']).toEqual(
       `${type};api-version=${utils.getApiVersion()}`
     )
     expect(options['Content-Type']).toEqual(type)
+    expect(options['Connection']).toEqual('Keep-Alive')
+    expect(options['Keep-Alive']).toEqual('10')
+    expect(options['Content-Encoding']).toEqual('gzip')
+    expect(options['x-tfs-filelength']).toEqual(uncompressedLength)
     expect(options['Content-Length']).toEqual(size)
     expect(options['Content-Range']).toEqual(range)
   })