Opened 9 years ago

Closed 9 years ago

#1062 closed task (complete)

add a base for DatabaseZoneHandle::find()

Reported by: jinmei Owned by: jelte
Priority: medium Milestone: Sprint-20110816
Component: data source Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 5.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

This is the first step to support normal query lookup in database
backends. This task includes:

  • define DatabaseConnection::searchForRecords() method. It takes a zone ID and a domain name, and invokes a search for matching rows of that condition in the underlying database. This method does not necessarily complete all transactions with the database. The actual matching rows will be retrieved via the getNextRecord() method.
  • Define DatabaseConnection::getNextRecord() method. It passes a reference to vector<string>, and let the underlying implementation fill in it with the columns of the next matching row as a result of searchForRecords().
  • implement SQLite3Connection versions of these methods.
  • implement a simplest form of DatabaseZoneHandle::find(). It only considers exact domain name matches (RR type, CNAME, NXRRSET) and NXDOMAIN (without wildcard consideration) case should be covered.

For prototype implementation, see commit
f57f8eef2d3d376ecedabc7fb7d3372784e2d429 of trac817a.
(but the actual implementation doesn't have to stick to the way that
the prototype does)

This task depends on #1060 and #1061.

Subtickets

Change History (17)

comment:1 Changed 9 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 5

comment:2 Changed 9 years ago by stephen

  • Milestone changed from Next-Sprint-Proposed to Sprint-20110802

comment:3 Changed 9 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

Not sure how far i can get, but at least i can try to make a start :)

comment:4 Changed 9 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

Ready for review.

Some notes;

I added a method typeCovered() to the (further incomplete) type dns/rdata/generic/rrsig_46.

The interface for getNextRecord is slightly different; the original was const, this one is not (it modifies internal state, as does searchForRecords btw), and it does not return void but bool (true if there was a next row, false if there was not).

