V3.2 IMAP Refactor

Document created by resplin Employee on Jun 6, 2015
Version 1Show Document
  • View in full screen mode

Obsolete Pages{{Obsolete}}

The official documentation is at: http://docs.alfresco.com




Phase One


COMPLETED!

The first part of the refactor will deal with code structure and critical fixes.  All changes listed must be implemented.  Mark them as fixed here with the appropriate CHK link e.g.



;Patches (Critical) FIXED https://issues.alfresco.com/jira/browse/CHK-XXXX

Patches (Critical)

  • Patches need to be implemented using the standard patch mechanism.  The patch definition ensures that the correct data is in the system.  There will therefore be no need to enable/disable the IMAP features based on whether or not the patches have been executed.  Fix this by adding the appropriate patch entries in patch-services-context.xml and incrementing the schema version in version.properties.  Ask for more details if this is not clear from all the other patch examples.  Let the normal patch services handle the patch execution.
  • The patch implementations will be dealt with later.
ImapHelper
The helper should contain nothing more than a few static helper methods to manipulate common structures or to do some error handling.

AlfrescoImapServer

  • This is fine, but remove the ImapHelper and therefore the check for the patch presence.  The patch MUST already be there if the system was upgraded - you can assume that the PatchService can do its job.
AlfrescoImapHostManager

  • This class should only serve to direct incoming IMAP requests through to an appropriate service.  It must not implement any business logic.
  • It should be created in Spring with ONLY an instance of ImapService (discussed later) and TransactionService.
  • Handle inbound messages by creating an appropriate (read-write, read-only) transaction and delegate to the ImapService.
ImapService
Interface to wrap all high-level IMAP business logic
API should mirror reqeusts made to IMAP excespt for minor, non-business logic.  The service must be able to fully service the AlfrescoImapHostManager.

ImapServiceImpl
Implementation of the service.
Inject ONLY the required services; FileFolderService, NodeService and ServiceRegistry.
Until further refactoring, this will be very similar to the existing AlfrescoImapHostManager.  Pull the implementation into here and further specifics will be ironed out later.
Several of the calls will act as factory methods for ImageMessage instances.
The separation of the business logic into this service will allow unit tests to be written.
Add ImapService to ServiceRegistry for quick access.

ImapUser
Remove redundant reference to ImapHostManager.

AlfrescoImapMailFolder
There is an interface, MailFolder.  Different behavioural code should be split according to a hierarchy.  This is standard OO.
Implement the following hierarchy for now: MailFolder -- AbstractMailFolder -- ImapMailFolder.
public abstract class AbstractImapMailFolder implements MailFolder
Construct this will the relevant folder-specific options (explicit values) and an instance of the ServiceRegistry.
Handle inbound calls by wrapping in appropriate (read-write, read-only) transactions, running as the correct user and doing proper error handling.  In other words, provide the basic services for derived classes.
Implement all IMAP MailFolder requests and defer to protected abstract methods for all IMAP specifics.
public class ImapMailFolder extends AbstractImapMailFolder
Construct with relevant IMAP-specific values and the ServiceRegistry (available via the base class).
Implement IMAP-specific logic, defering to the ImapService where necessary.


AlfrescoImapMessage
There should be a hierarchy to allow behavioural control based on the type of message e.g. ImapModelMessage and ContentModelMessage.
For now, implement the following hierarchy: MimeMessage -- AbstractMimeMessage -- ImapMimeMessage.
public abstract class AbstractMimeMessage
Construct with the relevant, explicit values and the ServiceRegistry.
Handle inbound calls by wrapping in appropriate (read-write, read-only) transactions, running as the correct user and doing proper error handling.  In other words, provide the basic services for derived classes.
public class ImapMimeMessage
Construct with the relevant IMAP-specific values and the ServiceRegistry (available via the base class).
Implement IMAP-specific logic, defering to the ImapService where necessary.


Unit Tests
Write ImapServiceImplTest using a plain TestCase (see FileFolderServiceImplTest).
Test each method of the ImapService and test the returned, constructed MimeMessage instances

