feat(storage-browser): validate schema of data received from read APIs #6213
Conversation
|
|
|
||
| describe('checkRequiredKeys', () => { | ||
| it('should not throw when all required keys are present', () => { | ||
| const object: TestObject = { |
There was a problem hiding this comment.
Prefer declarative naming of variables implying their use case (e.g. input)
| key1?: string | null; | ||
| key2?: string | null; | ||
| key3?: string | null; | ||
| extraKey?: string | null; |
There was a problem hiding this comment.
What if the values provided are of type number?
| @@ -127,4 +128,37 @@ describe('listLocationsHandler', () => { | |||
| 'Storage Browser: Must provide accountId to `listCallerAccessGrants`.' | |||
| ); | |||
| }); | |||
|
|
|||
| it('should throw if list does not provide locations', async () => { | |||
| input.config.accountId = accountId; | |||
There was a problem hiding this comment.
See that mutating the test input is an existing pattern but these new tests need to create a new input for each call to avoid non-deterministic behavior between individual tests, example:
await expect(listLocationsHandler({ ...input, config: { ...input.config, accountId }})).rejects.toThrow(
'Required keys missing for ListLocationsOutput: locations.'
);| @@ -54,7 +55,9 @@ export const downloadHandler: DownloadHandler = ({ | |||
| expectedBucketOwner: accountId, | |||
| }, | |||
| }) | |||
| .then(({ url }) => { | |||
| .then((result) => { | |||
| checkRequiredKeys(result, 'StorageGetUrlOutput', ['url']); | |||
There was a problem hiding this comment.
Does the UI handle the error thrown here gracefully?
| throw Error( | ||
| `Required keys missing for ${objectName}: ${[...missingValues].join( | ||
| ', ' | ||
| )}.\nObject: ${JSON.stringify(objectToCheck)}` | ||
| ); |
There was a problem hiding this comment.
How is this intended to be surfaced to an end user and what actions are they expected to take in a missing key scenario?
| @@ -150,6 +151,33 @@ describe('listLocationItemsHandler', () => { | |||
| }) | |||
| ); | |||
| }); | |||
|
|
|||
| it('should throw if list does not provide items', async () => { | |||
| mockList.mockResolvedValueOnce({} as any); | |||
There was a problem hiding this comment.
Prefer @ts-expect-error with a concise comment explaining the need for the invalid response
| ); | ||
| }); | ||
|
|
||
| ['path', 'lastModified', 'size'].forEach((requiredKey) => { |
| }); | ||
|
|
||
| await expect(listLocationItemsHandler(baseInput)).rejects.toThrow( | ||
| `Required keys missing for ListOutputItem #1: ${requiredKey}.` |
There was a problem hiding this comment.
If a single key is missing why pluralize?
| objectToCheck: T, | ||
| objectName: string, |
There was a problem hiding this comment.
Please updates these names to align with function parameter naming within the code base, for example:
- objectToCheck: T,
+ input: T,
- objectName: string,
+ displayName: string,| @@ -70,4 +71,15 @@ describe('downloadHandler', () => { | |||
| message: errorMessage, | |||
| }); | |||
| }); | |||
|
|
|||
| it('should fail if getUrl does not return a url', async () => { | |||
| mockGetUrl.mockResolvedValue({} as any); | |||
There was a problem hiding this comment.
Prefer @ts-expect-error with a concise comment explaining the need for the invalid response
Description of changes
checkRequiredKeysis intentionally forwards-compatible so these checks do not have to be updated every time amplify-js adds properties to the response that storage browser doesn't use.Issue #, if available
Description of how you validated changes
Checklist
yarn testpasses and tests are updated/addeddocs,e2e,examples, or other private packages.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.