Code review comment for lp:~aaronp/rnr-server/delete-review

Revision history for this message
Michael Nelson (michael.nelson) wrote :

Actually, I was just chatting with achuni about this:

14:25 <achuni> we need to add a bit of doc or a comment to
14:26 <achuni> so that people consider the http/https issue when adding a new url
14:26 <achuni> while you're there...
14:27 <achuni> could you ask him to change it to /reviews/delete/<id>/?
14:27 <achuni> I know it's nicer to be able to say /reviews/42 -> brings the review, and /reviews/42/delete -> deletes it
14:28 <achuni> but /reviews/42 is a public (http) request, and /reviews/42/delete needs to be authenticated
14:28 <noodles> oh,
14:29 <noodles> that rules out my suggestion, which was GET /reviews/42 to get, and PUT /reviews/42 to update it (for eg., setting date_deleted)
14:29 <achuni> as a rule of thumb, it's best to leave all the variable part of the url pattern at the end, and make sure it has a unique non-variable prefix
14:29 <achuni> ah that would be extremely hard to do also :-/
14:29 <noodles> yep.
14:29 <achuni> well, unless we don't care about doing https for everybody
14:29 <achuni> but it screws with caching
14:30 <achuni> (I understand retrieved reviews aren't cached if they're fetched over https)
14:30 <noodles> Is it not possible to do GET as http, and PUT/POST/DELETE etc. as https? or is it url only?
14:30 <achuni> (piston-mini-client should fix that really... sucky pos)
14:31 <achuni> atm piston-mini-client distinguishes them by url
14:31 <achuni> ...and Apache's redirect rules do so too I think?
14:31 <achuni> or can Apache say "this url, if it's a GET request, redirect to http"?
14:31 <achuni> I'd bet it can do that, yeah
14:32 <achuni> but we'd still need to realize (on the server) that one url would sometimes need to be authenticated, others not
14:32 <noodles>

So for the moment, it might be best just to modify the url and not worry about the handler changes I mentioned as possibilities. If we find we can do per method apache redirects as above, then I'll do a separate branch updating the handlers.

« Back to merge proposal