Package Naming
For now, put all classes in the current imap package.




Phase Two


Further changes required


Missing com.beetstra.jutf7 package
Is it really necessary? Does the library conform to the required license (check with Ash)?
Missed addition in SVN?
Please add to svn .classpath as well.
Remove ALL exception absorbtion from Impl methods e.g. AlfrescoImapFolder#copyMessageInternal
The transaction will be marked for rollback so the exception must be propagated out of the transaction.
You can handle specific exceptions outside the transactions created by the CommandCallback but never absorb the exception
Add @Override to your impl classes' implementations of the abstract methods
This makes the code easier to refactor in the future
Remove unused methods
e.g. AlfrescoImapFolder: processTextMessage and saveAttachment
NodeService property setting
Instead of doing NodeService.getProperties() ... addAll ... NodeService.setProperties() use NodeService.addProperties()
IncomingImapMessage#buildMessageInternal was the only place I could find
File Exists
I couldn't find checks in the IMAP code for 'File exists' conditions.
file/folder creation should be pre-empted with existence checks
FileExistsException to be handled specially
Please add unit tests for duplicate files and folders
I don't know how IMAP does all of this exactly, so use your discretion

Changes I did


Unit Test
Fixed authentication to remove hard-coded 'admin' username.  We've been trying to get rid of these.

Let me know once these updates are done.  At the moment I can't test the upgrades because of the missing library so let me know once that is in and I'll test the patches.




Phase Three


com.beetstra.jutf7
Is this package really necessary?
Please confirm that there are no alternatives to conversion to/from UTF7.
Please confirm that the library meets the licensing requirements.
Patched libraries
Please add diff files for source patches of 3rd party libraries that you change.
Branch Integration [DONE]
Perform a reintegration of changes from HEAD to the DEV/IMAP3 branch
See 'svn merge --reintegrate' OR create a new DEV/BELARUS/IMAP4 branch from HEAD and merge the IMAP3 changes into that.
Upgrade Tests
Perform upgrades to the IMAP3 codeline from V3.1 and HEAD and validate UI-based functionality
Default Startup
The IMAP subsystem should be enabled by default, but the IMAP server must be disabled.  Please modify the 'imap' subsystem bean as follows:

   <bean id='imap' class='org.alfresco.repo.management.subsystems.ChildApplicationContextFactory' parent='abstractPropertyBackedBean'>
       <property name='autoStart'>
           <value>true</value>
       </property>
       <property name='compositePropertyTypes'>
           <map>
               <entry key='imap.server.mountPoints'>
                   <value>org.alfresco.repo.imap.config.ImapConfigBean</value>
               </entry>
           </map>
       </property>
   </bean>

The IMAP-specific enable/disable flag needs to be used in the various bootstrap beans.  If IMAP is disabled, the startup() methods must not be called during bootstrap.
Ensure proper separation of the Spring bean creation phase and the Spring application context startup phase. [DONE]
IMAP Unit Tests
These need to be run against a clean repo without additional input or assumptions.
In your test setup() method, create a folder to use for IMAP and set the necessary properties.
Then use the application context to get the bean 'imap' and call the stop() method.  Set the required property for the IMAP path and then start() the subsystem again.
You can shut the subsystem down in tearDown
General Unit Tests
Ensure that the general unit tests run from ant.
It is possible to make ant ignore failures by editing macros.xml
Patches
ImapUsersPatch is not required.  Any new users added to the system will also need a folder added to the IMAP home.  The IMAP home is configurable as well.  Therefore, make the creation of a user's home in the IMAP home happen as required.  Remove the ImapUsersPatch.  [DONE]
ImapFoldersPatch
Please confer with me so that we determine what needs to be patched in and how best to get it done.
Using properties to determine paths to well-known (system-critical) folder locations is not advised (despite the fact that we did it before)
Use well-known paths that are not configurable to look up system-critical locations.

Engineering Notes

Attachments

    Outcomes