After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 521921 - google calendar backend does not support redirects
google calendar backend does not support redirects
Status: RESOLVED FIXED
Product: evolution-data-server
Classification: Platform
Component: Calendar
2.24.x (obsolete)
Other All
: High critical
: ---
Assigned To: evolution-calendar-maintainers
Evolution QA team
: 525605 530288 536343 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2008-03-12 04:46 UTC by Sebastian Keller
Modified: 2013-09-14 16:52 UTC
See Also:
GNOME target: ---
GNOME version: 2.21/2.22


Attachments
Patch to make libsoup behave like rfc 2616 and google calender expects (846 bytes, patch)
2008-05-13 21:31 UTC, Sebastian Keller
none Details | Review
patch for libgdata-google to follow redirects like google expects (846 bytes, patch)
2008-05-14 15:53 UTC, Sebastian Keller
none Details | Review
the real patch (1.87 KB, patch)
2008-05-14 15:56 UTC, Sebastian Keller
needs-work Details | Review
improved google redirect patch (1.89 KB, patch)
2008-06-12 12:20 UTC, Sebastian Keller
reviewed Details | Review
improved google redirect patch (5.27 KB, patch)
2008-06-12 19:00 UTC, Sebastian Keller
committed Details | Review
eds patch (for a reference only) (6.18 KB, text/plain)
2008-06-13 08:06 UTC, Milan Crha
  Details

Description Sebastian Keller 2008-03-12 04:46:54 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.
Comment 1 Juan Rodriguez 2008-03-26 19:55:00 UTC
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?)
Comment 2 Johnny Jacob 2008-04-14 11:09:25 UTC
*** Bug 525605 has been marked as a duplicate of this bug. ***
Comment 3 Wang Xin 2008-04-22 12:09:32 UTC
The bug can be reproduced on Solaris.
Comment 4 Thomas Presthus 2008-05-02 16:28:21 UTC
This can be reproduced in Debian Unstable as well.
Comment 5 Mika Hanhijärvi 2008-05-02 20:49:39 UTC
I confirm that this bug can be reproduced on Debian Lenny too.
Comment 6 Wang Xin 2008-05-04 09:25:27 UTC
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.
Comment 7 Jose Rojas 2008-05-12 03:25:40 UTC
I am getting same error. I am using Ubuntu Hardy Heron (AMD 64 bits).
Comment 8 Sebastian Keller 2008-05-12 03:37:34 UTC
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.
Comment 9 Sebastian Keller 2008-05-13 21:31:46 UTC
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.
Comment 10 Srinivasa Ragavan 2008-05-14 06:29:23 UTC
Dan, this is a libsoup patch.
Comment 11 Dan Winship 2008-05-14 12:23:09 UTC
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.
Comment 12 Sebastian Keller 2008-05-14 15:53:38 UTC
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.
Comment 13 Sebastian Keller 2008-05-14 15:56:04 UTC
Created attachment 110909 [details] [review]
the real patch

Oops, wrong patch, this is the real one.
Comment 14 Milan Crha 2008-06-12 09:53:11 UTC
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);'
Comment 15 Sebastian Keller 2008-06-12 12:20:04 UTC
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.
Comment 16 Milan Crha 2008-06-12 13:35:44 UTC
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.
Comment 17 Sebastian Keller 2008-06-12 19:00:16 UTC
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
Comment 18 Milan Crha 2008-06-13 07:37:54 UTC
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.
Comment 19 Milan Crha 2008-06-13 08:06:22 UTC
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.
Comment 20 Milan Crha 2008-06-13 08:39:32 UTC
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.
Comment 21 Akhil Laddha 2008-09-26 03:52:35 UTC
*** Bug 530288 has been marked as a duplicate of this bug. ***
Comment 22 Akhil Laddha 2008-11-03 12:59:29 UTC
*** Bug 536343 has been marked as a duplicate of this bug. ***