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

DATAPLTING-181 - Dependency upgrades #30

Merged
merged 12 commits into from
Oct 30, 2023
Merged

Conversation

immad-imtiaz
Copy link
Contributor

  • Upgrades dependencies
  • Releases a new version 2.0.0
  • Documentation upgrade

@immad-imtiaz immad-imtiaz changed the title Dependency upgrades DATAPLTING-181 - Dependency upgrades Oct 24, 2023
index.js Outdated Show resolved Hide resolved
api.md Show resolved Hide resolved
lib/get.js Outdated Show resolved Hide resolved
test/index.test.js Show resolved Hide resolved
Copy link

@jpallari jpallari left a comment

Choose a reason for hiding this comment

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

Did you get the errors.test.js working with new version of Node?

.eslintrc Outdated Show resolved Hide resolved
@immad-imtiaz
Copy link
Contributor Author

immad-imtiaz commented Oct 27, 2023

Did you get the errors.test.js working with new version of Node?

@jkpl I can not generate this use case of Truncated response from S3, apparently in newer version of node http the request will never terminate from the new client if the Content-Length is greater than message body (until it times out). Tried all other http server modules like express, koa but I can not regenerate that case.

I can not use mocking libraries here as the package is heavily reliant on AWS.Request events, as it AWS.Request is used underneath AWS S3 it has to be real SDK client.

I did though verified that the events like extract-data those test are verifying are triggered on successful data use case. So incase S3 sends us truncated data those check will work and it will retry.

    }).on('extractData', function(res) {
      if (res.data.Body.length !== Number(res.data.ContentLength)) {
        res.data = null;
        res.error = {
          code: 'TruncatedResponseError',
          message: 'Content-Length does not match response body length'
        };
      }
    }).on('retry', function(res) {
      if (res.error) {
        if (res.error.code === 'TruncatedResponseError') res.error.retryable = true;
      }
    });
  }

Small script to see how it reacts in node 18

const AWS = require('aws-sdk');
const mock = require('./test/mockServer')

mock().start()

const s3 = new AWS.S3({
    endpoint: new AWS.Endpoint('http://localhost:3000'),
    s3BucketEndpoint: true,
    accessKeyId: 'your-access-key',
    secretAccessKey: 'your-secret-key',
    httpOptions: { timeout: 10 },
    maxRetries: 10
});

  // const params = {
  //   Bucket: 'truncated',
  //   Key: 'truncated',
  // };

const params = {
  Bucket: 'bucket',
  Key: '/?prefix=empty'
}
console.log('here')

s3.getObject(params, function (err, data) {
    console.log(err, data);
}).on('extractData', function (res) {
    console.log('called here', res.data)
    if (res.data.Body.length !== Number(res.data.ContentLength)) {
      res.data = null;
      res.error = {
        code: 'TruncatedResponseError',
        message: 'Content-Length does not match response body length'
      };
    }
  });


@immad-imtiaz
Copy link
Contributor Author

immad-imtiaz commented Oct 27, 2023

I did this packages locally to see if its working as expected.

  • I ran the integration test locally with my AWS CREDS to see if the test are working, these test cover complete functionality of this package
    image

  • I installed this globally on my local using npm install -g . , then I ran the following test
    image

test/mockServer.js Fixed Show fixed Hide fixed
test/mockServer.js Fixed Show fixed Hide fixed
@jpallari
Copy link

Make sure to squash the intermediate commits.

@immad-imtiaz immad-imtiaz merged commit 18c7b0c into master Oct 30, 2023
2 checks passed
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.

2 participants