As for Finder::find(), for the direct (full) match and CNAME, it also support RRSIGS (but doesn't do NSEC for NXDOMAIN/NXRRSET yet). I added a class RRsigStore in local namespace for that (to keep track of the rrsigs it encounters while walking through the data, see the inline comments on why). If we want to move this to util and add some tests for it, I'm not opposed, though if we only need it here I think this way is quite ok.

comment:5 Changed 9 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:6 Changed 9 years ago by jinmei

First off, I made a few minor suggested changes to the branch. I
believe they are trivial and non controversial, but please check.

General Discussions

  • I agree with changing getNextRecord() to a non const member function for the reason you explained. But this would also mean find() shouldn't be a cost member function of DatabaseClient::Finder (and therefore, of ZoneFinder?). It's not really good if we give up the constness at that abstract level (because for other types of data source clients such as the in memory one we'd be able to keep them const), but we shouldn't pretend that it's immutable when it's simply hidden inside a pointer (which gives the caller a false sense of safety).
  • Don't we need to use loggers in database and sqlite3_connection?
  • in database.cc, I guess the exception/error handling needs to be revised. If I read the current implementation correctly, if some data in the database is broken and libdns++ throws an exception, the Finder can keep an ongoing transaction (e.g., in the case of sqlite3, it can perform sqlite3_step(), encounter an exception, and exit without doing sqlite3_reset() (btw I think such a case should be explicitly tested). Also, depending on the underlying database and the connection implementation, searchForRecords() may also result in an exception with some incomplete state (even though it's not the case for sqlite3). So, I guess we need something like this:
    • introduce a cleanup method to DatabaseConnection?
    • enclose the entire block that performs any connection operations with try-catch
      DatabaseClient::Finder::find(...) {
          try {
              connection_->searchForRecords(zone_id_, name.toText());
      
              while (connection_->getNextRecord(columns)) {
              ...
              }
          } catch (const datasrc::DatabaseError& ex) {
              // this corresponds to a database system error such as connection
              // reset.  not sure how to react to it at this moment, but we should
              // probably separate this case with other isc::Exception's.
          } catch (const isc::Exception& ex) {
              // call the cleanup
      	isc_throw(DataSourceError, ...);
          } catch (const std::exception& ex) {
              // call the cleanup
      	throw;
          }
      
      (we may want to perform the cleanup in an RAII-like manner instead of calling the cleanup method explicitly with the catch block)

database.h/cc

  • maybe we want to use a plain old array instead of vector for columns, if we are sure it has a fixed size? Even though we'd rely on hot spot caching for performance, find() is performance sensitive and would generally be nicer if it could be efficient. Using a plain old type might also be beneficial because it's used as an interface with the database backend, which would often rely on C-based library (like libsqlite3, libmysql, etc). On the other hand, it may not matter much especially because the same columns object will be used multiple times in find(), and once its used the allocated space will be reused. So I don't have a strong opinion about this, but if we keep using a vector, maybe it's better to reserve the space on initialization:
        std::vector<std::string> columns(4);
    
    (but see also below on using the magic number).
  • (general) I think it's better to define an enum or something to represent the semantics of the vector used in getNextRecord(), rather than hardcoding the magic numbers:
            if (columns.size() != 4) {
    ...
                const isc::dns::RRType cur_type(columns[0]);
                const isc::dns::RRTTL cur_ttl(columns[1]);
    
  • I suggest removing the indentation before the beginning of lines in the unnamed namespace:
    namespace {
        // Adds the given Rdata to the given RRset
    ...
    
    to keep the lines shorter, especially if you don't like to rely on "using" directives.
  • getNextRecord() documentation in .h: since the semantics of "columns" vector is a public contract to add support for other databases (possibly by an external contributor), I think we should be a bit more detailed, e.g.,
      The \c columns must have exactly four entries, and these entries
      (in the appearing order) must meet the following conditions:
      - rrtype: The type of the RR.  The text must be accepted by the
        dns::RRType() constructor.
      - ttl: The TTL of the RR.  The text must be accepted by the
        dns::RRTTL() constructor.
      ...
    
    (btw, I'd call the first entry "rrtype" instead of "rdtype". rdtype is a BIND 9 term, which has the notion of "rdataset", but protocol-wise this is not really the right term (IMO). For the same reason, I personally think we should rename it in the sqlite3 schema accordingly before it's too late, but that's definitely beyond the scope of this ticket).
  • addOrCreate(): it's not clear how it's possible that type != rrset->getType():
                // make sure the type is correct
                if (type != rrset->getType()) {
    
    Is it intended to exclude the case of CNAME + other type? If so, I'd personally catch that case explicitly in find() and would rather assert() the type equality here. But it's probably a matter of preference, so I don't insist. If you want to handle it here, however, please clarify the expected case(s) in a comment. (BTW, lcov indicates this case isn't covered by tests)
  • addOrCreate(): according to lcov, this case isn't covered by tests:
                if (ttl < rrset->getTTL()) {
                    rrset->setTTL(ttl);
                }
    
  • addOrCreate(): when could rdata_str be validly an empty string?
            if (rdata_str != "") {
    
    For an RR that may have an empty RDATA? In any case, I don't think we can simply ignore the case of an empty string because it may not be allowed depending on the RR type (actually in many cases it shouldn't be allowed). It seems we should simply pass rdata_str to createRdata() unconditionally and let the function check whether an empty string is acceptable.
  • addOrCreate(): you don't need toText() for name or type:
                    isc_throw(DataSourceError,
                              "bad rdata in database for " << name.toText() << " "
                              << type.toText() << " " << ivrt.what());
    
  • RRsigStore class: I'd use ConstRdataPtr? instead of RdataPtr? throughout this class (and its caller). Unfortunately, however, it probably wouldn't work because RRset::addRRsig() expects a non const parameter. There doesn't seem to be a reason why it needs a mutable RdataPtr? and I suspect we should be able to change that, but that would be beyond the scope of this ticket.
  • addSig(): it seems the insertion logic could be much more simplified:
                sigs[type_covered].push_back(sig_rdata);
    #ifdef no_need_for_this
                if (!haveSigsFor(type_covered)) {
                    sigs[type_covered] = std::vector<isc::dns::rdata::RdataPtr>();
                }
                sigs.find(type_covered)->second.push_back(sig_rdata);
    #endif
    
    Note also that if we do this we don't need a separate method of haveSigsFor() any more. (I'd also note that haveSigsFor() would better be a private function if we want to keep it, especially if and when we want to use this class publicly)
  • addSig(): if and when we want to use it publicly, I'd change static_cast to dynamic_cast or have the caller pass type_covered information and avoid casting.
  • appendSignatures(): maybe a matter of taste, but I'd rather simply get the iterator via find() unconditionally:
                std::map<isc::dns::RRType, std::vector<isc::dns::rdata::RdataPtr> >::const_iterator found =
                    sigs.find(rrset->getType());
                if (found != sigs.end()) {
                    BOOST_FOREACH(isc::dns::rdata::RdataPtr sig, found->second) {
                        rrset->addRRsig(sig);
                    }
                }
    
    (Although the first line looks too weird and we'd need some shortcut typedef). When find() fails, the additional cost is just the call overhead of end(), which should be cheap. If it succeeds we need the matching iterator anyway so it wouldn't be much costly (actually, in this case the original code performs the search twice and might be a bit more expensive). And, this way, we could eliminate the additional haveSigsFor() method.
  • find(): what if columns[2] is not an empty string for columns[0] other than RRSIG? Likewise, what if columns[2] for RRSIG is not equal to the actual "type covered" in the RDATA? Actually, I personally this is a flaw in the schema, not an issue at this level, and I hope we'll fix the schema so that we won't have to bother this type of question. But that'd be beyond the scope of this ticket. For now I suggest leaving some comments about that and doing nothing.
  • find(): I'd note why this is commented out or simply remove it for now:
                //cur_sigtype(columns[2]);
    
  • find(): this comment is incomplete, or the second "," should be "."?
                    // There should be no other data, so cur_rrset should be empty,
    
    and I suspect "cur_rrset" should be "result_rrset".
  • find(): I'd avoid using an additional variable in terms of preferring shorter length of methods:
                    isc::dns::rdata::RdataPtr cur_rrsig(
                        isc::dns::rdata::createRdata(cur_type, getClass(),
                                                     columns[3]));
                    sig_store.addSig(cur_rrsig);
    
    but that's probably a matter of taste.
  • find(): there's a more critical point in this code: createRdata() can throw an exception, and it's not caught. But I think this should be addressed in a higher level context of how we generally handle error cases in find(). (See the higher level discussions above)

sqlite3_connection.cc

  • In searchForRecords(), sqlite3_bind_text() can fail (and sqlite3_bind_int() may also fail in theory?), and we should catch the case even though it would be a rare event.
  • searchForRecords(): it assumes int for zone_id. Since this is derived from the base class, we are stating all DB backend uses an integer to represent a specific zone. maybe it's okay, but we may want to make it a bit more abstract (or we may revisit this point as we support other types of databases). at this moment this is just a comment, not (necessarily) requesting to change the interface/implementation.
  • convertToPlainChar(): (I'm asking partly because I forgot:-) in which case can ucp be NULL? Also, the main purpose of this helper function is to work around the C++ strictness on the difference between 'unsigned char*' and 'char *' (sqlite3 uses the former throughout its API, while std::string accepts the latter). And, from a point of type safe purist this conversion isn't correct (because in theory it's not guaranteed that these two pointer types are convertible). I'd note these points as comments.
  • getNextRecord(): although it should be rare and might be purely in theory, this method can internally throw an exception (bad_alloc). we should probably catch that within this method and convert it to DataSourceError? so that the application can handle that situation more gracefully (e.g., exit after closing the DB connection).
  • getNextRecord(): this implementation does not provide the strong exception guarantee. Considering the typical usage, I think it's acceptable, but I'd note the fact in the DatabaseConnection? documentation.

sqlite3_connection_unittest.cc

  • why do we need to include <memory>? (strangely, we would have needed it before this branch because it previously used auto_ptr. but this branch actually removed it)
  • not a big deal especially in tests, but if I understand it correctly "conn" can be scoped_ptr instead of shared_ptr:
    +    boost::shared_ptr<SQLite3Connection> conn;
    
    As far as I can see the only reason why this is defined as a shared_ptr is because it's passed to countRecords(), but in this use case we can pass *conn (and countRecords() accepts it as SQLite3Connection&).
  • I'd examine the contents of the returned vector (not just the number of matching records)
  • if possible, I'd like to test the case where getNextRecord() results in DataSourceError?().
  • if possible and making sense, I'd test searchForRecords() directly.
  • doFindTest(): toText() can be omitted here:
        ASSERT_EQ(expected_result, result.code) << name.toText() << " " << type.toText();
    
    (besides, the current line is a bit too long)
  • DatabaseClientTest::find: I'd check some additional cases:
    • some possible cases were already mentioned above
    • CNAME then A (in addition to A then CNAME)
    • multiple CNAMEs
    • mixed order RRs without RRSIGs, such as "A, AAAA, then A", and try to find the A RRset
    • if not very hard, I'd check RDATA(s), not only the number of RDATAs and counts. maybe we could use rrset[s]Check defined in lib/testutils/dnsmessage_test.h.
    • have createRdata() for RRSIG throw an exception (as pointed out for the main code the exception seems to be uncaught right now)
    • explicitly find a CNAME (which shouldn't result in
    • ZoneFinder::CNAME)
    • "sigtype for non RRSIG", sigtype mismatches RRSIG type covered (see the corresponding comment for find().) This would be a placeholder test and would actually simply ignore the oddity for now.

comment:7 Changed 9 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:8 follow-up: Changed 9 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

First off, I made a few minor suggested changes to the branch. I
believe they are trivial and non controversial, but please check.

look good, thanks

General Discussions

  • I agree with changing getNextRecord() to a non const member function for the reason you explained. But this would also mean find() shouldn't be a cost member function of DatabaseClient::Finder (and therefore, of ZoneFinder?). It's not really good if we give up the constness at that abstract level (because for other types of data source clients such as the in memory one we'd be able to keep them const), but we shouldn't pretend that it's immutable when it's simply hidden inside a pointer (which gives the caller a false sense of safety).

ok, unconsted it

  • Don't we need to use loggers in database and sqlite3_connection?

hmm, yes, completely forgot about those :/

I added some in find(), it'll log either a successful return, or one of the
errors as caught by the big try/catch introduced in this revision.

I'm at this point not sure whether we need more lowlevel logging (in theory the
problems in sqlite3 should bubble up as exceptions which are then logged. We may
find that this is not the case and that we do need to go deeper).

  • in database.cc, I guess the exception/error handling needs to be revised. If I read the current implementation correctly, if some data in the database is broken and libdns++ throws an exception, the Finder can keep an ongoing transaction (e.g., in the case of sqlite3, it can perform sqlite3_step(), encounter an exception, and exit without doing sqlite3_reset() (btw I think such a case should be explicitly tested). Also, depending on the underlying database and the connection implementation, searchForRecords() may also result in an exception with some incomplete state (even though it's not the case for sqlite3). So, I guess we need something like this:
    • introduce a cleanup method to DatabaseConnection?
    • enclose the entire block that performs any connection operations with try-catch
      DatabaseClient::Finder::find(...) {
          try {
              connection_->searchForRecords(zone_id_, name.toText());
      
              while (connection_->getNextRecord(columns)) {
              ...
              }
          } catch (const datasrc::DatabaseError& ex) {
              // this corresponds to a database system error such as connection
              // reset.  not sure how to react to it at this moment, but we should
              // probably separate this case with other isc::Exception's.
          } catch (const isc::Exception& ex) {
              // call the cleanup
      	isc_throw(DataSourceError, ...);
          } catch (const std::exception& ex) {
              // call the cleanup
      	throw;
          }
      
      (we may want to perform the cleanup in an RAII-like manner instead of calling the cleanup method explicitly with the catch block)

I've implemented this approach for now, doing it with RAII is probably better,
but may mean some other interface changes as well, and probably an entire
different approach to the way we use the sqlite stmt variables right now
(although i guess some of the potential problems i see would also be present
now)

I did add some tests that make sure resetSearch() (the cleanup method) is
called, both on completion of a call to find() and on any exception that is
raised.

database.h/cc

  • maybe we want to use a plain old array instead of vector for columns, if we are sure it has a fixed size? Even though we'd rely on hot spot caching for performance, find() is performance sensitive and would generally be nicer if it could be efficient. Using a plain old type might also be beneficial because it's used as an interface with the database backend, which would often rely on C-based library (like libsqlite3, libmysql, etc). On the other hand, it may not matter much especially because the same columns object will be used multiple times in find(), and once its used the allocated space will be reused. So I don't have a strong opinion about this, but if we keep using a vector, maybe it's better to reserve the space on initialization:
        std::vector<std::string> columns(4);
    
    (but see also below on using the magic number).

Hmm, yes, a direct array is probably better, though also slightly more dangerous.

Therefore I also added a second argument, the vector size. Doesn't provide full
protection against passing in a smaller array, but at least it's something; the
dabaseimplementation should check it.

  • (general) I think it's better to define an enum or something to represent the semantics of the vector used in getNextRecord(), rather than hardcoding the magic numbers:
            if (columns.size() != 4) {
    ...
                const isc::dns::RRType cur_type(columns[0]);
                const isc::dns::RRTTL cur_ttl(columns[1]);
    

ack, done

  • I suggest removing the indentation before the beginning of lines in the unnamed namespace:
    namespace {
        // Adds the given Rdata to the given RRset
    ...
    
    to keep the lines shorter, especially if you don't like to rely on "using" directives.

it's true, i don't :) (then again, I personally don't have much problems with
wide lines, which you have probably noticed)
anyhew removed indentation (and in one test, i found an anonymous namespace
inside an anonymous namespace (we need to go deeper!), removed it)

  • getNextRecord() documentation in .h: since the semantics of "columns" vector is a public contract to add support for other databases (possibly by an external contributor), I think we should be a bit more detailed, e.g.,
      The \c columns must have exactly four entries, and these entries
      (in the appearing order) must meet the following conditions:
      - rrtype: The type of the RR.  The text must be accepted by the
        dns::RRType() constructor.
      - ttl: The TTL of the RR.  The text must be accepted by the
        dns::RRTTL() constructor.
      ...
    
    (btw, I'd call the first entry "rrtype" instead of "rdtype". rdtype is a BIND 9 term, which has the notion of "rdataset", but protocol-wise this is not really the right term (IMO). For the same reason, I personally think we should rename it in the sqlite3 schema accordingly before it's too late, but that's definitely beyond the scope of this ticket).

ack, added documentation.

  • addOrCreate(): it's not clear how it's possible that type != rrset->getType():
                // make sure the type is correct
                if (type != rrset->getType()) {
    
    Is it intended to exclude the case of CNAME + other type? If so,

No, CNAME+other type is already explicitely caught. This is to catch our own
code in find() doing bad things :)

EDIT; due to the missing test case you caught for CNAMEs, the 'other' cname
error did indeed end up being caught by this, fixed the caller.

I'd personally catch that case explicitly in find() and would rather
assert() the type equality here. But it's probably a matter of
preference, so I don't insist. If you want to handle it here,
however, please clarify the expected case(s) in a comment.
(BTW, lcov indicates this case isn't covered by tests)

yes well with the current code it shouldn't be possible, and since addOrCreate
is in anonymous namespace we can't directly call it (or did we have a clean way
to do that?), so it's not covered by a test...

made it an assert.

  • addOrCreate(): according to lcov, this case isn't covered by tests:
                if (ttl < rrset->getTTL()) {
                    rrset->setTTL(ttl);
                }
    

ohright. added.

  • addOrCreate(): when could rdata_str be validly an empty string?
            if (rdata_str != "") {
    
    For an RR that may have an empty RDATA? In any case, I don't think we can simply ignore the case of an empty string because it may not be allowed depending on the RR type (actually in many cases it shouldn't be allowed). It seems we should simply pass rdata_str to createRdata() unconditionally and let the function check whether an empty string is acceptable.

ok, removed

  • addOrCreate(): you don't need toText() for name or type:
                    isc_throw(DataSourceError,
                              "bad rdata in database for " << name.toText() << " "
                              << type.toText() << " " << ivrt.what());
    

ack, removed

  • RRsigStore class: I'd use ConstRdataPtr? instead of RdataPtr? throughout this class (and its caller). Unfortunately, however, it probably wouldn't work because RRset::addRRsig() expects a non const parameter. There doesn't seem to be a reason why it needs a mutable RdataPtr? and I suspect we should be able to change that, but that would be beyond the scope of this ticket.

right. Should we make that a ticket, we should add a note to update this code :)

  • addSig(): it seems the insertion logic could be much more simplified:
                sigs[type_covered].push_back(sig_rdata);
    #ifdef no_need_for_this
                if (!haveSigsFor(type_covered)) {
                    sigs[type_covered] = std::vector<isc::dns::rdata::RdataPtr>();
                }
                sigs.find(type_covered)->second.push_back(sig_rdata);
    #endif
    
    Note also that if we do this we don't need a separate method of haveSigsFor() any more. (I'd also note that haveSigsFor() would better be a private function if we want to keep it, especially if and when we want to use this class publicly)

ack. less code good. (originally i didn't do appendSignatures

  • addSig(): if and when we want to use it publicly, I'd change static_cast to dynamic_cast or have the caller pass type_covered information and avoid casting.

in this case the caller would have to cast anyway (though it's closer to the
creation and probably safer. Added a note that we should do that if we move it
out)

  • appendSignatures(): maybe a matter of taste, but I'd rather simply get the iterator via find() unconditionally:
                std::map<isc::dns::RRType, std::vector<isc::dns::rdata::RdataPtr> >::const_iterator found =
                    sigs.find(rrset->getType());
                if (found != sigs.end()) {
                    BOOST_FOREACH(isc::dns::rdata::RdataPtr sig, found->second) {
                        rrset->addRRsig(sig);
                    }
                }
    
    (Although the first line looks too weird and we'd need some shortcut typedef). When find() fails, the additional cost is just the call overhead of end(), which should be cheap. If it succeeds we need the matching iterator anyway so it wouldn't be much costly (actually, in this case the original code performs the search twice and might be a bit more expensive). And, this way, we could eliminate the additional haveSigsFor() method.

ok, taken over your code. For one single local use I don't mind having no
typedef btw.

  • find(): what if columns[2] is not an empty string for columns[0] other than RRSIG? Likewise, what if columns[2] for RRSIG is not equal to the actual "type covered" in the RDATA? Actually, I personally this is a flaw in the schema, not an issue at this level, and I hope we'll fix the schema so that we won't have to bother this type of question. But that'd be beyond the scope of this ticket. For now I suggest leaving some comments about that and doing nothing.

yes. I was wondering what to do about sigtype anyway, since we technically don't
need it (though it could be a good optimization), and don't use it here (yet).

which is why i left this in:

            //cur_sigtype(columns[2]);

I added some comments.

  • find(): this comment is incomplete, or the second "," should be "."?
                    // There should be no other data, so cur_rrset should be empty,
    
    and I suspect "cur_rrset" should be "result_rrset".

Ack, should be "." and result_rrset. Changed.

  • find(): I'd avoid using an additional variable in terms of preferring shorter length of methods:
                    isc::dns::rdata::RdataPtr cur_rrsig(
                        isc::dns::rdata::createRdata(cur_type, getClass(),
                                                     columns[3]));
                    sig_store.addSig(cur_rrsig);
    
    but that's probably a matter of taste.

no you are right, it can be done wihtout the var, changed.

  • find(): there's a more critical point in this code: createRdata() can throw an exception, and it's not caught. But I think this should be addressed in a higher level context of how we generally handle error cases in find(). (See the higher level discussions above)

Actually, while the higher-level catch should at clean this up correctly,
i also added a specific exception here (next to InvalidRRType and InvalidRRTTL),
where we still have access to the source data, for a better exception message.

sqlite3_connection.cc

  • In searchForRecords(), sqlite3_bind_text() can fail (and sqlite3_bind_int() may also fail in theory?), and we should catch the case even though it would be a rare event.

ack, done

  • searchForRecords(): it assumes int for zone_id. Since this is derived from the base class, we are stating all DB backend uses an integer to represent a specific zone. maybe it's okay, but we may want to make it a bit more abstract (or we may revisit this point as we support other types of databases). at this moment this is just a comment, not (necessarily) requesting to change the interface/implementation.

Yes I wondered about this as well. I thought about changing this to some
abstract Handle class, but did not want to change that part of the API in this
ticket. We could either make this just a simple container (for, in the case of
sqlite3 and probably most sqls, an integer), but possibly this could even do the
RAII for the current transaction. (though probably not, and we should keep that
to searchForRecords and getNextRecord)

  • convertToPlainChar(): (I'm asking partly because I forgot:-) in which case can ucp be NULL? Also, the main purpose of this helper function is to work around the C++ strictness on the difference between 'unsigned char*' and 'char *' (sqlite3 uses the former throughout its API, while std::string accepts the latter). And, from a point of type safe purist this conversion isn't correct (because in theory it's not guaranteed that these two pointer types are convertible). I'd note these points as comments.

IIRC the return value from sqlite_column_text() can be NULL if the database
field itself was NULL or if it has run out of memory. But the documentation
seems to be a bit unclear as to this specific value (it does mention NULL for
BLOB, but not explicitely for string).

I've added some comments. BTW, I can also get around the conversion by using
string::assign(), which makes the compiler do its magic on its own. But explicit
casts are probably better :)

  • getNextRecord(): although it should be rare and might be purely in theory, this method can internally throw an exception (bad_alloc). we should probably catch that within this method and convert it to DataSourceError? so that the application can handle that situation more gracefully (e.g., exit after closing the DB connection).

done

  • getNextRecord(): this implementation does not provide the strong exception guarantee. Considering the typical usage, I think it's acceptable, but I'd note the fact in the DatabaseConnection? documentation.

added a bit

sqlite3_connection_unittest.cc

  • why do we need to include <memory>? (strangely, we would have needed it before this branch because it previously used auto_ptr. but this branch actually removed it)

oops, leftover from an earlier bugfix problem (related to the inclusion of
<memory>). Removed.

  • not a big deal especially in tests, but if I understand it correctly "conn" can be scoped_ptr instead of shared_ptr:
    +    boost::shared_ptr<SQLite3Connection> conn;
    
    As far as I can see the only reason why this is defined as a shared_ptr is because it's passed to countRecords(), but in this use case we can pass *conn (and countRecords() accepts it as SQLite3Connection&).

ack, changed. Actually, for countRecords, see below.

  • I'd examine the contents of the returned vector (not just the number of matching records)

Done. The change made getRecordCount unnecessary, so i removed it (there is,
however, now a new helper that checks the columns).

  • if possible, I'd like to test the case where getNextRecord() results in DataSourceError?().

as a side-effect of adding a column-count, i had also added one check for that.
But I assume you mean for the DataSourceError? that was already in there :)

I do not know of any dependable way to make sqlite3_step return an error, though...

  • if possible and making sense, I'd test searchForRecords() directly.

I don't know what to test; success is implicitely tested by the
search+getnextrecord tests. We could perhaps try to force the now-checked bind()
calls to error, but I don't know how :)

  • doFindTest(): toText() can be omitted here:
        ASSERT_EQ(expected_result, result.code) << name.toText() << " " << type.toText();
    
    (besides, the current line is a bit too long)

ack, changed.

  • DatabaseClientTest::find: I'd check some additional cases:
    • some possible cases were already mentioned above
    • CNAME then A (in addition to A then CNAME)

added (badcname2.example.org)

  • multiple CNAMEs

added (badcname3.example.org)

  • mixed order RRs without RRSIGs, such as "A, AAAA, then A", and try to find the A RRset

added ('www2.example.org')

  • if not very hard, I'd check RDATA(s), not only the number of RDATAs and counts. maybe we could use rrset[s]Check defined in lib/testutils/dnsmessage_test.h.
  • have createRdata() for RRSIG throw an exception (as pointed out for the main code the exception seems to be uncaught right now)

added ('badsig.example.org')

  • explicitly find a CNAME (which shouldn't result in ZoneFinder::CNAME)

added, (second test for cname.example.org)

  • "sigtype for non RRSIG", sigtype mismatches RRSIG type covered (see the corresponding comment for find().) This would be a placeholder test and would actually simply ignore the oddity for now.

btw, i've removed emptyvector; it would only test the fake test database...

comment:9 in reply to: ↑ 8 Changed 9 years ago by jinmei

Replying to jelte:

A few preliminary notes:

  • (as always) I made some minor editorial changes
  • please note that in #1061 it's likely we'll change "connection" to "accessor". some of internal variable names, test cases, etc. may have to be adjusted as a result.
  • I know comments below are not short, but most of them are quite minor or trivial to address. The (almost) only real point to discuss is the use of try-catch in find().

General Discussions

  • I agree with changing getNextRecord() to a non const member function for the reason you explained. But this would also mean find() shouldn't be a cost member function of DatabaseClient::Finder (and therefore, of ZoneFinder?). [...]

ok, unconsted it

Hmm, it doesn't seem to be changed...

  • Don't we need to use loggers in database and sqlite3_connection?

hmm, yes, completely forgot about those :/

I added some in find(), it'll log either a successful return, or one of the
errors as caught by the big try/catch introduced in this revision.

Looks good, but I'd also include the class name and some identifier of
the underlying database (maybe we need a method in
DatabaseConnection??) for all these log messages, e.g.:

    logger.debug(DBG_TRACE_DETAILED, DATASRC_DATABASE_FIND_RECORDS).arg(name).arg(getClass()).arg(type).arg(connection_->getDBName());

(I assume a new method "getDBName()", which would result in something
like "sqlite3 (+ perhaps filename)")

  • in database.cc, I guess the exception/error handling needs to be revised. [...]

I've implemented this approach for now, doing it with RAII is probably better,
but may mean some other interface changes as well, and probably an entire
different approach to the way we use the sqlite stmt variables right now
(although i guess some of the potential problems i see would also be present
now)

The revised code seems to do the right thing, but I'm personally
concerned about the heavily nested try-catch logic:

  • the outer most one in find()
  • the one in the while loop
  • the one in addOrCreate(), which is called in the second try block

In general, if we catch specific exceptions at each logical level like
this, the resulting code will be quite similar to the plain old
C-style error handling, where the normal code logic and error cases
are quite mixed and the code is not very readable.

In fact, due to the many catch-es, the find() method is already pretty
big, and I suspect it will be even more complicated as we implement
more search logic (longest match, wildcard, delegation, etc), although
at that point we'll need to refactor the method with some smaller
helper private methods anyway.

I guess you caught exceptions in every level so that you can provide
detailed information about what's wrong in the database when some
bogus data is returned from the backend as you commented:

Actually, while the higher-level catch should at clean this up correctly,
i also added a specific exception here (next to InvalidRRType and InvalidRRTTL),
where we still have access to the source data, for a better exception message.

I see the point of the motivation, and I do not necessarily say it's a
bad idea, but personally the current balance of complexity and the
benefit of providing detailed error info is not good.

Now, if my argument is convincing and you agree with simplifying the
try-catch block, that's of course fine to me.

If not, which is also understandable,

  • if you can come up with a middle ground solution, that would be nice.
  • otherwise, let's leave it for now (maybe we can revisit it as we add more logic to find()), but please leave some comments about the pros and cons of this method and the rationale of the current decision.

I did add some tests that make sure resetSearch() (the cleanup method) is
called, both on completion of a call to find() and on any exception that is
raised.

Regarding tests for resetSearch(): why are some of the tests commented
out?

    // Trigger the hardcoded exceptions and see if find() has cleaned up
    /*
    EXPECT_THROW(finder->find(isc::dns::Name("dsexception.in.search."),
                                              isc::dns::RRType::A(),
                                              NULL, ZoneFinder::FIND_DEFAULT),
                 DataSourceError);
...

database.h/cc

  • maybe we want to use a plain old array instead of vector for columns, if we are sure it has a fixed size? [...]

Hmm, yes, a direct array is probably better, though also slightly
more dangerous. Therefore I also added a second argument, the
vector size. Doesn't provide full protection against passing in a
smaller array, but at least it's something; the dabaseimplementation
should check it.

If you like the efficiency of the array but want to make it safe, an
alternative would be to use boost::array (at the cost of the
additional dependency) and use at() instead of operator[] inside the
callee. I don't have a strong opinion, and leave it to you.

  • (general) I think it's better to define an enum or something to represent the semantics of the vector used in getNextRecord(), rather than hardcoding the magic numbers:
            if (columns.size() != 4) {
    ...
                const isc::dns::RRType cur_type(columns[0]);
                const isc::dns::RRTTL cur_ttl(columns[1]);
    

ack, done

Looks good, but I guess RecordColumnCount? should also be all upper
cased (with _, i.e. RECORD_COLUMN_COUNT) as it's a constant according
to the coding guideline.

Also, this comment seems incomplete:

        TTL_COLUMN = 1,     ///< The TTL of the record (a
  • addOrCreate(): it's not clear how it's possible that type != rrset->getType():

[...]

(BTW, lcov indicates this case isn't covered by tests)

yes well with the current code it shouldn't be possible, and since
addOrCreate is in anonymous namespace we can't directly call it (or
did we have a clean way to do that?), so it's not covered by a
test...

made it an assert.

With the revised (with assert), I think code missing the test is okay.
One thing I didn't notice in the previous code: the assert() should
probably be better before the TTL check:

        // This is a check to make sure find() is not messing things up
        assert(type == rrset->getType());

        if (ttl < rrset->getTTL()) {
            rrset->setTTL(ttl);
        }

BTW maybe we want to log the event when we perform setTTL()?

sqlite3_connection.cc

  • In searchForRecords(), sqlite3_bind_text() can fail (and sqlite3_bind_int() may also fail in theory?), and we should catch the case even though it would be a rare event.

ack, done

Looks good, but in #1068 I noticed we might rather use
sqlite3_errmsg() rather than including the raw result code. That is,
instead of:

    int result;
    result = sqlite3_bind_int(dbparameters_->q_any_, 1, zone_id);
    if (result != SQLITE_OK) {
        isc_throw(DataSourceError,
                  "Error in sqlite3_bind_int() for zone_id " <<
                  zone_id << ", sqlite3 result code: " << result);
    }

we do:

    if (sqlite3_bind_int(dbparameters_->q_any_, 1, zone_id) != SQLITE_OK) {
        isc_throw(DataSourceError,
                  "Error in sqlite3_bind_int() for zone_id " <<
                  zone_id << ": " << sqlite3_errmsg(dbparameters_->db_));
    }

This way, we can eliminate the temporary variable, can make methods
shorter, and can provide more human readable what() message.

  • convertToPlainChar(): (I'm asking partly because I forgot:-) in which case can ucp be NULL? Also, the main purpose of this helper function is to work around the C++ strictness on the difference between 'unsigned char*' and 'char *' (sqlite3 uses the former throughout its API, while std::string accepts the latter). And, from a point of type safe purist this conversion isn't correct (because in theory it's not guaranteed that these two pointer types are convertible). I'd note these points as comments.

IIRC the return value from sqlite_column_text() can be NULL if the database
field itself was NULL or if it has run out of memory. [...]

Ah, okay, I experimentally commented out the NULL check and saw the
tests triggered segfault:-) Hmm, so, technically, I think we
should separate these two cases: if ucp == NULL and sqlite3_errcode()
is NOMEM we should probably throw an exception rather than silently
convert it to ""...

I've added some comments. BTW, I can also get around the conversion by using
string::assign(), which makes the compiler do its magic on its own. But explicit
casts are probably better :)

Oh, right. When you use assign() you can change the pointer
conversion to plain char conversion:

  for (i = 0; i < num_of_chars; ++i) {
     string.internal_char_buf[i] = unsigned_char_buf[i];
  }

(but I think we'll keep casting in this case anyway)

  • getNextRecord(): although it should be rare and might be purely in theory, this method can internally throw an exception (bad_alloc). we should probably catch that within this method and convert it to DataSourceError? so that the application can handle that situation more gracefully (e.g., exit after closing the DB connection).

done

bad_alloc can happen only here:

                columns[column] = convertToPlainChar(sqlite3_column_text(
                                                    current_stmt, column));

so we could narrow the try-catch scope. But you might rather want to
avoid missing a non obvious case, so I'd leave it to you.

BTW, I now notice one another point in this method: we should probably
do validate column_count before sqlite3_step.

Also note you may want to use sqlite3_errmsg() instead of playing with
the result code directly (see above).

  • not a big deal especially in tests, but if I understand it correctly "conn" can be scoped_ptr instead of shared_ptr:

ack, changed. Actually, for countRecords, see below.

Okay, and I suspect you can remove this:

#include <boost/shared_ptr.hpp>
  • if possible, I'd like to test the case where getNextRecord() results in DataSourceError?().

[...]

I do not know of any dependable way to make sqlite3_step return an error, though...

With luck I guess we could make it fail with 'busy', but I admit we
probably cannot do it reliably. So, it's okay.

  • if not very hard, I'd check RDATA(s), not only the number of RDATAs and counts. maybe we could use rrset[s]Check defined in lib/testutils/dnsmessage_test.h.

This doesn't seem to be addressed. Did you think it too hard?

comment:10 follow-up: Changed 9 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:11 in reply to: ↑ 10 ; follow-up: Changed 9 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

Replying to jelte:

A few preliminary notes:

  • (as always) I made some minor editorial changes
  • please note that in #1061 it's likely we'll change "connection" to "accessor". some of internal variable names, test cases, etc. may have to be adjusted as a result.

Yes I noticed, I did a quick check yesterday and luckily git seems quite good at
handling the changes for existing things. But yeah I'll still need to run
through the things I added.

  • I know comments below are not short, but most of them are quite minor or trivial to address. The (almost) only real point to discuss is the use of try-catch in find().

General Discussions

  • I agree with changing getNextRecord() to a non const member function for the reason you explained. But this would also mean find() shouldn't be a cost member function of DatabaseClient::Finder (and therefore, of ZoneFinder?). [...]

ok, unconsted it

Hmm, it doesn't seem to be changed...

it appears I lost one of my commits :/ anyway. Unconsted again.

  • Don't we need to use loggers in database and sqlite3_connection?

hmm, yes, completely forgot about those :/

I added some in find(), it'll log either a successful return, or one of the
errors as caught by the big try/catch introduced in this revision.

Looks good, but I'd also include the class name and some identifier of
the underlying database (maybe we need a method in
DatabaseConnection??) for all these log messages, e.g.:

    logger.debug(DBG_TRACE_DETAILED, DATASRC_DATABASE_FIND_RECORDS).arg(name).arg(getClass()).arg(type).arg(connection_->getDBName());

(I assume a new method "getDBName()", which would result in something
like "sqlite3 (+ perhaps filename)")

I'm afraid putting in the full path may be a bit long, so I added yet
another utility method to util/filename.h; getNameAndExtension, and the result
for getDBName() is now "sqlite_<slitefile>". This can be ambiguous should there
be more than one sqlite database with the same name in different directories,
but I personally prefer it over full paths. Maybe we should make it something
user-settable, esp if we want to support any number of
any type of backend (where there may not be something like a file to base it
on), and/or make it user-settable but default to something like this. But both
of those are possible future enhancements :)

  • in database.cc, I guess the exception/error handling needs to be revised. [...]

I've implemented this approach for now, doing it with RAII is probably better,
but may mean some other interface changes as well, and probably an entire
different approach to the way we use the sqlite stmt variables right now
(although i guess some of the potential problems i see would also be present
now)

The revised code seems to do the right thing, but I'm personally
concerned about the heavily nested try-catch logic:

Yes, we should do RAII, and we should probably do it in a form where
searchForRecords returns some object you can call getNext() on (apparently
michal is creating something similar for #1067, and the base class seems the
same as what i need here).

I am passing this back to you now though, though so you can look at these other
changes

I did add some tests that make sure resetSearch() (the cleanup method) is
called, both on completion of a call to find() and on any exception that is
raised.

Regarding tests for resetSearch(): why are some of the tests commented
out?

    // Trigger the hardcoded exceptions and see if find() has cleaned up
    /*
    EXPECT_THROW(finder->find(isc::dns::Name("dsexception.in.search."),
                                              isc::dns::RRType::A(),
                                              NULL, ZoneFinder::FIND_DEFAULT),
                 DataSourceError);
...

woops, they should not have been, fixed

database.h/cc

  • maybe we want to use a plain old array instead of vector for columns, if we are sure it has a fixed size? [...]

Hmm, yes, a direct array is probably better, though also slightly
more dangerous. Therefore I also added a second argument, the
vector size. Doesn't provide full protection against passing in a
smaller array, but at least it's something; the dabaseimplementation
should check it.

If you like the efficiency of the array but want to make it safe, an
alternative would be to use boost::array (at the cost of the
additional dependency) and use at() instead of operator[] inside the
callee. I don't have a strong opinion, and leave it to you.

no current array is fine imo, there has to be some method of foot-shooting, right ;)

  • (general) I think it's better to define an enum or something to represent the semantics of the vector used in getNextRecord(), rather than hardcoding the magic numbers:
            if (columns.size() != 4) {
    ...
                const isc::dns::RRType cur_type(columns[0]);
                const isc::dns::RRTTL cur_ttl(columns[1]);
    

ack, done

Looks good, but I guess RecordColumnCount? should also be all upper
cased (with _, i.e. RECORD_COLUMN_COUNT) as it's a constant according
to the coding guideline.

ack

Also, this comment seems incomplete:

        TTL_COLUMN = 1,     ///< The TTL of the record (a
  • addOrCreate(): it's not clear how it's possible that type != rrset->getType():

[...]

(BTW, lcov indicates this case isn't covered by tests)

yes well with the current code it shouldn't be possible, and since
addOrCreate is in anonymous namespace we can't directly call it (or
did we have a clean way to do that?), so it's not covered by a
test...

made it an assert.

With the revised (with assert), I think code missing the test is okay.
One thing I didn't notice in the previous code: the assert() should
probably be better before the TTL check:

        // This is a check to make sure find() is not messing things up
        assert(type == rrset->getType());

        if (ttl < rrset->getTTL()) {
            rrset->setTTL(ttl);
        }

Should not matter in practice, but yeah, is better :)

BTW maybe we want to log the event when we perform setTTL()?

good idea (long discussion on jabber room about level. no real consensus, making
it INFO (but I can easily see it being DBG_TRACE_BASIC as well)

btw, i also changed the if; if we have such a message we should also print it when the first TTL happens to be the lowest one

sqlite3_connection.cc

  • In searchForRecords(), sqlite3_bind_text() can fail (and sqlite3_bind_int() may also fail in theory?), and we should catch the case even though it would be a rare event.

ack, done

Looks good, but in #1068 I noticed we might rather use
sqlite3_errmsg() rather than including the raw result code. That is,

This way, we can eliminate the temporary variable, can make methods
shorter, and can provide more human readable what() message.

ack, changed

  • convertToPlainChar(): (I'm asking partly because I forgot:-) in which case can ucp be NULL? Also, the main purpose of this helper function is to work around the C++ strictness on the difference between 'unsigned char*' and 'char *' (sqlite3 uses the former throughout its API, while std::string accepts the latter). And, from a point of type safe purist this conversion isn't correct (because in theory it's not guaranteed that these two pointer types are convertible). I'd note these points as comments.

IIRC the return value from sqlite_column_text() can be NULL if the database
field itself was NULL or if it has run out of memory. [...]

Ah, okay, I experimentally commented out the NULL check and saw the
tests triggered segfault:-) Hmm, so, technically, I think we
should separate these two cases: if ucp == NULL and sqlite3_errcode()
is NOMEM we should probably throw an exception rather than silently
convert it to ""...

done. Added the dbparameters as an argument, needed for sqlite3_errcode

  • getNextRecord(): although it should be rare and might be purely in theory, this method can internally throw an exception (bad_alloc). we should probably catch that within this method and convert it to DataSourceError? so that the application can handle that situation more gracefully (e.g., exit after closing the DB connection).

done

bad_alloc can happen only here:

                columns[column] = convertToPlainChar(sqlite3_column_text(
                                                    current_stmt, column));

so we could narrow the try-catch scope. But you might rather want to
avoid missing a non obvious case, so I'd leave it to you.

nah, reduced scope.

BTW, I now notice one another point in this method: we should probably
do validate column_count before sqlite3_step.

ack, moved.

Also note you may want to use sqlite3_errmsg() instead of playing with
the result code directly (see above).

yes, though here it is slightly trickier, as we had a resetSearch() there (which
made a couple of sqlite3_ calls that would cause the errmsg value to be either
undefined or unrelated). But since we now catch the error in the calling method,
and do resetSearch() there, I just removed the resetSearch() here.
(the other option would be to copy the errmsg() result first)

  • not a big deal especially in tests, but if I understand it correctly "conn" can be scoped_ptr instead of shared_ptr:

ack, changed. Actually, for countRecords, see below.

Okay, and I suspect you can remove this:

#include <boost/shared_ptr.hpp>

yup

  • if not very hard, I'd check RDATA(s), not only the number of RDATAs and counts. maybe we could use rrset[s]Check defined in lib/testutils/dnsmessage_test.h.

This doesn't seem to be addressed. Did you think it too hard?

oh, oops, somehow read over this one. Added it by passing a vector of
expected_rdatas and expected_sig_rdatas; the expected_counts that were there are
now unnecessary (same as size() on those vectors), so i removed those as
arguments. (i did not want to do something clever with the data as it is already
in the database; as that could potentially cause internal problems we miss, like
data not being checked at all)

comment:12 in reply to: ↑ 11 ; follow-up: Changed 9 years ago by jinmei

Replying to jelte:

Hmm, it doesn't seem to be changed...

it appears I lost one of my commits :/ anyway. Unconsted again.

Maybe we should explain the intent. Proposed text (which should go to
the ZoneFinder::find() documentation):

    /// \note Maybe counter intuitively, this method is not a const member
    /// function.  This is intentional; some of the underlying implementations
    /// are expected to use a database backend, and would internally contain
    /// some abstraction of "database connection".  In the most strict sense
    /// any (even read only) operation might change the internal state of
    /// such a connection, and in that sense the operation cannot be considered
    /// "const".  In order to avoid giving a false sense of safety to the
    /// caller, we indicate a call to this method may have a surprising
    /// side effect.  That said, this view may be too strict and it may
    /// make sense to say the internal database connection doesn't affect
    /// external behavior in terms of the interface of this method.  As
    /// we gain more experiences with various kinds of backends we may
    /// revisit the constness.

Looks good, but I'd also include the class name and some identifier of
the underlying database (maybe we need a method in
DatabaseConnection??) [...]

I'm afraid putting in the full path may be a bit long, so I added yet
another utility method to util/filename.h; getNameAndExtension, [...]

Okay, and I have some suggested editorial/documentation updates and
committed them to the branch.

  • in database.cc, I guess the exception/error handling needs to be revised. [...]

The revised code seems to do the right thing, but I'm personally
concerned about the heavily nested try-catch logic:

Yes, we should do RAII, and we should probably do it in a form where
searchForRecords returns some object you can call getNext() on (apparently
michal is creating something similar for #1067, and the base class seems the
same as what i need here).

I am passing this back to you now though, though so you can look at these other
changes

Since other things depend on this ticket/branch, I'm inclined to merge
this at the moment and create a separate ticket for further
consideration. Even though the result of it may involve interface
change and thus affect subsequent tasks, I guess it's more efficient
way of development. But if you want to fix that point within this
ticket, that's fine to me, too.

Looks good, but I guess RecordColumnCount? should also be all upper
cased (with _, i.e. RECORD_COLUMN_COUNT) as it's a constant according
to the coding guideline.

ack

Hmm, it's now RECORDCOLUMNCOUNT...isn't it a bit unreadable? If the
total length due to underscores is an issue, maybe we use RECORD_COUNT
or COLUMN_COUNT? (I guess the semantics should probably be
sufficiently clear where it's used).

BTW maybe we want to log the event when we perform setTTL()?

good idea (long discussion on jabber room about level. no real consensus, making
it INFO (but I can easily see it being DBG_TRACE_BASIC as well)

btw, i also changed the if; if we have such a message we should also print it when the first TTL happens to be the lowest one

Okay. As for the log message, we should probably add DBName too.

Also note you may want to use sqlite3_errmsg() instead of playing with
the result code directly (see above).

yes, though here it is slightly trickier, [...]
and do resetSearch() there, I just removed the resetSearch() here.
(the other option would be to copy the errmsg() result first)

I think it's okay. The caller can encounter an exception with the
underlying lookup/transaction still active (e.g. when the stored data
is bogus wrt the DNS protocol) in which case the app is responsible
for resetting the context anyway.

  • if not very hard, I'd check RDATA(s), [...]

oh, oops, somehow read over this one. Added it by passing a vector of
expected_rdatas and expected_sig_rdatas; the expected_counts that were there are
now unnecessary (same as size() on those vectors), so i removed those as
arguments. (i did not want to do something clever with the data as it is already
in the database; as that could potentially cause internal problems we miss, like
data not being checked at all)

Okay. I have one comment regarding the new tests: in doFindTest(),
you can omit checking these because it's done in rrsetCheck:

        EXPECT_EQ(expected_rdatas.size(), result.rrset->getRdataCount());
        EXPECT_EQ(expected_ttl, result.rrset->getTTL());
        EXPECT_EQ(expected_type, result.rrset->getType());

comment:13 Changed 9 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:14 in reply to: ↑ 12 Changed 9 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

Replying to jelte:

Maybe we should explain the intent. Proposed text (which should go to
the ZoneFinder::find() documentation):

ok, added, thanks

Yes, we should do RAII, and we should probably do it in a form where
searchForRecords returns some object you can call getNext() on (apparently
michal is creating something similar for #1067, and the base class seems the
same as what i need here).

I am passing this back to you now though, though so you can look at these other
changes

Since other things depend on this ticket/branch, I'm inclined to merge
this at the moment and create a separate ticket for further
consideration. Even though the result of it may involve interface
change and thus affect subsequent tasks, I guess it's more efficient
way of development. But if you want to fix that point within this
ticket, that's fine to me, too.

ok, i agree, and actually, seeing as there are other interface things (e.g. direct arguments for each column vs. the array, which michal chose in 1067), it may be a good idea to sync up a bit

Hmm, it's now RECORDCOLUMNCOUNT...isn't it a bit unreadable? If the
total length due to underscores is an issue, maybe we use RECORD_COUNT
or COLUMN_COUNT? (I guess the semantics should probably be
sufficiently clear where it's used).

yes, made it COLUMN_COUNT

btw, i also changed the if; if we have such a message we should also print it when the first TTL happens to be the lowest one

Okay. As for the log message, we should probably add DBName too.

done

  • if not very hard, I'd check RDATA(s), [...]

oh, oops, somehow read over this one. Added it by passing a vector of
expected_rdatas and expected_sig_rdatas; the expected_counts that were there are
now unnecessary (same as size() on those vectors), so i removed those as
arguments. (i did not want to do something clever with the data as it is already
in the database; as that could potentially cause internal problems we miss, like
data not being checked at all)

Okay. I have one comment regarding the new tests: in doFindTest(),
you can omit checking these because it's done in rrsetCheck:

        EXPECT_EQ(expected_rdatas.size(), result.rrset->getRdataCount());
        EXPECT_EQ(expected_ttl, result.rrset->getTTL());
        EXPECT_EQ(expected_type, result.rrset->getType());

yes, and I just noticed that I forgot to check the RRSIGS. Luckily that is mostly the same operation (create an rrset, fill it, then run rrsetCheck(), so that action now has its own local function, making doFindTest quite clean compared to what it was).

comment:15 Changed 9 years ago by jinmei

It's now almost okay (I noticed a typo in.mes and fixed it), one minor
last thing: we should probably print the DB name in the following logs
too:

    if (!result_rrset) {
        if (records_found) {
            logger.debug(DBG_TRACE_DETAILED, DATASRC_DATABASE_FOUND_NXRRSET)
                        .arg(name).arg(getClass()).arg(type);
            result_status = NXRRSET;
        } else {
            logger.debug(DBG_TRACE_DETAILED, DATASRC_DATABASE_FOUND_NXDOMAIN)
                        .arg(name).arg(getClass()).arg(type);
            result_status = NXDOMAIN;
        }
    } else {
        sig_store.appendSignatures(result_rrset);
        logger.debug(DBG_TRACE_DETAILED,
                    DATASRC_DATABASE_FOUND_RRSET).arg(*result_rrset);
    }

This should be quite straightforward, so I don't think we need another
cycle of review just for that. Please update the code as you think
appropriate and merge it.

comment:16 Changed 9 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:17 Changed 9 years ago by jelte

  • Resolution set to complete
  • Status changed from reviewing to closed

merged, closing ticket

Note: See TracTickets for help on using tickets.