GNOME Bugzilla – Bug 699847
Build requests according to the correct Exchange server version
Last modified: 2013-05-16 09:34:52 UTC
Some functionalities are only implemented in newer versions of Exchange Server. Another ones are being improved from version to version. Getting the server version after the first connection and using it when necessary is the safest way we have to support new features without break the compatibility with older server versions.
Created attachment 243642 [details] [review] Clean-up in some EEwsConnection's functions APIs
Created attachment 243643 [details] [review] Bug #699847 - Build requests according to the correct Exchange server version Some functionalities are only implemented in newer versions of Exchange Server. Another ones are being improved from version to version. Getting the server version after the first connection and using it when necessary is the safest way we have to support new features without break the compatibility with older server versions. Note that, to use the server version is strongly recommended that you ensure that the version passed in the request is newer than the minimum required. Check how it is being done in: e_ews_connection_create_attachments() and e_ews_connection_get_items()
Review of attachment 243642 [details] [review]: I didn't find anything wrong with this patch, below are just random ideas which I had when going through the patch (expect of the first four comments). Definitely a nice cleanup work, thanks for it. I'm accepting it for master only. ::: src/calendar/e-cal-backend-ews.c @@ +1386,3 @@ const gchar *comp_uid; + if (!e_ews_connection_create_attachments_finish (cnc, &change_key, &ids, res, &error)) { initialize 'ids' to NULL, it's safer than later access uninitialized memory @@ +1387,3 @@ + if (!e_ews_connection_create_attachments_finish (cnc, &change_key, &ids, res, &error)) { + /* make sure there was no error */ the comment doesn't make sense, because if the function returns false, it's expected that there was an error, either server-side, or local (like incorrect parameters). @@ +1391,3 @@ + g_warning ("Error while creating attachments: %s\n", error->message); + g_clear_error (&error); + return; you should return also when no 'error' is set, the function's call failed. @@ +1503,3 @@ + e_data_cal_respond_create_objects (create_data->cal, create_data->context, error, NULL, NULL); + return; + } similar comments as above, only use "Unknown error" if there is no 'error' returned. @@ +4006,3 @@ /* FIXME free free_busy_sl ? */ + g_slist_free_full (free_busy, g_free); + g_slist_free_full (free_busy_data->users, free); g_free, please :) ::: src/server/e-ews-connection.c @@ +6074,1 @@ + if (!parents_ids) better to follow the same pattern, aka positive branch first, then negative, the same as you have it in the above e_ews_connection_create_attachments_finish() @@ +6269,1 @@ *items = async_data->items; I know it doesn't make much sense here, though what about checking for 'if (items) ... else ...' too? It's only to be consistent with other functions. ::: src/server/e-ews-item.c @@ +273,2 @@ + g_slist_free_full (priv->attachments_list, g_free); + priv->attachments_list = NULL; Would it make sense to rename 'attachments_list' to 'attachments_ids'? You know, those are ids, but with attachment infos and ids I looked into the code and checked whether this is storing strings or objects, because the variable name sounds like it stores objects (I know it's from times before attachment info was added, thus this is just an idea).
Review of attachment 243643 [details] [review]: Overall looks good, and works the same. You also got rid of one code duplication with this cleanup, which is appreciated (the before-cleanup code didn't give us many other options, than just duplicate the code). Please consider the below comments before committing to master (only). Thanks. ::: src/addressbook/e-book-backend-ews.c @@ +229,3 @@ + + current_version = e_ews_connection_get_server_version (cnc); + return current_version >= required_version; If you fail to discover server version, then the 'current_version' will be E_EWS_EXCHANGE_UNKNOWN, thus the test will always fail. Is it what we want? ::: src/server/e-ews-message.c @@ +73,3 @@ server_ver = "Exchange2010_SP2"; else server_ver = "Exchange2007"; It'll be good to change the all if-s into a 'switch' and mention also the UNKNOWN value. Just in case. ::: src/server/e-ews-oof-settings.c @@ +977,3 @@ message = e_ews_message_new_with_header ( uri, impersonate_user, "SetUserOofSettingsRequest", + NULL, NULL, e_ews_connection_get_server_version (connection)); Changes in this file don't feel correct. If I connect to a plain 2007 server, then you pass incorrect version for the request, and the server will return an XSD error (or some such), most likely.
> @@ +1503,3 @@ > + e_data_cal_respond_create_objects (create_data->cal, > create_data->context, error, NULL, NULL); > + return; > + } > > similar comments as above, only use "Unknown error" if there is no 'error' > returned. > Makes sense to use g_warning with "Unknown error" in the above function. But makes sense for this one? If does, so, should I basically set the error according to expected by e_data_cal_respond_create_objects() to treat the error correctly?
(In reply to comment #4) > Review of attachment 243643 [details] [review]: > > Overall looks good, and works the same. You also got rid of one code > duplication with this cleanup, which is appreciated (the before-cleanup code > didn't give us many other options, than just duplicate the code). Please > consider the below comments before committing to master (only). Thanks. > Thanks. I also merged it this patch your suggestion (#658892) to check for the server version inside the e_ews_message_new_with_header(). However, I disagree with you about pass the connection to the function. I really would prefer pass the server version, the minimum required version and the boolean to force the minimum required version (yeah, let's be paranoid :-)). I'll reattach the patch as soon I finish the fixes in the first one.
(In reply to comment #5) > Makes sense to use g_warning with "Unknown error" in the above function. But > makes sense for this one? If does, so, should I basically set the error > according to expected by e_data_cal_respond_create_objects() to treat the error > correctly? The reason is to provide a GError, if there was an error behaviour. The GError is the only way how to notify client side about an issue, and if you will not set it, then the client will think that everything worked as expected. (In reply to comment #6) > I also merged it this patch your suggestion (#658892) to check for the server > version inside the e_ews_message_new_with_header(). However, I disagree with > you about pass the connection to the function. OK, I do not mind. My idea was to not pass 3 more arguments, but only one (or two, with the 'force_minimum'). It seemed simpler, and the e_ews_message_new_with_header() is always called in an EEwsConnection, thus the connection is known, but there is not much difference between passing an enum or a connection, thus can be the enum too.
Created attachment 243977 [details] [review] Clean-up in some EEwsConnection's functions APIs
Created attachment 243978 [details] [review] Bug #699847 - Build requests according to the correct Exchange server version Some functionalities are only implemented in newer versions of Exchange Server. Another ones are being improved from version to version. Getting the server version after the first connection and using it when necessary is the safest way we have to support new features without break the compatibility with older server versions. Note that you can force to use the minimum required server version passing force_minimum_version as TRUE to e_ews_message_new_with_header()
Created attachment 243980 [details] [review] Clean-up in some EEwsConnection's functions APIs
Created attachment 243981 [details] [review] Bug #699847 - Build requests according to the correct Exchange server version Some functionalities are only implemented in newer versions of Exchange Server. Another ones are being improved from version to version. Getting the server version after the first connection and using it when necessary is the safest way we have to support new features without break the compatibility with older server versions. Note that you can force to use the minimum required server version passing force_minimum_version as TRUE to e_ews_message_new_with_header()
Review of attachment 243980 [details] [review]: I didn't retest this, but the changes looks fine, please commit to master.
Review of attachment 243981 [details] [review]: Untested, but looks fine. Just fix the below things and commit, please. Thanks. ::: src/addressbook/e-book-backend-ews.c @@ +229,3 @@ + + current_version = e_ews_connection_get_server_version (cnc); + return current_version >= required_version; still not checking for UNKNOWN version ::: src/server/e-ews-message.c @@ +70,3 @@ + /* server info */ + switch (version) { + case (E_EWS_EXCHANGE_2007): why are those enum names closed in brackets? @@ +93,3 @@ + */ + case (E_EWS_EXCHANGE_UNKNOWN): + default: Do not use 'default' here, compiler will claim if you have missing enum values in the switch. (then you can move the UNKNOWN enum at the top too.) ::: src/server/e-soap-response.c @@ +234,3 @@ } + extra empty line
(In reply to comment #13) > ::: src/addressbook/e-book-backend-ews.c > @@ +229,3 @@ > + > + current_version = e_ews_connection_get_server_version (cnc); > + return current_version >= required_version; > > still not checking for UNKNOWN version But it is exactly what I want to do. If we fail to discover the server version, we cannot call any function that expects EWS Server higher than 2007 SP1.
(In reply to comment #14) > But it is exactly what I want to do. > If we fail to discover the server version, we cannot call any function that > expects EWS Server higher than 2007 SP1. OK then. I thought you would want to be more forgiving, the server will report error anyway, but I'm fine if you do this intentionally. Maybe just write a comment there, thus it's obvious also the next time I'll look (go through) the code in sources ;)
Pushed! https://git.gnome.org/browse/evolution-ews/commit/?id=044b4950ffeb596b3bf9d957a87f715c39a918be https://git.gnome.org/browse/evolution-ews/commit/?id=bd23b39f3600dcc93c43e9b5fc17206386037e46
On master: CC libeews_1_2_la-e-ews-item-change.lo CC libeews_1_2_la-e-ews-message.lo e-ews-message.c: In function 'e_ews_message_new_with_header': e-ews-message.c:104:31: warning: 'server_ver' may be used uninitialized in this function [-Wmaybe-uninitialized] e_soap_message_add_attribute (msg, "Version", server_ver, NULL, NULL); ^
Err, pity, I blamed gcc to not claim such things, but I was told that it does it only with certain level of optimization, while I compile with -O0, thus I have bad luck on some of these warnings (it can catch other of them, but this one is not part of the group for me). Fidencio, please fix it as soon as possible. Thanks.
Okay, just pushed. https://git.gnome.org/browse/evolution-ews/commit/?id=d3d8982cee10288876cbf9b4173d37d254df0c53 David, thanks for your report.