Can't get Upload mock to work for small files

obones commented 5 months ago

Checklist

[X] I have read Caveats documentation and didn't find a solution for this problem there.

Bug description

Hello,

I'm using this library to mock S3 and this works just fine:

// arrange
const { S3Client } = require("@aws-sdk/client-s3");

const s3ClientMock = mockClient(S3Client);
s3ClientMock.on(HeadObjectCommand).rejects({name:"NotFound"});

// act
const { S3 } = require("@aws-sdk/client-s3");

const s3 = new S3();
let fileExists = await s3.headObject({Bucket: "bucket", Key: "filename"}).then(
    () =>
    {
        return true;
    },
    err =>
    {
        if (err.name === "NotFound")
        {
            return false;
        }
        throw err;
    }
);

// assert
fileExists.should.equal(false);

But I'm also trying to mock the Upload class and did this, inspired by the Readme code:

// arrange
const { S3Client } = require("@aws-sdk/client-s3");

const s3ClientMock = mockClient(S3Client);
s3ClientMock.on(CreateMultipartUploadCommand).resolves({UploadId: '1'});
s3ClientMock.on(UploadPartCommand).resolves({ETag: '1'});
s3ClientMock.on(PutObjectCommand).resolves({});

// act
const { Upload } = require("@aws-sdk/lib-storage");
const { S3 } = require("@aws-sdk/client-s3");

const s3 = new S3();
const markerS3Options = { Bucket: "myBucket", Key: "myFile", Body: "test" };
await new Upload(
    {
        client: s3,
        params: markerS3Options
    }
).done();

// assert (nothing, should just not crash)

Sadly, this does not work, I'm getting the "Region not set" error which is customary when a mock is missing. I have seen that I'm mocking S3Client and using S3 with Upload but I thought it would work as it works just fine with HeadObjectCommand. I also tried mocking S3 in the arrange part, or using S3Client in the act part, but none of this helped.

If that helps, here is the call stack for the error

Error: Region is missing
    at default (node_modules\.pnpm\@smithy+config-resolver@3.0.3\node_modules\@smithy\config-resolver\dist-cjs\index.js:117:11)
    at D:\test\node_modules\.pnpm\@smithy+node-config-provider@3.1.2\node_modules\@smithy\node-config-provider\dist-cjs\index.js:90:104
    at D:\test\node_modules\.pnpm\@smithy+property-provider@3.1.2\node_modules\@smithy\property-provider\dist-cjs\index.js:97:33
    at async coalesceProvider (node_modules\.pnpm\@smithy+property-provider@3.1.2\node_modules\@smithy\property-provider\dist-cjs\index.js:124:18)
    at async D:\test\node_modules\.pnpm\@smithy+property-provider@3.1.2\node_modules\@smithy\property-provider\dist-cjs\index.js:135:20
    at async useFipsEndpoint (node_modules\.pnpm\@smithy+config-resolver@3.0.3\node_modules\@smithy\config-resolver\dist-cjs\index.js:146:68)
    at async resolveParams (node_modules\.pnpm\@smithy+middleware-endpoint@3.0.3\node_modules\@smithy\middleware-endpoint\dist-cjs\index.js:144:32)
    at async getEndpointFromInstructions (node_modules\.pnpm\@smithy+middleware-endpoint@3.0.3\node_modules\@smithy\middleware-endpoint\dist-cjs\index.js:123:26)
    at async _Upload.__uploadUsingPut (node_modules\.pnpm\@aws-sdk+lib-storage@3.600.0_@aws-sdk+client-s3@3.600.0\node_modules\@aws-sdk\lib-storage\dist-cjs\index.js:245:9)
    at async _Upload.__doConcurrentUpload (node_modules\.pnpm\@aws-sdk+lib-storage@3.600.0_@aws-sdk+client-s3@3.600.0\node_modules\@aws-sdk\lib-storage\dist-cjs\index.js:309:16)
    at async Promise.all (index 0)
    at async _Upload.__doMultipartUpload (node_modules\.pnpm\@aws-sdk+lib-storage@3.600.0_@aws-sdk+client-s3@3.600.0\node_modules\@aws-sdk\lib-storage\dist-cjs\index.js:394:5)
    at async _Upload.done (node_modules\.pnpm\@aws-sdk+lib-storage@3.600.0_@aws-sdk+client-s3@3.600.0\node_modules\@aws-sdk\lib-storage\dist-cjs\index.js:213:12)
    at async Context.<anonymous> (test\index.js:243:9)

Reproduction

// arrange
const { S3Client } = require("@aws-sdk/client-s3");

const s3ClientMock = mockClient(S3Client);
s3ClientMock.on(CreateMultipartUploadCommand).resolves({UploadId: '1'});
s3ClientMock.on(UploadPartCommand).resolves({ETag: '1'});
s3ClientMock.on(PutObjectCommand).resolves({});

// act
const { Upload } = require("@aws-sdk/lib-storage");
const { S3 } = require("@aws-sdk/client-s3");

const s3 = new S3();
const markerS3Options = { Bucket: "myBucket", Key: "myFile", Body: "test" };
await new Upload(
    {
        client: s3,
        params: markerS3Options
    }
).done();

// assert (nothing, should just not crash)

Environment

Node version: v16.20.2
Testing lib and version: mocha 10.3.0
Typescript version: none, this is done in vanilla JS
AWS SDK v3 Client mock version: 3.0.1
AWS JS SDK libs and versions: @aws-sdk/client-s3 : 3.171.0 @aws-sdk/lib-storage : 3.171.0

obones commented 5 months ago

