Skip to content

Add files list support #26

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

Closed
wants to merge 2 commits into from
Closed

Add files list support #26

wants to merge 2 commits into from

Conversation

mario
Copy link

@mario mario commented Nov 9, 2016

I want to add files listing support so I could eventually add files cleanup feature in Parse.

Since this is my first attempt at contributing to the Parse project, please let me know if something is not up to your standards.

@codecov-io
Copy link

codecov-io commented Nov 9, 2016

Current coverage is 68.18% (diff: 33.33%)

Merging #26 into master will decrease coverage by 27.05%

@@             master        #26   diff @@
==========================================
  Files             2          2          
  Lines           126        132     +6   
  Methods           9         11     +2   
  Messages          0          0          
  Branches         25         26     +1   
==========================================
- Hits            120         90    -30   
- Misses            6         42    +36   
  Partials          0          0          

Powered by Codecov. Last update e564407...e0735ba

@flovilmart
Copy link
Contributor

I'm not sure it actually fits here...

@mario
Copy link
Author

mario commented Nov 9, 2016

@flovilmart that's why I want your opinion - my thought was we need to have it in every storage adapter before I can work on files cleanup feature, at least according to parse-community/parse-server#1023.

@flovilmart
Copy link
Contributor

You should probably implement it first in Parse-server then update the adapters. Also, I don't believe it belongs to parse-server, but more a command line tool that can interact with parse server like Parse-files-utils.

Also note that what you're attempting to do require you to list all files that are attached to objects, then making the diff against whatever storage it is.

So I'd say it still doesn't belong to the adapter, unless there is a specific implementation of that cleanup tool that we want to support.

In summary, write the cleanup tool, adapter agnostic, heavily test it, and then open a PR on the adapter if this is required (which I don't believe is).

@flovilmart flovilmart closed this Nov 10, 2016
@mario
Copy link
Author

mario commented Nov 10, 2016

@flovilmart thank you for your comment. I am aware that I will need to list all files that are attached to the objects, and that in itself is a relatively trivial task as I can easily iterate over all existing classes and find rows of type File.

I will think about alternative approaches, but I think having a nice "Cleanup files" as part of App settings is a nice way to do this, as far as UX is concerned.

@flovilmart
Copy link
Contributor

For having started the parse-files-utils project, and actually having written the initial files listing code, I can tell you there is nothing trivial in implementing that (think millions of objects), that doesn't fit in memory. Also, for the case of cleanup, if you miss one, the file will be deleted. If you have a memory issue mid flight, you might end up with deletions that are not expected.

I don't believe you réalize that the general case for such feature is immensely tricky to achieve.

@mario
Copy link
Author

mario commented Nov 10, 2016

@flovilmart I agree with you, but (for now at least) my use-case is not millions of objects. Anyway, I'll make an implementation, use it if it works, and share it - doesn't have to be merged in. Thank you for your comments, I truly appreciate them.

@flovilmart
Copy link
Contributor

Also, s3-ls is definitively a no go, as it's unlikely to perform adequately on large amount of files. Given the source code, it doesn't support paging, and can be replaced with a few lines (that actually support paging)

@flovilmart
Copy link
Contributor

We cant have implementations that only work for specific use cases in the main repositories.

Have a look a parse-files-utils, it's been built around a file lister ( that just outputed all files to the command line)

Then the migration support was added.

For your use case, you definitely need to create a local DB/text file With all the file URLs, index on those, then ask S3 for the files it holds, find them by batch of 100, match against your local list, and create a second list from the missing matches.

From that list, you'"" be able to perform the deletion.

@flovilmart
Copy link
Contributor

Also most HTTP servers have a socket timeout of 30s that would effectively kill your transaction to delete the files... it's a worker process, not a transactional rest endpoint

@imana97
Copy link

imana97 commented Apr 8, 2017

One other way is to create beforeSave hooks and check if the file attr of the parseobject has a new value. Of course this is very manual. but it might help.

Parse.Cloud.beforeSave('yourObject', function (req, res) {
  if (req.original) {
    if (req.original.get('fileAttr') && req.object.get('fileAttr')) {
      if (req.original.get('fileAttr').url() != req.object.get('fileAttr').url()) {
        // File has changed, you can delete the original file at req.original.get('fileAttr').url()
      }
    }
  }
  
  res.success();
});

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