return NULL if PathNotFoundException is thrown

[jschanz]

reverts a feature or modification from f90852c
discussion also see #4376

works like described in the manual
https://docs.openhab.org/v2.2/addons/transformations/jsonpath/readme.html#example
and it's necessary to parse json which doesn't contain the path
every time you query the json, otherwise the logfile is filled up with the json
stuff
for example, see https://www.dwd.de/DWD/warnungen/warnapp/json/warnings.json

Signed-off-by: Jens Schanz mail@jensschanz.de

[sjsf]

This PR is doing the exact opposite of what was discussed in the referenced issue which was created by you. And it also violates the API contract again. Therefore I'm struggling a bit with the reasoning as "it's necessary to parse json which doesn't contain the path" isn't really explanatory.

I guess this all boils down to the question whether the absence of a property in the json document should be interpreted as the value bing null or the document/jsonpath is invalid and therefore it cannot be parsed? This question already came up in #4379 and I guess in the end it's more or less a matter of personal preference and the concrete use-case how it should be interpreted. I don't have any strong opinion on that, so I would be okay with this change in general.

otherwise the logfile is filled up with the json stuff

If it is just about that and the actual null state itself is not of any interest, then of course it would also be an option to reduce the log severity...

[sjsf]

Returning null is not allowed by the API contract, see also #4870

[jschanz]

I'm not sure, if #4870 addresses the same problem.
Let me describe, why I think, an exception in case of a PathNotFound isn't the right way to handle this, as it is always possible, that a path doesn't exist in a json-Document. The main question seems to ... How to deal with that correctly?

In my case, I'll query the Weather warnings from the DWD. The DWD publishes a json-Document with all known warnings. The json-Document only contains warnings. If no warnings exists for your town or district, it doesn't show up in the json-Document.
If you do now something like the following in a rule, it works, when a warning exsits, but do some strange things, if your town and district is not included in the json-Document. The "old" way was easy to handle, because the variable was NULL in case of a pathNotFound-Exception. Now it includes the whole json-String :-/

For example, this works before the original change

var String json = sendHttpGetRequest("https://www.dwd.de/DWD/warnungen/warnapp/json/warnings.json")
var String headline = transform("JSONPATH", "$.warnings.108425000[0].headline", json_1)

if (headline === null) {
     headline = "Keine Unwetterwarnung"
 }
 postUpdate(weather_warning_dwd_headline, headline)

Now, I receive an exception in my openhab.log, every the document doesn't contain my path (108425000):

2018-02-06 12:39:59.938 [ERROR] [ore.transform.actions.Transformation] - Error executing the transformation 'JSONPATH': Invalid path '$.warnings.108425000[0].headline' in '{"time":1517916800000,"warnings":{"106535000":[{"regionName":"Vogelsbergkreis","start":1517907600000,"end":1517936400000,"level":2,"type":5,"state":"Hessen","description":"Es tritt oberhalb 400 m leichter Frost zwischen 0 °C und -4 °C auf.","headline":"Amtliche WARNUNG vor FROST","event":"FROST","instruction":"","stateShort":"HE","altitudeStart":400,"altitudeEnd":null},{"regionName":"Vogelsbergkreis","start":1517907600000,"end":1517936400000,"level":2,"type":5,"state":"Hessen","description":"Es tritt leichter Frost zwischen 0 °C und -4 °C auf.","headline":"Amtliche WARNUNG vor FROST","event":"FROST","instruction":"","stateShort":"HE","altitudeStart":null,"altitudeEnd":null}],"103401000":[{"regionName":"Stadt Delmenhorst","start":1517914800000,"end":1517936400000,"level":2,"type":5,"state":"Niedersachsen","description":"Es tritt leichter ...

So it works like designed in f90852c

But the main problem in my case is now, the variable "headline" is now filled up with the complete json-document:

logInfo("weather.rules","[DWD-Wetter] Meldung: " + headline)

