Avoid error when putting objects in R2/S3

bosschaert commented 3 months ago

Description

This fixes the following error that shows up in the da-admin console:

  [ERROR] Are you using a Stream of unknown length as the Body of a PutObject
  request? Consider using Upload instead from @aws-sdk/lib-storage.

Related Issue

Fixes #43

How Has This Been Tested?

New unit tests
Tested with the Playwright versions test
Tested locally with da-live and da-collab

Types of changes

[X] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[X] I have signed the Adobe Open Source CLA.
[X] My code follows the code style of this project.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[X] I have read the CONTRIBUTING document.
[X] I have added tests to cover my changes.
[X] All new and existing tests passed.

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 88.46154% with 3 lines in your changes missing coverage. Please review.

Project coverage is 83.64%. Comparing base (4224204) to head (78f378e).

Files	Patch %	Lines
src/storage/version/put.js	88.00%	3 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #44 +/- ## ========================================== + Coverage 83.40% 83.64% +0.24% ========================================== Files 15 15 Lines 940 960 +20 ========================================== + Hits 784 803 +19 - Misses 156 157 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

auniverseaway commented 3 months ago

What makes this different than our normal storage/object/put API? If I understand this correctly, it's because we're allowing plain strings for body as part of the API.

I would probably move to keeping this more aligned with the existing storage/object/put api... in fact, I would say we should explore reusing that and allow passing in metadata strings rather than building an entire new file we have to maintain just for versions.

I will beat this drum a lot, but it's only a matter of time before DA Admin becomes a complicated mess. When we provide many options of different ways to put something, we create complexity we will have to maintain for eternity. We should start with a consistent way to save data (body.data) and then only expand as real-world use cases are found.

I know our existing storage/object/put has some stuff in there for strings, but I think we need to move away from that... at the very least, standardize on the File detection that's already in there as well as the body.data pattern that was established. Then we could re-use storage/object/put...

const body = {
  data: new File(),
  metadata: { users: '...' },
};

If if we do have to keep the put files separate, I would prefer if our data structures to save objects was as consistent as possible.

bosschaert commented 3 months ago

What makes this different than our normal storage/object/put API? If I understand this correctly, it's because we're allowing plain strings for body as part of the API.

Hi @auniverseaway we're actually not sending plain strings as part of the body of the version APIs. When content is saved with a version, this is purely a serverside operation, where the body content is streamed from one bucket to another. (@karlpauls correct me if I'm wrong).

This PR is to ensure that the ContentLength property is set on the PutObject API because otherwise it gives the warning mentioned above. Having said that, I spoke to Karl and could simplify the implementation so that it doesn't need to read the stream to get its length as that information is already available as output of the getObject API. With that, the code now always has the ContentLength without having to drain the data, whether the object being sent is a Stream (bucket-to-bucket copy), File (coming from the client) or String (generally an empty string '' in case there is no body).

bosschaert commented 2 months ago

@auniverseaway @karlpauls this PR doesn't much change functionality but only fixes this error which is happening regularly on the servcer:

  [ERROR] Are you using a Stream of unknown length as the Body of a PutObject
  request? Consider using Upload instead from @aws-sdk/lib-storage.

The content length can in all cases be obtained from already available information, so no pre-streaming of content is needed.

adobe / da-admin