smarthomeNG / plugins

Plugins for SmartHomeNG - The device integration platform for your smart home
https://www.smarthomeNG.de
45 stars 99 forks source link

Plugin 'database' from section 'database' exception: could not convert string to float #317

Closed freget closed 4 years ago

freget commented 4 years ago

The following database plugin configuration (as documented in https://www.smarthomeng.de/user/plugins/database/README.html)

database:
  class_name: Database
  class_path: plugins.database
  driver: sqlite3
  connect:
    - database: /home/smarthome/log.db
    - check_same_thread: 0

yields the following error at startup:

ERROR    lib.plugin          Plugin 'database' from section 'database' exception: could not convert string to float: 'database: /home/smarthome/log.db'
Traceback (most recent call last):
  File "/usr/local/smarthome/lib/plugin.py", line 144, in __init__
    plugin_thread = PluginWrapper(smarthome, plugin, classname, classpath, args, instance, self.meta)
  File "/usr/local/smarthome/lib/plugin.py", line 594, in __init__
    (plugin_params, params_ok, hide_params) = self.meta.check_parameters(args)
  File "/usr/local/smarthome/lib/metadata.py", line 929, in check_parameters
    value = self._expand_listvalues(param, value)
  File "/usr/local/smarthome/lib/metadata.py", line 516, in _expand_listvalues
    result = Utils.string_to_list(value)
  File "/usr/local/smarthome/lib/utils.py", line 465, in string_to_list
    er=float(er)
ValueError: could not convert string to float: 'database: /home/smarthome/log.db'

Affected version Most recent develop branch at the time of writing this issue: Main: Branch develop, last commit 537948d12cfa340796a8048042d5743a6daa5834 Plugins: Branch develop, last commit a1f4bbe9d15bf6e88b485571b3b769a55d1125bd Python: v3.7.3 final

freget commented 4 years ago

It works with

database:
  class_name: Database
  class_path: plugins.database
  driver: sqlite3
  connect:
    - "database: /home/smarthome/log.db"
    - "check_same_thread: 0"

We should either change the documentation or (probably better) allow proper yaml parameters.

msinn commented 4 years ago

It works if your type it like it is written in the documentation (without spaces after the colons in the option stings)

Those Option Strings are not YAML and have to be formed the way the chosen database (mysql, sqlite3 or others) system describes them.

freget commented 4 years ago

I really dislike code that is space sensitive.

My suggestion would be to change the documentation to the version with explicit quotes. This is much clearer to read and is not affected by a missing space or not. Otherwise, at least give the hint that the strings are space sensitive.

ohinckel commented 4 years ago

I just updated the code (lib/db.py to support OrderedDict and plugins/database/__init.py__) to support the standard YAML configuration (documentation also update) in develop branch (of smarthome and plugins).

With the new version you're now able to configure the parameters (the old way, like the way mentioned above, and the new way) using YAML style:

database:
  ...
  connect:
    database: /home/smarthome/log.db
    check_same_thread: 0
freget commented 4 years ago

Cool! Thank you!

psilo909 commented 4 years ago

my config is now crashing: 2020-01-05 11:30:51 ERROR lib.metadata plugin 'database': Invalid definition in metadata file 'plugins/database/plugin.yaml': type 'dict(str)' for parameter 'connect' -> u sing type 'foo' instead

database:
    plugin_name: database
    driver: pymysql
    connect:
    -   host:127.0.0.1
    -   user:root
    -   passwd:xxx
    -   port:3307
    -   db:smarthome
    instance: mysqldb
psilo909 commented 4 years ago

now tried to convert the config, which made it only worse:

2020-01-05 11:43:26 ERROR lib.metadata plugin 'database': Invalid definition in metadata file 'plugins/database/plugin.yaml': type 'dict(str)' for parameter 'connect' -> u sing type 'foo' instead
2020-01-05 11:43:26 ERROR root Database [Database]: Could not connect to the database: %d format: a number is required, not str
2020-01-05 11:43:26 ERROR plugins.database mysqldb@: Database: Initialization failed: %d format: a number is required, not str

database:
    plugin_name: database
    driver: pymysql
    connect:
        host: 127.0.0.1
        user: root
        passwd: xxx
        port: 3307
        db: smarthome
    instance: mysqldb
bmxp commented 4 years ago

@ohinckel Maybe you should still accept the old fashion parameters and give a deprecated warning when they are used. Otherwise this could be a breaking change for a lot of users that can't cope with it and it might well increase the requests for support in knxuf significantly...

psilo909 commented 4 years ago

i just switched the "dict(str)" to "dict" in plugin.yaml, as the documentation does not tell about dict() https://www.smarthomeng.de/developer/metadata/parameters.html. but now i am getting connection errors to localhost, which was working before?

psilo909 commented 4 years ago

i think one problem is, that the Port needs to be a number and not a str. so dict(str) is wrong:

ERROR root Database [Database]: Could not connect to the database: %d format: a number is required, not str <-- tried to specify it with port: '3307'

ohinckel commented 4 years ago

I change the plugin to keep the old parameter connect as is and introduced a new one named database. When using the old parameter a warning is logged to let the user know to update the configuration.

The new parameter database is not working currently, since I want to use dict as parameter type. But the metadata library issues an error since the test on the correct type only checks for a dict and not (additionally) for an OrderedDict which is created by the YAML parse - I guess.

Is there something what I'm doing wrong here? Are there other examples using dict as parameter type?

psilo909 commented 4 years ago

I change the plugin to keep the old parameter connect as is

so why did you change to to a dict in plugin.yaml then?

connect:
        type: list(str)  -->   type: dict(str)
ohinckel commented 4 years ago

I though it was working with both configuration setup - but I was wrong. In the meantime I change it back again to list(str) for the connect parameter.

But I want to use type dict for the new database parameter (which is the same, but not a list of strings instead it's a dict). Currently it doesn't work becase I get an error when using the database parameter with type dict or dict(foo) and configuration like

database:
  ...
  database:
    host: somehost
    user: username
    passwd: password
    db: databasename
    port: 3306

What type should be used in plugin.yaml to support such a structure?

psilo909 commented 4 years ago

@msinn told me on gitter, that multi layered configurations are not supported. so this could be a general problem for your approach.

where i succeeded was putting the values in '...' and setting a dict(str). but then the port makes problems when doing the database connection

msinn commented 4 years ago

I think structuring connection strings in a dict ist not a good idea. It puts the burden off building a valid connection string on the database plugin. Connection strings can be quite tricky (depending on the database system) and we don't know all the options someone might want to use to connect to his database.

ohinckel commented 4 years ago

I think structuring connection strings in a dict ist not a good idea.

At least it's more flexible: you only have to list the settings there if you want to use them which it's a generic approach, too.

I don't like the previous implementation because it has more "magic" implemented in extracting the connection parameter names and values. This could be just handled by the YAML structure itself.

I also don't like configuring one connection string (e.g. a DSN including parameters). We need to know how this is parsed (and maybe which parameters are supported by which driver implementation).

The plugin is based on the DB-API2 specification which is describing a connect() method which will get (currently) simple parameters (string, int, etc.) to setup a connection:

As a guideline the connection constructor parameters should be implemented as keyword parameters for more intuitive use and follow this order of parameters:

So mapping these simple keyword parameters dict makes sense to me.

msinn commented 4 years ago

At least it's more flexible: you only have to list the settings there if you want to use them which it's a generic approach, too.

It might work von sqlite and MySQL but I know of connection strings for other database systems (e.g. for Oracle DB) that won't fit in this dict structure very well.

If DB-API2 keeps track of this, it is fine with me. I was not aware, that DB-API2 does the magic to make a connection string that the database system understands.

ohinckel commented 4 years ago

I also know connection strings, but the DB-API specifies a connect() method which will have parameters. For the moment it doesn't matter if the parameter is a connection string (like a full DSN including parameters) or if they are specified as different parameters. But still both are supported when using a dict structure:

database:
  ...
  database:
    host: somehost
    user: someuser
    ...

vs.

database:
  ...
  database:
    dsn: somehost:port/dbname?param1=value&param2=other

The arguments depends on the implementation of connect() method of the driver. As long as only simple types are used as parameters (like str for a DSN string or str and int for host and port argument) it will work.

Further it is not very different from the current implementation: it also does only support these simple types (only the format is different: - host:name or for DSN - dsn:somedsn), but is an own implementation instead of relying on the YAML parser. I would prefer to use the YAML library to parse this instead of using an own implementation.

But currently the second level structure seems not to be supported. Will this be changed in the future? If not we need to stick to the current implementation.

But I also saw types using list(dict(str)), which looks like a nested structure (like in avm plugin, but it's in plugin function parameters not in common plugin parameters - seems to behave differently in handing the type attribute).

msinn commented 4 years ago

The metadata for Plugin Parameters (and Item Attributes) at the moment don't do type checking on more complex data structures. I am not sure (code was written some time ago): It might work if you specify dict instead of dict(str).

dict(str) is not supported and would not be very clear:

Just using dict and knowing there is no data type checking when loading the parameters would leave room to read in dicts and do the type checking in the Plugin.

ohinckel commented 4 years ago

Just using dict and knowing there is no data type checking when loading the parameters would leave room to read in dicts and do the type checking in the Plugin.

I would vote to support this. So we can use a YAML structure for different stuff too and the plugin has to decide how to deal with the data.

Alternatively we could implement top-level items prefixed with "connect_" to indicate that these values are connection parameters. But we can not declare such fields in plugin.yaml - so maybe not the best solution.

My prefered way would be to just use dict. But this is causing a metadata error (as mentioned in the comment earlier):

But the metadata library issues an error since the test on the correct type only checks for a dict and not (additionally) for an OrderedDict which is created by the YAML parse - I guess.

2020-01-05  13:04:53 ERROR    lib.metadata      plugin 'database': Found invalid value 'OrderedDict([('host', 'XXX'), ('user', 'XXX'), ('passwd', 'XXX'), ('
db', 'XXX'), ('port', 'XXX')])' for parameter 'database' (type dict) in /etc/plugin.yaml, using default value '{}' instead

Just if we want to support type specifier for subtypes we could use the following syntax (similar to list:

(we could also use a different delimiter instead of |, e.g. :, ; or /)

msinn commented 4 years ago

@ohinckel

My prefered way would be to just use dict. But this is causing a metadata error

No, the error is not from the definition dict but the actual /etc/plugin.yaml contains a list -> The error message is correct! Have a look at OrderedDict([('host', 'XXX') Do you see the square bracket? So: lib metadata could not convert a list (of tuples) to an ordered dict.

The error should disappear when the /etc/plugin.yaml data is corrected.

ohinckel commented 4 years ago

Why is it not able to convert it into a dict? Of course the output of an OrderedDict will dump the content as a list of tuples, but dealing with an OrderedDict should be (mostly) the same as dealing with a dict:

>>> import collections
>>> o=collections.OrderedDict()
>>> o["first"]="value"
>>> o["second"]="other"
>>> o
OrderedDict([('first', 'value'), ('second', 'other')])
>>> [key for key in o]
['first', 'second']
>>> [o.get(key) for key in o]
['value', 'other']
>>> dict(o)
{'second': 'other', 'first': 'value'}
>>> d={'second': 'other', 'first': 'value'}
>>> [key for key in d]
['second', 'first']
>>> [d.get(key) for key in d]
['other', 'value']
>>> 

The error message shows an error in /etc/plugin.yaml which does not exists in my system. I guess it mean the plugin.yaml in my SHNG installation - which is somewhere else. But this contains the following (no configuration as list at all):

db:
    class_name: Database
    class_path: plugins.database
    instance: dblog
    cycle: 20
    precision: -1
    driver: pymysql
    database:
      host: dbhost
      user: dbuser
      passwd: dbpasswd
      db: dbname
      port: 3306
msinn commented 4 years ago

Ok, then you are out of luck.

To handle it, we would need an special parser for etc/plugin.yaml and etc/module.yaml which interprets dicts.

At the moment the parser is used for all config files (.CONF or .YAML) including item definitions. For an item definition the parser could not distinguish between an item having a parameter of type dict and an item having a subitem. That is, because the config file parser doesn't know about the metadata. The metadata is applied only at the moment shng is initializing a plugin.

ohinckel commented 4 years ago

Ok, then you are out of luck.

That's bad, but I reverted the changes to the plugin is working as before.

[...] the parser could not distinguish between an item having a parameter of type dict and an item having a subitem.

Long time ago I asked to introduce a configuration object which could also handle this situation.

To fix this issue I updated the documentation to put the settings into quotes (just in case one is using them like a YAML structure).

msinn commented 4 years ago

Long time ago I asked to introduce a configuration object which could also handle this situation.

But the configuration object would not solve the problem that has its bases in the structure of the item configuration definition (regardless if .YAML or .CONF is used). I had to work around that when introducing the public properties of items as well.

To introduce dicts as datatype in items we have to have a fundamental change in the way item configurations are stored. That would leave a lot of users behind, especially those still using .CONF files.

ohinckel commented 4 years ago

At least it helps when parsing different configuration files. Of course it does not help for items configuration, since it's still depend on the dict structure for child items. But for (all) other configuration files it could help.

Are there still users using .CONF files? I thought this was deprecated years ago already.

To introduce dicts as datatype in items we have to have a fundamental change

Could be easily adapted by introducing a "child" element which contains child items. Migration script would also be easy.

But thanks anyway for you input. ATM I don't see any good solution for this.

bmxp commented 4 years ago

As of current documentation support of *.CONF files is deprecated but supported until being dropped with Release 2.0.

msinn commented 4 years ago

When reading the forum, you see still a lot of questions with examples given as .CONF files