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

Add endpoints for bulk deletes. #602

Merged
merged 6 commits into from
Nov 19, 2020

Conversation

taras-skycoin
Copy link
Member

Did you run make format && make check?

Fixes #500

Changes:

  • Add endpoints for bulk deletes of the routes and transports.

How to test this PR:

  1. Create a few transports and routes
  2. Run next code to delete routes:
curl 'http://localhost:8000/api/visors/02db5bda652554bef5b6166be2cd41f9dec75eff39bd31af0764028b3db921a5ff/routes/' -X DELETE -H 'Content-Type: application/json' --data '["1","2"]'
  1. Run next code to delete transports:
 curl 'http://localhost:8000/api/visors/02db5bda652554bef5b6166be2cd41f9dec75eff39bd31af0764028b3db921a5ff/transports/' -X DELETE -H 'Content-Type: application/json'   --data '["b02db85f-1b5c-0c6d-b76f-9bd974e03aa9", "82a61140-03c8-0766-aca8-e7f078aef46b"]'  

@i-hate-nicknames
Copy link
Contributor

i-hate-nicknames commented Nov 16, 2020

Just off the top of my head, could there be any issues with partial success? As it currently is, it seems that if we are halfway through deletion process and we encounter an error, the status returned will be an error, while in reality it was a partial success.

I'm not sure what a good solution might be, or if it's even worth the effort, but I'd probably go through the whole list and gather errors, and then return the list of errors to the client. Maybe an overkill in this case if the client doesn't make any use of the response

@jdknives
Copy link
Member

@Senyoret1 please check this PR and whether it does work for you. Maybe you have some preference for how to handle partial completion of bulk deletes.

@taras-skycoin
Copy link
Member Author

Right now it responds with answers like these:

curl 'http://localhost:8000/api/visors/02db5bda652554bef5b6166be2cd41f9dec75eff39bd31af0764028b3db921a5ff/routes/' -X DELETE -H 'Content-Type: application/json' --data '["1","2"]'
{"1":{"success":true,"error":""},"2":{"success":true,"error":""}}

curl 'http://localhost:8000/api/visors/02db5bda652554bef5b6166be2cd41f9dec75eff39bd31af0764028b3db921a5ff/transports/' -X DELETE -H 'Content-Type: application/json'   --data '["b02db85f-1b5c-0c6d-b76f-9bd974e03aa9", "82a61140-03c8-0766-aca8-e7f078aef46b", "1"]'
{"1":{"success":false,"error":"invalid UUID length: 1"},"82a61140-03c8-0766-aca8-e7f078aef46b":{"success":true,"error":""},"b02db85f-1b5c-0c6d-b76f-9bd974e03aa9":{"success":true,"error":""}}

@taras-skycoin
Copy link
Member Author

@i-hate-nicknames good suggestion. Thank you. I have implemented it. Would you mind reviewing the pull request again?

@Senyoret1
Copy link
Contributor

I tested the changes and found some things:

  • Removing one or more transports is working well. With routes is the same.
  • If I try to remove a transport using an id which is valid (not a random string) but is not in the node, the response returns transport "success": true for that id, which does not appear to be correct. In fact, if you try to remove the same transport several times, it will always return transport "success": true. This is different to how the /visors/{pk}/transports/{tid} API endpoint works, which returns "error": "transport of ID 5c91c9b4-4ab0-0de8-b88a-f0e065071161 is not found" in that case.
  • Trying to remove inexistent routes works in a similar way: there is no indication of error when trying to remove a route which is not part of the visor. However, the API reacts differently when removing routes: it only returns results when there are errors, there is no response for routes that were correctly removed. I do not have preferences in this case, I think that returning and not returning responses for deleted transports and routes is ok, but the behavior should eb consistent.

@taras-skycoin
Copy link
Member Author

@Senyoret1 thank you very much for your important suggestions. I have improved error handling and added requests to check if we have that transport or route before deleting. I think it more consistent right now.

Would you mind testing it again and tell if it meets your expectations?

Copy link
Contributor

@Senyoret1 Senyoret1 left a comment

Choose a reason for hiding this comment

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

Great, I think it is working well now 👍. Thank you

@jdknives jdknives merged commit 48b7867 into skycoin:develop Nov 19, 2020
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

4 participants