Apply setting subscriber's partition to empty set (eProsima#2108)

* Refs 12317: Apply setting subscriber's partition to empty set

Signed-off-by: Eduardo Ponz Segrelles <eduardoponz@eprosima.com>

* Refs 12317: Test for checking that partition QoS are updated with Subscriber::set_qos

Signed-off-by: Eduardo Ponz Segrelles <eduardoponz@eprosima.com>

* Refs 12317: Blackbox test for clearing a non-empty set of partitions in subscriber

Signed-off-by: Eduardo Ponz Segrelles <eduardoponz@eprosima.com>

* Refs 12317: Clarify doxygen

Signed-off-by: Eduardo Ponz Segrelles <eduardoponz@eprosima.com>

* Refs 12317: Address Windows warning

Signed-off-by: Eduardo Ponz Segrelles <eduardoponz@eprosima.com>
Signed-off-by: Samuel Wilhelmsson <samuel@halodi.com>

include/fastdds/dds/core/policy/QosPolicies.hpp

@@ -1459,7 +1459,7 @@ class PartitionQosPolicy : public Parameter_t, public QosPolicy
     }
 
     /**
-     * @brief Setter for the maximum size
+     * @brief Setter for the maximum size reserved for partitions (in bytes)
      * @param size Size to be set
      */
     void set_max_size (
@@ -1470,7 +1470,7 @@ class PartitionQosPolicy : public Parameter_t, public QosPolicy
     }
 
     /**
-     * @brief Getter for the maximum size
+     * @brief Getter for the maximum size (in bytes)
      * @return uint32_t with the maximum size
      */
     uint32_t max_size () const

src/cpp/fastdds/subscriber/SubscriberImpl.cpp

@@ -593,7 +593,7 @@ void SubscriberImpl::set_qos(
         to.presentation() = from.presentation();
         to.presentation().hasChanged = true;
     }
-    if (from.partition().names().size() > 0)
+    if (!(to.partition() == from.partition()))
     {
         to.partition() = from.partition();
         to.partition().hasChanged = true;

src/cpp/fastdds/subscriber/qos/ReaderQos.cpp

@@ -82,7 +82,7 @@ void ReaderQos::setQos(
         m_presentation = qos.m_presentation;
         m_presentation.hasChanged = true;
     }
-    if (qos.m_partition.names().size() > 0)
+    if (qos.m_partition.names() != m_partition.names())
     {
         m_partition = qos.m_partition;
         m_partition.hasChanged = true;

test/blackbox/api/dds-pim/PubSubReader.hpp

@@ -1333,6 +1333,12 @@ class PubSubReader
         return (ReturnCode_t::RETCODE_OK == subscriber_->set_qos(subscriber_qos_));
     }
 
+    bool clear_partitions()
+    {
+        subscriber_qos_.partition().clear();
+        return (ReturnCode_t::RETCODE_OK == subscriber_->set_qos(subscriber_qos_));
+    }
+
     /*** Function for discovery callback ***/
 
     void wait_discovery_result()

test/blackbox/api/fastrtps_deprecated/PubSubReader.hpp

@@ -1102,6 +1102,12 @@ class PubSubReader
         return subscriber_->updateAttributes(subscriber_attr_);
     }
 
+    bool clear_partitions()
+    {
+        subscriber_attr_.qos.m_partition.clear();
+        return subscriber_->updateAttributes(subscriber_attr_);
+    }
+
     /*** Function for discovery callback ***/
 
     void wait_discovery_result()

test/blackbox/common/BlackboxTestsDiscovery.cpp

@@ -632,7 +632,8 @@ TEST(Discovery, LocalInitialPeers)
     reader.block_for_all();
 }
 
