[Kde-pim] Review Request: Refactoring handling IMAP responses during login phase in Akonadi

[Kevin Krammer]

> On Feb. 3, 2012, 5:07 p.m., Kevin Krammer wrote:
> > kimap/loginjob.cpp, line 272
> > <http://git.reviewboard.kde.org/r/103854/diff/1/?file=48560#file48560line272>
> >
> >     I am wondering whether this is not something that is useful in more places. My guess is that other jobs have to check response codes as well.
> >     Maybe as a function in JobPrivate?
> 
> Oleg Girko wrote:
>     As far as I understand from reading sources for other jobs, responses are already being handled relatively uniform way either by using KIMAP::Job::handleErrorReplies() method or even by not overriding KIMAP::Job::handleResponse() method at all. Most of the jobs just send a single command and expect "OK" response, so default behaviour is sufficient for them.
>     
>     KIMAP::LoginJob is a special complex case because it implements a finite state machine which traverses through several different states in sequence by sending commands and handling their responses. Hence, default response handling is insufficient for KIMAP::LoginJob, so its overriden handleResponse() method uses more complex algorthm to handle all responses by itself.
>     
>     Of course, some refactoring of other jobs could be beneficial for better correctness and readability, but IMHO those changes are logically independent, so they should be placed in separate commits instead of piling them up in a single large change.
> 
> Kevin Krammer wrote:
>     I only did a quick check but I found several occurences of code like this
>     
>     if ( !response.content.isEmpty() && response.content[0].toString() == "+" ) // continuation
>     
>     which seems to be one of the cases handled here as well.
>     
>     I am not saying that this has to be changed everywhere in this change set, just that obviously it is useful in other places as well and thus makes little sense to create yet another occurence of the same sharable piece of code.
>     
>     The thing that triggered my interest was the enum declaration inside a method's body and a string to enum value mapping right after. It just screamed "helper function" to me. I only then checked whether it would be useful elsewhere to determine where to put it.
>     
>     Maybe something like
>     static ResponseCode responseCode( const Message &response )
> 
> Oleg Girko wrote:
>     There is no need for such enum in other job classes.
>     
>     There are 29 job classes in KIMAP namespace. All of them obviously inherit from KIMAP::Job class. There is the default handleResponse() method in this class which contains just one statement:
>       handleErrorReplies(response);
>     The KIMAP::Job::handleErrorReplies() method handles tagged responses (both OK and error conditions), and emits the result if there are no more pending responses for this job. This is sufficient for most jobs.
>     
>     10 jobs do not override handleResponse() method at all. These jobs are: CloseJob, CopyJob, CreateJob, DeleteAclJob, DeleteJob, LogoutJob, RenameJob, SetAclJob, SubscribeJob, UnsubscribeJob.
>     
>     17 jobs override handleResponse() method, but still use KIMAP::Job:handleErrorReplies() method to handle tagged responses. These jobs are: AppendJob, CapabilitiesJob, ExpungeJob, FetchJob, GetAclJob, GetMetaDataJob, GetQuotaJob, GetQuotaRootJob, IdleJob, ListJob, ListRightsJob, MyRightsJob, NamespaceJob, SearchJob, SelectJob, SetQuotaJob, StoreJob. Most of these jobs handle untagged responses only. Only 3 of them handle continuation responses: AppendJob, IdleJob, SearchJob. Comparison like ( response.content[0].toString() == "+" ) is sufficient in these 3 cases.
>     
>     Only 2 jobs handle tagged responses in overridden handleResponse() method by themselves: LoginJob and SetMetaDataJob. Besides tagged responses, LoginJob handles both untagged and continuation responses, whereas SetMetaDataJob handles just continuation responses. Also, SetMetaDataJob handles non-OK tagged responses with finer granularity, making difference between "NO" and other error responses.
>     
>     This means that enum ResponseCode from my change makes sense only in one module: LoginJob, where it is already located. There is no use for OK and ERR values outside of LoginJob, and continuation responses are expected by very few jobs.
>     
>     Also, my plans are not as ambitious as refactoring the whole KIMAP module. I just want to fix the incorrect behaviour in LoginJob, and refactoring of its handleResponse() method is needed to prevent my fix to make its algorithm even more difficult to understand.

Thorough analysis. I was just grepping for "OK" and "+" and found several hits each so it looked useful elsewhere as well.

So the best place for the new method seems to be LoginJobPrivate. If somebody else would like to reuse it they can just move it to JobPrivate and be done.


- Kevin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103854/#review10324
-----------------------------------------------------------


On Feb. 5, 2012, 7:02 p.m., Oleg Girko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103854/
> -----------------------------------------------------------
> 
> (Updated Feb. 5, 2012, 7:02 p.m.)
> 
> 
> Review request for KDEPIM-Libraries.
> 
> 
> Description
> -------
> 
> Refactoring KIMAP::LoginJob::handleResponse() method in kimap/loginjob.cpp file to make its logic more readable, straightforward and correct.
> As a side effect, it fixes https://bugs.kde.org/show_bug.cgi?id=249992 by handling untagged CAPABILITY responses more correct and uniform way.
> 
> This change is trivially backportable to KDE 4.7 (tested with KDE 4.7.4).
> 
> 
> This addresses bug 249992.
>     http://bugs.kde.org/show_bug.cgi?id=249992
> 
> 
> Diffs
> -----
> 
>   kimap/loginjob.cpp fad276d957e46fd00efa20a5f235d02a639ab2c4 
> 
> Diff: http://git.reviewboard.kde.org/r/103854/diff/diff
> 
> 
> Testing
> -------
> 
> Successfully tested with Dovecot IMAP server 2.0.17 using CRAM-MD5 and GSSAPI authentication with unencrypted and SSL connection.
> 
> 
> Thanks,
> 
> Oleg Girko
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/