Merge pull request eclipse-iceoryx#917 from ApexAI/iox-#415-fix-revie…

…w-findings

iox-eclipse-iceoryx#415 Rename variable and test case name in `ServiceRegistry_test` and fix bug in `ServiceDescription` c'tor

CHANGELOG.md

@@ -157,6 +157,25 @@ if(reader.isOpen()) {
 static auto& logger = createLogger("", "", iox::log::LogManager::GetLogManager().DefaultLogLevel());
 ```
 
+Instead of using a constructor a `ServiceDescription` is now deserialized via a
+static method with error handling:
+
+```cpp
+// before
+iox::cxx::Serialization serializedObj;
+iox::capro::ServiceDescription service(serializedObj);
+
+// after
+iox::cxx::Serialization serialisedObj;
+capro::ServiceDescription::deserialize(serialisedObj)
+    .and_then([](auto& value){
+        // Do something with the deserialized object
+    })
+    .or_else([](auto& error){
+        // Handle the error
+    });
+```
+
 The service-related methods have been moved from `PoshRuntime` to a separate class (TBD):
 
 ```cpp

CONTRIBUTING.md

@@ -126,6 +126,9 @@ information about logging and error handling.
   * Public members of structs and classes do not have the `m_` prefix
 * Namespaces in `lower_snake_case` : `my_namespace`
 * Aliases have a `_t` postfix : `using FooString_t = iox::cxx::string<100>;`
+* Objects created from a method returning a `cxx::optional<Foo>` shall be named `maybeFoo`
+* Objects created from a method returning a `cxx::expected<Foo, FooError>` shall
+contain the name `result` e.g. `getChunkResult` for the method `getChunk()`
 
 ### Doxygen
 

iceoryx_posh/include/iceoryx_posh/capro/service_description.hpp

@@ -90,9 +90,6 @@ class ServiceDescription
         uint32_t data[CLASS_HASH_ELEMENT_COUNT];
     };
 
-    /// @brief construction of the capro service description using serialized strings
-    ServiceDescription(const cxx::Serialization& serial) noexcept;
-
     /// @brief default C'tor
     ServiceDescription() noexcept;
     ServiceDescription(const ServiceDescription&) noexcept = default;
@@ -123,12 +120,18 @@ class ServiceDescription
     /// @brief serialization of the capro description.
     operator cxx::Serialization() const noexcept;
 
+    /// @brief de-serialization of a ServiceDescription.
+    /// @param[in] serialized, Serialization object from which the ServiceDescription shall be created
+    /// @return cxx::expected that either has a ServiceDescription or cxx::Serialization::Error stored inside
+    static cxx::expected<ServiceDescription, cxx::Serialization::Error>
+    deserialize(const cxx::Serialization& serialized) noexcept;
+
     // @brief Returns if this service description is used for an RouDi-internal channel
     bool isInternal() const noexcept;
     // @brief Set this service description to be is used for an RouDi-internal channel
     void setInternal() noexcept;
     /// @brief Returns the scope of a ServiceDescription
-    Scope getScope() noexcept;
+    Scope getScope() const noexcept;
 
     /// @brief Returns true for valid ServiceDescription
     /// false for ServiceDescription that contain InvalidStrings.

iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp

@@ -55,10 +55,9 @@ class ServiceRegistry
     /// @return ServiceRegistryError, error wrapped in cxx::expected
     cxx::expected<Error> add(const capro::ServiceDescription& serviceDescription) noexcept;
 
-    /// @brief Removes given service description from registry
+    /// @brief Removes given service description from registry if service is found
     /// @param[in] serviceDescription, service to be removed
-    /// @return true, if service description was removed, false otherwise
-    bool remove(const capro::ServiceDescription& serviceDescription) noexcept;
+    void remove(const capro::ServiceDescription& serviceDescription) noexcept;
 
     /// @brief Removes given service description from registry
     /// @param[in] searchResult, reference to the vector which will be filled with the results

iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp

@@ -47,6 +47,7 @@ class NodeData;
 enum class FindServiceError
 {
     INVALID_STATE,
+    DESERIALIZATION_FAILED,
     UNABLE_TO_WRITE_TO_ROUDI_CHANNEL, /// @todo #415 remove as IPC channel won't be used
     INSTANCE_CONTAINER_OVERFLOW       /// @todo #415 set container to iox::MAX_NUMBER_OF_SERVICES and remove error
 };

iceoryx_posh/source/capro/service_description.cpp

@@ -69,37 +69,6 @@ bool ServiceDescription::ClassHash::operator!=(const ClassHash& rhs) const noexc
     return !operator==(rhs);
 }
 
-ServiceDescription::ServiceDescription(const cxx::Serialization& serial) noexcept
-{
-    std::underlying_type<Scope>::type scope = 0;
-    std::underlying_type<Interfaces>::type interfaceSource = 0;
-    serial.extract(m_serviceString,
-                   m_instanceString,
-                   m_eventString,
-                   m_classHash[0u],
-                   m_classHash[1u],
-                   m_classHash[2u],
-                   m_classHash[3u],
-                   scope,
-                   interfaceSource);
-    if (scope > static_cast<std::underlying_type<Scope>::type>(Scope::INVALID))
-    {
-        m_scope = Scope::INVALID;
-    }
-    else
-    {
-        m_scope = static_cast<Scope>(scope);
-    }
-    if (interfaceSource > static_cast<std::underlying_type<Interfaces>::type>(Interfaces::INTERFACE_END))
-    {
-        m_interfaceSource = Interfaces::INTERFACE_END;
-    }
-    else
-    {
-        m_interfaceSource = static_cast<Interfaces>(interfaceSource);
-    }
-}
-
 ServiceDescription::ServiceDescription() noexcept
     : ServiceDescription(InvalidIdString, InvalidIdString, InvalidIdString)
 {
@@ -181,6 +150,38 @@ ServiceDescription::operator cxx::Serialization() const noexcept
                                       interface);
 }
 
+cxx::expected<ServiceDescription, cxx::Serialization::Error>
+ServiceDescription::deserialize(const cxx::Serialization& serialized) noexcept
+{
+    ServiceDescription deserializedObject;
+
+    using ScopeUnderlyingType = std::underlying_type<Scope>::type;
+    using InterfaceUnderlyingType = std::underlying_type<Interfaces>::type;
+
+    ScopeUnderlyingType scope{0};
+    InterfaceUnderlyingType interfaceSource{0};
+
+    auto deserializationSuccessful = serialized.extract(deserializedObject.m_serviceString,
+                                                        deserializedObject.m_instanceString,
+                                                        deserializedObject.m_eventString,
+                                                        deserializedObject.m_classHash[0u],
+                                                        deserializedObject.m_classHash[1u],
+                                                        deserializedObject.m_classHash[2u],
+                                                        deserializedObject.m_classHash[3u],
+                                                        scope,
+                                                        interfaceSource);
+    if (!deserializationSuccessful || scope >= static_cast<ScopeUnderlyingType>(Scope::INVALID)
+        || interfaceSource >= static_cast<InterfaceUnderlyingType>(Interfaces::INTERFACE_END))
+    {
+        return cxx::error<cxx::Serialization::Error>(cxx::Serialization::Error::DESERIALIZATION_FAILED);
+    }
+
+    deserializedObject.m_scope = static_cast<Scope>(scope);
+    deserializedObject.m_interfaceSource = static_cast<Interfaces>(interfaceSource);
+
+    return cxx::success<ServiceDescription>(deserializedObject);
+}
+
 IdString_t ServiceDescription::getServiceIDString() const noexcept
 {
     return m_serviceString;
@@ -206,7 +207,7 @@ void ServiceDescription::setInternal() noexcept
     m_scope = Scope::INTERNAL;
 }
 
-Scope ServiceDescription::getScope() noexcept
+Scope ServiceDescription::getScope() const noexcept
 {
     return m_scope;
 }

iceoryx_posh/source/popo/client_options.cpp

@@ -46,29 +46,15 @@ ClientOptions::deserialize(const cxx::Serialization& serialized) noexcept
                                                         responseQueueFullPolicy,
                                                         serverTooSlowPolicy);
 
-    if (!deserializationSuccessful)
+    if (!deserializationSuccessful
+        || responseQueueFullPolicy > static_cast<QueueFullPolicyUT>(QueueFullPolicy2::DISCARD_OLDEST_DATA)
+        || serverTooSlowPolicy > static_cast<ConsumerTooSlowPolicyUT>(ConsumerTooSlowPolicy::DISCARD_OLDEST_DATA))
     {
         return cxx::error<cxx::Serialization::Error>(cxx::Serialization::Error::DESERIALIZATION_FAILED);
     }
 
-    if (responseQueueFullPolicy > static_cast<QueueFullPolicyUT>(QueueFullPolicy2::DISCARD_OLDEST_DATA))
-    {
-        return cxx::error<cxx::Serialization::Error>(cxx::Serialization::Error::DESERIALIZATION_FAILED);
-    }
-    else
-    {
-        clientOptions.responseQueueFullPolicy = static_cast<QueueFullPolicy2>(responseQueueFullPolicy);
-    }
-
-    if (serverTooSlowPolicy > static_cast<ConsumerTooSlowPolicyUT>(ConsumerTooSlowPolicy::DISCARD_OLDEST_DATA))
-    {
-        return cxx::error<cxx::Serialization::Error>(cxx::Serialization::Error::DESERIALIZATION_FAILED);
-    }
-    else
-    {
-        clientOptions.serverTooSlowPolicy = static_cast<ConsumerTooSlowPolicy>(serverTooSlowPolicy);
-    }
-
+    clientOptions.responseQueueFullPolicy = static_cast<QueueFullPolicy2>(responseQueueFullPolicy);
+    clientOptions.serverTooSlowPolicy = static_cast<ConsumerTooSlowPolicy>(serverTooSlowPolicy);
     return cxx::success<ClientOptions>(clientOptions);
 }
 } // namespace popo

iceoryx_posh/source/roudi/roudi.cpp

@@ -236,7 +236,16 @@ void RouDi::processMessage(const runtime::IpcMessage& message,
         }
         else
         {
-            capro::ServiceDescription service(cxx::Serialization(message.getElementAtIndex(2)));
+            auto deserializationResult =
+                capro::ServiceDescription::deserialize(cxx::Serialization(message.getElementAtIndex(2)));
+            if (deserializationResult.has_error())
+            {
+                LogError() << "Deserialization failed when '" << message.getElementAtIndex(2).c_str()
+                           << "' was provided\n";
+                break;
+            }
+            const auto& service = deserializationResult.value();
+
             cxx::Serialization portConfigInfoSerialization(message.getElementAtIndex(7));
 
             if (!service.isValid())
@@ -288,7 +297,16 @@ void RouDi::processMessage(const runtime::IpcMessage& message,
         }
         else
         {
-            capro::ServiceDescription service(cxx::Serialization(message.getElementAtIndex(2)));
+            auto deserializationResult =
+                capro::ServiceDescription::deserialize(cxx::Serialization(message.getElementAtIndex(2)));
+            if (deserializationResult.has_error())
+            {
+                LogError() << "Deserialization failed when '" << message.getElementAtIndex(2).c_str()
+                           << "' was provided\n";
+                break;
+            }
+
+            const auto& service = deserializationResult.value();
             cxx::Serialization portConfigInfoSerialization(message.getElementAtIndex(8));
 
             if (!service.isValid())

iceoryx_posh/source/roudi/service_registry.cpp

@@ -48,7 +48,7 @@ cxx::expected<ServiceRegistry::Error> ServiceRegistry::add(const capro::ServiceD
     return cxx::success<>();
 }
 
-bool ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription) noexcept
+void ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription) noexcept
 {
     bool removedElement{false};
     uint64_t index = 0U;
@@ -66,7 +66,7 @@ bool ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription
             }
             else
             {
-                return true;
+                return;
             }
             // There can't be more than one element
             break;
@@ -97,8 +97,6 @@ bool ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription
         removeIndexFromMap(m_serviceMap, index);
         removeIndexFromMap(m_instanceMap, index);
     }
-
-    return removedElement;
 }
 
 void ServiceRegistry::find(ServiceDescriptionVector_t& searchResult,

iceoryx_posh/source/runtime/posh_runtime_impl.cpp

@@ -419,8 +419,13 @@ PoshRuntimeImpl::findService(const cxx::variant<Wildcard_t, capro::IdString_t> s
     uint32_t numberOfServices = algorithm::min(capacity, numberOfElements);
     for (uint32_t i = 0U; i < numberOfServices; ++i)
     {
-        capro::ServiceDescription service(cxx::Serialization(requestResponse.getElementAtIndex(i)));
-        serviceContainer.push_back(service);
+        auto deserializationResult =
+            capro::ServiceDescription::deserialize(cxx::Serialization(requestResponse.getElementAtIndex(i)));
+        if (deserializationResult.has_error())
+        {
+            return cxx::error<FindServiceError>(FindServiceError::DESERIALIZATION_FAILED);
+        }
+        serviceContainer.push_back(deserializationResult.value());
     }
 
     if (numberOfElements > capacity)

iceoryx_posh/test/moduletests/test_capro_service.cpp

@@ -148,23 +148,29 @@ TEST_F(ServiceDescription_test, ServiceDescriptionSerializationCreatesServiceDes
                                                      static_cast<uint16_t>(testScope),
                                                      static_cast<uint16_t>(testInterfaceSource));
 
-    ServiceDescription serviceDescription1 = ServiceDescription(serialObj);
-
-    EXPECT_THAT(serviceDescription1.getServiceIDString(), Eq(testService));
-    EXPECT_THAT(serviceDescription1.getInstanceIDString(), Eq(testInstance));
-    EXPECT_THAT(serviceDescription1.getEventIDString(), Eq(testEvent));
-    EXPECT_THAT((serviceDescription1.getClassHash())[0], Eq(testHash[0]));
-    EXPECT_THAT((serviceDescription1.getClassHash())[1], Eq(testHash[1]));
-    EXPECT_THAT((serviceDescription1.getClassHash())[2], Eq(testHash[2]));
-    EXPECT_THAT((serviceDescription1.getClassHash())[3], Eq(testHash[3]));
-    EXPECT_THAT(serviceDescription1.getScope(), Eq(Scope::INTERNAL));
-    EXPECT_THAT(serviceDescription1.getSourceInterface(), Eq(Interfaces::INTERNAL));
+
+    ServiceDescription::deserialize(serialObj)
+        .and_then([&](const auto& service) {
+            EXPECT_THAT(service.getServiceIDString(), Eq(testService));
+            EXPECT_THAT(service.getInstanceIDString(), Eq(testInstance));
+            EXPECT_THAT(service.getEventIDString(), Eq(testEvent));
+            EXPECT_THAT((service.getClassHash())[0], Eq(testHash[0]));
+            EXPECT_THAT((service.getClassHash())[1], Eq(testHash[1]));
+            EXPECT_THAT((service.getClassHash())[2], Eq(testHash[2]));
+            EXPECT_THAT((service.getClassHash())[3], Eq(testHash[3]));
+            EXPECT_THAT(service.getScope(), Eq(Scope::INTERNAL));
+            EXPECT_THAT(service.getSourceInterface(), Eq(Interfaces::INTERNAL));
+        })
+        .or_else([](const auto& error) {
+            FAIL() << "Deserialization should not fail but failed with: " << static_cast<uint32_t>(error);
+        });
 }
 
 /// @attention The purpose of the Serialization is not to be an alternative Constructor. It is intended to send/receive
 /// the ServiceDescription over communication protocols which transfers strings like the MessageQueue. The testcase is
 /// only intended to check the functionality by injecting the valus directly.
-TEST_F(ServiceDescription_test, ServiceDescriptionObjectInitialisationWithOutOfBoundaryScopeSetsTheScopeToInvalid)
+TEST_F(ServiceDescription_test,
+       ServiceDescriptionObjectInitialisationWithOutOfBoundaryScopeLeadsToInvalidDeserialization)
 {
     ServiceDescription::ClassHash testHash = {14U, 28U, 42U, 56U};
     testService = "Service";
@@ -180,16 +186,17 @@ TEST_F(ServiceDescription_test, ServiceDescriptionObjectInitialisationWithOutOfB
                                                      testHash[3],
                                                      invalidScope);
 
-    ServiceDescription serviceDescription1 = ServiceDescription(serialObj);
+    auto deserializationResult = ServiceDescription::deserialize(serialObj);
 
-    EXPECT_THAT(serviceDescription1.getScope(), Eq(Scope::INVALID));
+    ASSERT_TRUE(deserializationResult.has_error());
+    EXPECT_THAT(deserializationResult.get_error(), Eq(iox::cxx::Serialization::Error::DESERIALIZATION_FAILED));
 }
 
 /// @attention The purpose of the Serialization is not to be an alternative Constructor. It is intended to send/receive
 /// the ServiceDescription over communication protocols which transfers strings like the MessageQueue. The testcase is
 /// only intended to check the functionality by injecting the valus directly.
 TEST_F(ServiceDescription_test,
-       ServiceDescriptionObjectInitialisationWithOutOfBoundaryInterfaceSourceSetsTheInterfaceSourceToInterfaceEnd)
+       ServiceDescriptionObjectInitialisationWithOutOfBoundaryInterfaceSourceLeadsToInvalidDeserialization)
 {
     ServiceDescription::ClassHash testHash = {17U, 34U, 51U, 68U};
     testService = "Service";
@@ -207,12 +214,23 @@ TEST_F(ServiceDescription_test,
                                                      static_cast<uint16_t>(testScope),
                                                      invalidInterfaceSource);
 
-    ServiceDescription serviceDescription1 = ServiceDescription(serialObj);
+    auto deserializationResult = ServiceDescription::deserialize(serialObj);
+
+    ASSERT_TRUE(deserializationResult.has_error());
+    EXPECT_THAT(deserializationResult.get_error(), Eq(iox::cxx::Serialization::Error::DESERIALIZATION_FAILED));
+}
+
+TEST_F(ServiceDescription_test, ServiceDescriptionObjectInitialisationWithEmptyStringLeadsToInvalidDeserialization)
+{
+    std::string emptyString;
+    iox::cxx::Serialization invalidSerialObj{emptyString};
+
+    auto deserializationResult = ServiceDescription::deserialize(invalidSerialObj);
 
-    EXPECT_THAT(serviceDescription1.getSourceInterface(), Eq(Interfaces::INTERFACE_END));
+    ASSERT_TRUE(deserializationResult.has_error());
+    EXPECT_THAT(deserializationResult.get_error(), Eq(iox::cxx::Serialization::Error::DESERIALIZATION_FAILED));
 }
 
-/// @todo remove
 TEST_F(ServiceDescription_test, ServiceDescriptionDefaultCtorInitializesStringsToInvalidString)
 {
     ServiceDescription serviceDescription1 = ServiceDescription();
@@ -433,7 +451,6 @@ TEST_F(ServiceDescription_test, LessThanOperatorReturnsFalseIfEventStringOfFirst
     EXPECT_FALSE(serviceDescription1 < serviceDescription2);
 }
 
-/// @todo add new tests for service description?
 
 /// END SERVICEDESCRIPTION TESTS