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

 Replying to [comment:7 jelte]:

 > > - exposing the updater seems to make the integrity of the Diff class
 > >   fragile.  [...]
 >
 > 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.

 Okay, and this reminds me of another thing.  For the "smarter"
 behavior, I guess another approach is

 - ddns (session) can make changes in the update section one by one,
   without doing any buffering
 - if this processing needs to examine the latest snapshot of the
   updated zone (before commit), ddns (session) can do it with the
   updater's finder.
 - extend the datasrc API in order to allow installing diffs only
   (separately from the corresponding changes to the zone)
 - ddns (session) normalize the entire changes into the IXFR-friendly
   form of diffs, and install it using the extended API, just before
   committing.
 - ddns (session) finally commits the changes (with the diff update)

 This is similar to BIND 9's behavior.  Of course, that's beyond the
 scope of this ticket in any case.

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

 In conclusion I don't oppose to this change, but as I suggested in
 bind10-dev, I think we should do it with documented clarification.
 We should also add a test that ACL is really checked before
 prerequisite (the fact that you can simply change the ordering seems
 to suggest we currently don't have that type of test).

 But, again, these will be beyond the scope of this ticket.  My
 personal suggestion is to keep the current (RFC-described) ordering
 just in this task, and defer this clarification with doc to a separate
 task.  But, assuming that no one will object to the eventual behavior
 of BIND 10, if you really want to make this change now, I don't oppose
 to that.

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

 Okay.  In the actual diff I assumed the changes on this was just
 straightforward extraction, without any semantics change, and didn't
 pay a full attention to the details.

 Some other points to the changes:

 '''session.py'''

 - maybe we can define a shortcut for
   `ZoneFinder.NO_WILDCARD | ZoneFinder.FIND_GLUE_OK` with the rationale
   and use it throughout the module?  Alternatively, we might even make
  it the default of Diff.find() and find_all().
 {{{#!python
         result, _, _ = self.__diff.find(rrset.get_name(),
 rrset.get_type(),
                                         ZoneFinder.NO_WILDCARD |
                                         ZoneFinder.FIND_GLUE_OK)
 }}}

 '''diff.py'''

 - in find/find_all() description, this is not 100% correct:
 {{{#!python
         Diff's ZoneUpdater, i.e. the find() on the zone as it was on the
         moment this Diff object got created.
 }}}
   if there is any call to apply(), these find variants can find the
   applied changes.
 - we should probably do the `__check_commited()` check for these
   methods, too.

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