-// Test created to check bug #2010 (Github #90)
+// Test created to check bug #2010 (Github #90).
+// It also checks https://github.com/eProsima/Fast-DDS/issues/2107
 TEST_P(Discovery, PubSubAsReliableHelloworldPartitions)
 {
     PubSubReader<HelloWorldType> reader(TEST_TOPIC_NAME);
@@ -665,11 +666,26 @@ TEST_P(Discovery, PubSubAsReliableHelloworldPartitions)
     // Block reader until reception finished or timeout.
     reader.block_for_all();
 
+    // Change reader to different partition to check un-matching
     ASSERT_TRUE(reader.update_partition("OtherPartition"));
 
     reader.wait_writer_undiscovery();
     writer.wait_reader_undiscovery();
 
+    // Reset partition and wait for discovery to check that emptying the list triggers un-matching.
+    // This is to check Github #2107
+    ASSERT_TRUE(reader.update_partition("PartitionTests"));
+
+    writer.wait_discovery();
+    reader.wait_discovery();
+
+    ASSERT_TRUE(reader.clear_partitions());
+
+    reader.wait_writer_undiscovery();
+    writer.wait_reader_undiscovery();
+
+    // Set reader and writer in compatible partitions
+    ASSERT_TRUE(reader.update_partition("OtherPartition"));
     ASSERT_TRUE(writer.update_partition("OtherPart*"));
 
     writer.wait_discovery();

test/unittest/dds/subscriber/SubscriberTests.cpp

@@ -522,6 +522,74 @@ TEST(SubscriberTests, UnsupportedPublisherMethods)
     ASSERT_EQ(DomainParticipantFactory::get_instance()->delete_participant(participant), ReturnCode_t::RETCODE_OK);
 }
 
+/**
+ * This test checks that changing the PartitionQosPolicy on a subscriber takes effect on changing the actual QoS.
+ * It was discovered in https://github.com/eProsima/Fast-DDS/issues/2107 that this was not correctly handled when
+ * setting an empty partitions list on a subscriber that already had some partitions. The test does the following:
+ *
+ *    1. Create a subscriber with default QoS
+ *    2. Add a partition
+ *    3. Add three more partitions
+ *    4. Remove 1 partition
+ *    5. Remove 2 more partition
+ *    6. Remove all partitions
+ */
+TEST(SubscriberTests, UpdatePartitions)
+{
+    DomainParticipant* participant =
+            DomainParticipantFactory::get_instance()->create_participant(0, PARTICIPANT_QOS_DEFAULT);
+    ASSERT_NE(participant, nullptr);
+    Subscriber* subscriber = participant->create_subscriber(SUBSCRIBER_QOS_DEFAULT);
+    ASSERT_NE(subscriber, nullptr);
+    ASSERT_EQ(subscriber->get_qos().partition().size(), 0u);
+
+    // Add 1 partition to subscriber
+    SubscriberQos sub_qos;
+    PartitionQosPolicy partitions;
+    partitions.push_back("partition_1");
+    sub_qos.partition() = partitions;
+    subscriber->set_qos(sub_qos);
+    ASSERT_EQ(subscriber->get_qos().partition().size(), 1u);
+    ASSERT_EQ(partitions, subscriber->get_qos().partition());
+
+    // Add 3 more partitions to subscriber
+    partitions.push_back("partition_2");
+    partitions.push_back("partition_3");
+    partitions.push_back("partition_4");
+    sub_qos.partition() = partitions;
+    subscriber->set_qos(sub_qos);
+    ASSERT_EQ(subscriber->get_qos().partition().size(), 4u);
+    ASSERT_EQ(partitions, subscriber->get_qos().partition());
+
+    // Remove 1 partition from subscriber
+    partitions.clear();
+    ASSERT_TRUE(static_cast<bool>(partitions.empty()));
+    partitions.push_back("partition_1");
+    partitions.push_back("partition_2");
+    partitions.push_back("partition_3");
+    sub_qos.partition() = partitions;
+    subscriber->set_qos(sub_qos);
+    ASSERT_EQ(subscriber->get_qos().partition().size(), 3u);
+    ASSERT_EQ(partitions, subscriber->get_qos().partition());
+
+    // Remove 2 more partitions from the subscriber
+    partitions.clear();
+    ASSERT_TRUE(partitions.empty());
+    partitions.push_back("partition_1");
+    sub_qos.partition() = partitions;
+    subscriber->set_qos(sub_qos);
+    ASSERT_EQ(subscriber->get_qos().partition().size(), 1u);
+    ASSERT_EQ(partitions, subscriber->get_qos().partition());
+
+    // Remove all partitions from the subscriber
+    partitions.clear();
+    ASSERT_TRUE(partitions.empty());
+    sub_qos.partition() = partitions;
+    subscriber->set_qos(sub_qos);
+    ASSERT_EQ(subscriber->get_qos().partition().size(), 0u);
+    ASSERT_EQ(partitions, subscriber->get_qos().partition());
+}
+
 } // namespace dds
 } // namespace fastdds
 } // namespace eprosima