Address.email: Why not use _email?

[hoffie]

When handling Zarafa-internal addresses, python-zarafa explicitly looks up the user name from the known entryid using OpenEntry.
Is there a reason why this is necessary?
As far as I can see, the username is stored in the email property (Address._email) if addrtype is ZARAFA. Am I missing something?

If not, the following patch gives a performance boost on my system:

diff --git a/zarafa/__init__.py b/zarafa/__init__.py
index f742f52..5992ede 100644
--- a/zarafa/__init__.py
+++ b/zarafa/__init__.py
@@ -2539,8 +2539,8 @@ class Address:

         if self.addrtype == 'ZARAFA':
             try:
-                mailuser = self.server.mapisession.OpenEntry(self.entryid, None, 0)
-                return self.server.user(HrGetOneProp(mailuser, PR_ACCOUNT).Value).email # XXX PR_SMTP_ADDRESS_W from mailuser?
+                # with type ZARAFA, the email property contains the username.
+                return self.server.user(self._email).email # XXX PR_SMTP_ADDRESS_W from mailuser?
             except (ZarafaException, MAPIErrorNotFound): # XXX deleted user
                 return None # XXX 'Support Delft'??
         else:

[srepmub]

thanks! I'm afraid that I'm not 100% sure that the 'email' property passed to Address.init is always set.. will check this hopefully soon.

[hoffie]

Ah right, I did not consider that. So the previous code should probably be left in for such cases. To avoid making this part so complicated, I would rather introduce a username property.
I am attaching an untested patch which should at least conceptionally break lesser things than the first attempt.

There are probably more optimization opportunities (e.g. when repeatedly accessing the .email field or something), but this is currently not a priority for me.

diff --git a/zarafa/__init__.py b/zarafa/__init__.py
index f742f52..b9d50f3 100644
--- a/zarafa/__init__.py
+++ b/zarafa/__init__.py
@@ -2526,6 +2526,10 @@ class Address:
         self._name = name
         self._email = email
         self.entryid = entryid
+        if addrtype == 'ZARAFA' and email:
+            self._username = email
+        else:
+            self._username = None

     @property
     def name(self):
@@ -2534,13 +2538,19 @@ class Address:
         return self._name

     @property
+    def username(self):
+        if not self._username:
+            mailuser = self.server.mapisession.OpenEntry(self.entryid, None, 0)
+            self._username = HrGetOneProp(mailuser, PR_ACCOUNT).Value
+        return self._username
+
+    @property
     def email(self):
         """ Email address """

         if self.addrtype == 'ZARAFA':
             try:
-                mailuser = self.server.mapisession.OpenEntry(self.entryid, None, 0)
-                return self.server.user(HrGetOneProp(mailuser, PR_ACCOUNT).Value).email # XXX PR_SMTP_ADDRESS_W from mailuser?
+                return self.server.user(self.username).email # XXX PR_SMTP_ADDRESS_W from mailuser?
             except (ZarafaException, MAPIErrorNotFound): # XXX deleted user
                 return None # XXX 'Support Delft'??
         else:

[srepmub]

it would be useful to be able to resolve the zarafa user for a given address. but then 'user' sounds more generally useful than just 'username'. how about something like this:

+    @property(self)
+    def user(self):
+        try:
+            mailuser = self.server.mapisession.OpenEntry(self.entryid, None, 0)
+            return self.server.user(HrGetOneProp(mailuser, PR_ACCOUNT).Value) # XXX user(entryid=self.entryid)
+        except MAPIErrorNotFound, ZarafaExceptionNotFound:
+            pass
+   
     @property
     def email(self):
         """ Email address """

         if self.addrtype == 'ZARAFA':
-            try:
-                mailuser = self.server.mapisession.OpenEntry(self.entryid, None, 0)
-                return self.server.user(HrGetOneProp(mailuser, PR_ACCOUNT).Value).email # XXX PR_SMTP_ADDRESS_W from mailuser?
-            except (ZarafaException, MAPIErrorNotFound): # XXX deleted user
-                return None # XXX 'Support Delft'??
+            return self._email or self.user.email
         else:   
             return self._email

[hoffie]

I agree, API-wise this is certainly nicer and would be an improvement for the current code.
A minor performance-relevant difference to my proposal would be the repeated user lookup when repeatedly accessing the email property. However, this can be cached by potential callers if necessary and I am not sure whether this would warrant complicating the implementation by caching the user object.
Also, I think there is a bug in exception handling: The two exceptions should be enclosed within brackets, otherwise this would probably be equivalent to except MAPIErrorNotFound as ZarafaExceptionNotFound which is probably not meant. :)

[srepmub]

in general, we probably want to cache things better. but for now we are mostly focusing on getting the API right and optimize things where needed/most useful.

good point about the exception, I make that mistake more often.. :-)