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

Endpoint for purging all objects in class #2032

Merged
merged 4 commits into from
Jun 14, 2016

Conversation

Marco129
Copy link
Contributor

Special master key only endpoint related to parse-community/parse-dashboard#124
Usage: DELETE /classes/<className>

@codecov-io
Copy link

codecov-io commented Jun 11, 2016

Current coverage is 91.29%

Merging #2032 into master will increase coverage by 0.10%

@@             master      #2032   diff @@
==========================================
  Files            92         93     +1   
  Lines          6517       6537    +20   
  Methods        1161       1169     +8   
  Messages          0          0          
  Branches       1368       1370     +2   
==========================================
+ Hits           5943       5968    +25   
+ Misses          574        569     -5   
  Partials          0          0          

Powered by Codecov. Last updated by 324cd85...4d2be8a

@@ -95,6 +95,10 @@ export class MongoStorageAdapter {
});
}

purgeCollection(name: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have deleteObjectsByQuery which will remove all objects if you pass {} as the query. Can you use that instead?

@ghost
Copy link

ghost commented Jun 11, 2016

@Marco129 updated the pull request.

1 similar comment
@ghost
Copy link

ghost commented Jun 11, 2016

@Marco129 updated the pull request.

}).then(() => {
if (className == '_Session') {
var cacheAdapter = config.cacheController;
cacheAdapter.user.clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll need to do something similar for role cache. Probably also worth adding a test to make sure that deleting all objects actually causes the cache to get cleared (eg. create a role, add a user to it, delete all roles using this endpoint, and ensure that the user no longer has access to objects that only that role could access)

@drew-gross
Copy link
Contributor

Needs a few changes, which I see you are already starting on :) You should join our development slack channel, shoot me an email (see my profile) and I can add you.

@ghost
Copy link

ghost commented Jun 12, 2016

@Marco129 updated the pull request.

@ghost
Copy link

ghost commented Jun 12, 2016

@Marco129 updated the pull request.

@drew-gross
Copy link
Contributor

Seems like tests are failing now :( maybe something you were relying on changed?

@Marco129
Copy link
Contributor Author

They passed on local :( will take a look again

@drew-gross
Copy link
Contributor

Might be fixed by #2038.

@ghost
Copy link

ghost commented Jun 12, 2016

@Marco129 updated the pull request.

@drew-gross
Copy link
Contributor

Yeah , the failures look like the exact same symptoms as the deepcopy 0.6.1 symptoms. Rebasing onto master should fix the issue.

@ghost
Copy link

ghost commented Jun 13, 2016

@Marco129 updated the pull request.

@ghost
Copy link

ghost commented Jun 13, 2016

@Marco129 updated the pull request.

@drew-gross
Copy link
Contributor

Looks good to me. Since this involves a new public API, I'm wait a bit more to see if anyone else has any comments.

@lklots
Copy link

lklots commented Jun 13, 2016

if someone screws up a delete request and posts /1/classes/${className}/${undefined} will this delete the whole collection? If so that is truly terrible. I would suggest requiring users to pass an additional header like: "X-I-KNOW-WHAT-I-AM-DOING-AND-WILLING-TO-PAY-THE-CONSEQUENCES" or something similar.

@drew-gross
Copy link
Contributor

That is actually a really good point. The endpoint does require the master key, but you could be deleting an object using buggy cloud code, where the master key is expected to be used, and also you may end up with an empty string there accidentally.

@drew-gross
Copy link
Contributor

In the old Parse.com API we would use something like /collections/:className/clear, but it's conceivable that there could be an object whose id is clear so I don't want to go with DELETE /classes/:className/clear.

What about DELETE /purge/:className? I'm not a huge fan of the idea of using a header, that would make implementing "Delete all rows" on the dashboard unnecessarily complicated, and also it would make it impossible to use this endpoint from the API Console.

@lklots
Copy link

lklots commented Jun 14, 2016

reasonable compromise! /purge/++

@flovilmart flovilmart closed this Jun 14, 2016
@flovilmart flovilmart reopened this Jun 14, 2016
@flovilmart
Copy link
Contributor

Why not on the schema endpoint then?

@flovilmart
Copy link
Contributor

Purge endpoint makes sense

@blacha
Copy link
Contributor

blacha commented Jun 14, 2016

Why do we limit this by master key, could we not allow a user to delete all their data?

@ghost
Copy link

ghost commented Jun 14, 2016

@Marco129 updated the pull request.

1 similar comment
@facebook-github-bot
Copy link

@Marco129 updated the pull request.

@drew-gross
Copy link
Contributor

Cool, I think we've reached a good consensus. If people have other ideas, we can still change this before we push the next release.

@blacha allowing a user to delete all data they have write access for seems a little dangerous to me. I think we would want to restrict to classes they also have find permission on, as people may be protecting objects by preventing their object ID from getting out. I think it could be a good idea, but needs more thought.

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.

None yet

7 participants