[Mailman-Developers] Mysql MemberAdaptor 1.61 and Mailman 2.1.6

[Mark Sapiro]

Adrian Wells wrote:
>
>Mark Sapiro <msapiro at value.net> on Wednesday, October 26, 2005 at 9:06 PM
>+0000 wrote:
>>
>>Actually, it is not really doing the right thing because it is not
>>supposed to be aware of what's in the _BounceInfo class. The info that
>>is passed to it is a string representation of the _BounceInfo
>>instance, and it should really just be saving and retrieving that.
>>IMO, there should be just one column in the MySQL table for this
>>string representation. The only possible snag I see is that the string
>>contains new-lines, and I don't know MySQL so I don't know if
>>new-lines are allowed in a string field/column.
>>
>>If MysqlMemberships.py were just storing and retrieving the
>>representation that it is passed, it wouldn't have to worry about
>>things like the fact that the 'cookie' argument disappeared from the
>>_BounceInfo instantiation call in Mailman 2.1.4
>
>
>So, if I understand this, ideally, Bouncer.py should not be changed to
>include additional calls to setBounceInfo(), right?  I'm still trying to
>understand whether this is a hack to allow MysqlMemberships.py to work as
>is (more or less) OR if setBounceInfo() calls were original "missing" in
>Bouncer.py.  I gather the answer is the former.



No. I think the additional calls to setBounceInfo() are required for
any MemberAdaptor that doesn't store the bounce info in a list
attribute.

I.e., they ARE required for MysqlMemberships.py, but they should be
minimized because for MysqlMemberships.py and perhaps other
MemberAdaptors they involve database access which may be relatively
expensive. I think the patch we arrived at does achieve the minimum.

The issue with the existing MysqlMemberships.py is that it should not
be burdened with knowing any details about the bounce info. As the
documentation (in MemberAdaptor.py) says, bounce info is opaque to the
MemberAdaptor.  It is set by setBounceInfo() and returned by
getBounceInfo() without modification.

Obviously, the MemberAdaptor has to know enough about the bounce info
it gets (e.g., maximum length) so it can store and return it without
modification, and getBounceInfo() has to know to return None when
there is no previous bounce info for the member, but that should be
it. The lengths to which MysqlMemberships.py goes to extract
attributes from the bounce info, save them separately, and construct a
_BounceInfo instance to return the data only get it in trouble when
aspects of the _BounceInfo class change from version to version.



>I have done some searching in the mailman-developers' archives to try to
>understand why it was decided to separate BounceInfo.  Here are some
>findings:
>
><http://www.mail-archive.com/mailman-developers@python.org/msg06790.html>:
>"I'm putting the "info" parameter from setBounceInfo directly into the
>database, which I think is an array itself, not a single value, and the
>above doesn't look like Python's just traversing an array, and dumping it
>into the database(the LHS names don't tie up with what I think are the
>keys for the subelements of "info"), so it looks like I'll have to take a
>"best guess" at how to implement this."
>>
>
><http://www.mail-archive.com/mailman-developers@python.org/msg06806.html>:
>"...the only changes of any import that I've made are that the Member data
>structures are stored in a way that fits MySQL and converted as they are
>loaded to the way that fits Mailman, which you'd expect..."
>
>I surmise that the rationale for storing the BounceInfo in separate
>columns is to provide easier access via SQL queries to the information
>that would otherwise be stored in this object.  I can imagine where this
>would be desirable (e.g. quickly querying which members recently received
>an increased bounce score).


I can see that would be desirable, but the price is difficulty of
maintenance because then the get and set BounceInfo methods have to
know things about the _BounceInfo class that may change, however see
below for a compromise.


>>Yes. I definitely overlooked the info.reset() two lines before the end
>>of registerBounce. Good Catch!
>>
>>However in the earlier part of registerBounce, I deliberately combined
>>your two calls to setBounceInfo() in the "if info is stale" clause and
>>its "else" clause into a single call following the if - else but still
>>within the containing else.
>>
>>I did this even though I think it is logically equivalent, because I
>>think that all else equal, fewer lines is better.
>
>
>Agreed.  Fewer lines is preferred.  I apologize for not recognizing what
>you had done there.


No problem.


>>As I indicate above, I think the better way to fix MysqlMemberships.py
>>is to remove its knowledge of the _BounceInfo class and just save and
>>retrieve the string representation that it is handed.


See below.

>
>
><http://www.mail-archive.com/mailman-developers@python.org/msg06808.html>:
>"My suggestion would be to pickle the BounceInfo object on the way into
>the database, and unpickle it on the way out."
>
>Or pickle and unpickle this information, right?  Making this change, of
>course, will require more effort to extract information stored in MySQL
>for other purposes (e.g. a custom web interface) but if it's the best way
>to handle this information then I would consider making these changes.  I
>will try like to discuss this with the original author of
>MysqlMemberships.py.


Looking at this more deeply, I think it is not as simple as I first
thought to simply save the string representation and return it. I
think pickle.dump() to a StringIO file and then saving its contents in
the set... method and then retrieving the string and returning
pickle.loads() in the get... method is the way to go. It has the
advantage that the string written to the database is a bit shorter too.

If you also want to be able to see and use some of the bounce info, you
could save certain attributes of the _BounceInfo instance in
additional columns of the database table.

I think discussing with the author is a good idea.

-- 
Mark Sapiro <msapiro at value.net>       The highway is for gamblers,
San Francisco Bay Area, California    better use your sense - B. Dylan