[influxdb] Fix for influxdbv1 retention and table names containing keywords or special chars

[fremel75]

Bugfix for using itemnames beginning with numbers or having retention namnes beginning with numbers
Also solves if using old db where retention was named default wich is now a influxQL keyword

Resolves #9790
Resolves #10398

Added doubleqoutes for retention and table name as documentation for influxQL states
https://docs.influxdata.com/influxdb/v1.8/query_language/spec/#identifiers

[J-N-K]

IIRC item names are not allowed to start with a number.

[lujop]

Thanks for the PR.
I think that the retentionPolicy doesn't need to be escaped because it's covered by the appendName function logic.
About the tablename it's ok to escape for the default keyword that isn't covered by the appendName but only when escapeTableName is true.
When it's false it's because tableName it's /.*/ and it must not be escaped.

[fremel75]

Thanks for the feedback
I have reverted the code for escapeTableName function

But the appendName function doesent seem to work for this properly i guess since its not escaped?
Since it does not work without this change

From influx code
public static StringBuilder appendName(final String name, final StringBuilder stringBuilder) { String trimmedName = name.trim(); if (trimmedName.startsWith("\"") || COLUMN_NAME_PATTERN.matcher(trimmedName).matches()) { stringBuilder.append(trimmedName); } else { stringBuilder.append('"').append(trimmedName).append('"'); } return stringBuilder; }

[lujop]

Thanks for the feedback
I have reverted the code for escapeTableName function

But the appendName function doesent seem to work for this properly i guess since its not escaped?
Since it does not work without this change

From influx code
public static StringBuilder appendName(final String name, final StringBuilder stringBuilder) { String trimmedName = name.trim(); if (trimmedName.startsWith("\"") || COLUMN_NAME_PATTERN.matcher(trimmedName).matches()) { stringBuilder.append(trimmedName); } else { stringBuilder.append('"').append(trimmedName).append('"'); } return stringBuilder; }

The way it is now looks good to me, except that I think that the escaping inside the escapeTablename if it's redundant.

Can you provide a case where it isn't working with the inputs?

[fremel75]

I have tested with "default" as retention name since this is a keyword now for the default retention in influx.

14:52:45.806 [DEBUG] [e.influxdb.InfluxDBPersistenceService] - Got a query for historic points!
14:52:45.829 [ERROR] [.internal.JSONResponseExceptionMapper] - Unexpected exception occurred while processing REST request.
org.influxdb.InfluxDBException: error parsing query: found DEFAULT, expected identifier at line 1, char 40

And with my item named "1WireOutsideTempSensor_Temperature"

23:47:15.841 [DEBUG] [e.influxdb.InfluxDBPersistenceService] - Got a query for historic points!
23:47:15.844 [ERROR] [.internal.JSONResponseExceptionMapper] - Unexpected exception occurred while processing REST request.
org.influxdb.InfluxDBException: error parsing query: found .1, expected ; at line 1, char 47

or by rest api
"{\"error\":{\"message\":\"error parsing query: found .1, expected ; at line 1, char 47\",\"http-code\":500,\"exception\":{\"class\":\"org.influxdb.InfluxDBException\",\"message\":\"error parsing query: found .1, expected ; at line 1, char 47\",\"localized-message\":\"error parsing query: found .1, expected ; at line 1, char 47\"}}}"

I see now that Documentation for Openhab states "Names must not begin with number" i did create this in Webui so i thought it was ok and in rrd4j it was working
https://www.openhab.org/docs/configuration/items.html#name

[lujop]

I have tested with "default" as retention name since this is a keyword now for the default retention in influx.

14:52:45.806 [DEBUG] [e.influxdb.InfluxDBPersistenceService] - Got a query for historic points!
14:52:45.829 [ERROR] [.internal.JSONResponseExceptionMapper] - Unexpected exception occurred while processing REST request.
org.influxdb.InfluxDBException: error parsing query: found DEFAULT, expected identifier at line 1, char 40

And with my item named "1WireOutsideTempSensor_Temperature"

23:47:15.841 [DEBUG] [e.influxdb.InfluxDBPersistenceService] - Got a query for historic points!
23:47:15.844 [ERROR] [.internal.JSONResponseExceptionMapper] - Unexpected exception occurred while processing REST request.
org.influxdb.InfluxDBException: error parsing query: found .1, expected ; at line 1, char 47

or by rest api
"{\"error\":{\"message\":\"error parsing query: found .1, expected ; at line 1, char 47\",\"http-code\":500,\"exception\":{\"class\":\"org.influxdb.InfluxDBException\",\"message\":\"error parsing query: found .1, expected ; at line 1, char 47\",\"localized-message\":\"error parsing query: found .1, expected ; at line 1, char 47\"}}}"

I see now that Documentation for Openhab states "Names must not begin with number" i did create this in Webui so i thought it was ok and in rrd4j it was working
https://www.openhab.org/docs/configuration/items.html#name

But with current PR code the retentionPolicy isn't escaped, isn't it?
I can't see why the default isn't escaped with current PR code.

[fremel75]

