Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix]: Storage Input/Output types #13270

Merged
merged 20 commits into from
Apr 23, 2024
Merged

[Fix]: Storage Input/Output types #13270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
7e743a8
Revert "Fix Overload return types for storage APIs (#13239)"
ashika112 Apr 19, 2024
f656f7a
make a first pass at type update
ashika112 Apr 19, 2024
f78dc56
download & remove base update
ashika112 Apr 19, 2024
7447bc3
getProps & getUrl base update
ashika112 Apr 19, 2024
2f9e5d5
update list base tpes
ashika112 Apr 19, 2024
41333cb
update copy type
ashika112 Apr 19, 2024
a248e0f
update to fn declaration
ashika112 Apr 21, 2024
e34e2ca
update to fn declaration
ashika112 Apr 21, 2024
708eebc
update server def
ashika112 Apr 21, 2024
935a058
adding tests
ashika112 Apr 22, 2024
515b3cb
updating tests - copy,remove
ashika112 Apr 22, 2024
2808329
update tests -list/upload
ashika112 Apr 22, 2024
350ca6c
update misc
ashika112 Apr 22, 2024
2c0a88d
type name update
ashika112 Apr 22, 2024
80448d1
update deprecation
ashika112 Apr 23, 2024
857f152
type the tests
ashika112 Apr 23, 2024
3c18446
cleanup
ashika112 Apr 23, 2024
55a587a
fix unit test
ashika112 Apr 23, 2024
771e49e
address Pr comments
ashika112 Apr 23, 2024
36d0bf3
missed a tiny bit
ashika112 Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 36 additions & 30 deletions packages/storage/__tests__/providers/s3/apis/copy.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,16 @@
// SPDX-License-Identifier: Apache-2.0

import { AWSCredentials } from '@aws-amplify/core/internals/utils';
import { Amplify } from '@aws-amplify/core';
import { Amplify, StorageAccessLevel } from '@aws-amplify/core';
import { StorageError } from '../../../../src/errors/StorageError';
import { StorageValidationErrorCode } from '../../../../src/errors/types/validation';
import { copyObject } from '../../../../src/providers/s3/utils/client';
import { copy } from '../../../../src/providers/s3/apis';
import {
CopySourceOptionsWithKey,
CopyDestinationOptionsWithKey,
CopyInput,
CopyWithPathInput,
CopyOutput,
CopyWithPathOutput,
} from '../../../../src/providers/s3/types';

jest.mock('../../../../src/providers/s3/utils/client');
Expand Down Expand Up @@ -67,6 +69,8 @@ describe('copy API', () => {

describe('Happy Cases', () => {
describe('With key', () => {
const copyWrapper = async (input: CopyInput): Promise<CopyOutput> =>
copy(input);
beforeEach(() => {
mockCopyObject.mockImplementation(() => {
return {
Expand All @@ -77,7 +81,14 @@ describe('copy API', () => {
afterEach(() => {
jest.clearAllMocks();
});
[
const testCases: Array<{
source: { accessLevel?: StorageAccessLevel; targetIdentityId?: string };
Copy link
Member Author

@ashika112 ashika112 Apr 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note on this: This is how a customer would have to pass in today if it is not done directly on API call. Please let me know if that is not the case since this is the pattern I followed on all APIs

destination: {
accessLevel?: StorageAccessLevel;
};
expectedSourceKey: string;
expectedDestinationKey: string;
}> = [
{
source: { accessLevel: 'guest' },
destination: { accessLevel: 'guest' },
Expand Down Expand Up @@ -150,7 +161,8 @@ describe('copy API', () => {
expectedSourceKey: `${bucket}/protected/${targetIdentityId}/${sourceKey}`,
expectedDestinationKey: `protected/${defaultIdentityId}/${destinationKey}`,
},
].forEach(
];
testCases.forEach(
({
source,
destination,
Expand All @@ -160,24 +172,18 @@ describe('copy API', () => {
const targetIdentityIdMsg = source?.targetIdentityId
? `with targetIdentityId`
: '';
const copyResult = {
key: destinationKey,
path: expectedDestinationKey,
};

it(`should copy ${source.accessLevel} ${targetIdentityIdMsg} -> ${destination.accessLevel}`, async () => {
expect(
await copy({
source: {
...(source as CopySourceOptionsWithKey),
key: sourceKey,
},
destination: {
...(destination as CopyDestinationOptionsWithKey),
key: destinationKey,
},
}),
).toEqual(copyResult);
const { key } = await copyWrapper({
source: {
...source,
key: sourceKey,
},
destination: {
...destination,
key: destinationKey,
},
});
expect(key).toEqual(destinationKey);
expect(copyObject).toHaveBeenCalledTimes(1);
expect(copyObject).toHaveBeenCalledWith(copyObjectClientConfig, {
...copyObjectClientBaseParams,
Expand All @@ -190,6 +196,10 @@ describe('copy API', () => {
});

describe('With path', () => {
const copyWrapper = async (
input: CopyWithPathInput,
): Promise<CopyWithPathOutput> => copy(input);
Comment on lines +199 to +201
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Possible to hoist this up a few levels to re-use between suites?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So i purposefully did this so we can have both direct usage and wrapper usage. Meaning This is used in all path happy cases but error cases one will have the direct usage. I mean we want to test both ways


beforeEach(() => {
mockCopyObject.mockImplementation(() => {
return {
Expand Down Expand Up @@ -222,15 +232,11 @@ describe('copy API', () => {
destinationPath,
expectedDestinationPath,
}) => {
expect(
await copy({
source: { path: sourcePath },
destination: { path: destinationPath },
}),
).toEqual({
path: expectedDestinationPath,
key: expectedDestinationPath,
const { path } = await copyWrapper({
source: { path: sourcePath },
destination: { path: destinationPath },
});
expect(path).toEqual(expectedDestinationPath);
expect(copyObject).toHaveBeenCalledTimes(1);
expect(copyObject).toHaveBeenCalledWith(copyObjectClientConfig, {
...copyObjectClientBaseParams,
Expand Down
Loading
Loading