search

vikin91 / BibSpace

BibSpace: Online Bibtex Publications Management Software for Authors and Research Groups
Other
6 stars 2 forks source link

Bug in redirect DELETE /backups/:id #5

Closed jberger closed 7 years ago

jberger commented 7 years ago

Hi. I was just reading through SO and saw your unanswered question. I posted a response http://stackoverflow.com/a/39553660/468327 . It is kinda embarassing because I had to admit that Mojolicious has a bug (at least I assume it is one). That said, it has an important thing in it for your project. I think by using the wrong type of redirect you are accidentally redirecting from DELETE /backups/:id to DELETE /backups. This seems like it might be serious so I wanted to open this issue to be sure you saw it.

Until the Mojo bug is fixed you can't easily test the behavior of 303 redirects, but you should be able to see the different in a browser, which is the important thing I suppose.

jberger commented 7 years ago

This is the Mojo bug in question https://github.com/kraih/mojo/issues/999 and this is a PR which hopefully addresses it https://github.com/kraih/mojo/pull/1000. As I say, the bug only affects your tests, browsers should handle 303s correctly and it won't involve accidentally deleting more than you intended to!

jberger commented 7 years ago

Sorry it is late and I've started to ramble. I've not been very clear. (If it weren't so late I'd probably make you a PR). Even before the Mojo bug is fixed, you should add $self->res->code(303); here https://github.com/vikin91/BibSpace/blob/master/lib/BibSpace/Controller/Backup.pm#L150 . That will cause your redirects to end up as GET requests to /backups. Once the bug is fixed you will be able to write tests for it. (sorry for the rambling, time for bed :-P)

vikin91 commented 7 years ago

Thanks for the information, I will give it a look. I am happy that I could contribute towards discovering this bug.