GNOME Bugzilla – Bug 521921
google calendar backend does not support redirects
Last modified: 2013-09-14 16:52:48 UTC
Please describe the problem: When creating appointments with Evolution-calendar in a google-calendar they don't show up in the google-calendar web-frontend and disappear after restarting Evolution. So I used wireshark to see what could be the problem. It seems everytime Evolution sends a PUT-request the google server answers with a redirect. Evolution however does not follow those redirects, so the appointment does not get added. Steps to reproduce: 1. Add an appointment to a google-calendar in Evolution Actual results: The appointment appears in Evolution, but is not there anymore after restarting Evolution. It also does not appear in the webfrontend of google-calendar. Expected results: The appointment should actually be stored. Also if it fails Evolution should not pretend to have stored it, but show an error instead. Does this happen every time? No, it seems google does not send the redirect everytime. However it only happened to me once, that i did not get a redirect. Other information: http://code.google.com/apis/calendar/developers_guide_protocol.html#CreatingEvents This mentions the redirect as part of the specs.
I got the same issue. Using Evolution 2.22 under Fedora 9 Beta. Created a Google calendar (not WebDAV), it loaded my previous events fine, however I can't create events on Evolution and have them stored in Google Calendar. They dissapear from Evolution after some time. Another tiny bug is that Google Calendars added through Evolution won't show up as Appointments in the Gnome Calendar Panel. (Is this expected behavior?)
*** Bug 525605 has been marked as a duplicate of this bug. ***
The bug can be reproduced on Solaris.
This can be reproduced in Debian Unstable as well.
I confirm that this bug can be reproduced on Debian Lenny too.
I added some printf in libsoup to debug this. As far as I saw, google calendar backend could handle redirect when the backend fetched my events from the server. But when I created a new event with POST command, libsoup just could not read the response. It reported that the connection breaks for some reason I do not know. This is what I got from Solaris.
I am getting same error. I am using Ubuntu Hardy Heron (AMD 64 bits).
I don't think this is related to soup. If you look at servers/google/libgdata-google/gdata-google-service.c of evolution-data-server you can see, that the http_status for 302 (redirect) simply is not handled there. I'll change this bug to e-d-s and version to 2.23.x since i checked it with latest svn.
Created attachment 110877 [details] [review] Patch to make libsoup behave like rfc 2616 and google calender expects I was wrong, this is related to soup. Soup changes every request to a GET-request if it recieves a 302, so it worked for the GET-requests, but not for the POST-requests. The problem is in redirect_handler of soup-session.c, because the second if matches SOUP_STATUS_FOUND (302) and 303. However only in case of 303 the request should be changed to GET according to RFC 2616. But maybe there is a good reason for this behaviour, since a comment in RFC 2616 mentions that many user agents treat 302 like 303. In this case this problem would have to be solved in libgdata-google by disabling the soup-redirect handling and handle 302 for POSTs there. So I'll leave moving the bugreport to the e-d-s developers. I attached a patch to libsoup, which changes this to the expected behaviour and makes adding appointments in Evolution work for me.
Dan, this is a libsoup patch.
Alas, RFC 2616 also says "If the 302 status code is received in response to a request other than GET or HEAD, the user agent MUST NOT automatically redirect the request unless it can be confirmed by the user, since this might change the conditions under which the request was issued." So the strictly-RFC-correct implementation would be to move SOUP_STATUS_FOUND to the first clause of the if statement with MOVED_PERMANENTLY and TEMPORARY_REDIRECT, causing libsoup to ignore the redirection entirely because it's on a POST. (I can't imagine why Google chose to return a 302 here rather than a 307, since the RFC explicitly notes that "most existing user agent implementations treat 302 as if it were a 303 response" and "The status codes 303 and 307 have been added for servers that wish to make unambiguously clear which kind of reaction is expected of the client." Treating 302 as 303 is pretty much required for interoperability with the web as a whole.) Anyway, like I said, if we were going to fix libsoup to be more compliant, that still wouldn't fix your bug, because libsoup wouldn't know that it was actually safe to re-POST at the new URL. And obviously it doesn't make sense to patch libsoup to be *less* compliant. So you'll want to go with Sebastian's other suggestion, to use SOUP_MESSAGE_NO_REDIRECTS and then handle redirects yourself.
Created attachment 110907 [details] [review] patch for libgdata-google to follow redirects like google expects This is the approach of fixing this bug in libgdata-google instead of libsoup.
Created attachment 110909 [details] [review] the real patch Oops, wrong patch, this is the real one.
a) I would prefer SOUP_STATUS_IS_REDIRECTION (msg->status_code) instead of comparing to some concrete value. b) Do not use g_return_if_fail in function which has return value. c) I'm not sure why you use soup_message_set_status_full and two lines below you call soup_status_get_phrase. It returns different text, you know. d) 'soup_message_set_flags (msg, (SoupMessageFlags)NULL);' should be '..., 0);'
Created attachment 112610 [details] [review] improved google redirect patch This is the patch with the changes you suggested, but I currently have not tested it, but it should still work, since these changes are not that fundamentally. I also reverted a small change I made, the patch now checks again for 201 instead of SOUP_STATUS_CREATED since this makes comparing the source to google api documentation easier. If this patch is ok, the same should be done for delete_entry, since there the seem to be redirects, too. I have not yet tested update_entry. Maybe it would also make sense to write a function to handle the redirects so there is not so much code duplication. Also for consistency reasons it might make sense to get rid of all "int http_status" in that file and use msg->status_code instead.
Oh, you scared me a bit. I thought you tested it (even I read above it is not mandatory to be redirected, so it can be harder to test), but anyway, if you see there places which should be fixed too, then do that please. I agree with those you wrote above, so please do that at once, not one by one. Thanks in advance. The patch looks good and as long as you will have this code in one function, then it will be easy to fix, so please do. Also please do a ChangeLog entry. Note: I still didn't try to compile it, because I do want to do that only once.
Created attachment 112632 [details] [review] improved google redirect patch I updated the patch once again to contain all suggested improvements. Additionally I changed all soup_status_get_phrase() to msg->reason_phrase mainly for consistency reasons with the changes I had to make. The redirect also to be added to the update-function. The data seems to be send correctly now (302 -> redirect -> 201), however there might be a problem elsewhere, since the changes don't seem to get applied. However this should be unrelated to this problem and requires further investigation. Delete and insert work as expected now with redirects. Changelog: ** Fixes bug #521921 * servers/google/libgdata-google/gdata-google-service.c: Handle redirects in a way that works with Google API. Fixes some cases of entries not being created or deleted. Patch by Sebastian Keller
Works good, I'll commit to trunk a slightly modified patch, changing function 'handle_google_redirection' to static void send_and_handle_google_redirection (SoupSession *soup_session, SoupMessage *msg) { soup_message_set_flags (msg, SOUP_MESSAGE_NO_REDIRECT); soup_session_send_message (soup_session, msg); soup_message_set_flags (msg, 0); if (SOUP_STATUS_IS_REDIRECTION (msg->status_code)) { ..... } } I tested it and by chance was also redirected on a first shot, and it works. Thanks for the patch.
Created attachment 112663 [details] eds patch (for a reference only) This is whole patch I'll commit to trunk. It's jut for a reference, thus marked as plain text.
Slightly modified version committed to trunk. Committed revision 8978. I did want to commit to stable (gnome-2-22) too, but the code there is very different from the trunk.
*** Bug 530288 has been marked as a duplicate of this bug. ***
*** Bug 536343 has been marked as a duplicate of this bug. ***