BIND 10 #2018: Use updater's finder in DDNS

[BIND 10 Development]

#2018: Use updater's finder in DDNS
-------------------------------------+-------------------------------------
                   Reporter:  jelte  |                 Owner:  jinmei
                       Type:         |                Status:  reviewing
  defect                             |             Milestone:
                   Priority:         |  Sprint-20120612
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  Unclassified                       |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 '''general'''

 - get_updater() is not directly tested (indirectly it is, though, in
   ddns/tests/seesion_tests).
 - exposing the updater seems to make the integrity of the Diff class
   fragile.  Even if there's a note that the main purpose is to allow
   the caller to use its finder, it can easily misuse the updater to
   make its own arbitrary updates.  If we expect it to be used for a
   longer term, I'd add find/find_all methods to the Diff class, which
   would be a trivial wrapper to these methods on `Diff.__updater`.
   Alternatively, I'd introduce an intermediate layer like this:
 {{{#!python
 class DiffFinder:
     def __init__(self, updater):
         self.__updater = updater
     def find(self, args):
         return self.__updater.find(args)
     def find_all(self, args):
         return self.__updater.find_all(args)
 }}}
   and the get_updater would be renamed (e.g.) get_finder with the
   following definition:
 {{{#!python
     def get_finder(self):
         return DiffFinder(self.__updater)
 }}}

 '''session.py'''

 - the doc string for `__get_update_zone` should mention `__diff` too
   (but see also below)
 - If we create a Diff in `__get_update_zone`, we start a DB
   transaction at this point.  But we might want to delay it as much as
   possible due to the overhead and increasing the risk of contention.
   This is especially so if we decide to change the place of ACL check
   before the prerequisite check (against RFC), or to add another layer
   of ACL check to prevent information leak for access controlled
   authoritative zones...But, that's probably premature anyway, and it
   should be possible to change this only if and when we see the need
   for it without breaking anything outside of the module.  So, in
   conclusion, I'm okay with either way.
 - If we create Diff in `__get_update_zone`, I believe we don't need
   `self.__datasrc_client` any more.

 '''session_tests.py'''

 It's not really that surprising to me that you needed to destroy the
 session in `SessionTestBase.tearDown`: in my understand, the
 unittest module keeps all test objects until the end of the whole
 set of tests.  So `SessionTestBase.tearDown.__diff` (and `__finder`)
 remains active once constructed, and if one of them hasn't committed
 the changes it would keep the DB lock.

 While this (the unit test trouble) would be an extreme case, it's
 probably a good idea to make sure the necessary cleanup is made within
 `UpdateSession` since it's up to the caller how long it keeps the
 session object (although it's generally expected to be ephemeral).
 It's probably better to defer it to a separate ticket though, because
 #2018 itself is an unplanned ticket and should better be closed
 sooner, and b10-ddns itself shouldn't have this problem (it shouldn't
 hold the session object too long).

 Some related notes: it's probably better to move tests for non method
 functions defined in session.py outside `SessionTestBase`.  These
 tests don't depend on the Session class and wouldn't need much (if
 any) of the initial setup done in `SessionTestBase`.  Since
 `SessionTestBase` calls `__get_update_zone()`, which starts a
 transaction, that would result in some surprising state in the system.

 And, related to this, why do we need to call `__get_update_zone()`?
 If we need to call it for the convenience of the test, I'd personally
 make it "protected" with a note that why it's defined so.

 '''other'''

 I happen to notice an unrelated error in the log message.  The second
 '%1' should be '%3' here:
 {{{
 % LIBDDNS_UPDATE_UNCAUGHT_EXCEPTION update client %1 for zone %2: uncaught
 exception while processing update section: %1
 }}}

-- 
Ticket URL: <http://bind10.isc.org/ticket/2018#comment:5>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development