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

refactor: rewrite listParts in TypeScript #1160

Merged
merged 17 commits into from
Jun 21, 2023
Merged

Conversation

trim21
Copy link
Contributor

@trim21 trim21 commented May 24, 2023

No description provided.

@trim21 trim21 marked this pull request as ready for review May 24, 2023 19:53
src/internal/client.ts Show resolved Hide resolved
src/internal/client.ts Show resolved Hide resolved
src/minio.js Outdated Show resolved Hide resolved
@prakashsvmx
Copy link
Member

prakashsvmx commented May 29, 2023


var s3Client = new Minio.Client({
    endPoint: 's3.amazonaws.com',
    accessKey: 's3accssKey',
    secretKey: 's3secretkey',
    //region: 'us-east-1',
    // pathStyle: true,
    // useSSL: true,
})


Specifically with s3,
, if no region is specified,
TypeError: Invalid region: undefined is thrown.

MinIO does not care about region.

@trim21
Copy link
Contributor Author

trim21 commented Jun 1, 2023


var s3Client = new Minio.Client({
    endPoint: 's3.amazonaws.com',
    accessKey: 's3accssKey',
    secretKey: 's3secretkey',
    //region: 'us-east-1',
    // pathStyle: true,
    // useSSL: true,
})

Specifically with s3, , if no region is specified, TypeError: Invalid region: undefined is thrown.

MinIO does not care about region.

can you paste full stack trace?

@prakashsvmx
Copy link
Member

With older versions, with or without region specified in client options, it works.
with this pr, it does not work as expected. may be the default region is not used i guess.

var s3Client = new Minio.Client({
    endPoint: 's3.amazonaws.com',
    accessKey: 's3-acc-key',
    secretKey: 's3-password-tbd,
    region: 'us-east-1', // try with or without region. prior releases work as expected. 
    //pathStyle: true,
    //useSSL: true,
})

s3Client.traceOn()

const checkConn = () => {
    const objectName = '1.png'
    const bucketName = 'sph-test-inc-upld'
    s3Client.statObject(bucketName, objectName, (e, d) => {
        console.log(e, d)
    })
}

Trace without region specified


REQUEST: GET /sph-test-inc-upld?location
host: s3.amazonaws.com
user-agent: MinIO (linux; x64) minio-js/7.1.2
x-amz-date: 20230602T072956Z
x-amz-content-sha256: UNSIGNED-PAYLOAD
authorization: AWS4-HMAC-SHA256 Credential=*REDACTED*/20230602/us-east-1/s3/aws4_request, SignedHeaders=host;x-amz-content-sha256;x-amz-date, Signature=**REDACTED**

RESPONSE: 200
x-amz-id-2: Gz66xxXsGF/dTBI6mUtzTC8YnjoMUX8zmDIrhKJUUgzcz8pOqHhCuih2CTMct7vrjV4x2WwSvSE=
x-amz-request-id: 4DMRG7P9217KS5A9
date: Fri, 02 Jun 2023 07:29:58 GMT
content-type: application/xml
transfer-encoding: chunked
server: AmazonS3
connection: close

S3Error: 400
    at Object.parseResponseError (/home/prakash/MinIO/GIT/sdks/minio-js/dist/main/internal/xml-parser.js:82:13)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async Client.makeRequestStreamAsync (/home/prakash/MinIO/GIT/sdks/minio-js/dist/main/internal/client.js:465:19) {
  code: 'UnknownError',
  amzRequestid: 'QAV1HKWZ8K42HBF4',
  amzId2: 'dzxhKWk7kaEZuQDiVIirrq3lHhuIgzKEu4wIn4YXeETY5vs8Iww1lDbgQ7F2FIZ+sfXM3L4W0LQ=',
  amzBucketRegion: undefined,
  [cause]: {
    amzRequestid: 'QAV1HKWZ8K42HBF4',
    amzId2: 'dzxhKWk7kaEZuQDiVIirrq3lHhuIgzKEu4wIn4YXeETY5vs8Iww1lDbgQ7F2FIZ+sfXM3L4W0LQ=',
    amzBucketRegion: undefined
  }
} undefined

@trim21
Copy link
Contributor Author

trim21 commented Jun 2, 2023

With older versions, with or without region specified in client options, it works. with this pr, it does not work as expected. may be the default region is not used i guess.