2018-02-06 12:39:59.944 [INFO ] [smarthome.model.script.weather.rules] - [DWD-Wetter] Meldung: {"time":1517916800000,"warnings":{"106535000":[{"regionName":"Vogelsbergkreis","start":1517907600000,"end":1517936400000,"level":2,"type":5,"state":"Hessen","description":"Es tritt oberhalb 400 m leichter Frost zwischen 0 °C und -4 °C auf.","headline":"Amtliche WARNUNG vor FROST","event":"FROST","instruction":"","stateShort":"HE","altitudeStart":400,"altitudeEnd":null},{"regionName":"Vogelsbergkreis","start":1517907600000,"end":1517936400000,"level":2,"type":5,"state":"Hessen","description":"Es tritt leichter Frost zwischen 0 °C und -4 °C auf.","headline":"Amtliche WARNUNG vor FROST","event":"FROST","instruction":"","stateShort":"HE","altitudeStart":null,"altitudeEnd":null}],"103401000":[{"regionName":"Stadt Delmenhorst","start":1517914800000,"end":1517936400000,"level":2,"type":5,"state":"Niedersachsen","description":"Es tritt leichter Frost zwischen 0 °C und -4 °C auf.","headline":"Amtliche WARNUNG vor FROST",
...

And due to this behaviour (not null, in case if it's not existing), the item also gets updated.

2018-02-06 12:40:00.177 [vent.ItemStateChangedEvent] - weather_warning_dwd_headline changed from {"time":1517916800000,"warnings":{"106535000":[{"regionName":"Vogelsbergkreis","start":1517907600000,"end":1517936400000,"level":2,"type":5,"state":"Hessen","description":"Es tritt oberhalb 400 m leichter Frost zwischen 0 °C und -4 °C auf.","headline":"Amtliche WARNUNG vor FROST","event":"FROST","instruction":"","stateShort":"HE","altitudeStart":400,"altitudeEnd":null},{"regionName":"Vogelsbergkreis","start":1517907600000,"end":1517936400000,"level":2,"type":5,"state":"Hessen","description":"Es tritt leichter Frost zwischen 0 °C und -4 °C auf.","headline":"Amtliche WARNUNG vor FROST","event":"FROST","instruction":"","stateShort":"HE","altitudeStart":null,"altitudeEnd":null}],"103401000":[{"regionName":"Stadt Delmenhorst","start":1517914800000,"end":1517936400000,"level":2,"type":5,"state":"Niedersachsen","description":"Es tritt leichter Frost zwischen 0 °C und -4 °C auf.","headline":"Amtliche WARNUNG vor FROST","event":"FROST","instruction":"","stateShort":"NS","altitudeStart":null,"altitudeEnd":null}],"107132000":[{"regionName":"Kreis Altenkirchen","start":1517907600000,"end":1517936400000,"level":2,"type":5,"state":"Rheinland-Pfalz","description":"Es tritt oberhalb 400 m leichter Frost zwischen 0 °C und -4 °C auf.","headline":"Amtliche WARNUNG vor FROST","event":"FROST","instruction":"","stateShort":"RP","altitudeStart":400,"altitudeEnd":null},{"regionName":"Kreis Altenkirchen","start":1517907600000,"end":1517936400000,"level":2,"type":5,"state":"Rheinland-Pfalz","description":"Es tritt leichter Frost zwischen
...

I'm not sure, what's the correct way to fix this problem. A binding maybee could solve it more elegant than plain json and transforming it, but doesn't exists right now.
Catching the exception in the rules seems to be another way to deal with it.

   try {
      // do some stuff
   }
   catch(Throwable t) {
      logError("Error", "Some bad stuff happened in my rule: " + T.toString)
   }
   finally {
      // always runs even if there was an error, good place for cleanup
   }

Any other suggestions welcome ...
Anyway .. The documentation should also be updated, because due to the change it's not correct for the actual 2.2. version.

[sjsf]

I'm not sure, if #4870 addresses the same problem.

You are right, #4870 is addressing another problem - but it also had to return that "the value does not exist". So all I wanted to do is pointing you to an example how it can be done, i.e. return UnDefType.NULL.toFullString() as a valid state instead of null.

The documentation should also be updated

yes!

[sjsf]

The overall question remains: Should the absence of a property be interpreted as

1.) "deliberately null", i.e. change the item state to UnDefType.NULL or rather as
2.) an "incremental update", meaning that the previous item state is still valid

?

In your use-case it's clearly 1.). I'm just not sure if that's always the case. It needs to be done somehow, but in the end always somebody might be unhappy with it. I'm not sure which one is the more frequent use-case.

/cc @martinvw & @ThomDietrich

[sjsf]

On a second though: The item state already has changed - so it's rather a matter of displaying the whole json bogus vs. NULL. And suddenly I tend to agree with you, UnDefType.NULL sounds much more appealing 😉

[martinvw]

I would vote 1)

[ThomDietrich]

Hey guys, I didn't read the whole issue but wrt your last question I have to answer with 2.
It is actually fairly common (and useful?) to receive a complex json with only sparse content. The non-existence of a value has to mean that no new information regarding this datapoint is available, meaning the item has to stay unchanged. I have an example in my setup, please see this issue: arendst/Tasmota#1215 (comment)

To me the important question is: Does this case have to be handled in the transformation or in the component calling it - the MQTT Binding in my case?

[jschanz]

Also voting for 1)
It makes no sense to me, to get the whole json as result, if an exception occurs during a transformation.
The point is ... How to deal with problems like that in the future?

var String headline = transform("JSONPATH", "$.warnings.108425000[0].headline", json)

Before #4870 headline was NULL, if the path doesn't exists. Now headline contains the whole json-document.

[sjsf]

To me the important question is: Does this case have to be handled in the transformation or in the component calling it - the MQTT Binding in my case?

I was on the same track at first too: that needs to be done in the binding. For the transformation it is too late already - the item already changed its state. The transformation only determines how it should be presented. The choice to "stay with the previous value" is non-existent anymore at that point in time.

[ThomDietrich]

Just read the whole issue and I guess we are all correct in some ways.

@jschanz you are correct to request a null/UnDefType.NULL returned instead of the whole json.
@SJKA your (1) or (2) option above are coined at "item change" which confused me as this issue only addresses what the json transformation should return. My mqtt1 Binding example from above is therefore unrelated to the issue here. Btw. David already confirmed that the new MQTT Binding will handle null returns correctly (i.e. no item state change). 👍

@jschanz you've removed the exception. I'd suggest to keep the line but as a log call.

[htreu]

I clearly vote for 1).
Returning "NULL" for unknown paths as well as absent values gives the chance to react during rule execution very easily. And it fits the assumption that JSON is build/send incrementally with null values left out.

[openhab-bot]

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

https://community.openhab.org/t/first-major-update-in-2-3-0-snapshot/38801/12

[sjsf]

Pretty clear outcome of the discussion!

@jschanz it's your turn, I think. As mentioned inline: returning null is not an option, but UnDefType.NULL.toFullString() clearly is.

[ThomDietrich]

For all it's worth, I'd have stayed with the previous string.
Please start the line with a capital letter and don't mention details a user can not comprehend.
Debug level sems a good choice.

[jschanz]

@ThomDietrich
I'm not sure, what you mean with

Please start the line with a capital letter and don't mention details a user can not comprehend.

Should the debug note start with an upper case letter? Also remove the hint to PathNotFoundException?

Because there are other debug note starting lower case.

There is no previous string when you do something like that:

var String headline = transform("JSONPATH", "$.warnings.108425000[0].headline", json)

You'll get NULL in openHAB 2.1 and the whole json string in openHAB 2.2 if the path doesn't match an entry in the json.

2.1 -> NULL
2.2 -> {"time":1518717463000,"warnings":{"103401000":[{"regionName":"Stadt Delmenhor...

[ThomDietrich]

I wanted to leave the details to you. An uppercase letter at the beginning of a message sounds logical to me and most log Messages start with one. Furthermore I would remove the "PathNotFoundException" mentioning, yes, because it's an internal detail. Lastly I'd give the user details he/she will be able to work with. I'm not sure what's really needed, posting both variables might be too verbose or just right. How about something along this:

logger.debug("Given JSONPath selector expression '" + jsonPathExpression + "' does not match the input string '" + source + "'");

[jschanz]

Any news here for the 2.3 release?

[kaikreuzer]

There is no ESH 2.3 release for the next couple of years...

I didn't follow this discussion here in detail, but what I read when scanning over it simply does not make sense to me. A transformation service delivers a String, not a State. Delivering the String "NULL" imho makes no sense at all, since you could never ever distinguish it from the potential value "NULL" that could be the content of the property.

As discussed in #5365 (review), I think null or an Exception are the only valid solutions and we should rather adapt the callers of this method to handle such situations appropriately.

[jschanz]

Sorry ... I mean the upcoming openhab-2.3 release.
The code change, introduced in openhab-2.2, break a lot in case of a path not found exception.
In earlier versions it was possible, also mentioned in the documentation, that if a path isn't included in the json, that the return value is NULL. Since openhab-2.2 the return value contains the whole json-string and breaks not only the logfile with a huge error message for a valid operation, it also possibly kills the complete openhab instance, if the output (whole json) is written to an item, because no further error handling is possible (check for if NULL).
This has nothing todo with other "bugs" and "NULLS" in the document (missing property).

Maybee there are good reasons I don't know to not accept this merge, but the JSONPATH-transform is actually broken and useless if you use dynamic generated json documents (e.g. https://www.dwd.de/DWD/warnungen/warnapp/json/warnings.json). The warning.json from DWD contains only WARNINGS, if there is no warning for your city, the path doesn't match, because there is no entry for the path definied in the transform-rule.

For example ...

var String headline = transform("JSONPATH", "$.warnings.108425000[0].headline", json)

You'll get NULL as return value in openHAB 2.1 and the whole json string as return value in openHAB 2.2 if the path doesn't match an entry in the json.

2.1 -> NULL
2.2 -> {"time":1518717463000,"warnings":{"103401000":[{"regionName":"Stadt Delmenhor...

So I tried to catch the exception in the rules, but this seems not possible. Also I tried several approaches to handle the exception and the string with patterns and regexes, but nothing seems to work. I always get a huge exception in the code and blow up my item with garbage response.

The documentation for openhab-2.2 says
https://docs.openhab.org/addons/transformations/jsonpath/readme.html#further-reading

Returns null if the JsonPath expression could not be found.

Thats wrong. In openhab-2.2 it returns the whole json document now.

I'm no JAVA developer nor anyone who has a deep dive done in the smarthome code, but maybee anyone could have a try to reproduce this an also provide a (simple) solution how to deal with a missing path in a json document.

rule "Prüfe auf Unwetterwarnung des DWD"
    when
        Time cron "0 0 * * * ?"
	or
	System started
    then
        logInfo("weather.rules","-> Prüfe auf Unwetterwarnung des DWD")
	logInfo("weather.rules","Hole Wetterwarnungen ...")
        var String json = sendHttpGetRequest("https://www.dwd.de/DWD/warnungen/warnapp/json/warnings.json")

	logInfo("weather.rules","Verarbeite Wetterwarnungen ...")
        var String json_1 = json.substring(24, json.length-2)

	logInfo("weather.rules","Transformiere Wetterwarnungen ...")

	var String region = transform("JSONPATH", "$.warnings.108425000[0].regionName", json_1)
	logInfo("weather.rules","Region: " + region)

	var String headline = transform("JSONPATH", "$.warnings.108425000[0].headline", json_1)
	logInfo("weather.rules","Meldung: " + headline)

	var String description = transform("JSONPATH", "$.warnings.108425000[0].description", json_1)
        logInfo("weather.rules","Beschreibung: " + description)	
		
        if (region === NULL) {
        	region = "Alb-Donau-Kreis und Stadt Ulm"
        }
        postUpdate(weather_warning_dwd_region, region)
        
        if (headline === NULL) {
        	headline = "Keine Unwetterwarnung"
        }
        postUpdate(weather_warning_dwd_headline, headline)
        
        if (description === NULL) {
        	description = "Es liegt aktuell keine Unwetterwarnung vor"
        }        
	postUpdate(weather_warning_dwd_description, description)
		
end

[triller-telekom]

@jschanz In my PR #5365 where I declare the null contract of the transformation services I now allow null as a return value for transformation services.

I also have reverted a change so the JsonPathTransformation now returns null instead of NULL.

Also I added a transformRaw method which you can access from your scripts which throws the Exception all the way to the caller:

https://github.com/eclipse/smarthome/pull/5365/files#diff-9d3e7ccaed33705693f548bcf6acbcfdR77

For your use-case you can parse the content of this Exception:

https://github.com/eclipse/smarthome/blob/ba50fec662840c5c04d4f3ef6fc7f7c0ceb41ebd/extensions/transform/org.eclipse.smarthome.transform.jsonpath/src/main/java/org/eclipse/smarthome/transform/jsonpath/internal/JSonPathTransformationService.java#L67

to see if the Path did not exist. I hope that solves your issue.

It is planned to include my PR soon so it reaches openHAB release 2.3.

[kaikreuzer]

I think #5365 now implemented a clean solution, which should also flexibly allow using transformation services in any possible way from within rules. I therefore close this PR.

[jschanz]

Thank you very much @kaikreuzer ...