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

s3.createBucket method is mutating the object passed as parameter #3112

Closed
3 tasks done
ericmustin opened this issue Feb 24, 2020 · 5 comments · Fixed by #3138
Closed
3 tasks done

s3.createBucket method is mutating the object passed as parameter #3112

ericmustin opened this issue Feb 24, 2020 · 5 comments · Fixed by #3138
Labels
feature-request A feature should be added or improved.

Comments

@ericmustin
Copy link
Contributor

Confirm by changing [ ] to [x] below to ensure that it's a bug:

Describe the bug
This is the same bug previously reported here #3013 .

i've been testing my s3.createBucket calls using localstack, which requires me to modify the endpoint to localhost:4572 and for some reason this causes the parameter object that has been passed in to get mutated and a CreateBucketConfiguration key gets erroneously added to it, which breaks future calls using that parameter object. There is probably a missing Object.assign call somewhere but I haven't had the time to track it down, otherwise would just make the PR.

Is the issue in the browser/Node.js?
Node.js

Details of the browser/Node.js version
v10.15.1

SDK version number
└── aws-sdk@2.624.0

To Reproduce (observed behavior)
Reproduction snippet below, please note it only appears to occur when modifying the endpoint config option

// npm install -g aws-sdk
// https://github.com/localstack/localstack
// run localstack docker container with s3 listening at localhost:4572

const AWS = require('aws-sdk')
const assert = require('assert')
const ep = new AWS.Endpoint('http://localhost:4572')
const s3 = new AWS.S3({endpoint: ep, s3ForcePathStyle: true})
const bucket = 'node-sdk-sample'  
const params = { Bucket: bucket }
const originalKeys = Object.keys(params)

// Create a bucket and upload something into it
s3.createBucket(params, function(err, data) {
  let mutatedKeys = Object.keys(params)

  console.log('original object keys: ', originalKeys)
  console.log('mutated object keys: ', mutatedKeys)

  assert.deepEqual(originalKeys, mutatedKeys)
})

Expected behavior
parameters object passed into s3.createObject should not be mutated, as this can cause future calls that re-use that same parameters object to throw validation errors

Screenshots

reproduction snapshot

Additional context
Given that this issue has been opened (and auto-closed 😐 ) before, as seen here #3013 it seems very likely there is a legitimate bug. If you are having trouble reproducing please @ me and let me know and i'll be happy to provide any additional relevant context. And in general thanks for the great work y'all do maintaining this repo, it's great and very well documented!

@ericmustin ericmustin added the bug This issue is a bug. label Feb 24, 2020
@ajredniwja ajredniwja added the investigating Issue has been looked at and needs deep dive work by OSDS. label Feb 25, 2020
@ericmustin
Copy link
Contributor Author

ericmustin commented Feb 25, 2020

@ajredniwja issue is here, fwiw,

params.CreateBucketConfiguration = { LocationConstraint: this.config.region };

Up to y'all on how to fix, depends if you want to allocate another object , and how you want to go about doing that. something like the below probably works, although it should be note that Object.assign will miss things like properties that only exist on the params object prototype chain, and also non-enumerable properties, things like Symbols or what have you

  createBucket: function createBucket(params, callback) {
    if (typeof params === 'function' || !params) {
      callback = callback || params;
      params = {};
    }
    var hostname = this.endpoint.hostname;
    var updatedParams = Object.assign({}, params);
    if (hostname !== this.api.globalEndpoint && !updatedParams.CreateBucketConfiguration) {
      updatedParams.CreateBucketConfiguration = { LocationConstraint: this.config.region };
    }
    return this.makeRequest('createBucket', updatedParams, callback);
  },

@ajredniwja ajredniwja added feature-request A feature should be added or improved. and removed bug This issue is a bug. investigating Issue has been looked at and needs deep dive work by OSDS. labels Mar 2, 2020
@ajredniwja
Copy link
Contributor

I was able to reproduce this, while this is not potentially is not a bug with the SDK but something that can be improved, I marked this as a feature request, would be glad to take a PR, otherwise will let the team evaluate the priority and work on it later.

@ericmustin
Copy link
Contributor Author

@ajredniwja alrighty, thanks for taking a look and the feedback. I have made a PR to address this and also wrote a unit test to prevent a regression in future versions #3138

As an aside, i think marking this a feature request is not in line with my understanding of the term.

@ajredniwja
Copy link
Contributor

@ericmustin I agree with you but I marked this as a feature request since its more of a behavior issue, which can be enhanced and not necessarily a bug, meaning not really a bug.
Since you have already created a PR, I would let the team review it.

trivikr pushed a commit that referenced this issue Mar 18, 2020
@lock
Copy link

lock bot commented Mar 27, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked as resolved and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request A feature should be added or improved.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants