From 85146abca57aad4dc2e805e97f0b314e11f627e8 Mon Sep 17 00:00:00 2001 From: Michael Zanetti Date: Mon, 29 Jun 2020 23:50:11 +0200 Subject: [PATCH 1/3] 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. --- .../thingmanagerimplementation.cpp | 212 ++++++++---------- libnymea/integrations/pluginmetadata.cpp | 45 +++- libnymea/integrations/thing.cpp | 38 +++- libnymea/types/statetype.cpp | 6 + libnymea/types/statetype.h | 2 + tests/auto/actions/testactions.cpp | 1 - 6 files changed, 172 insertions(+), 132 deletions(-) diff --git a/libnymea-core/integrations/thingmanagerimplementation.cpp b/libnymea-core/integrations/thingmanagerimplementation.cpp index 0c858a83..73813907 100644 --- a/libnymea-core/integrations/thingmanagerimplementation.cpp +++ b/libnymea-core/integrations/thingmanagerimplementation.cpp @@ -199,10 +199,7 @@ Thing::ThingError ThingManagerImplementation::setPluginConfig(const PluginId &pl settings.beginGroup(plugin->pluginId().toString()); foreach (const Param ¶m, pluginConfig) { - settings.beginGroup(param.paramTypeId().toString()); - settings.setValue("type", static_cast(param.value().type())); - settings.setValue("value", param.value()); - settings.endGroup(); + settings.setValue(param.paramTypeId().toString(), param.value()); } 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()) { - // Note: since nymea 0.12.2 the param type gets saved too for better data converting - foreach (const QString ¶mTypeIdString, settings.childGroups()) { - ParamTypeId paramTypeId(paramTypeIdString); - ParamType paramType = pluginIface->configurationDescription().findById(paramTypeId); - if (!paramType.isValid()) { - qCWarning(dcThingManager()) << "Not loading Param for plugin" << pluginIface->pluginName() << "because the ParamType for the saved Param" << ParamTypeId(paramTypeIdString).toString() << "could not be found."; - continue; - } - - 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 ¶mTypeIdString, 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 ¶mType, pluginIface->configurationDescription()) { - Param param(paramType.id(), paramType.defaultValue()); - params.append(param); + settings.beginGroup(pluginIface->pluginId().toString()); + foreach (const ParamType ¶mType, pluginIface->configurationDescription()) { + QVariant value = 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 it in subgroups + settings.beginGroup(paramType.id().toString()); + value = settings.value("value"); + settings.endGroup(); } + 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 ¶mTypeIdString, 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 ¶mTypeIdString, 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 ¶mType, thingClass.paramTypes()) { - if (!params.hasParam(paramType.id())) { - params.append(Param(paramType.id(), paramType.defaultValue())); + QVariant value = 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 ¶mTypeIdString, 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 ¶mTypeIdString, 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 - QVariant paramValue; - settings.beginGroup(paramTypeIdString); - paramValue = settings.value("value", paramType.defaultValue()); - paramValue.convert(settings.value("type").toInt()); - thingSettings.append(Param(paramTypeId, paramValue)); - settings.endGroup(); // ParamId - } - } else { - foreach (const QString ¶mTypeIdString, settings.allKeys()) { - params.append(Param(ParamTypeId(paramTypeIdString), settings.value(paramTypeIdString))); + foreach (const ParamType ¶mType, thingClass.settingsTypes()) { + QVariant value = 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); + thingSettings.append(param); } - // Fill in any missing params with defaults - thingSettings = buildParams(thingClass.settingsTypes(), thingSettings); + settings.endGroup(); // Settings 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 ¶m, thing->params()) { - settings.beginGroup(param.paramTypeId().toString()); - settings.setValue("type", static_cast(param.value().type())); - settings.setValue("value", param.value()); - settings.endGroup(); // ParamTypeId + settings.setValue(param.paramTypeId().toString(), param.value()); } settings.endGroup(); // Params settings.beginGroup("Settings"); foreach (const Param ¶m, thing->settings()) { - settings.beginGroup(param.paramTypeId().toString()); - settings.setValue("type", static_cast(param.value().type())); - settings.setValue("value", param.value()); - settings.endGroup(); // ParamTypeId + settings.setValue(param.paramTypeId().toString(), param.value()); } 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 ¶mType, 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 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; - // First try to load new style - if (settings.childGroups().contains(stateType.id().toString())) { + QVariant value(stateType.defaultValue()); + + 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.convert(settings.value("type").toInt()); + value = settings.value("value"); 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.beginGroup(stateType.id().toString()); - settings.setValue("type", static_cast(thing->stateValue(stateType.id()).type())); - settings.setValue("value", thing->stateValue(stateType.id())); - settings.endGroup(); - } + settings.setValue(stateType.id().toString(), thing->stateValue(stateType.id())); } settings.endGroup(); } diff --git a/libnymea/integrations/pluginmetadata.cpp b/libnymea/integrations/pluginmetadata.cpp index 524043e2..cd71154c 100644 --- a/libnymea/integrations/pluginmetadata.cpp +++ b/libnymea/integrations/pluginmetadata.cpp @@ -359,17 +359,28 @@ void PluginMetadata::parse(const QJsonObject &jsonObject) stateType.setUnit(unitVerification.second); } - stateType.setDefaultValue(st.value("defaultValue").toVariant()); - if (st.contains("minValue")) - stateType.setMinValue(st.value("minValue").toVariant()); + QVariant defaultValue = st.value("defaultValue").toVariant(); + defaultValue.convert(stateType.type()); + stateType.setDefaultValue(defaultValue); - if (st.contains("maxValue")) - stateType.setMaxValue(st.value("maxValue").toVariant()); + if (st.contains("minValue")) { + 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 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 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); } diff --git a/libnymea/integrations/thing.cpp b/libnymea/integrations/thing.cpp index 0ec62516..65f040b3 100644 --- a/libnymea/integrations/thing.cpp +++ b/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 - // to prevent an invalid state type from the plugin side + QVariant oldValue = m_states.at(i).value(); - State newState(stateTypeId, m_id); - newState.setValue(value); - m_states[i] = newState; + if (oldValue == newValue) { + qCDebug(dcThing()).nospace() << m_name << ": Discarding state change for " << stateType.name() << " as the value did not actually change. Old value:" << oldValue << "New value:" << newValue; + 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. */ diff --git a/libnymea/types/statetype.cpp b/libnymea/types/statetype.cpp index ab505a90..2550380b 100644 --- a/libnymea/types/statetype.cpp +++ b/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 &other) { foreach (const StateType &st, other) { diff --git a/libnymea/types/statetype.h b/libnymea/types/statetype.h index 0bfdd6ee..f87e91eb 100644 --- a/libnymea/types/statetype.h +++ b/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; diff --git a/tests/auto/actions/testactions.cpp b/tests/auto/actions/testactions.cpp index dc028e7b..2cc576fb 100644 --- a/tests/auto/actions/testactions.cpp +++ b/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 From 5002fad6d9d856fc0c4074a6c6501763efbc1575 Mon Sep 17 00:00:00 2001 From: Michael Zanetti Date: Sun, 26 Jul 2020 00:51:56 +0200 Subject: [PATCH 2/3] fix tests --- .../thingmanagerimplementation.cpp | 30 +++++++++---------- libnymea/integrations/thingutils.cpp | 5 ++-- tests/auto/devices/testdevices.cpp | 4 +-- 3 files changed, 19 insertions(+), 20 deletions(-) diff --git a/libnymea-core/integrations/thingmanagerimplementation.cpp b/libnymea-core/integrations/thingmanagerimplementation.cpp index 73813907..2de8e413 100644 --- a/libnymea-core/integrations/thingmanagerimplementation.cpp +++ b/libnymea-core/integrations/thingmanagerimplementation.cpp @@ -185,10 +185,10 @@ Thing::ThingError ThingManagerImplementation::setPluginConfig(const PluginId &pl return Thing::ThingErrorPluginNotFound; } - ParamList params = buildParams(plugin->configurationDescription(), pluginConfig); - Thing::ThingError verify = ThingUtils::verifyParams(plugin->configurationDescription(), params); + Thing::ThingError verify = ThingUtils::verifyParams(plugin->configurationDescription(), pluginConfig); if (verify != Thing::ThingErrorNoError) return verify; + ParamList params = buildParams(plugin->configurationDescription(), pluginConfig); Thing::ThingError result = plugin->setConfiguration(params); if (result != Thing::ThingErrorNoError) @@ -256,14 +256,14 @@ ThingDiscoveryInfo* ThingManagerImplementation::discoverThings(const ThingClassI return discoveryInfo; } - ParamList effectiveParams = buildParams(thingClass.discoveryParamTypes(), params); - Thing::ThingError result = ThingUtils::verifyParams(thingClass.discoveryParamTypes(), effectiveParams); + Thing::ThingError result = ThingUtils::verifyParams(thingClass.discoveryParamTypes(), params); if (result != Thing::ThingErrorNoError) { qCWarning(dcThingManager) << "Thing discovery failed. Parameter verification failed."; ThingDiscoveryInfo *discoveryInfo = new ThingDiscoveryInfo(thingClassId, params, this); discoveryInfo->finish(result); return discoveryInfo; } + ParamList effectiveParams = buildParams(thingClass.discoveryParamTypes(), params); ThingDiscoveryInfo *discoveryInfo = new ThingDiscoveryInfo(thingClassId, effectiveParams, this, 30000); connect(discoveryInfo, &ThingDiscoveryInfo::finished, this, [this, discoveryInfo](){ @@ -393,14 +393,14 @@ ThingSetupInfo *ThingManagerImplementation::reconfigureThingInternal(Thing *thin return info; } - ParamList finalParams = buildParams(thing->thingClass().paramTypes(), params); - Thing::ThingError result = ThingUtils::verifyParams(thing->thingClass().paramTypes(), finalParams); + Thing::ThingError result = ThingUtils::verifyParams(thing->thingClass().paramTypes(), params); if (result != Thing::ThingErrorNoError) { qCWarning(dcThingManager()) << "Cannot reconfigure thing. Params failed validation."; ThingSetupInfo *info = new ThingSetupInfo(nullptr, this); info->finish(result); return info; } + ParamList finalParams = buildParams(thing->thingClass().paramTypes(), params); // first remove the thing in the plugin plugin->thingRemoved(thing); @@ -460,13 +460,13 @@ Thing::ThingError ThingManagerImplementation::setThingSettings(const ThingId &th qCWarning(dcThingManager()) << "Cannot set thing settings. Thing" << thingId.toString() << "not found"; return Thing::ThingErrorThingNotFound; } - // build a list of settings using: a) provided new settings b) previous settings and c) default values - ParamList effectiveSettings = buildParams(thing->thingClass().settingsTypes(), settings, thing->settings()); - Thing::ThingError status = ThingUtils::verifyParams(thing->thingClass().settingsTypes(), effectiveSettings); + Thing::ThingError status = ThingUtils::verifyParams(thing->thingClass().settingsTypes(), settings); if (status != Thing::ThingErrorNoError) { qCWarning(dcThingManager()) << "Error setting thing settings for" << thing->name() << thing->id().toString(); return status; } + // build a list of settings using: a) provided new settings b) previous settings and c) default values + ParamList effectiveSettings = buildParams(thing->thingClass().settingsTypes(), settings, thing->settings()); thing->setSettings(effectiveSettings); return Thing::ThingErrorNoError; } @@ -689,8 +689,7 @@ ThingSetupInfo* ThingManagerImplementation::addConfiguredThingInternal(const Thi } // set params - ParamList effectiveParams = buildParams(thingClass.paramTypes(), params); - Thing::ThingError paramsResult = ThingUtils::verifyParams(thingClass.paramTypes(), effectiveParams); + Thing::ThingError paramsResult = ThingUtils::verifyParams(thingClass.paramTypes(), params); if (paramsResult != Thing::ThingErrorNoError) { qCWarning(dcThingManager()) << "Cannot add thing. Parameter verification failed."; ThingSetupInfo *info = new ThingSetupInfo(nullptr, this); @@ -698,6 +697,7 @@ ThingSetupInfo* ThingManagerImplementation::addConfiguredThingInternal(const Thi return info; } + ParamList effectiveParams = buildParams(thingClass.paramTypes(), params); Thing *thing = new Thing(plugin->pluginId(), thingClass, thingId, this); thing->setParentId(parentId); if (name.isEmpty()) { @@ -1206,14 +1206,14 @@ ThingActionInfo *ThingManagerImplementation::executeAction(const Action &action) return info; } - ParamList finalParams = buildParams(actionType.paramTypes(), action.params()); - Thing::ThingError paramCheck = ThingUtils::verifyParams(actionType.paramTypes(), finalParams); + Thing::ThingError paramCheck = ThingUtils::verifyParams(actionType.paramTypes(), action.params()); if (paramCheck != Thing::ThingErrorNoError) { qCWarning(dcThingManager()) << "Cannot execute action. Parameter verification failed."; ThingActionInfo *info = new ThingActionInfo(thing, action, this); info->finish(paramCheck); return info; } + ParamList finalParams = buildParams(actionType.paramTypes(), action.params()); finalAction.setParams(finalParams); ThingActionInfo *info = new ThingActionInfo(thing, finalAction, this, 30000); @@ -1909,9 +1909,9 @@ void ThingManagerImplementation::slotThingNameChanged() emit thingChanged(thing); } +// Merges params from first and second. First has higher priority than second. If neither are given, the default is used - if any ParamList ThingManagerImplementation::buildParams(const ParamTypes &types, const ParamList &first, const ParamList &second) { - // 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 ¶mType, types) { QVariant value; @@ -1927,8 +1927,8 @@ ParamList ThingManagerImplementation::buildParams(const ParamTypes &types, const if (!success) { qCWarning(dcThingManager()) << "Type mismatch in param" << paramType.name() << "for value" << value; } + finalParams.append(Param(paramType.id(), value)); } - finalParams.append(Param(paramType.id(), value)); } return finalParams; } diff --git a/libnymea/integrations/thingutils.cpp b/libnymea/integrations/thingutils.cpp index 22522203..8a1c97bb 100644 --- a/libnymea/integrations/thingutils.cpp +++ b/libnymea/integrations/thingutils.cpp @@ -41,8 +41,7 @@ ThingUtils::ThingUtils() } -/*! Verify if the given \a params matches the given \a paramTypes. Ith \a requireAll - * is true, all \l{ParamList}{Params} has to be valid. Returns \l{Device::DeviceError} to inform about the result.*/ +/*! Verify if the given \a params matches the given \a paramTypes.*/ Thing::ThingError ThingUtils::verifyParams(const QList paramTypes, const ParamList ¶ms) { foreach (const Param ¶m, params) { @@ -52,7 +51,7 @@ Thing::ThingError ThingUtils::verifyParams(const QList paramTypes, co } } foreach (const ParamType ¶mType, paramTypes) { - bool found = false; + bool found = !paramType.defaultValue().isNull(); foreach (const Param ¶m, params) { if (paramType.id() == param.paramTypeId()) { found = true; diff --git a/tests/auto/devices/testdevices.cpp b/tests/auto/devices/testdevices.cpp index 998aed0e..bc2f5d4c 100644 --- a/tests/auto/devices/testdevices.cpp +++ b/tests/auto/devices/testdevices.cpp @@ -382,10 +382,10 @@ void TestDevices::addConfiguredDevice_data() QTest::newRow("User, JustAdd, wrong param") << mockThingClassId << invalidDeviceParams << true << Device::DeviceErrorInvalidParameter; deviceParams.clear(); deviceParams << httpportParam << fakeparam; - QTest::newRow("USer, JustAdd, additional invalid param") << mockThingClassId << deviceParams << false << Device::DeviceErrorNoError; + QTest::newRow("User, JustAdd, additional invalid param") << mockThingClassId << deviceParams << false << Device::DeviceErrorInvalidParameter; deviceParams.clear(); deviceParams << httpportParam << fakeparam2; - QTest::newRow("USer, JustAdd, additional param, valid but unused") << mockThingClassId << deviceParams << true << Device::DeviceErrorNoError; + QTest::newRow("User, JustAdd, duplicate param") << mockThingClassId << deviceParams << true << Device::DeviceErrorInvalidParameter; } From 399f406d103af3dc03476466cc2e1521d6e00b86 Mon Sep 17 00:00:00 2001 From: Michael Zanetti Date: Mon, 27 Jul 2020 14:36:44 +0200 Subject: [PATCH 3/3] fixes --- .../integrations/thingmanagerimplementation.cpp | 6 +++--- libnymea/types/statedescriptor.cpp | 6 +++--- plugins/mock/integrationpluginmock.json | 2 +- tests/auto/integrations/testintegrations.cpp | 4 ++-- tests/auto/rules/testrules.cpp | 13 +++++++------ tests/testlib/nymeatestbase.h | 2 +- 6 files changed, 17 insertions(+), 16 deletions(-) diff --git a/libnymea-core/integrations/thingmanagerimplementation.cpp b/libnymea-core/integrations/thingmanagerimplementation.cpp index 2de8e413..3c5ca71d 100644 --- a/libnymea-core/integrations/thingmanagerimplementation.cpp +++ b/libnymea-core/integrations/thingmanagerimplementation.cpp @@ -1697,19 +1697,19 @@ void ThingManagerImplementation::onAutoThingDisappeared(const ThingId &thingId) Thing *thing = m_configuredThings.value(thingId); if (!thing) { - qWarning(dcThingManager) << "Received an autoThingDisappeared signal but this thing is unknown:" << thingId; + qCWarning(dcThingManager) << "Received an autoThingDisappeared signal but this thing is unknown:" << thingId; return; } ThingClass thingClass = m_supportedThings.value(thing->thingClassId()); if (thingClass.pluginId() != plugin->pluginId()) { - qWarning(dcThingManager) << "Received a autoThingDisappeared signal but emitting plugin does not own the thing"; + qCWarning(dcThingManager) << "Received a autoThingDisappeared signal but emitting plugin does not own the thing"; return; } if (!thing->autoCreated()) { - qWarning(dcThingManager) << "Received an autoThingDisappeared signal but thing creationMethod is not CreateMothodAuto"; + qCWarning(dcThingManager) << "Received an autoThingDisappeared signal but thing creationMethod is not CreateMothodAuto"; return; } diff --git a/libnymea/types/statedescriptor.cpp b/libnymea/types/statedescriptor.cpp index 22e0de29..2dc1e84b 100644 --- a/libnymea/types/statedescriptor.cpp +++ b/libnymea/types/statedescriptor.cpp @@ -171,11 +171,11 @@ bool StateDescriptor::operator ==(const State &state) const if ((m_stateTypeId != state.stateTypeId()) || (m_thingId != state.thingId())) { return false; } - if (!m_stateValue.canConvert(state.value().type())) { + QVariant convertedValue = m_stateValue; + bool res = convertedValue.convert(state.value().type()); + if (!res) { return false; } - QVariant convertedValue = m_stateValue; - convertedValue.convert(state.value().type()); switch (m_operatorType) { case Types::ValueOperatorEquals: return convertedValue == state.value(); diff --git a/plugins/mock/integrationpluginmock.json b/plugins/mock/integrationpluginmock.json index 35d4fdd6..14953bec 100644 --- a/plugins/mock/integrationpluginmock.json +++ b/plugins/mock/integrationpluginmock.json @@ -98,7 +98,7 @@ "name": "double", "displayName": "Dummy double state", "displayNameEvent": "Dummy double state changed", - "type": "int", + "type": "double", "minValue": 0, "maxValue": 100, "defaultValue": 2.7 diff --git a/tests/auto/integrations/testintegrations.cpp b/tests/auto/integrations/testintegrations.cpp index 1eced86d..b3c6e373 100644 --- a/tests/auto/integrations/testintegrations.cpp +++ b/tests/auto/integrations/testintegrations.cpp @@ -382,10 +382,10 @@ void TestIntegrations::addThing_data() QTest::newRow("User, JustAdd, wrong param") << mockThingClassId << invalidThingParams << true << Thing::ThingErrorInvalidParameter; thingParams.clear(); thingParams << httpportParam << fakeparam; - QTest::newRow("USer, JustAdd, additional invalid param") << mockThingClassId << thingParams << false << Thing::ThingErrorNoError; + QTest::newRow("USer, JustAdd, additional invalid param") << mockThingClassId << thingParams << false << Thing::ThingErrorInvalidParameter; thingParams.clear(); thingParams << httpportParam << fakeparam2; - QTest::newRow("USer, JustAdd, additional param, valid but unused") << mockThingClassId << thingParams << true << Thing::ThingErrorNoError; + QTest::newRow("USer, JustAdd, duplicate param") << mockThingClassId << thingParams << true << Thing::ThingErrorInvalidParameter; } diff --git a/tests/auto/rules/testrules.cpp b/tests/auto/rules/testrules.cpp index 0f5c986a..df196e67 100644 --- a/tests/auto/rules/testrules.cpp +++ b/tests/auto/rules/testrules.cpp @@ -265,13 +265,14 @@ void TestRules::setWritableStateValue(const ThingId &thingId, const StateTypeId QVariantList stateChangedVariants = checkNotifications(stateSpy, "Integrations.StateChanged"); QVERIFY2(stateChangedVariants.count() == 1, "Did not get Integrations.StateChanged notification."); + qCDebug(dcTests()) << "Notification content:" << qUtf8Printable(QJsonDocument::fromVariant(stateChangedVariants).toJson()); QVariantMap notification = stateChangedVariants.first().toMap().value("params").toMap(); QVERIFY2(notification.contains("thingId"), "Integrations.StateChanged notification does not contain thingId"); QVERIFY2(ThingId(notification.value("thingId").toString()) == thingId, "Integrations.StateChanged notification does not contain the correct thingId"); QVERIFY2(notification.contains("stateTypeId"), "Integrations.StateChanged notification does not contain stateTypeId"); QVERIFY2(StateTypeId(notification.value("stateTypeId").toString()) == stateTypeId, "Integrations.StateChanged notification does not contain the correct stateTypeId"); QVERIFY2(notification.contains("value"), "Integrations.StateChanged notification does not contain new state value"); - QVERIFY2(notification.value("value") == QVariant(value), "Integrations.StateChanged notification does not contain the new value"); + QVERIFY2(notification.value("value") == QVariant(value), QString("Integrations.StateChanged notification does not contain the new value. Got: %1, Expected: %2").arg(notification.value("value").toString()).arg(QVariant(value).toString()).toUtf8()); } } @@ -1990,11 +1991,11 @@ void TestRules::testChildEvaluator_data() QTest::addColumn("trigger"); QTest::addColumn("active"); - QTest::newRow("Unchanged | 2 | 2.5 | String value 1 | #FF0000") << testThingId << ruleMap << 2 << 2.5 << "String value 1" << "#FF0000" << false << false; - QTest::newRow("Unchanged | 60 | 2.5 | String value 2 | #FF0000") << testThingId << ruleMap << 60 << 2.5 << "String value 2" << "#FF0000" << false << false; - QTest::newRow("Unchanged | 60 | 20.5 | String value 2 | #FF0000") << testThingId << ruleMap << 60 << 20.5 << "String value 2" << "#FF0000" << false << false; - QTest::newRow("Active | 60 | 20.5 | String value 2 | #00FF00") << testThingId << ruleMap << 60 << 20.5 << "String value 2" << "#00FF00" << true << true; - QTest::newRow("Active | 60 | 20.5 | String value 2 | #00FF00") << testThingId << ruleMap << 60 << 20.5 << "String value 2" << "#00FF00" << true << true; + QTest::newRow("Unchanged | 2 | 2.5 | String value 1 | #FF0000") << testThingId << ruleMap << 2 << 2.5 << "String value 1" << "#ff0000" << false << false; + QTest::newRow("Unchanged | 60 | 2.5 | String value 2 | #FF0000") << testThingId << ruleMap << 60 << 2.5 << "String value 2" << "#ff0000" << false << false; + QTest::newRow("Unchanged | 60 | 20.5 | String value 2 | #FF0000") << testThingId << ruleMap << 60 << 20.5 << "String value 2" << "#ff0000" << false << false; + QTest::newRow("Active | 60 | 20.5 | String value 2 | #00FF00") << testThingId << ruleMap << 60 << 20.5 << "String value 2" << "#00ff00" << true << true; + QTest::newRow("Active | 60 | 20.5 | String value 2 | #00FF00") << testThingId << ruleMap << 60 << 20.5 << "String value 2" << "#00ff00" << true << true; } void TestRules::testChildEvaluator() diff --git a/tests/testlib/nymeatestbase.h b/tests/testlib/nymeatestbase.h index ef98d6d1..dc84f738 100644 --- a/tests/testlib/nymeatestbase.h +++ b/tests/testlib/nymeatestbase.h @@ -118,7 +118,7 @@ protected: // just for debugging inline void printJson(const QVariant &response) { QJsonDocument jsonDoc = QJsonDocument::fromVariant(response); - qDebug() << jsonDoc.toJson(); + qCDebug(dcTests()) << jsonDoc.toJson(); } void waitForDBSync();