But with current PR code the retentionPolicy isn't escaped, isn't it?
I can't see why the default isn't escaped with current PR code.
This PR adds ecaped doubleqoutes on retentionpolicy and item name
Since influx function appendName only adds doubleqoutes and that is not good enough
As you could see the appendName function does deal with if it called with """ so there is no problem doing it this way

[lujop]

But with current PR code the retentionPolicy isn't escaped, isn't it?
I can't see why the default isn't escaped with current PR code.
This PR adds ecaped doubleqoutes on retentionpolicy and item name
Since influx function appendName only adds doubleqoutes and that is not good enough
As you could see the appendName function does deal with if it called with """ so there is no problem doing it this way

Excuseme, my last comment was incorrect, I wanted to say, the retentionPolicy is escaped, isn't it?

But, despite my mistake, I think that we have some confusion in the latests comments. For my the actual PR is ok, I only think that the doublequotes inside the escapetablename are redundant and not needed, but not problematic.

When you say that you have problems, do you mean with this PR code or with actual code?

[fremel75]

First i experience no problems with the PR
I understand that you wonder about escapetablename but the appender function used there is not working for this plugin.
I dont understand either but it needs to be escaped doubleqoute for the plugin to work and the appenderfunction does not do that as it only doubleqoutes.

[lujop]

I don't understand what do you mean with:
the appenderfunction does not do that as it only doubleqoutes.
The PR is doing exactly that, doublequoting the tablename. Why do you say that the appenderfunction does not work?

That's the code of the appended function:

private static final Pattern COLUMN_NAME_PATTERN = Pattern.compile("\\w+(?:\\[.+\\])?");
public static StringBuilder appendName(String name, StringBuilder stringBuilder) {
        String trimmedName = name.trim();
        if (!trimmedName.startsWith("\"") && !COLUMN_NAME_PATTERN.matcher(trimmedName).matches()) {
            stringBuilder.append('"').append(trimmedName).append('"');
        } else {
            stringBuilder.append(trimmedName);
        }

        return stringBuilder;
    }

Can you debug or check logs and say for which input it doesn't work?
For words, it's not escaped, but in theory, it isn't needed.
I'm fine with the escaping, but want to know in which case it's needed and not done by the appendName

[fremel75]

I don't understand what do you mean with:
the appenderfunction does not do that as it only doubleqoutes.
The PR is doing exactly that, doublequoting the tablename. Why do you say that the appenderfunction does not work?

No it escapes the doubleqoutes also something that appendName function does not do

That's the code of the appended function:

No its not i have no idea where you found that but i have showed the function in #11139 (comment)
https://github.com/influxdata/influxdb-java/blob/master/src/main/java/org/influxdb/querybuilder/Appender.java#L76
Not that it matter in this case since its doing the same work

private static final Pattern COLUMN_NAME_PATTERN = Pattern.compile("\w+(?:\[.+\])?");
public static StringBuilder appendName(String name, StringBuilder stringBuilder) {
String trimmedName = name.trim();
if (!trimmedName.startsWith(""") && !COLUMN_NAME_PATTERN.matcher(trimmedName).matches()) {
stringBuilder.append('"').append(trimmedName).append('"');
} else {
stringBuilder.append(trimmedName);
}
return stringBuilder;
}

Can you debug or check logs and say for which input it doesn't work?
For words, it's not escaped, but in theory, it isn't needed.
I'm fine with the escaping, but want to know in which case it's needed and not done by the appendName

Tests here #11139 (comment)
Escaping is apparantly needed for these cases

I dont care about this anymore, feel free to close this PR
Sorry for anyone else that has not moved on
Trying to help OH just sucks to much energy

You should close these also and tell them that item or retention starting with numbers is not allowed
#9790
#10398
and anyone else that have a old influxdb with retentionname default

[lujop]

Thanks, @fremel75 , and I'm sorry you are disappointed.
But the addon has several edge cases maintaining retro compatibility, version updates, and others and I want to be sure before accepting a change.

I previously had understood the need for the default keyword, but didn't understand some of the comments and also I got confused because I didn't remember that \w also includes numbers.

Thank you a lot for your patience and comments.

[lujop]

Looks good

[lujop]

Excuseme after accepting I've seen that better to don't use the Appender.appendName in that cases, because it has no sense as with the " it allays simply appends.

[fwolter]

@lujop Your proposed changes are now part of this PR. Are you confident that these will work? Can you test them?

[lujop]

Hi fwolter, I'm pretty sure, but I hadn't time to test.
I will try next week, and also check if I can finish the SQL part on DBQuery addon.

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/js-scripting-previousstate/130432/21

[lujop]

@fwolter I've done some little tests and it seems to work and escape it's done:
2022-01-07 23:51:31.025 [TRACE] [.influxdb.InfluxDBPersistenceService] - Query SELECT "value"::field,"item"::tag FROM "autogen"."LocalMoon_StartTime" ORDER BY time DESC LIMIT 1;

[fwolter]

There are some formatting issues. You can fix them with mvn spotless:apply.

[lsiepel]

@lujop could you fix the formatting to get this build going?

[jlaur]

@fwolter, @lsiepel - applied spotless.