[1177] Refactor getRRset

That one was getting really hairy and it wouldn't serve it's purpose in
the following work.

Currently it takes set of types we're interested in and returns all of
them, the logic to decide which one to take is left to the caller. This
simplifies the method and doesn't hurt the rest much, as the rest needed
to know what to ask for anyway.

Also, it allows getting NSEC or NSEC3 records in the same database
search almost for free.

src/lib/datasrc/database.cc

@@ -174,16 +174,13 @@ private:
 };
 }
 
-std::pair<bool, isc::dns::RRsetPtr>
-DatabaseClient::Finder::getRRset(const isc::dns::Name& name,
-                                 const isc::dns::RRType* type,
-                                 bool want_cname, bool want_dname,
-                                 bool want_ns,
-                                 const isc::dns::Name* construct_name)
+DatabaseClient::Finder::FoundRRsets
+DatabaseClient::Finder::getRRsets(const Name& name, const WantedTypes& types,
+                                  bool check_ns, const Name* construct_name)
 {
     RRsigStore sig_store;
     bool records_found = false;
-    isc::dns::RRsetPtr result_rrset;
+    std::map<RRType, RRsetPtr> result;
 
     // Request the context
     DatabaseAccessor::IteratorContextPtr
@@ -198,81 +195,19 @@ DatabaseClient::Finder::getRRset(const isc::dns::Name& name,
     if (construct_name == NULL) {
         construct_name = &name;
     }
+
+    bool seen_cname(false);
+    bool seen_other(false);
+    bool seen_ns(false);
+
     while (context->getNext(columns)) {
-        if (!records_found) {
-            records_found = true;
-        }
+        // The domain is not empty
+        records_found = true;
 
         try {
-            const isc::dns::RRType cur_type(columns[DatabaseAccessor::
-                                            TYPE_COLUMN]);
-            const isc::dns::RRTTL cur_ttl(columns[DatabaseAccessor::
-                                          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]);
-
-            // Check for delegations before checking for the right type.
-            // This is needed to properly delegate request for the NS
-            // record itself.
-            //
-            // This happens with NS only, CNAME must be alone and DNAME
-            // is not checked in the exact queried domain.
-            if (want_ns && cur_type == isc::dns::RRType::NS()) {
-                if (result_rrset &&
-                    result_rrset->getType() != isc::dns::RRType::NS()) {
-                    isc_throw(DataSourceError, "NS found together with data"
-                              " in non-apex domain " + name.toText());
-                }
-                addOrCreate(result_rrset, *construct_name, getClass(),
-                            cur_type, cur_ttl,
-                            columns[DatabaseAccessor::RDATA_COLUMN],
-                            *accessor_);
-            } else if (type != NULL && cur_type == *type) {
-                if (result_rrset &&
-                    result_rrset->getType() == isc::dns::RRType::CNAME()) {
-                    isc_throw(DataSourceError, "CNAME found but it is not "
-                              "the only record for " + name.toText());
-                } else if (result_rrset && want_ns &&
-                           result_rrset->getType() == isc::dns::RRType::NS()) {
-                    isc_throw(DataSourceError, "NS found together with data"
-                              " in non-apex domain " + name.toText());
-                }
-                addOrCreate(result_rrset, *construct_name, getClass(),
-                            cur_type, cur_ttl,
-                            columns[DatabaseAccessor::RDATA_COLUMN],
-                            *accessor_);
-            } else if (want_cname && 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, *construct_name, getClass(),
-                            cur_type, cur_ttl,
-                            columns[DatabaseAccessor::RDATA_COLUMN],
-                            *accessor_);
-            } else if (want_dname && cur_type == isc::dns::RRType::DNAME()) {
-                // There should be max one RR of DNAME present
-                if (result_rrset &&
-                    result_rrset->getType() == isc::dns::RRType::DNAME()) {
-                    isc_throw(DataSourceError, "DNAME with multiple RRs in " +
-                              name.toText());
-                }
-                addOrCreate(result_rrset, *construct_name, getClass(),
-                            cur_type, cur_ttl,
-                            columns[DatabaseAccessor::RDATA_COLUMN],
-                            *accessor_);
-            } else if (cur_type == isc::dns::RRType::RRSIG()) {
+            const RRType cur_type(columns[DatabaseAccessor::TYPE_COLUMN]);
+
+            if (cur_type == 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
@@ -280,27 +215,67 @@ DatabaseClient::Finder::getRRset(const isc::dns::Name& name,
                 // 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[DatabaseAccessor::RDATA_COLUMN]));
+                sig_store.addSig(rdata::createRdata(cur_type, getClass(),
+                     columns[DatabaseAccessor::RDATA_COLUMN]));
+            }
+
+            if (types.find(cur_type) != types.end()) {
+                // This type is requested, so put it into result
+                const RRTTL cur_ttl(columns[DatabaseAccessor::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]);
+                addOrCreate(result[cur_type], *construct_name, getClass(),
+                            cur_type, cur_ttl,
+                            columns[DatabaseAccessor::RDATA_COLUMN],
+                            *accessor_);
+            }
+
+            if (cur_type == RRType::CNAME()) {
+                seen_cname = true;
+            } else if (cur_type == RRType::NS()) {
+                seen_ns = true;
+            } else if (cur_type != RRType::RRSIG()) {//RRSIG can live anywhere
+                // FIXME: Is something else allowed in the delegation point? DS?
+                seen_other = true;
             }
-        } catch (const isc::dns::InvalidRRType& irt) {
+        } catch (const InvalidRRType& irt) {
             isc_throw(DataSourceError, "Invalid RRType in database for " <<
                       name << ": " << columns[DatabaseAccessor::
                       TYPE_COLUMN]);
-        } catch (const isc::dns::InvalidRRTTL& irttl) {
+        } catch (const InvalidRRTTL& irttl) {
             isc_throw(DataSourceError, "Invalid TTL in database for " <<
                       name << ": " << columns[DatabaseAccessor::
                       TTL_COLUMN]);
-        } catch (const isc::dns::rdata::InvalidRdataText& ird) {
+        } catch (const rdata::InvalidRdataText& ird) {
             isc_throw(DataSourceError, "Invalid rdata in database for " <<
                       name << ": " << columns[DatabaseAccessor::
                       RDATA_COLUMN]);
         }
     }
-    if (result_rrset) {
-        sig_store.appendSignatures(result_rrset);
+    if (seen_cname && (seen_other || seen_ns)) {
+        isc_throw(DataSourceError, "CNAME shares domain " << name <<
+                  " with something else");
+    }
+    if (check_ns && seen_ns && seen_other) {
+        isc_throw(DataSourceError, "NS shares domain " << name <<
+                  " with something else");
     }
-    return (std::pair<bool, isc::dns::RRsetPtr>(records_found, result_rrset));
+    // Add signatures to all found RRsets
+    for (std::map<RRType, RRsetPtr>::iterator i(result.begin());
+         i != result.end(); ++ i) {
+        sig_store.appendSignatures(i->second);
+    }
+
+    return (FoundRRsets(records_found, result));
 }
 
 bool
@@ -317,6 +292,21 @@ DatabaseClient::Finder::hasSubdomains(const std::string& name) {
     return (context->getNext(columns));
 }
 
+// Some manipulation with RRType sets
+namespace {
+
+std::set<RRType> empty_types;
+
+// To conveniently put the RRTypes into the sets. This is not really clean
+// design, but it is hidden inside this file and makes the calls much more
+// convenient.
+std::set<RRType> operator +(std::set<RRType> set, const RRType& type) {
+    set.insert(type);
+    return (set);
+}
+
+}
+
 ZoneFinder::FindResult
 DatabaseClient::Finder::find(const isc::dns::Name& name,
                              const isc::dns::RRType& type,
@@ -329,7 +319,7 @@ DatabaseClient::Finder::find(const isc::dns::Name& name,
     bool glue_ok(options & FIND_GLUE_OK);
     isc::dns::RRsetPtr result_rrset;
     ZoneFinder::Result result_status = SUCCESS;
-    std::pair<bool, isc::dns::RRsetPtr> found;
+    FoundRRsets found;
     logger.debug(DBG_TRACE_DETAILED, DATASRC_DATABASE_FIND_RECORDS)
         .arg(accessor_->getDBName()).arg(name).arg(type);
     // In case we are in GLUE_OK mode and start matching wildcards,
@@ -345,39 +335,52 @@ DatabaseClient::Finder::find(const isc::dns::Name& name,
     // This is how many labels we remove to get origin
     size_t remove_labels(current_label_count - origin_label_count);
 
+    // Type shortcut, used a lot here
+    typedef std::map<RRType, RRsetPtr>::const_iterator FoundIterator;
+
     // Now go trough all superdomains from origin down
     for (int i(remove_labels); i > 0; --i) {
         Name superdomain(name.split(i));
         // Look if there's NS or DNAME (but ignore the NS in origin)
-        found = getRRset(superdomain, NULL, false, true,
-                         i != remove_labels && !glue_ok);
+        static WantedTypes delegation_types(empty_types + RRType::DNAME() +
+                                            RRType::NS());
+        found = getRRsets(superdomain, delegation_types, i != remove_labels);
         if (found.first) {
             // It contains some RRs, so it exists.
             last_known = superdomain.getLabelCount();
-            // In case we are in GLUE_OK, we want to store the highest
-            // encountered RRset.
-            if (glue_ok && !first_ns && i != remove_labels) {
-                first_ns = getRRset(superdomain, NULL, false, false,
-                                    true).second;
-            }
-        }
-        if (found.second) {
-            // We found something redirecting somewhere else
-            // (it can be only NS or DNAME here)
-            result_rrset = found.second;
-            if (result_rrset->getType() == isc::dns::RRType::NS()) {
+
+            const FoundIterator nsi(found.second.find(RRType::NS()));
+            const FoundIterator dni(found.second.find(RRType::DNAME()));
+            // In case we are in GLUE_OK mode, we want to store the
+            // highest encountered NS (but not apex)
+            if (glue_ok && !first_ns && i != remove_labels &&
+                nsi != found.second.end()) {
+                first_ns = nsi->second;
+            } else if (!glue_ok && i != remove_labels &&
+                       nsi != found.second.end()) {
+                // Do a NS delegation, but ignore NS in glue_ok mode. Ignore
+                // delegation in apex
                 LOG_DEBUG(logger, DBG_TRACE_DETAILED,
                           DATASRC_DATABASE_FOUND_DELEGATION).
                     arg(accessor_->getDBName()).arg(superdomain);
+                result_rrset = nsi->second;
                 result_status = DELEGATION;
-            } else {
+                // No need to go lower, found
+                break;
+            } else if (dni != found.second.end()) {
+                // Very similar with DNAME
                 LOG_DEBUG(logger, DBG_TRACE_DETAILED,
                           DATASRC_DATABASE_FOUND_DNAME).
                     arg(accessor_->getDBName()).arg(superdomain);
+                result_rrset = dni->second;
                 result_status = DNAME;
+                if (result_rrset->getRdataCount() != 1) {
+                    isc_throw(DataSourceError, "DNAME at " << superdomain <<
+                              " has " << result_rrset->getRdataCount() <<
+                              " rdata, 1 expected");
+                }
+                break;
             }
-            // Don't search more
-            break;
         }
     }
 
@@ -385,21 +388,37 @@ DatabaseClient::Finder::find(const isc::dns::Name& name,
         // Try getting the final result and extract it
         // It is special if there's a CNAME or NS, DNAME is ignored here
         // And we don't consider the NS in origin
-        found = getRRset(name, &type, true, false, name != origin && !glue_ok);
+
+        static WantedTypes final_types(empty_types + RRType::CNAME() +
+                                       RRType::NS());
+        found = getRRsets(name, final_types + type, name != origin);
         records_found = found.first;
-        result_rrset = found.second;
-        if (result_rrset && name != origin && !glue_ok &&
-            result_rrset->getType() == isc::dns::RRType::NS()) {
+
+        // NS records, CNAME record and Wanted Type records
+        const FoundIterator nsi(found.second.find(RRType::NS()));
+        const FoundIterator cni(found.second.find(RRType::CNAME()));
+        const FoundIterator wti(found.second.find(type));
+        if (name != origin && !glue_ok && nsi != found.second.end()) {
+            // There's a delegation at the exact node.
             LOG_DEBUG(logger, DBG_TRACE_DETAILED,
                       DATASRC_DATABASE_FOUND_DELEGATION_EXACT).
                 arg(accessor_->getDBName()).arg(name);
             result_status = DELEGATION;
-        } else if (result_rrset && type != isc::dns::RRType::CNAME() &&
-                   result_rrset->getType() == isc::dns::RRType::CNAME()) {
+            result_rrset = nsi->second;
+        } else if (type != isc::dns::RRType::CNAME() &&
+                   cni != found.second.end()) {
+            // A CNAME here
             result_status = CNAME;
-        }
-
-        if (!result_rrset && !records_found) {
+            result_rrset = cni->second;
+            if (result_rrset->getRdataCount() != 1) {
+                isc_throw(DataSourceError, "CNAME with " <<
+                          result_rrset->getRdataCount() <<
+                          " rdata at " << name << ", expected 1");
+            }
+        } else if (wti != found.second.end()) {
+            // Just get the answer
+            result_rrset = wti->second;
+        } else if (!records_found) {
             // Nothing lives here.
             // But check if something lives below this
             // domain and if so, pretend something is here as well.
@@ -423,8 +442,11 @@ DatabaseClient::Finder::find(const isc::dns::Name& name,
                     Name superdomain(name.split(i));
                     Name wildcard(star.concatenate(superdomain));
                     // TODO What do we do about DNAME here?
-                    found = getRRset(wildcard, &type, true, false, true,
-                                     &name);
+                    static WantedTypes wildcard_types(empty_types +
+                                                      RRType::CNAME() +
+                                                      RRType::NS());
+                    found = getRRsets(wildcard, wildcard_types + type, true,
+                                      &name);
                     if (found.first) {
                         if (first_ns) {
                             // In case we are under NS, we don't
@@ -445,7 +467,22 @@ DatabaseClient::Finder::find(const isc::dns::Name& name,
                             // domain, but it could be empty non-terminal. In
                             // that case, we need to cancel the match.
                             records_found = true;
-                            result_rrset = found.second;
+                            const FoundIterator
+                                cni(found.second.find(RRType::CNAME()));
+                            const FoundIterator
+                                nsi(found.second.find(RRType::NS()));
+                            const FoundIterator wti(found.second.find(type));
+                            if (cni != found.second.end() &&
+                                type != RRType::CNAME()) {
+                                result_rrset = cni->second;
+                                result_status = CNAME;
+                            } else if (nsi != found.second.end()) {
+                                result_rrset = nsi->second;
+                                result_status = DELEGATION;
+                            } else if (wti != found.second.end()) {
+                                result_rrset = wti->second;
+                            }
+
                             LOG_DEBUG(logger, DBG_TRACE_DETAILED,
                                       DATASRC_DATABASE_WILDCARD).
                                 arg(accessor_->getDBName()).arg(wildcard).

src/lib/datasrc/database.h

@@ -28,6 +28,9 @@
 #include <dns/name.h>
 #include <exceptions/exceptions.h>
 
+#include <map>
+#include <set>
+
 namespace isc {
 namespace datasrc {
 
@@ -609,54 +612,15 @@ public:
         boost::shared_ptr<DatabaseAccessor> accessor_;
         const int zone_id_;
         const isc::dns::Name origin_;
-
-        /**
-         * \brief Searches database for an RRset
-         *
-         * This method scans RRs of single domain specified by name and finds
-         * RRset with given type or any of redirection RRsets that are
-         * requested.
-         *
-         * This function is used internally by find(), because this part is
-         * called multiple times with slightly different parameters.
-         *
-         * \param name Which domain name should be scanned.
-         * \param type The RRType which is requested. This can be NULL, in
-         *     which case the method will look for the redirections only.
-         * \param want_cname If this is true, CNAME redirection may be returned
-         *     instead of the RRset with given type. If there's CNAME and
-         *     something else or the CNAME has multiple RRs, it throws
-         *     DataSourceError.
-         * \param want_dname If this is true, DNAME redirection may be returned
-         *     instead. This is with type = NULL only and is not checked in
-         *     other circumstances. If the DNAME has multiple RRs, it throws
-         *     DataSourceError.
-         * \param want_ns This allows redirection by NS to be returned. If
-         *     any other data is met as well, DataSourceError is thrown.
-         * \param construct_name If set to non-NULL, the resulting RRset will
-         *     be constructed for this name instead of the queried one. This
-         *     is useful for wildcards.
-         * \note It may happen that some of the above error conditions are not
-         *     detected in some circumstances. The goal here is not to validate
-         *     the domain in DB, but to avoid bad behaviour resulting from
-         *     broken data.
-         * \return First part of the result tells if the domain contains any
-         *     RRs. This can be used to decide between NXDOMAIN and NXRRSET.
-         *     The second part is the RRset found (if any) with any relevant
-         *     signatures attached to it.
-         * \todo This interface doesn't look very elegant. Any better idea
-         *     would be nice.
-         */
-        std::pair<bool, isc::dns::RRsetPtr> getRRset(const isc::dns::Name&
-                                                     name,
-                                                     const isc::dns::RRType*
-                                                     type,
-                                                     bool want_cname,
-                                                     bool want_dname,
-                                                     bool want_ns, const
-                                                     isc::dns::Name*
-                                                     construct_name = NULL);
-
+        //
+        /// \brief Shortcut name for the result of getRRsets
+        typedef std::pair<bool, std::map<dns::RRType, dns::RRsetPtr> >
+            FoundRRsets;
+        /// \brief Just shortcut for set of types
+        typedef std::set<dns::RRType> WantedTypes;
+        FoundRRsets getRRsets(const dns::Name& name, const WantedTypes& types,
+                              bool check_ns,
+                              const dns::Name* construct_name = NULL);
         /**
          * \brief Checks if something lives below this domain.
          *