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      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 Replying to [comment:5 jinmei]:
 > '''general'''
 >
 > - get_updater() is not directly tested (indirectly it is, though, in
 >   ddns/tests/seesion_tests).

 moot now, see your next point :)

 > - 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:

 That second approach seems nice, but I kind of prefer the first one, not
 really for now, but we may want to consider to have these find methods
 return the data not directly from the updater's find() and find_all(), but
 including the changes made in the diff so far. This would be a nontrivial
 task, but could make the calling side (related to #2016) much cleaner. A
 disadvantage would be that the find calls in diff and the add/delete calls
 would need to be way smarter than the simple things they are now. Not
 quite as smart as the real finds (since this should not need additional
 processing, and only work on raw data), but still, something to keep in
 mind.

 For now I'm just wrapping the find() and find_all() calls.

 >
 > '''session.py'''
 >
 > - the doc string for `__get_update_zone` should mention `__diff` too
 >   (but see also below)

 Not anymore :)

 > - 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.

 right, so in that case we'd need to split up !__get_update_zone into the
 zone part and the diff creation part.

 Current approach does have one also premature optimization, if the
 transaction cannot be started you'll know earlier ;)

 But yeah, it's not that big of a change, and is cleaner from an external
 point of view. Added a protected call _create_diff.

 AFAIK, we had already decided to do ACL checks before prereqs, so since
 that is now possible, i've move the order of the calls in handle() a bit
 (and removed the now obsolete comment about conceptual code there).

 > - If we create Diff in `__get_update_zone`, I believe we don't need
 >   `self.__datasrc_client` any more.
 >

 We do again, now :)

 > '''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.
 >

 Ah, I was not aware that objects were kept. In that case it does indeed
 make sense.

 > 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).
 >

 Right, created #2022

 > 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.
 >

 moved.

 > 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.
 >

 yup, need to call it (and not create_diff() as well) so that the private
 methods that rely on the members being set here can be tested, without
 calling handle() itself.

 added to docstrings

 > '''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
 > }}}

 fixed.

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