Commit f82dc7b0 authored by Jelte Jansen's avatar Jelte Jansen
Browse files

[1062] addressed review comments

parent 46b961d6
......@@ -71,93 +71,87 @@ DatabaseClient::Finder::Finder(boost::shared_ptr<DatabaseConnection>
{ }
namespace {
// Adds the given Rdata to the given RRset
// If the rrset is an empty pointer, a new one is
// created with the given name, class, type and ttl
// The type is checked if the rrset exists, but the
// name is not.
//
// Then adds the given rdata to the set
//
// Raises a DataSourceError if the type does not
// match, or if the given rdata string does not
// parse correctly for the given type and class
void addOrCreate(isc::dns::RRsetPtr& rrset,
const isc::dns::Name& name,
const isc::dns::RRClass& cls,
const isc::dns::RRType& type,
const isc::dns::RRTTL& ttl,
const std::string& rdata_str)
{
if (!rrset) {
rrset.reset(new isc::dns::RRset(name, cls, type, ttl));
} else {
if (ttl < rrset->getTTL()) {
rrset->setTTL(ttl);
}
// make sure the type is correct
if (type != rrset->getType()) {
isc_throw(DataSourceError,
"attempt to add multiple types to RRset in find()");
}
// Adds the given Rdata to the given RRset
// If the rrset is an empty pointer, a new one is
// created with the given name, class, type and ttl
// The type is checked if the rrset exists, but the
// name is not.
//
// Then adds the given rdata to the set
//
// Raises a DataSourceError if the type does not
// match, or if the given rdata string does not
// parse correctly for the given type and class
void addOrCreate(isc::dns::RRsetPtr& rrset,
const isc::dns::Name& name,
const isc::dns::RRClass& cls,
const isc::dns::RRType& type,
const isc::dns::RRTTL& ttl,
const std::string& rdata_str)
{
if (!rrset) {
rrset.reset(new isc::dns::RRset(name, cls, type, ttl));
} else {
if (ttl < rrset->getTTL()) {
rrset->setTTL(ttl);
}
if (rdata_str != "") {
try {
rrset->addRdata(isc::dns::rdata::createRdata(type, cls,
rdata_str));
} catch (const isc::dns::rdata::InvalidRdataText& ivrt) {
// at this point, rrset may have been initialised for no reason,
// and won't be used. But the caller would drop the shared_ptr
// on such an error anyway, so we don't care.
isc_throw(DataSourceError,
"bad rdata in database for " << name.toText() << " "
<< type.toText() << " " << ivrt.what());
}
// make sure the type is correct
// TODO Assert?
if (type != rrset->getType()) {
isc_throw(DataSourceError,
"attempt to add multiple types to RRset in find()");
}
}
try {
rrset->addRdata(isc::dns::rdata::createRdata(type, cls, rdata_str));
} catch (const isc::dns::rdata::InvalidRdataText& ivrt) {
// at this point, rrset may have been initialised for no reason,
// and won't be used. But the caller would drop the shared_ptr
// on such an error anyway, so we don't care.
isc_throw(DataSourceError,
"bad rdata in database for " << name << " "
<< type << ": " << ivrt.what());
}
}
// This class keeps a short-lived store of RRSIG records encountered
// during a call to find(). If the backend happens to return signatures
// before the actual data, we might not know which signatures we will need
// So if they may be relevant, we store the in this class.
//
// (If this class seems useful in other places, we might want to move
// it to util. That would also provide an opportunity to add unit tests)
class RRsigStore {
public:
// Adds the given signature Rdata to the store
// The signature rdata MUST be of the RRSIG rdata type
// (the caller must make sure of this)
void addSig(isc::dns::rdata::RdataPtr sig_rdata) {
const isc::dns::RRType& type_covered =
static_cast<isc::dns::rdata::generic::RRSIG*>(
sig_rdata.get())->typeCovered();
if (!haveSigsFor(type_covered)) {
sigs[type_covered] = std::vector<isc::dns::rdata::RdataPtr>();
}
sigs.find(type_covered)->second.push_back(sig_rdata);
}
// Returns true if this store contains signatures covering the
// given type
bool haveSigsFor(isc::dns::RRType type) const {
return (sigs.count(type) > 0);
}
// This class keeps a short-lived store of RRSIG records encountered
// during a call to find(). If the backend happens to return signatures
// before the actual data, we might not know which signatures we will need
// So if they may be relevant, we store the in this class.
//
// (If this class seems useful in other places, we might want to move
// it to util. That would also provide an opportunity to add unit tests)
class RRsigStore {
public:
// Adds the given signature Rdata to the store
// The signature rdata MUST be of the RRSIG rdata type
// (the caller must make sure of this).
// NOTE: if we move this class to a public namespace,
// we should add a type_covered argument, so as not
// to have to do this cast here.
void addSig(isc::dns::rdata::RdataPtr sig_rdata) {
const isc::dns::RRType& type_covered =
static_cast<isc::dns::rdata::generic::RRSIG*>(
sig_rdata.get())->typeCovered();
sigs[type_covered].push_back(sig_rdata);
}
// If the store contains signatures for the type of the given
// rrset, they are appended to it.
void appendSignatures(isc::dns::RRsetPtr& rrset) const {
if (haveSigsFor(rrset->getType())) {
BOOST_FOREACH(isc::dns::rdata::RdataPtr sig,
sigs.find(rrset->getType())->second) {
rrset->addRRsig(sig);
}
// If the store contains signatures for the type of the given
// rrset, they are appended to it.
void appendSignatures(isc::dns::RRsetPtr& rrset) const {
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);
}
}
}
private:
std::map<isc::dns::RRType, std::vector<isc::dns::rdata::RdataPtr> > sigs;
};
private:
std::map<isc::dns::RRType, std::vector<isc::dns::rdata::RdataPtr> > sigs;
};
}
......@@ -174,55 +168,75 @@ DatabaseClient::Finder::find(const isc::dns::Name& name,
ZoneFinder::Result result_status = SUCCESS;
RRsigStore sig_store;
connection_->searchForRecords(zone_id_, name.toText());
try {
connection_->searchForRecords(zone_id_, name.toText());
std::vector<std::string> columns;
while (connection_->getNextRecord(columns)) {
if (!records_found) {
records_found = true;
}
std::string columns[DatabaseConnection::RecordColumnCount];
while (connection_->getNextRecord(columns,
DatabaseConnection::RecordColumnCount)) {
if (!records_found) {
records_found = true;
}
if (columns.size() != 4) {
isc_throw(DataSourceError, "Datasource backend did not return 4 "
"columns in getNextRecord()");
}
try {
const isc::dns::RRType cur_type(columns[DatabaseConnection::TYPE_COLUMN]);
const isc::dns::RRTTL cur_ttl(columns[DatabaseConnection::TTL_COLUMN]);
// Ths sigtype column was an optimization for finding the relevant
// RRSIG RRs for a lookup. Currently this column is not used in this
// revised datasource implementation. We should either start using it
// again, or remove it from use completely (i.e. also remove it from
// the schema and the backend implementation).
// Note that because we don't use it now, we also won't notice it if
// the value is wrong (i.e. if the sigtype column contains an rrtype
// that is different from the actual value of the 'type covered' field
// in the RRSIG Rdata).
//cur_sigtype(columns[SIGTYPE_COLUMN]);
try {
const isc::dns::RRType cur_type(columns[0]);
const isc::dns::RRTTL cur_ttl(columns[1]);
//cur_sigtype(columns[2]);
if (cur_type == type) {
addOrCreate(result_rrset, name, getClass(), cur_type, cur_ttl,
columns[3]);
} else if (cur_type == isc::dns::RRType::CNAME()) {
// There should be no other data, so cur_rrset should be empty,
if (result_rrset) {
isc_throw(DataSourceError, "CNAME found but it is not "
"the only record for " + name.toText());
if (cur_type == type) {
addOrCreate(result_rrset, name, getClass(), cur_type,
cur_ttl, columns[DatabaseConnection::RDATA_COLUMN]);
} else if (cur_type == isc::dns::RRType::CNAME()) {
// There should be no other data, so result_rrset should be empty.
if (result_rrset) {
isc_throw(DataSourceError, "CNAME found but it is not "
"the only record for " + name.toText());
}
addOrCreate(result_rrset, name, getClass(), cur_type, cur_ttl,
columns[DatabaseConnection::RDATA_COLUMN]);
result_status = CNAME;
} else if (cur_type == isc::dns::RRType::RRSIG()) {
// If we get signatures before we get the actual data, we
// can't know which ones to keep and which to drop...
// So we keep a separate store of any signature that may be
// relevant and add them to the final RRset when we are done.
// A possible optimization here is to not store them for types
// we are certain we don't need
sig_store.addSig(isc::dns::rdata::createRdata(cur_type,
getClass(),
columns[DatabaseConnection::RDATA_COLUMN]));
}
addOrCreate(result_rrset, name, getClass(), cur_type, cur_ttl,
columns[3]);
result_status = CNAME;
} else if (cur_type == isc::dns::RRType::RRSIG()) {
// If we get signatures before we get the actual data, we
// can't know which ones to keep and which to drop...
// So we keep a separate store of any signature that may be
// relevant and add them to the final RRset when we are done.
// A possible optimization here is to not store them for types
// we are certain we don't need
isc::dns::rdata::RdataPtr cur_rrsig(
isc::dns::rdata::createRdata(cur_type, getClass(),
columns[3]));
sig_store.addSig(cur_rrsig);
} catch (const isc::dns::InvalidRRType& irt) {
isc_throw(DataSourceError, "Invalid RRType in database for " <<
name << ": " << columns[DatabaseConnection::TYPE_COLUMN]);
} catch (const isc::dns::InvalidRRTTL& irttl) {
isc_throw(DataSourceError, "Invalid TTL in database for " <<
name << ": " << columns[DatabaseConnection::TTL_COLUMN]);
} catch (const isc::dns::rdata::InvalidRdataText& ird) {
isc_throw(DataSourceError, "Invalid rdata in database for " <<
name << ": " << columns[DatabaseConnection::RDATA_COLUMN]);
}
} catch (const isc::dns::InvalidRRType& irt) {
isc_throw(DataSourceError, "Invalid RRType in database for " <<
name << ": " << columns[0]);
} catch (const isc::dns::InvalidRRTTL& irttl) {
isc_throw(DataSourceError, "Invalid TTL in database for " <<
name << ": " << columns[1]);
}
} catch (const DataSourceError& dse) {
// call cleanup and rethrow
connection_->resetSearch();
throw;
} catch (const isc::Exception& isce) {
// // cleanup, change it to a DataSourceError and rethrow
connection_->resetSearch();
isc_throw(DataSourceError, isce.what());
} catch (const std::exception& ex) {
connection_->resetSearch();
throw;
}
if (!result_rrset) {
......
......@@ -92,14 +92,59 @@ public:
* Returns a boolean specifying whether or not there was more data to read.
* In the case of a database error, a DatasourceError is thrown.
*
* The columns passed is an array of std::strings consisting of
* DatabaseConnection::RecordColumnCount elements, the elements of which
* are defined in DatabaseConnection::RecordColumns, in their basic
* string representation.
*
* If you are implementing a derived database connection class, you
* should have this method check the column_count value, and fill the
* array with strings conforming to their description in RecordColumn.
*
* \exception DatasourceError if there was an error reading from the database
*
* \param columns This vector will be cleared, and the fields of the record will
* be appended here as strings (in the order rdtype, ttl, sigtype,
* and rdata). If there was no data, the vector is untouched.
* \param columns The elements of this array will be filled with the data
* for one record as defined by RecordColumns
* If there was no data, the array is untouched.
* \return true if there was a next record, false if there was not
*/
virtual bool getNextRecord(std::vector<std::string>& columns) = 0;
virtual bool getNextRecord(std::string columns[], size_t column_count) = 0;
/**
* \brief Resets the current search initiated with searchForRecords()
*
* This method will be called when the called of searchForRecords() and
* getNextRecord() finds bad data, and aborts the current search.
* It should clean up whatever handlers searchForRecords() created, and
* any other state modified or needed by getNextRecord()
*
* Of course, the implementation of getNextRecord may also use it when
* it is done with a search. If it does, the implementation of this
* method should make sure it can handle being called multiple times.
*
* The implementation for this method should make sure it never throws.
*/
virtual void resetSearch() = 0;
/**
* Definitions of the fields as they are required to be filled in
* by getNextRecord()
*
* When implementing getNextRecord(), the columns array should
* be filled with the values as described in this enumeration,
* in this order.
*/
enum RecordColumns {
TYPE_COLUMN = 0, ///< The RRType of the record (A/NS/TXT etc.)
TTL_COLUMN = 1, ///< The TTL of the record (a
SIGTYPE_COLUMN = 2, ///< For RRSIG records, this contains the RRTYPE
///< the RRSIG covers. In the current implementation,
///< this field is ignored.
RDATA_COLUMN = 3 ///< Full text representation of the record's RDATA
};
/// The number of fields the columns array passed to getNextRecord should have
static const size_t RecordColumnCount = 4;
};
/**
......
......@@ -321,15 +321,30 @@ SQLite3Connection::getZone(const isc::dns::Name& name) const {
void
SQLite3Connection::searchForRecords(int zone_id, const std::string& name) {
sqlite3_reset(dbparameters_->q_any_);
sqlite3_clear_bindings(dbparameters_->q_any_);
sqlite3_bind_int(dbparameters_->q_any_, 1, zone_id);
resetSearch();
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);
}
// use transient since name is a ref and may disappear
sqlite3_bind_text(dbparameters_->q_any_, 2, name.c_str(), -1,
SQLITE_TRANSIENT);
result = sqlite3_bind_text(dbparameters_->q_any_, 2, name.c_str(), -1,
SQLITE_TRANSIENT);
if (result != SQLITE_OK) {
isc_throw(DataSourceError,
"Error in sqlite3_bind_text() for name " <<
name << ", sqlite3 result code: " << result);
}
};
namespace {
// This helper function converts from the unsigned char* type (used by
// sqlite3) to char* (wanted by std::string). Technically these types
// might not be directly convertable
// In case sqlite3_column_text() returns NULL, we just make it an
// empty string.
const char*
convertToPlainChar(const unsigned char* ucp) {
if (ucp == NULL) {
......@@ -341,31 +356,44 @@ convertToPlainChar(const unsigned char* ucp) {
}
bool
SQLite3Connection::getNextRecord(std::vector<std::string>& columns) {
sqlite3_stmt* current_stmt = dbparameters_->q_any_;
const int rc = sqlite3_step(current_stmt);
SQLite3Connection::getNextRecord(std::string columns[], size_t column_count) {
try {
sqlite3_stmt* current_stmt = dbparameters_->q_any_;
const int rc = sqlite3_step(current_stmt);
if (column_count != RecordColumnCount) {
isc_throw(DataSourceError,
"Datasource backend caller did not pass a column array "
"of size " << RecordColumnCount <<
" to getNextRecord()");
}
if (rc == SQLITE_ROW) {
columns.clear();
for (int column = 0; column < 4; ++column) {
columns.push_back(convertToPlainChar(sqlite3_column_text(
current_stmt, column)));
if (rc == SQLITE_ROW) {
for (int column = 0; column < column_count; ++column) {
columns[column] = convertToPlainChar(sqlite3_column_text(
current_stmt, column));
}
return (true);
} else if (rc == SQLITE_DONE) {
// reached the end of matching rows
resetSearch();
return (false);
}
return (true);
} else if (rc == SQLITE_DONE) {
// reached the end of matching rows
sqlite3_reset(current_stmt);
sqlite3_clear_bindings(current_stmt);
return (false);
resetSearch();
isc_throw(DataSourceError,
"Unexpected failure in sqlite3_step (sqlite result code " << rc << ")");
} catch (std::bad_alloc) {
isc_throw(DataSourceError, "bad_alloc in Sqlite3Connection::getNextRecord");
}
sqlite3_reset(current_stmt);
sqlite3_clear_bindings(current_stmt);
isc_throw(DataSourceError,
"Unexpected failure in sqlite3_step (sqlite result code " << rc << ")");
// Compilers might not realize isc_throw always throws
return (false);
}
void
SQLite3Connection::resetSearch() {
sqlite3_reset(dbparameters_->q_any_);
sqlite3_clear_bindings(dbparameters_->q_any_);
}
}
}
......@@ -95,6 +95,9 @@ public:
* This implements the searchForRecords from DatabaseConnection.
* This particular implementation does not raise DataSourceError.
*
* \exception DataSourceError when sqlite3_bind_int() or
* sqlite3_bind_text() fails
*
* \param zone_id The zone to seach in, as returned by getZone()
* \param name The name to find records for
*/
......@@ -107,12 +110,31 @@ public:
* This implements the getNextRecord from DatabaseConnection.
* See the documentation there for more information.
*
* If this method raises an exception, the contents of columns are undefined.
*
* \exception DataSourceError if there is an error returned by sqlite_step()
* When this exception is raised, the current
* search as initialized by searchForRecords() is
* NOT reset, and the caller is expected to take
* care of that.
* \param columns This vector will be cleared, and the fields of the record will
* be appended here as strings (in the order rdtype, ttl, sigtype,
* and rdata). If there was no data, the vector is untouched.
* and rdata). If there was no data (i.e. if this call returns
* false), the vector is untouched.
* \return true if there was a next record, false if there was not
*/
virtual bool getNextRecord(std::vector<std::string>& columns);
virtual bool getNextRecord(std::string columns[], size_t column_count);
/**
* \brief Resets any state created by searchForRecords
*
* This implements the resetSearch from DatabaseConnection.
* See the documentation there for more information.
*
* This function never throws.
*/
virtual void resetSearch();
private:
/// \brief Private database data
SQLite3Parameters* dbparameters_;
......
......@@ -15,6 +15,7 @@
#include <gtest/gtest.h>
#include <dns/name.h>
#include <dns/rrttl.h>
#include <exceptions/exceptions.h>
#include <datasrc/database.h>
......@@ -36,7 +37,7 @@ namespace {
*/
class MockConnection : public DatabaseConnection {
public:
MockConnection() { fillData(); }
MockConnection() : search_running_(false) { fillData(); }
virtual std::pair<bool, int> getZone(const Name& name) const {
if (name == Name("example.org")) {
......@@ -47,10 +48,23 @@ public:
}
virtual void searchForRecords(int zone_id, const std::string& name) {
search_running_ = true;
// 'hardcoded' name to trigger exceptions (for testing
// the error handling of find() (the other on is below in
// if the name is "exceptiononsearch" it'll raise an exception here
if (name == "dsexception.in.search.") {
isc_throw(DataSourceError, "datasource exception on search");
} else if (name == "iscexception.in.search.") {
isc_throw(isc::Exception, "isc exception on search");
} else if (name == "basicexception.in.search.") {
throw std::exception();
}
searched_name_ = name;
// we're not aiming for efficiency in this test, simply
// copy the relevant vector from records
cur_record = 0;
if (zone_id == 42) {
if (records.count(name) > 0) {
cur_name = records.find(name)->second;
......@@ -62,15 +76,38 @@ public:
}
};
virtual bool getNextRecord(std::vector<std::string>& columns) {
virtual bool getNextRecord(std::string columns[], size_t column_count) {
if (searched_name_ == "dsexception.in.getnext.") {
isc_throw(DataSourceError, "datasource exception on getnextrecord");
} else if (searched_name_ == "iscexception.in.getnext.") {
isc_throw(isc::Exception, "isc exception on getnextrecord");
} else if (searched_name_ == "basicexception.in.getnext.") {
throw std::exception();
}
if (column_count != DatabaseConnection::RecordColumnCount) {
isc_throw(DataSourceError, "Wrong column count in getNextRecord");
}
if (cur_record < cur_name.size()) {
columns = cur_name[cur_record++];
for (size_t i = 0; i < column_count; ++i) {
columns[i] = cur_name[cur_record][i];
}
cur_record++;
return (true);
} else {
resetSearch();
return (false);
}
};
virtual void resetSearch() {
search_running_ = false;
};
bool searchRunning() const {
return (search_running_);
}
private:
std::map<std::string, std::vector< std::vector<std::string> > > records;
// used as internal index for getNextRecord()
......@@ -80,6 +117,14 @@ private:
// fake data
std::vector< std::vector<std::string> > cur_name;
// This boolean is used to make sure find() calls resetSearch
// when it encounters an error
bool search_running_;
// We store the name passed to searchForRecords, so we can
// hardcode some exceptions into getNextRecord
std::string searched_name_;
// Adds one record to the current name in the database
// The actual data will not be added to 'records' until
// addCurName() is called
......@@ -121,6 +166,11 @@ private:
addRecord("AAAA", "3600", "", "2001:db8::2");
addCurName("www.example.org.");
addRecord("A", "3600", "", "192.0.2.1");
addRecord("AAAA", "3600", "", "2001:db8::1");
addRecord("A", "3600", "", "192.0.2.2");
addCurName("www2.example.org.");
addRecord("CNAME", "3600", "", "www.example.org.");
addCurName("cname.example.org.");
......@@ -165,18 +215,42 @@ private:
addRecord("A", "3600", "", "192.0.2.1");
addCurName("acnamesig3.example.org.");
addRecord("A", "3600", "", "192.0.2.1");
addRecord("A", "360", "", "192.0.2.2");
addCurName("ttldiff1.example.org.");