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

Use params for remove method #26

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

mdartic
Copy link

@mdartic mdartic commented Mar 30, 2022

Actually, only the key is used in the remove method.
But we could specify other options, like the Bucket where the object is store.

This PR will use the params property if it exist in the opts parameter of the remove method, like it's already done in others method of the blob store.

What do you think about it ?

@claustres
Copy link

It seems to me a great addition insuring create/remove methods are similar. A default bucket can be specified so that you can omit it in params or specify it if required.

@claustres
Copy link

There is a console.log() to be removed though.

@mdartic
Copy link
Author

mdartic commented Mar 30, 2022

Indeed, thanks :-)

index.js Outdated
@@ -100,7 +100,11 @@ S3BlobStore.prototype.createWriteStream = function (opts, s3opts, done) {
*/
S3BlobStore.prototype.remove = function (opts, done) {
var key = typeof opts === 'string' ? opts : opts.key;
this.s3.deleteObject({ Bucket: this.bucket, Key: key }, done);
var params = typeof opts === 'object' ? opts.params : {};
params.Bucket = params.Bucket || this.bucket;
Copy link
Owner

Choose a reason for hiding this comment

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

this would almost always crash because params would likely be undefined.

opts = { key: "somekey" }
var params = typeof opts === 'object' ? opts.params : {}

params would be undefined at this point

params.Bucket = undefined.Bucket <-- boom

Copy link
Author

Choose a reason for hiding this comment

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

Oops... sorry, you're absolutely right.

What do you think of this code :

  var params = {}
  if (typeof opts === 'string') {
    params.Key = opts;
  } else {
    opts = Object.assign({}, opts, {
      params: Object.assign({}, opts.params)
    });
    params.Key = opts.key;
    params.Bucket = opts.params.Bucket || this.bucket;
  }
  this.s3.deleteObject(params, done);
  return this;

Copy link
Owner

Choose a reason for hiding this comment

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

I think this should be backwards compatible? then you could just pass the whole params object as opts instead of having a params subkey?

let params = Object.assign({}, opts)
params.Key = params.Key || params.key
params.Bucket = params.Bucket || this.bucket
delete params.key

Copy link
Owner

Choose a reason for hiding this comment

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

actually the main issue is I'm not sure if s3.deleteObject gets mad if there are extra params, because there is probably code out there that passes something other than key in opts. I should test this...

Copy link
Owner

Choose a reason for hiding this comment

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

nm I'm dumb it's been so long since I used blob-store and I remember now that params have a standard interface so my suggestion doesn't make sense. I think params in opts is fine, will update soon.

Copy link
Owner

Choose a reason for hiding this comment

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

I just remembered we already have code to do this, I've refactored uploadParams into baseParams and did a PR here with the change: #27

Copy link
Author

Choose a reason for hiding this comment

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

I review your PR, and if it's merged, I think my PR will be useless :-)

index.js Outdated Show resolved Hide resolved
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.

3 participants