Re: Review Request 116017: Implement POST - POST redirection support in KIO
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/#review51568 --- This review has been submitted with commit 5fd9d1a7b2d0afda2bf76bcdbd4df5577b893387 by Dawit Alemayehu to branch KDE/4.13. - Commit Hook On Feb. 28, 2014, 5:44 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/ --- (Updated Feb. 28, 2014, 5:44 a.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Bugs: 330890 http://bugs.kde.org/show_bug.cgi?id=330890 Repository: kdelibs Description --- The attached patch implements support for redirecting one POST request to another in KIO. Unless implicitly disabled currently the automatic redirection handler in KIO always redirects any POST requests to a GET. Note this patch also changes the original KIO::http_post implementation that accepted a QByteArray to simply store the data in a QBuffer and call the newer implementation that uses a QIODevice. I have updated the documentation for the original implementation to state as such and encourage developers to directly use the newer http_post method instead. Not sure if everyone will agree with my implementation but it makes it much easier to resend POST data on redirection. Diffs - kio/kio/job.h aeaffa2 kio/kio/job.cpp edc5fed kio/kio/job_p.h 5ead3ed kioslave/http/http.cpp 9eba5d1 Diff: https://git.reviewboard.kde.org/r/116017/diff/ Testing --- http://greenbytes.de/tech/tc/httpredirects/t307methods.html http://greenbytes.de/tech/tc/httpredirects/t308methods.html http://www.w3.org/People/Bos/Test/redirect307.html Thanks, Dawit Alemayehu
Re: Review Request 116017: Implement POST - POST redirection support in KIO
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/ --- (Updated Feb. 27, 2014, 1:41 p.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Changes --- Added associated bug report number to the ticket. Bugs: 330890 http://bugs.kde.org/show_bug.cgi?id=330890 Repository: kdelibs Description --- The attached patch implements support for redirecting one POST request to another in KIO. Unless implicitly disabled currently the automatic redirection handler in KIO always redirects any POST requests to a GET. Note this patch also changes the original KIO::http_post implementation that accepted a QByteArray to simply store the data in a QBuffer and call the newer implementation that uses a QIODevice. I have updated the documentation for the original implementation to state as such and encourage developers to directly use the newer http_post method instead. Not sure if everyone will agree with my implementation but it makes it much easier to resend POST data on redirection. Diffs - kioslave/http/http.cpp 9eba5d1 kio/kio/job_p.h 5ead3ed kio/kio/job.h aeaffa2 kio/kio/job.cpp edc5fed Diff: https://git.reviewboard.kde.org/r/116017/diff/ Testing (updated) --- http://greenbytes.de/tech/tc/httpredirects/t307methods.html http://greenbytes.de/tech/tc/httpredirects/t308methods.html http://www.w3.org/People/Bos/Test/redirect307.html Thanks, Dawit Alemayehu
Re: Review Request 116017: Implement POST - POST redirection support in KIO
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/#review51063 --- kio/kio/job.cpp https://git.reviewboard.kde.org/r/116017/#comment35803 This seems to have no effect in the next GET request, shouldn't be added to the outgoing metadata? - Andrea Iacovitti On Feb. 27, 2014, 1:41 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/ --- (Updated Feb. 27, 2014, 1:41 p.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Bugs: 330890 http://bugs.kde.org/show_bug.cgi?id=330890 Repository: kdelibs Description --- The attached patch implements support for redirecting one POST request to another in KIO. Unless implicitly disabled currently the automatic redirection handler in KIO always redirects any POST requests to a GET. Note this patch also changes the original KIO::http_post implementation that accepted a QByteArray to simply store the data in a QBuffer and call the newer implementation that uses a QIODevice. I have updated the documentation for the original implementation to state as such and encourage developers to directly use the newer http_post method instead. Not sure if everyone will agree with my implementation but it makes it much easier to resend POST data on redirection. Diffs - kioslave/http/http.cpp 9eba5d1 kio/kio/job_p.h 5ead3ed kio/kio/job.h aeaffa2 kio/kio/job.cpp edc5fed Diff: https://git.reviewboard.kde.org/r/116017/diff/ Testing --- http://greenbytes.de/tech/tc/httpredirects/t307methods.html http://greenbytes.de/tech/tc/httpredirects/t308methods.html http://www.w3.org/People/Bos/Test/redirect307.html Thanks, Dawit Alemayehu
Re: Review Request 116017: Implement POST - POST redirection support in KIO
On Feb. 24, 2014, 7:17 p.m., David Faure wrote: kio/kio/job.h, line 307 https://git.reviewboard.kde.org/r/116017/diff/1/?file=245840#file245840line307 recommended you use the function for when... - rather unclear. Which function? In the commit log you said you wanted to encourage using the QIODevice overload directly (which I assume this is trying to do, although not clearly). But even then I don't think it makes sense. Anyone who has a QByteArray on their hands should call the function with a QByteArray rather than just create a QBuffer in order to be able to call the recommended function -- all this achieves is duplicating that bit of code into the apps. I would suggest removing the whole note, simply. Two overloads = pick the most convenient one for the calling code. Dawit Alemayehu wrote: Well the intent was to discourage people from using QByteArrays to store upload data in the first place so as to avoid the same mistake committed in KHTML. Currently KHTML stores the contents of an entire file, no matter how large, into memory whenever a user attempts to upload such files. My attempt to get the KHTML developers to realize that is the wrong thing to do did not get anywhere. Anyways, you are correct. It is pointless to attempt to discourage people from using API that has been available since the creation of KIO. Oh, I agree. If one can provide the data as a QIODevice that retrieves data on demand that is indeed much better. But simply deprecating the method that takes a QByteArray will lead to absurdity for people who actually have a QByteArray in the first place and no other choice: they'd have to create a QBuffer around it just to call the right method, that's just inconvenient. However I agree - if one has a choice, one should prefer not to create a qbytearray with the full data in the first place. has been available since the creation of kio isn't at all any reason for or against, times change and things improve :) We deprecate things for a good reason. I just don't think there's a good reason to deprecate the QByteArray overload. At most it can say in its documentation if you can, try not to load the full data upfront, and use the QIODevice overload instead. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/#review50701 --- On Feb. 27, 2014, 1:41 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/ --- (Updated Feb. 27, 2014, 1:41 p.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Bugs: 330890 http://bugs.kde.org/show_bug.cgi?id=330890 Repository: kdelibs Description --- The attached patch implements support for redirecting one POST request to another in KIO. Unless implicitly disabled currently the automatic redirection handler in KIO always redirects any POST requests to a GET. Note this patch also changes the original KIO::http_post implementation that accepted a QByteArray to simply store the data in a QBuffer and call the newer implementation that uses a QIODevice. I have updated the documentation for the original implementation to state as such and encourage developers to directly use the newer http_post method instead. Not sure if everyone will agree with my implementation but it makes it much easier to resend POST data on redirection. Diffs - kioslave/http/http.cpp 9eba5d1 kio/kio/job_p.h 5ead3ed kio/kio/job.h aeaffa2 kio/kio/job.cpp edc5fed Diff: https://git.reviewboard.kde.org/r/116017/diff/ Testing --- http://greenbytes.de/tech/tc/httpredirects/t307methods.html http://greenbytes.de/tech/tc/httpredirects/t308methods.html http://www.w3.org/People/Bos/Test/redirect307.html Thanks, Dawit Alemayehu
Re: Review Request 116017: Implement POST - POST redirection support in KIO
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/ --- (Updated Feb. 28, 2014, 5:42 a.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Changes --- Updated patch. Bugs: 330890 http://bugs.kde.org/show_bug.cgi?id=330890 Repository: kdelibs Description --- The attached patch implements support for redirecting one POST request to another in KIO. Unless implicitly disabled currently the automatic redirection handler in KIO always redirects any POST requests to a GET. Note this patch also changes the original KIO::http_post implementation that accepted a QByteArray to simply store the data in a QBuffer and call the newer implementation that uses a QIODevice. I have updated the documentation for the original implementation to state as such and encourage developers to directly use the newer http_post method instead. Not sure if everyone will agree with my implementation but it makes it much easier to resend POST data on redirection. Diffs (updated) - kio/kio/job.h aeaffa2 kio/kio/job.cpp edc5fed kio/kio/job_p.h 5ead3ed kioslave/http/http.cpp 9eba5d1 Diff: https://git.reviewboard.kde.org/r/116017/diff/ Testing --- http://greenbytes.de/tech/tc/httpredirects/t307methods.html http://greenbytes.de/tech/tc/httpredirects/t308methods.html http://www.w3.org/People/Bos/Test/redirect307.html Thanks, Dawit Alemayehu
Re: Review Request 116017: Implement POST - POST redirection support in KIO
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/ --- (Updated Feb. 28, 2014, 5:44 a.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Changes --- Updated the branch this patch applies to. Bugs: 330890 http://bugs.kde.org/show_bug.cgi?id=330890 Repository: kdelibs Description --- The attached patch implements support for redirecting one POST request to another in KIO. Unless implicitly disabled currently the automatic redirection handler in KIO always redirects any POST requests to a GET. Note this patch also changes the original KIO::http_post implementation that accepted a QByteArray to simply store the data in a QBuffer and call the newer implementation that uses a QIODevice. I have updated the documentation for the original implementation to state as such and encourage developers to directly use the newer http_post method instead. Not sure if everyone will agree with my implementation but it makes it much easier to resend POST data on redirection. Diffs - kio/kio/job.h aeaffa2 kio/kio/job.cpp edc5fed kio/kio/job_p.h 5ead3ed kioslave/http/http.cpp 9eba5d1 Diff: https://git.reviewboard.kde.org/r/116017/diff/ Testing --- http://greenbytes.de/tech/tc/httpredirects/t307methods.html http://greenbytes.de/tech/tc/httpredirects/t308methods.html http://www.w3.org/People/Bos/Test/redirect307.html Thanks, Dawit Alemayehu
Re: Review Request 116017: Implement POST - POST redirection support in KIO
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/ --- (Updated Feb. 26, 2014, 8:34 a.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Changes --- Updated version of the patch that cleans up the HTTP protocol changes and tested using both webkit and khtml engines against the greenbytes test server. Repository: kdelibs Description --- The attached patch implements support for redirecting one POST request to another in KIO. Unless implicitly disabled currently the automatic redirection handler in KIO always redirects any POST requests to a GET. Note this patch also changes the original KIO::http_post implementation that accepted a QByteArray to simply store the data in a QBuffer and call the newer implementation that uses a QIODevice. I have updated the documentation for the original implementation to state as such and encourage developers to directly use the newer http_post method instead. Not sure if everyone will agree with my implementation but it makes it much easier to resend POST data on redirection. Diffs (updated) - kio/kio/job.h aeaffa2 kio/kio/job.cpp edc5fed kio/kio/job_p.h 5ead3ed kioslave/http/http.cpp 9eba5d1 Diff: https://git.reviewboard.kde.org/r/116017/diff/ Testing --- http://greenbytes.de/tech/tc/httpredirects/t307methods.html http://greenbytes.de/tech/tc/httpredirects/t308methods.html Thanks, Dawit Alemayehu
Re: Review Request 116017: Implement POST - POST redirection support in KIO
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/#review50948 --- kio/kio/job.cpp https://git.reviewboard.kde.org/r/116017/#comment35767 This seems not needed. We can handle GET redirection just by setting redirect-to-get to true; in this case we already set d-m_command = CMD_GET before the switch statement, then case CMD_SPECIAL: will not be executed. (See also related comments on http.cpp part) kioslave/http/http.cpp https://git.reviewboard.kde.org/r/116017/#comment35760 You removed case 302 (may be unintentionally ?) We should check for m_request.sentMethodString == POST otherwise PUT (or DELETE) method with body data (i.e. m_request.method == HTTP_POST and m_request.sentMethod == (PUT || DELETE) ) will be rewritten to GET (wrong!) for 301/302 response code. This will happen in kio/job.cpp where we end up to execute the case CMD_SPECIAL code with redirectToGet != false (actually empty) kioslave/http/http.cpp https://git.reviewboard.kde.org/r/116017/#comment35764 It seems to me this case 307 is not needed, we know the only case in which a redirect to GET have to be performed is for 301/302 response and method POST (and for 303, but this case is already handled just above): 307, 308 never rewrite method. On overall i think this part of code doesn't need modifications, except removing the m_request.method = HTTP_GET; // FORCE a GET parts that, yes, seem not needed. - Andrea Iacovitti On Feb. 26, 2014, 8:34 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/ --- (Updated Feb. 26, 2014, 8:34 a.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Repository: kdelibs Description --- The attached patch implements support for redirecting one POST request to another in KIO. Unless implicitly disabled currently the automatic redirection handler in KIO always redirects any POST requests to a GET. Note this patch also changes the original KIO::http_post implementation that accepted a QByteArray to simply store the data in a QBuffer and call the newer implementation that uses a QIODevice. I have updated the documentation for the original implementation to state as such and encourage developers to directly use the newer http_post method instead. Not sure if everyone will agree with my implementation but it makes it much easier to resend POST data on redirection. Diffs - kio/kio/job.h aeaffa2 kio/kio/job.cpp edc5fed kio/kio/job_p.h 5ead3ed kioslave/http/http.cpp 9eba5d1 Diff: https://git.reviewboard.kde.org/r/116017/diff/ Testing --- http://greenbytes.de/tech/tc/httpredirects/t307methods.html http://greenbytes.de/tech/tc/httpredirects/t308methods.html Thanks, Dawit Alemayehu
Re: Review Request 116017: Implement POST - POST redirection support in KIO
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/#review50957 --- ... may be it's more clear what I mean in my comments by showing code, see http://paste.kde.org/pvzj0ppio I did many tests and didn't found issues so far - Andrea Iacovitti On Feb. 26, 2014, 8:34 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/ --- (Updated Feb. 26, 2014, 8:34 a.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Repository: kdelibs Description --- The attached patch implements support for redirecting one POST request to another in KIO. Unless implicitly disabled currently the automatic redirection handler in KIO always redirects any POST requests to a GET. Note this patch also changes the original KIO::http_post implementation that accepted a QByteArray to simply store the data in a QBuffer and call the newer implementation that uses a QIODevice. I have updated the documentation for the original implementation to state as such and encourage developers to directly use the newer http_post method instead. Not sure if everyone will agree with my implementation but it makes it much easier to resend POST data on redirection. Diffs - kio/kio/job.h aeaffa2 kio/kio/job.cpp edc5fed kio/kio/job_p.h 5ead3ed kioslave/http/http.cpp 9eba5d1 Diff: https://git.reviewboard.kde.org/r/116017/diff/ Testing --- http://greenbytes.de/tech/tc/httpredirects/t307methods.html http://greenbytes.de/tech/tc/httpredirects/t308methods.html Thanks, Dawit Alemayehu
Re: Review Request 116017: Implement POST - POST redirection support in KIO
On Feb. 26, 2014, 4:15 p.m., Andrea Iacovitti wrote: ... may be it's more clear what I mean in my comments by showing code, see http://paste.kde.org/pvzj0ppio I did many tests and didn't found issues so far No I completely understood your point. It was just that my implementation was from the perspective of I am going to preserve the current functionality and only allow the new POST - POST redirection when explicitly requested. However, there was no need for that since the behavior in the http ioslave was also changed at the same time. As such I will change KIO's default POST redirection behavior to do POST by default and that override that through the use of the new redirect-to-get meta-data. - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/#review50957 --- On Feb. 26, 2014, 8:34 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/ --- (Updated Feb. 26, 2014, 8:34 a.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Repository: kdelibs Description --- The attached patch implements support for redirecting one POST request to another in KIO. Unless implicitly disabled currently the automatic redirection handler in KIO always redirects any POST requests to a GET. Note this patch also changes the original KIO::http_post implementation that accepted a QByteArray to simply store the data in a QBuffer and call the newer implementation that uses a QIODevice. I have updated the documentation for the original implementation to state as such and encourage developers to directly use the newer http_post method instead. Not sure if everyone will agree with my implementation but it makes it much easier to resend POST data on redirection. Diffs - kio/kio/job.h aeaffa2 kio/kio/job.cpp edc5fed kio/kio/job_p.h 5ead3ed kioslave/http/http.cpp 9eba5d1 Diff: https://git.reviewboard.kde.org/r/116017/diff/ Testing --- http://greenbytes.de/tech/tc/httpredirects/t307methods.html http://greenbytes.de/tech/tc/httpredirects/t308methods.html Thanks, Dawit Alemayehu
Re: Review Request 116017: Implement POST - POST redirection support in KIO
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/ --- (Updated Feb. 27, 2014, 6:04 a.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Changes --- Final change based on feedback. Repository: kdelibs Description --- The attached patch implements support for redirecting one POST request to another in KIO. Unless implicitly disabled currently the automatic redirection handler in KIO always redirects any POST requests to a GET. Note this patch also changes the original KIO::http_post implementation that accepted a QByteArray to simply store the data in a QBuffer and call the newer implementation that uses a QIODevice. I have updated the documentation for the original implementation to state as such and encourage developers to directly use the newer http_post method instead. Not sure if everyone will agree with my implementation but it makes it much easier to resend POST data on redirection. Diffs (updated) - kioslave/http/http.cpp 9eba5d1 kio/kio/job_p.h 5ead3ed kio/kio/job.h aeaffa2 kio/kio/job.cpp edc5fed Diff: https://git.reviewboard.kde.org/r/116017/diff/ Testing --- http://greenbytes.de/tech/tc/httpredirects/t307methods.html http://greenbytes.de/tech/tc/httpredirects/t308methods.html Thanks, Dawit Alemayehu
Re: Review Request 116017: Implement POST - POST redirection support in KIO
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/#review50761 --- I quickly tested the patch using a basic html form and it seems it break http post operation (regardless of redirection). I get back a 400 Bad Request from the server and can see the following debug message: kio_http(17852)/kio (kioslave) KIO::SlaveBasePrivate::verifyState: special() did not call finished() or error()! Please fix the KIO slave. As for these page http://greenbytes.de/tech/tc/httpredirects/t307methods.html http://greenbytes.de/tech/tc/httpredirects/t308methods.html they can't be used for testing as they do make XHR requests with no body data. - Andrea Iacovitti On Feb. 24, 2014, 2:07 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/ --- (Updated Feb. 24, 2014, 2:07 p.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Repository: kdelibs Description --- The attached patch implements support for redirecting one POST request to another in KIO. Unless implicitly disabled currently the automatic redirection handler in KIO always redirects any POST requests to a GET. Note this patch also changes the original KIO::http_post implementation that accepted a QByteArray to simply store the data in a QBuffer and call the newer implementation that uses a QIODevice. I have updated the documentation for the original implementation to state as such and encourage developers to directly use the newer http_post method instead. Not sure if everyone will agree with my implementation but it makes it much easier to resend POST data on redirection. Diffs - kio/kio/job.h aeaffa2 kio/kio/job.cpp 9cf2b57 kioslave/http/http.cpp 9eba5d1 Diff: https://git.reviewboard.kde.org/r/116017/diff/ Testing --- http://greenbytes.de/tech/tc/httpredirects/t307methods.html http://greenbytes.de/tech/tc/httpredirects/t308methods.html Thanks, Dawit Alemayehu
Re: Review Request 116017: Implement POST - POST redirection support in KIO
On Feb. 24, 2014, 8:47 p.m., Andrea Iacovitti wrote: I quickly tested the patch using a basic html form and it seems it break http post operation (regardless of redirection). I get back a 400 Bad Request from the server and can see the following debug message: kio_http(17852)/kio (kioslave) KIO::SlaveBasePrivate::verifyState: special() did not call finished() or error()! Please fix the KIO slave. As for these page http://greenbytes.de/tech/tc/httpredirects/t307methods.html http://greenbytes.de/tech/tc/httpredirects/t308methods.html they can't be used for testing as they do make XHR requests with no body data. Fixed. It would actually help if the QBuffer was opened before attempting to read from it. :( - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/#review50761 --- On Feb. 24, 2014, 2:07 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116017/ --- (Updated Feb. 24, 2014, 2:07 p.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Repository: kdelibs Description --- The attached patch implements support for redirecting one POST request to another in KIO. Unless implicitly disabled currently the automatic redirection handler in KIO always redirects any POST requests to a GET. Note this patch also changes the original KIO::http_post implementation that accepted a QByteArray to simply store the data in a QBuffer and call the newer implementation that uses a QIODevice. I have updated the documentation for the original implementation to state as such and encourage developers to directly use the newer http_post method instead. Not sure if everyone will agree with my implementation but it makes it much easier to resend POST data on redirection. Diffs - kio/kio/job.h aeaffa2 kio/kio/job.cpp 9cf2b57 kioslave/http/http.cpp 9eba5d1 Diff: https://git.reviewboard.kde.org/r/116017/diff/ Testing --- http://greenbytes.de/tech/tc/httpredirects/t307methods.html http://greenbytes.de/tech/tc/httpredirects/t308methods.html Thanks, Dawit Alemayehu