var s3Client = new Minio.Client({
    endPoint: 's3.amazonaws.com',
    accessKey: 's3-acc-key',
    secretKey: 's3-password-tbd,
    region: 'us-east-1', // try with or without region. prior releases work as expected. 
    //pathStyle: true,
    //useSSL: true,
})

s3Client.traceOn()

const checkConn = () => {
    const objectName = '1.png'
    const bucketName = 'sph-test-inc-upld'
    s3Client.statObject(bucketName, objectName, (e, d) => {
        console.log(e, d)
    })
}

Trace without region specified


REQUEST: GET /sph-test-inc-upld?location
host: s3.amazonaws.com
user-agent: MinIO (linux; x64) minio-js/7.1.2
x-amz-date: 20230602T072956Z
x-amz-content-sha256: UNSIGNED-PAYLOAD
authorization: AWS4-HMAC-SHA256 Credential=*REDACTED*/20230602/us-east-1/s3/aws4_request, SignedHeaders=host;x-amz-content-sha256;x-amz-date, Signature=**REDACTED**

RESPONSE: 200
x-amz-id-2: Gz66xxXsGF/dTBI6mUtzTC8YnjoMUX8zmDIrhKJUUgzcz8pOqHhCuih2CTMct7vrjV4x2WwSvSE=
x-amz-request-id: 4DMRG7P9217KS5A9
date: Fri, 02 Jun 2023 07:29:58 GMT
content-type: application/xml
transfer-encoding: chunked
server: AmazonS3
connection: close

S3Error: 400
    at Object.parseResponseError (/home/prakash/MinIO/GIT/sdks/minio-js/dist/main/internal/xml-parser.js:82:13)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async Client.makeRequestStreamAsync (/home/prakash/MinIO/GIT/sdks/minio-js/dist/main/internal/client.js:465:19) {
  code: 'UnknownError',
  amzRequestid: 'QAV1HKWZ8K42HBF4',
  amzId2: 'dzxhKWk7kaEZuQDiVIirrq3lHhuIgzKEu4wIn4YXeETY5vs8Iww1lDbgQ7F2FIZ+sfXM3L4W0LQ=',
  amzBucketRegion: undefined,
  [cause]: {
    amzRequestid: 'QAV1HKWZ8K42HBF4',
    amzId2: 'dzxhKWk7kaEZuQDiVIirrq3lHhuIgzKEu4wIn4YXeETY5vs8Iww1lDbgQ7F2FIZ+sfXM3L4W0LQ=',
    amzBucketRegion: undefined
  }
} undefined

So this doesn't happened in current master branch, but on 5c91791?

@prakashsvmx
Copy link
Member

I tested with the past two releases. I can check on master and confirm.

@trim21
Copy link
Contributor Author

trim21 commented Jun 2, 2023

this may be caused by #1156

@prakashsvmx
Copy link
Member

may be. in master, i am seeing the below in both (with/without region) cases


 ➜ node examples/put-object.js
TypeError: Invalid region: undefined
    at getS3Endpoint (/home/prakash/MinIO/GIT/sdks/minio-js/dist/main/internal/s3-endpoints.js:48:11)
    at Client.getRequestOptions (/home/prakash/MinIO/GIT/sdks/minio-js/dist/main/internal/client.js:223:47)
    at Client.makeRequestStreamAsync (/home/prakash/MinIO/GIT/sdks/minio-js/dist/main/internal/client.js:438:29) undefined

@trim21
Copy link
Contributor Author

trim21 commented Jun 2, 2023

should fix this first...

@trim21
Copy link
Contributor Author

trim21 commented Jun 2, 2023

@prakashsvmx please try #1165 , I tested on s3, it looks fine。

@trim21 trim21 requested a review from prakashsvmx June 7, 2023 08:17
src/internal/xml-parser.ts Outdated Show resolved Hide resolved
prakashsvmx
prakashsvmx previously approved these changes Jun 12, 2023
Copy link
Member

@prakashsvmx prakashsvmx left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Contributor

@aldy505 aldy505 left a comment

Choose a reason for hiding this comment

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

So far it's all good (I can't press approve as the button is disabled), just a few notes that might be discussed.

src/internal/xml-parser.ts Outdated Show resolved Hide resolved
src/internal/xml-parser.ts Show resolved Hide resolved
@trim21
Copy link
Contributor Author

trim21 commented Jun 21, 2023

@kaankabalak please?

Copy link
Contributor

@kaankabalak kaankabalak left a comment

Choose a reason for hiding this comment

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

LGTM

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.

4 participants