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.

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): md5 calculation for react native #13836

Merged
merged 4 commits into from
Sep 23, 2024

Conversation

ashwinkumar6
Copy link
Member

@ashwinkumar6 ashwinkumar6 commented Sep 20, 2024

Description of changes

upload with data as ArrayBuffer or ArrayBufferView when objectLock is enabled in react native gives creating blobs from 'arraybuffer' and 'arraybufferview' are not supported error in react native.

PENDING: updating e2e tests for web and RN. Can come in separate PR

repro steps:

Amplify.configure(aws_outputs, {
  Storage: {
    S3: {
      isObjectLockEnabled: true,
    },
  },
});

await uploadData({
  key: 'public/filename',
  data: new ArrayBuffer(16),
}).result;

Description of how you validated changes

Enabled object lock for bucket manually in backend. upload data of types Blob, string, ArrayBuffer and ArrayBufferView in both browser and react native to test if object is successfully uploaded

Tested code snippets below in browser and RN

  • string
await upload({
  key: "string",
  data: "test",
}).result;
  • blob
await upload({
  key: "blob",
  data: new Blob(["test"]),
}).result;
  • arrayBuffer
await upload({
  key: "arrayBuffer",
  data: new ArrayBuffer(16),
}).result;
  • arrayBufferView
const arrayBufferView: ArrayBufferView = new Float32Array(
  new ArrayBuffer(16)
);    
await upload({
  key: "arrayBufferView",
  data: arrayBufferView,
}).result;

Checklist

  • PR description included
  • yarn test passes
  • Unit Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)

Checklist for repo maintainers

  • Verify E2E tests for existing workflows are working as expected or add E2E tests for newly added workflows
  • New source file paths included in this PR have been added to CODEOWNERS, if appropriate

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

hasher.update(content);
} else if (ArrayBuffer.isView(content) || content instanceof ArrayBuffer) {
const blob = new Blob([content]);
Copy link
Member Author

@ashwinkumar6 ashwinkumar6 Sep 20, 2024

Choose a reason for hiding this comment

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

Blob([arrayBuffer]) and Blob([arrayBufferView]) works in web but not RN. This can be removed for both platforms

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed for both platforms?

Copy link
Member Author

@ashwinkumar6 ashwinkumar6 Sep 21, 2024

Choose a reason for hiding this comment

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

Yep, since it's un-necessary code removing it from both web and RN

{ type: 'ArrayBuffer view', content: new Uint8Array() },
{ type: 'ArrayBuffer', content: new ArrayBuffer(8) },
{ type: 'Blob', content: new Blob([stringContent]) },
Copy link
Contributor

Choose a reason for hiding this comment

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

Why take this test out?

Copy link
Member Author

@ashwinkumar6 ashwinkumar6 Sep 21, 2024

Choose a reason for hiding this comment

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

Just refactored the test. we're testing all 4 data input types in unit tests,

{ type: 'ArrayBuffer view', content: new Uint8Array() },
{ type: 'ArrayBuffer', content: new ArrayBuffer(8) },
{ type: 'Blob', content: new Blob([stringContent]) },
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Member Author

@ashwinkumar6 ashwinkumar6 Sep 21, 2024

Choose a reason for hiding this comment

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

replied on this comment

hasher.update(content);
} else if (ArrayBuffer.isView(content) || content instanceof ArrayBuffer) {
const blob = new Blob([content]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Removed for both platforms?

hasher.update(content);
} else if (ArrayBuffer.isView(content) || content instanceof ArrayBuffer) {
const blob = new Blob([content]);
Copy link
Contributor

Choose a reason for hiding this comment

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

IF this works for web then why remove it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, since it's un-necessary code at this point removing it from both web and RN

@ashwinkumar6 ashwinkumar6 merged commit 63bceab into aws-amplify:main Sep 23, 2024
30 checks passed
@ashwinkumar6 ashwinkumar6 deleted the rn-upload-md5 branch September 23, 2024 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants