Fix type conversions of values

This fixes 5 issues in regard to types of values:

1) Default values for params in the metadata were not converted properly,
most visibly on integer values being loaded as double values.

2) Param values coming in from jsonrpc were not converted properly.

3) The plugin might set state values with invalid types or being out of range.

4) If, for some reason (e.g. earlier versions of nymea, or a plugin setting
its own params in code with a wrong type), there was a param value with a
wrong type in the system, we stored that wrong type and restored it on loading
of plugin params while instead it really should be converted to the specified
type in the ParamType.

5) If a plugin is not loaded for a configured thing, the old code would
not initialize the states properly but upon shutdown save those invalid
values to the cache.

libnymea-core/integrations/thingmanagerimplementation.cpp

@@ -199,10 +199,7 @@ Thing::ThingError ThingManagerImplementation::setPluginConfig(const PluginId &pl
     settings.beginGroup(plugin->pluginId().toString());
 
     foreach (const Param &param, pluginConfig) {
-        settings.beginGroup(param.paramTypeId().toString());
+        settings.setValue(param.paramTypeId().toString(), param.value());
-        settings.setValue("type", static_cast<int>(param.value().type()));
-        settings.setValue("value", param.value());
-        settings.endGroup();
     }
 
     settings.endGroup();
@@ -1373,42 +1370,25 @@ void ThingManagerImplementation::loadPlugin(IntegrationPlugin *pluginIface, cons
     NymeaSettings settings(NymeaSettings::SettingsRolePlugins);
     settings.beginGroup("PluginConfig");
     ParamList params;
-    if (settings.childGroups().contains(pluginIface->pluginId().toString())) {
-        settings.beginGroup(pluginIface->pluginId().toString());
 
-        if (!settings.childGroups().isEmpty()) {
+    settings.beginGroup(pluginIface->pluginId().toString());
-            // Note: since nymea 0.12.2 the param type gets saved too for better data converting
+    foreach (const ParamType &paramType, pluginIface->configurationDescription()) {
-            foreach (const QString &paramTypeIdString, settings.childGroups()) {
+        QVariant value = paramType.defaultValue();
-                ParamTypeId paramTypeId(paramTypeIdString);
+        if (settings.contains(paramType.id().toString())) {
-                ParamType paramType = pluginIface->configurationDescription().findById(paramTypeId);
+            value = settings.value(paramType.id().toString());
-                if (!paramType.isValid()) {
+        } else if (settings.childGroups().contains(paramType.id().toString())) {
-                    qCWarning(dcThingManager()) << "Not loading Param for plugin" << pluginIface->pluginName() << "because the ParamType for the saved Param" << ParamTypeId(paramTypeIdString).toString() << "could not be found.";
+            // 0.12.2 - 0.22 used to store it in subgroups
-                    continue;
+            settings.beginGroup(paramType.id().toString());
-                }
+            value = settings.value("value");
-
+            settings.endGroup();
-                QVariant paramValue;
-                settings.beginGroup(paramTypeIdString);
-                paramValue = settings.value("value", paramType.defaultValue());
-                paramValue.convert(settings.value("type").toInt());
-                params.append(Param(paramTypeId, paramValue));
-                settings.endGroup();
-            }
-        } else {
-            // Note: < nymea 0.12.2
-            foreach (const QString &paramTypeIdString, settings.allKeys()) {
-                params.append(Param(ParamTypeId(paramTypeIdString), settings.value(paramTypeIdString)));
-            }
-        }
-
-        settings.endGroup();
-    } else if (!pluginIface->configurationDescription().isEmpty()){
-        // plugin requires config but none stored. Init with defaults
-        foreach (const ParamType &paramType, pluginIface->configurationDescription()) {
-            Param param(paramType.id(), paramType.defaultValue());
-            params.append(param);
         }
+        value.convert(paramType.type());
+        Param param(paramType.id(), value);
+        params.append(param);
     }
-    settings.endGroup();
+    settings.endGroup(); // pluginId
+
+    settings.endGroup(); // PluginConfig
 
     if (params.count() > 0) {
         Thing::ThingError status = pluginIface->setConfiguration(params);
@@ -1489,69 +1469,70 @@ void ThingManagerImplementation::loadConfiguredThings()
 
         ParamList params;
         settings.beginGroup("Params");
-        if (!settings.childGroups().isEmpty()) {
-            foreach (const QString &paramTypeIdString, settings.childGroups()) {
-                ParamTypeId paramTypeId(paramTypeIdString);
-                ParamType paramType = thingClass.paramTypes().findById(paramTypeId);
-                QVariant defaultValue;
-                if (!paramType.isValid()) {
-                    // NOTE: We're not skipping unknown parameters to give plugins a chance to still access old values if they change their config and migrate things over.
-                    qCWarning(dcThingManager()) << "Unknown param" << paramTypeIdString << "for" << thing << ". ParamType could not be found in device class.";
-                }
 
-                // Note: since nymea 0.12.2
-                QVariant paramValue;
-                settings.beginGroup(paramTypeIdString);
-                paramValue = settings.value("value", paramType.defaultValue());
-                paramValue.convert(settings.value("type").toInt());
-                params.append(Param(paramTypeId, paramValue));
-                settings.endGroup(); // ParamId
-            }
-        } else {
-            foreach (const QString &paramTypeIdString, settings.allKeys()) {
-                params.append(Param(ParamTypeId(paramTypeIdString), settings.value(paramTypeIdString)));
-            }
-        }
-        // Make sure all params are around. if they aren't initialize with default values
         foreach (const ParamType &paramType, thingClass.paramTypes()) {
-            if (!params.hasParam(paramType.id())) {
+            QVariant value = paramType.defaultValue();
-                params.append(Param(paramType.id(), paramType.defaultValue()));
+            if (settings.contains(paramType.id().toString())) {
+                value = settings.value(paramType.id().toString());
+            } else if (settings.childGroups().contains(paramType.id().toString())) {
+                // 0.12.2 - 0.22 used to store in subgroups
+                settings.beginGroup(paramType.id().toString());
+                value = settings.value("value");
+                settings.endGroup(); // paramTypeId
+            }
+            value.convert(paramType.type());
+            Param param(paramType.id(), value);
+            params.append(param);
+        }
+
+        // In order to give plugins a chance to migrate stuff stored in the params (to e.g. pluginStorage()) we'll load
+        // params that might have disappeared from the ParamTypes but still have stuff stored in the config
+        foreach (const QString paramTypeIdString, settings.childKeys()) {
+            ParamTypeId paramTypeId(paramTypeIdString);
+            if (!params.hasParam(paramTypeId)) {
+                qCDebug(dcThingManager()) << "Loading legacy param" << paramTypeIdString << "for thing" << thing->name();
+                Param param(paramTypeId, settings.value(paramTypeIdString));
+                params.append(param);
             }
         }
-        thing->setParams(params);
+        // 0.12.2 - 0.22 used to store in subgroups
+        foreach (const QString &paramTypeIdString, settings.childGroups()) {
+            settings.beginGroup(paramTypeIdString);
+            ParamTypeId paramTypeId(paramTypeIdString);
+            if (!params.hasParam(paramTypeId)) {
+                qCDebug(dcThingManager()) << "Loading legacy param" << paramTypeIdString << "for thing" << thing->name();
+                Param param(paramTypeId, settings.value("value"));
+                params.append(param);
+            }
+            settings.endGroup(); // paramTypeId
+        }
+
         settings.endGroup(); // Params
 
+        thing->setParams(params);
+
         ParamList thingSettings;
         settings.beginGroup("Settings");
-        if (!settings.childGroups().isEmpty()) {
-            foreach (const QString &paramTypeIdString, settings.childGroups()) {
-                ParamTypeId paramTypeId(paramTypeIdString);
-                ParamType paramType = thingClass.settingsTypes().findById(paramTypeId);
-                if (!paramType.isValid()) {
-                    qCWarning(dcThingManager()) << "Not loading Setting for thing" << thing << "because the ParamType for the saved Setting" << ParamTypeId(paramTypeIdString).toString() << "could not be found.";
-                    continue;
-                }
 
-                // Note: since nymea 0.12.2
+        foreach (const ParamType &paramType, thingClass.settingsTypes()) {
-                QVariant paramValue;
+            QVariant value = paramType.defaultValue();
-                settings.beginGroup(paramTypeIdString);
+            if (settings.contains(paramType.id().toString())) {
-                paramValue = settings.value("value", paramType.defaultValue());
+                value = settings.value(paramType.id().toString());
-                paramValue.convert(settings.value("type").toInt());
+            } else if (settings.childGroups().contains(paramType.id().toString())) {
-                thingSettings.append(Param(paramTypeId, paramValue));
+                // 0.12.2 - 0.22 used to store in subgroups
-                settings.endGroup(); // ParamId
+                settings.beginGroup(paramType.id().toString());
-            }
+                value = settings.value("value");
-        } else {
+                settings.endGroup(); // paramTypeId
-            foreach (const QString &paramTypeIdString, settings.allKeys()) {
-                params.append(Param(ParamTypeId(paramTypeIdString), settings.value(paramTypeIdString)));
             }
+            value.convert(paramType.type());
+            Param param(paramType.id(), value);
+            thingSettings.append(param);
         }
 
-        // Fill in any missing params with defaults
+        settings.endGroup(); // Settings
-        thingSettings = buildParams(thingClass.settingsTypes(), thingSettings);
 
         thing->setSettings(thingSettings);
 
-        settings.endGroup(); // Settings
         settings.endGroup(); // ThingId
 
         // We always add the thing to the list in this case. If it's in the stored things
@@ -1622,19 +1603,13 @@ void ThingManagerImplementation::storeConfiguredThings()
 
         settings.beginGroup("Params");
         foreach (const Param &param, thing->params()) {
-            settings.beginGroup(param.paramTypeId().toString());
+            settings.setValue(param.paramTypeId().toString(), param.value());
-            settings.setValue("type", static_cast<int>(param.value().type()));
-            settings.setValue("value", param.value());
-            settings.endGroup(); // ParamTypeId
         }
         settings.endGroup(); // Params
 
         settings.beginGroup("Settings");
         foreach (const Param &param, thing->settings()) {
-            settings.beginGroup(param.paramTypeId().toString());
+            settings.setValue(param.paramTypeId().toString(), param.value());
-            settings.setValue("type", static_cast<int>(param.value().type()));
-            settings.setValue("value", param.value());
-            settings.endGroup(); // ParamTypeId
         }
         settings.endGroup(); // Settings
 
@@ -1939,13 +1914,21 @@ ParamList ThingManagerImplementation::buildParams(const ParamTypes &types, const
     // Merge params from discovered descriptor and additional overrides provided on API call. User provided params have higher priority than discovery params.
     ParamList finalParams;
     foreach (const ParamType &paramType, types) {
+        QVariant value;
         if (first.hasParam(paramType.id())) {
-            finalParams.append(Param(paramType.id(), first.paramValue(paramType.id())));
+            value = first.paramValue(paramType.id());
         } else if (second.hasParam(paramType.id())) {
-            finalParams.append(Param(paramType.id(), second.paramValue(paramType.id())));
+            value = second.paramValue(paramType.id());
         } else if (paramType.defaultValue().isValid()){
-            finalParams.append(Param(paramType.id(), paramType.defaultValue()));
+            value = paramType.defaultValue();
         }
+        if (!value.isNull()) {
+            bool success = value.convert(paramType.type());
+            if (!success) {
+                qCWarning(dcThingManager()) << "Type mismatch in param" << paramType.name() << "for value" << value;
+            }
+        }
+        finalParams.append(Param(paramType.id(), value));
     }
     return finalParams;
 }
@@ -1998,13 +1981,6 @@ ThingSetupInfo* ThingManagerImplementation::setupThing(Thing *thing)
     ThingClass thingClass = findThingClass(thing->thingClassId());
     IntegrationPlugin *plugin = m_integrationPlugins.value(thingClass.pluginId());
 
-    if (!plugin) {
-        qCWarning(dcThingManager) << "Can't find a plugin for this thing" << thing;
-        ThingSetupInfo *info = new ThingSetupInfo(thing, this);
-        info->finish(Thing::ThingErrorPluginNotFound, tr("The plugin for this thing is not loaded."));
-        return info;
-    }
-
     QList<State> states;
     foreach (const StateType &stateType, thingClass.stateTypes()) {
         State state(stateType.id(), thing->id());
@@ -2017,8 +1993,14 @@ ThingSetupInfo* ThingManagerImplementation::setupThing(Thing *thing)
     connect(thing, &Thing::settingChanged, this, &ThingManagerImplementation::slotThingSettingChanged);
     connect(thing, &Thing::nameChanged, this, &ThingManagerImplementation::slotThingNameChanged);
 
-
     ThingSetupInfo *info = new ThingSetupInfo(thing, this, 30000);
+
+    if (!plugin) {
+        qCWarning(dcThingManager) << "Can't find a plugin for this thing" << thing;
+        info->finish(Thing::ThingErrorPluginNotFound, tr("The plugin for this thing is not loaded."));
+        return info;
+    }
+
     plugin->setupThing(info);
 
     return info;
@@ -2039,16 +2021,17 @@ void ThingManagerImplementation::loadThingStates(Thing *thing)
     ThingClass thingClass = m_supportedThings.value(thing->thingClassId());
     foreach (const StateType &stateType, thingClass.stateTypes()) {
         if (stateType.cached()) {
-            QVariant value;
+            QVariant value(stateType.defaultValue());
-            // First try to load new style
+
-            if (settings.childGroups().contains(stateType.id().toString())) {
+            if (settings.contains(stateType.id().toString())) {
+                value = settings.value(stateType.id().toString());
+            } else if (settings.childGroups().contains(stateType.id().toString())) {
+                // 0.9 - 0.22 used to store in a subgroup
                 settings.beginGroup(stateType.id().toString());
-                value = settings.value("value", stateType.defaultValue());
+                value = settings.value("value");
-                value.convert(settings.value("type").toInt());
                 settings.endGroup();
-            } else { // Try to fall back to the pre 0.9.0 way of storing states
-                value = settings.value(stateType.id().toString(), stateType.defaultValue());
             }
+            value.convert(stateType.type());
             thing->setStateValue(stateType.id(), value);
         } else {
             thing->setStateValue(stateType.id(), stateType.defaultValue());
@@ -2119,12 +2102,7 @@ void ThingManagerImplementation::storeThingStates(Thing *thing)
     settings.beginGroup(thing->id().toString());
     ThingClass thingClass = m_supportedThings.value(thing->thingClassId());
     foreach (const StateType &stateType, thingClass.stateTypes()) {
-        if (stateType.cached()) {
+        settings.setValue(stateType.id().toString(), thing->stateValue(stateType.id()));
-            settings.beginGroup(stateType.id().toString());
-            settings.setValue("type", static_cast<int>(thing->stateValue(stateType.id()).type()));
-            settings.setValue("value", thing->stateValue(stateType.id()));
-            settings.endGroup();
-        }
     }
     settings.endGroup();
 }

libnymea/integrations/pluginmetadata.cpp

@@ -359,17 +359,28 @@ void PluginMetadata::parse(const QJsonObject &jsonObject)
                     stateType.setUnit(unitVerification.second);
                 }
 
-                stateType.setDefaultValue(st.value("defaultValue").toVariant());
+                QVariant defaultValue = st.value("defaultValue").toVariant();
-                if (st.contains("minValue"))
+                defaultValue.convert(stateType.type());
-                    stateType.setMinValue(st.value("minValue").toVariant());
+                stateType.setDefaultValue(defaultValue);
 
-                if (st.contains("maxValue"))
+                if (st.contains("minValue")) {
-                    stateType.setMaxValue(st.value("maxValue").toVariant());
+                    QVariant minValue = st.value("minValue").toVariant();
+                    minValue.convert(stateType.type());
+                    stateType.setMinValue(minValue);
+                }
+
+                if (st.contains("maxValue")) {
+                    QVariant maxValue = st.value("maxValue").toVariant();
+                    maxValue.convert(stateType.type());
+                    stateType.setMaxValue(maxValue);
+                }
 
                 if (st.contains("possibleValues")) {
                     QVariantList possibleValues;
                     foreach (const QJsonValue &possibleValueJson, st.value("possibleValues").toArray()) {
-                        possibleValues.append(possibleValueJson.toVariant());
+                        QVariant possibleValue = possibleValueJson.toVariant();
+                        possibleValue.convert(stateType.type());
+                        possibleValues.append(possibleValue);
                     }
                     stateType.setPossibleValues(possibleValues);
 
@@ -824,7 +835,13 @@ QPair<bool, ParamTypes> PluginMetadata::parseParamTypes(const QJsonArray &array)
             m_validationErrors.append("Param type \"" + paramName + "\" has duplicate UUID: " + paramTypeId.toString());
             hasErrors = true;
         }
-        ParamType paramType(paramTypeId, paramName, t, pt.value("defaultValue").toVariant());
+        QVariant defaultValue = pt.value("defaultValue").toVariant();
+        if (!defaultValue.isNull()) {
+            // Only convert if there actually is a value as we want it to be null if it isn't specced
+            // explicitly and convert() would initialize it to the variant's default value
+            defaultValue.convert(t);
+        }
+        ParamType paramType(paramTypeId, paramName, t, defaultValue);
         paramType.setDisplayName(pt.value("displayName").toString());
 
 
@@ -861,7 +878,19 @@ QPair<bool, ParamTypes> PluginMetadata::parseParamTypes(const QJsonArray &array)
             paramType.setReadOnly(pt.value("readOnly").toBool());
 
         paramType.setAllowedValues(allowedValues);
-        paramType.setLimits(pt.value("minValue").toVariant(), pt.value("maxValue").toVariant());
+        QVariant minValue = pt.value("minValue").toVariant();
+        if (!minValue.isNull()) {
+            // Only convert if there actually is a value as we want it to be null if it isn't specced
+            // explicitly and convert() would initialize it to the variant's default value
+            minValue.convert(t);
+        }
+        QVariant maxValue = pt.value("maxValue").toVariant();
+        if (!maxValue.isNull()) {
+            // Only convert if there actually is a value as we want it to be null if it isn't specced
+            // explicitly and convert() would initialize it to the variant's default value
+            maxValue.convert(t);
+        }
+        paramType.setLimits(minValue, maxValue);
         paramType.setIndex(index++);
         paramTypes.append(paramType);
     }

libnymea/integrations/thing.cpp

@@ -327,23 +327,49 @@ QVariant Thing::stateValue(const StateTypeId &stateTypeId) const
 /*! For convenience, this finds the \l{State} matching the given \a stateTypeId in this thing and sets the current value to \a value. */
 void Thing::setStateValue(const StateTypeId &stateTypeId, const QVariant &value)
 {
+    StateType stateType = m_thingClass.stateTypes().findById(stateTypeId);
+    if (!stateType.isValid()) {
+        qCWarning(dcThing()) << "No such state type" << stateTypeId.toString() << "in" << m_name << "(" + thingClass().name() + ")";
+        return;
+    }
     for (int i = 0; i < m_states.count(); ++i) {
         if (m_states.at(i).stateTypeId() == stateTypeId) {
             if (m_states.at(i).value() == value)
                 return;
 
+            QVariant newValue = value;
+            if (!newValue.convert(stateType.type())) {
+                qCWarning(dcThing()).nospace() << m_name << ": Invalid value " << value << " for state " << stateType.name() << ". Type mismatch. Expected type: " << QVariant::typeToName(stateType.type()) << " (Discarding change)";
+                return;
+            }
+            if (stateType.minValue().isValid() && value < stateType.minValue()) {
+                qCWarning(dcThing()).nospace() << m_name << ": Invalid value " << value << " for state " << stateType.name() << ". Out of range: " << stateType.minValue() << " - " << stateType.maxValue() << " (Correcting to closest value within range)";
+                newValue = stateType.minValue();
+            }
+            if (stateType.maxValue().isValid() && value > stateType.maxValue()) {
+                qCWarning(dcThing()).nospace() << m_name << ": Invalid value " << value << " for state " << stateType.name() << ". Out of range: " << stateType.minValue() << " - " << stateType.maxValue() << " (Correcting to closest value within range)";
+                newValue = stateType.maxValue();
+            }
+            if (!stateType.possibleValues().isEmpty() && !stateType.possibleValues().contains(value)) {
+                qCWarning(dcThing()).nospace() << m_name << ": Invalid value " << value << " for state " << stateType.name() << ". Not an accepted value. Possible values: " << stateType.possibleValues() << " (Discarding change)";
+                return;
+            }
 
-            // TODO: check min/max value + possible values
+            QVariant oldValue = m_states.at(i).value();
-            //       to prevent an invalid state type from the plugin side
 
-            State newState(stateTypeId, m_id);
+            if (oldValue == newValue) {
-            newState.setValue(value);
+                qCDebug(dcThing()).nospace() << m_name << ": Discarding state change for " << stateType.name() << " as the value did not actually change. Old value:" << oldValue << "New value:" << newValue;
-            m_states[i] = newState;
+                return;
+            }
+
+            qCDebug(dcThing()).nospace() << m_name << ": State " << stateType.name() << " changed from " << oldValue << " to " << newValue;
+            m_states[i].setValue(newValue);
             emit stateValueChanged(stateTypeId, value);
             return;
         }
     }
-    qCWarning(dcThingManager) << "Failed setting state for" << m_name << value;
+    Q_ASSERT_X(false, m_name.toUtf8(), QString("Failed setting state %1 to %2").arg(stateType.name()).arg(value.toString()).toUtf8());
+    qCWarning(dcThing).nospace() << m_name << ": Failed setting state " << stateType.name() << "to" << value;
 }
 
 /*! Returns the \l{State} with the given \a stateTypeId of this thing. */

libnymea/types/statetype.cpp

@@ -222,6 +222,12 @@ QStringList StateType::mandatoryTypeProperties()
     return QStringList() << "id" << "name" << "displayName" << "displayNameEvent" << "type" << "defaultValue";
 }
 
+/*! Returns true if this state type has an ID, a type and a name set. */
+bool StateType::isValid() const
+{
+    return !m_id.isNull() && m_type != QVariant::Invalid && !m_name.isEmpty();
+}
+
 StateTypes::StateTypes(const QList<StateType> &other)
 {
     foreach (const StateType &st, other) {

libnymea/types/statetype.h

@@ -96,6 +96,8 @@ public:
     static QStringList typeProperties();
     static QStringList mandatoryTypeProperties();
 
+    bool isValid() const;
+
 private:
     StateTypeId m_id;
     QString m_name;

tests/auto/actions/testactions.cpp

@@ -85,7 +85,6 @@ void TestActions::executeAction()
     params.insert("deviceId", deviceId);
     params.insert("params", actionParams);
     QVariant response = injectAndWait("Actions.ExecuteAction", params);
-    qDebug() << "executeActionresponse" << response;
     verifyError(response, "deviceError", enumValueName(error));
 
     // Fetch action execution history from mock device