votdev
commented
3 months ago Do you have an example how to reproduce that? What about unit tests? Doing code changes in the database implementation might cause fatal side effects.
Closed ror3d closed 3 months ago
votdev
commented
3 months ago Do you have an example how to reproduce that? What about unit tests? Doing code changes in the database implementation might cause fatal side effects.
votdev
commented
3 months ago Please point me to the plugin and its datamodel we are talking about here. When designing the data model, certain things must be taken into account because the database implementation is limited. The conversion from XML to JSON is not very nice when it comes to lists. Sometimes you have to adapt the data model to the capabilities and conditions of the database implementation behind it. Not nice, but unfortunately that's the way it is.
ror3d
commented
3 months ago I have not yet uploaded the plugin, but basically the structure that gives this error is:
{
"type": "config",
"id": "conf.system.hddfanctrl",
"title": "FanCtrl",
"queryinfo": {
"xpath": "//system/hddfanctrl",
"iterable": false
},
"properties": {
"drives": {
"type": "array",
"items": {
"type": "object",
"properties": {
"uuid": {
"type": "string",
"format": "uuidv4"
},
"is_cooled": {
"type": "boolean"
}
}
}
}
}
}I add this to the config database by calling this from confdb/create.d
omv-confdbadm read --defaults "conf.system.hddfanctrl" | omv-confdbadm update "conf.system.hddfanctrl" -If I then do omv-confdbadm read "conf.system.hddfanctrl", I get the crash shown above.
ror3d
commented
3 months ago About tests, I first wanted to see if this was something that you felt would be an acceptable fix for this, if so I will prepare the tests and so on to ensure that this works as expected. Sorry for not specifying yesterday, it was a bit late for me and I missed that and the repro case.
votdev
commented
3 months ago Your datamodel is incorrect. The „drives“ is not necessary if you want to have an list of drives. Check https://github.com/openmediavault/openmediavault/blob/master/deb/openmediavault-apt/usr/share/openmediavault/datamodels/conf.system.apt.source.json and the RPC implementation of the openmediavault-apt plugin.
votdev
commented
3 months ago Not directly related to this, but when debugging the file I also noticed a possible issue, but not sure if it really is: _force_list_tags and _force_dict_tags seem to keep just the name of the tag that should be a list, not the path, so it might be that if a name is repeated in different places of the same datamodel, objects could be converted to lists inadvertently.
When designing datamodels, you should take care with the prop names, keep the limitations of the implementation in mind and use only specific keywords for lists. Some of them are hardcoded in the PHP code. If you respect this limitations, there shouldn't be any problems.
I know this is not the best solution, but it is historical and derived from version to version to keep backward compatibility. Someday the whole database implementation will be replaced by a key/value store or something else. But until then, plugin devs have to follow some rules to get it working.
votdev
commented
3 months ago This solution might break a different kind of use case though
This is a reason to not accept the PR because it introduces side-effects that might break existing code. As mentioned before, the current implementation has its peculiarities, but it has been working for >15 years.
ror3d
commented
3 months ago Ah, I see.
I had seen lists that work kinda like that, such as https://github.com/openmediavault/openmediavault/blob/master/deb/openmediavault-ftp/usr/share/openmediavault/datamodels/conf.service.ftp.json which lists https://github.com/openmediavault/openmediavault/blob/master/deb/openmediavault-ftp/usr/share/openmediavault/datamodels/conf.service.ftp.share.json
but the way it's defined with the share inside the shares and the duplication of the internal fields of the object in both files felt overly complex, so I thought the simpler version would work too.
The way I set it up did seem to work, although I didn't get to test it fully. I guess I will follow the existing implementations to avoid issues just in case.
I noticed that for a datamodel defining a list of objects, if the config database is queried for that datamodel when the list is empty, the program crashes with:
Because in the reader, None values are converted to empty string, and then that value is added into a list as if it wasn't actually an empty list but a list with an empty string inside.
This solution might break a different kind of use case though: if a list of strings is defined, then this would skip the first string if it's the empty string. I think that problem might be less common, but solving it would require checking the model schema for the type of the sub-elements and act according to that.
Not directly related to this, but when debugging the file I also noticed a possible issue, but not sure if it really is:
_force_list_tagsand_force_dict_tagsseem to keep just the name of the tag that should be a list, not the path, so it might be that if a name is repeated in different places of the same datamodel, objects could be converted to lists inadvertently.