Just a note here, the versions I gave above are from the package.json specifiers but because I used the ^ prefix, the real versions are those:

  aws-sdk-client-mock: 3.1.0
  '@aws-sdk/client-s3': 3.600.0
  '@aws-sdk/lib-storage': 3.600.0

I tried looking at the changes for v4 but I couldn't see any changes that would help here, but I may have missed some.

obones commented 5 months ago

Ok, doing step by step debugging, I see the issue comes from this line: https://github.com/aws/aws-sdk-js-v3/blob/main/lib/lib-storage/src/Upload.ts#L142

It awaits two promises, the first is rightfully mocked but the second one is not. And because my client config does not define endpoint the second promises resolves to undefined

In turn, this leads to calling getEndpointFromInstructions which tries to find an endpoint and for this has to make a call that is not mocked, triggering the error that I reported.

obones commented 5 months ago

Well, the SUT could be changed to have this right before giving the client to Upload

s3.config.endpoint = () => ({hostname: "mocked_out"});

But that's not safe when used in production. The only way that I could find was doing this:

let uploadDoneMock = sinon.stub(Upload.prototype, "done");
uploadDoneMock.resolves({});

// call code to be tested

uploadDoneMock.restore();

But it feels a bit out of place.

obones commented 2 months ago

Note that it works for trivial situations but if you use Upload with a stream as the s3 options Body, then this stream will never be read from and the whole pipeline that depends on it will get stuck.

Not sure how to solve this.

matsvanaudenaeren98 commented 2 months ago

I'm running into the same problem. Can you provide the fully working code for this test please? I've tried the mockLibStorageUpload from npmjs.com but that doesn't work either.

Thanks in advance!

obones commented 2 months ago

mockLibStorageUpload has been deprecated for quite a while and actually did just the same thing as what is recommended in the current README file so I'm not surprised it doesn't work either.

As to providing working code, there is the one that I provided here directly using as sinon stub. It has the drawback that it does not consume the body if it is a Readable stream, but it may not affect you

m-radzikowski commented 2 months ago

I can't reproduce it, the code does not throw any error for me. Could you please create a small repo with reproduction, where I can run (p)npm install and (p)npm run and get the error?

obones commented 2 months ago

Here you go:

node_mock_upload.zip

I ran these commands:

npm install
npx mocha test.js

and I got this output

  upload mocking
    1) should not crash

  0 passing (416ms)
  1 failing

  1) upload mocking
       should not crash:
     Error: Region is missing
      at default (node_modules\@smithy\config-resolver\dist-cjs\index.js:117:11)
      at K:\Tests\node_mock_upload\node_modules\@smithy\node-config-provider\dist-cjs\index.js:90:104
      at K:\Tests\node_mock_upload\node_modules\@smithy\property-provider\dist-cjs\index.js:97:33
      at async coalesceProvider (node_modules\@smithy\property-provider\dist-cjs\index.js:124:18)
      at async K:\Tests\node_mock_upload\node_modules\@smithy\property-provider\dist-cjs\index.js:135:20
      at async useFipsEndpoint (node_modules\@smithy\config-resolver\dist-cjs\index.js:146:68)
      at async resolveParams (node_modules\@smithy\middleware-endpoint\dist-cjs\index.js:156:32)
      at async getEndpointFromInstructions (node_modules\@smithy\middleware-endpoint\dist-cjs\index.js:135:26)
      at async _Upload.__uploadUsingPut (node_modules\@aws-sdk\lib-storage\dist-cjs\index.js:245:9)
      at async _Upload.__doConcurrentUpload (node_modules\@aws-sdk\lib-storage\dist-cjs\index.js:310:16)
      at async Promise.all (index 0)
      at async _Upload.__doMultipartUpload (node_modules\@aws-sdk\lib-storage\dist-cjs\index.js:395:5)
      at async _Upload.done (node_modules\@aws-sdk\lib-storage\dist-cjs\index.js:213:12)
      at async Context.<anonymous> (test.js:28:17)

Here you can see the getEndpointFromInstructions call in the call stack.

m-radzikowski commented 2 months ago

Hm...

The actually installed @aws-sdk/* packages version is 3.654.0 (but this is true in package-lock.json so no update on my side) but even changing from ^3.635.0 to 3.635.0 to force this version does not change the fact the test passes for me.

obones commented 2 months ago

That's extremely weird, the only thing that I see is that I use a different version of node than yours. When I first reported this, it was v16 but it's now v18.20.4 with no change on my side.

And the code in the SDK is still the same: https://github.com/aws/aws-sdk-js-v3/blob/v3.654.0/lib/lib-storage/src/Upload.ts#L142

There is no endpoint in the config, so it tries to call getEndpointFromInstructions at line 148 and that's where it ends in the call stack above.

Could it be that you have global AWS settings or environment variables that change the behavior?

m-radzikowski commented 2 months ago

Yeah, I had leftover AWS credentials active with the region set 🤦‍♂️

So, in the line you referenced, the lib-storage resolves AWS configuration and must find the value to build the endpoint, even though it's different behavior from the rest of the SDK. Well.

There are 2 possible solutions:

Add AWS_REGION env variable with any value at any point before the Upload call in the test:

...
s3ClientMock.on(PutObjectCommand).resolves({});
process.env.AWS_REGION = "";
...

It works, but it can theoretically impact other tests if they use the AWS_REGION variable too, if you do not restore it to previous value.

Inject the endpoint value when calling PutObjectCommand:

s3ClientMock.on(PutObjectCommand).callsFake(async (input, getClient) => {
    getClient().config.endpoint = () => ({hostname: ""});
    return {};
});

In TypeScript, it must be () => ({hostname: ""}) as any. The configuration is changed for this and potentially all next commands, but since you should reset mocks on every test this should be safer.

m-radzikowski / aws-sdk-client-mock