[Discovery] Fix bug in PTR expiry

This patch fixes a bug where PTR expire updates with a different TTL
from the current record are not considered to be updates, leaving the
now expired record in the graph and returning an error.

A new equality operator, "IsUpdateOf", is added to MdnsRecord to allow
for checking this specific case.

Bug: b/168800070
Change-Id: I5c5d4372d3ded791d102cb95cc37fdf5e944d404
Reviewed-on: https://chromium-review.googlesource.com/c/openscreen/+/2785583
Commit-Queue: Jordan Bayles <jophba@chromium.org>
Reviewed-by: Ryan Keane <rwkeane@google.com>

4 files changed, 29 insertions, 4 deletions

diff --git a/discovery/dnssd/impl/dns_data_graph.cc b/discovery/dnssd/impl/dns_data_graph.cc
index 9e6f166e..079dbf7d 100644
--- a/discovery/dnssd/impl/dns_data_graph.cc
+++ b/discovery/dnssd/impl/dns_data_graph.cc
@@ -252,7 +252,10 @@ Error DnsDataGraphImpl::Node::ApplyDataRecordChange(MdnsRecord record,
 
   if (record.dns_type() == DnsType::kPTR) {
     child_name = absl::get<PtrRecordRdata>(record.rdata()).ptr_domain();
-    it = std::find(records_.begin(), records_.end(), record);
+    it = std::find_if(records_.begin(), records_.end(),
+                      [record](const MdnsRecord& rhs) {
+                        return record.IsReannouncementOf(rhs);
+                      });
   } else {
     if (record.dns_type() == DnsType::kSRV) {
       child_name = absl::get<SrvRecordRdata>(record.rdata()).target();
diff --git a/discovery/dnssd/impl/dns_data_graph_unittest.cc b/discovery/dnssd/impl/dns_data_graph_unittest.cc
index 0af3aa19..a32b942c 100644
--- a/discovery/dnssd/impl/dns_data_graph_unittest.cc
+++ b/discovery/dnssd/impl/dns_data_graph_unittest.cc
@@ -236,6 +236,21 @@ TEST_F(DnsDataGraphTests, DeleteFailsForNonExistingRecord) {
   EXPECT_EQ(graph_->GetTrackedDomainCount(), size_t{2});
 }
 
+TEST_F(DnsDataGraphTests, DeleteSucceedsForRecordWithDifferentTtl) {
+  const MdnsRecord ptr = GetFakePtrRecord(primary_domain_);
+  TriggerRecordCreationWithCallback(ptr, primary_domain_);
+
+  EXPECT_CALL(callbacks_, OnStopTracking(primary_domain_));
+  const MdnsRecord new_ptr(ptr.name(), ptr.dns_type(), ptr.dns_class(),
+                           ptr.record_type(),
+                           ptr.ttl() + std::chrono::seconds(10), ptr.rdata());
+  const auto result =
+      ApplyDataRecordChange(new_ptr, RecordChangedEvent::kExpired);
+
+  EXPECT_TRUE(result.ok());
+  EXPECT_EQ(graph_->GetTrackedDomainCount(), size_t{1});
+}
+
 TEST_F(DnsDataGraphTests, UpdateEndpointsWorksAsExpected) {
   auto ptr = GetFakePtrRecord(primary_domain_);
   auto srv = GetFakeSrvRecord(primary_domain_, secondary_domain_);
diff --git a/discovery/mdns/mdns_records.cc b/discovery/mdns/mdns_records.cc
index 4768e2e4..1b7b441b 100644
--- a/discovery/mdns/mdns_records.cc
+++ b/discovery/mdns/mdns_records.cc
@@ -606,9 +606,7 @@ bool MdnsRecord::IsValidConfig(const DomainName& name,
 }
 
 bool MdnsRecord::operator==(const MdnsRecord& rhs) const {
-  return dns_type_ == rhs.dns_type_ && dns_class_ == rhs.dns_class_ &&
-         record_type_ == rhs.record_type_ && ttl_ == rhs.ttl_ &&
-         name_ == rhs.name_ && rdata_ == rhs.rdata_;
+  return IsReannouncementOf(rhs) && ttl_ == rhs.ttl_;
 }
 
 bool MdnsRecord::operator!=(const MdnsRecord& rhs) const {
@@ -654,6 +652,12 @@ bool MdnsRecord::operator>=(const MdnsRecord& rhs) const {
   return !(*this < rhs);
 }
 
+bool MdnsRecord::IsReannouncementOf(const MdnsRecord& rhs) const {
+  return dns_type_ == rhs.dns_type_ && dns_class_ == rhs.dns_class_ &&
+         record_type_ == rhs.record_type_ && name_ == rhs.name_ &&
+         rdata_ == rhs.rdata_;
+}
+
 size_t MdnsRecord::MaxWireSize() const {
   auto wire_size_visitor = [](auto&& arg) { return arg.MaxWireSize(); };
   // NAME size, 2-byte TYPE, 2-byte CLASS, 4-byte TTL, RDATA size
diff --git a/discovery/mdns/mdns_records.h b/discovery/mdns/mdns_records.h
index de37932c..ce1be4f6 100644
--- a/discovery/mdns/mdns_records.h
+++ b/discovery/mdns/mdns_records.h
@@ -475,6 +475,9 @@ class MdnsRecord {
   bool operator<=(const MdnsRecord& other) const;
   bool operator>=(const MdnsRecord& other) const;
 
+  // While not being "equivalent", a record could be said to be an update of
+  // a different record if it is the same, excepting TTL.
+  bool IsReannouncementOf(const MdnsRecord& other) const;
   size_t MaxWireSize() const;
   const DomainName& name() const { return name_; }
   DnsType dns_type() const { return dns_type_; }