eclipse-archived / smarthome

Eclipse SmartHome™ project
https://www.eclipse.org/smarthome/
Eclipse Public License 2.0
862 stars 786 forks source link

JSONPATH is no JSONPATH anymore. #5132

Open Josar opened 6 years ago

Josar commented 6 years ago

https://community.openhab.org/t/jsonpath-transformation-service-vs-jsonpath/40439/5

As discussed there i propose to add an additional transformation called jsonobj whihc returns an evaluated object. In means of only one single element, without the list and slashes.

This can be used to parse json in item return values, labels etc.

And to make jsonpath transformation a full fledged jsonpath. As one would expect of it.

sjsf commented 6 years ago

To my understanding we need to distinguish to use-cases here:

  1. the transformation of item state, e.g. in item labels and widget labels. There it is absolutely crucial to have a single value as an outcome of the transformation. Neither the original JSON nor a list of matches is acceptable there, no matter how it is formatted.

  2. the use of the transformation service in rules in order to apply JSONPATH to a JSON structure, wherever it comes from.

The first use-case drives the API of the transformation service, this is what is was designed for. This is also the reason why it always returns a string - pretty inconvenient in case you are interested in null values or a lists as a return value. And honestly, splitting the result by ,s in order to get the list again might be a valid hack, but in no means a proper solution!

Because of these different, contradicting requirements I'd suggest to separate these two use-cases.

The only problem now is: How can JSONPATH be made available in the old rule-engine directly? I don't have a good plan for that yet...

Josar commented 6 years ago

So you suggestion is basically the same as mine here

https://community.openhab.org/t/jsonpath-transformation-service-vs-jsonpath/40439/5?u=josar

But it would be a much better solution if the transfromation just behaves different when called from different areas.

  1. Called by a rule it will be Jsonpath as there we want this feature and also are able to reformat teh result if neccessary.
  2. Called by a label or, widget, a thing, or when evver we expect it to be a single argument as result. Because everything else would not fit the usecase. Return an evaluated result.

Is it possible to get the caller somehow?

sjsf commented 6 years ago

No, my suggestion is actually quite different to yours. By adding yet another transformation service you will not solve the problem that the return type of transformation services are bound to strings and must not return null. My suggestion is to offer a completely different way of using jsonpath, ideally directly and not encapsulated into ESH's transformation API.

Josar commented 6 years ago
  1. For me it does not matter where the solution is placed. Offering a second JSON api anywhere is the same as offering a second transformation. IMHO. And where and how are second thoughts. And as a valid JSON is nothing else then a string it can also be a transformation, there are only some restrictions on the format of the string.

  2. But whats with the second suggestions? Changing the behavior depending on from where it was called.

jboeddeker commented 6 years ago

@sjka, I think you are missing an important use case here. JSONPATH tranformation is also used for collecting data in items.

In my example, the JSON i receive from a web service (http binding) is sometimes (currently it is) about 150kbytes in size. This dataset is reduced to its important parts (a list of ids). The resulting usually 50 bytes are stored in the item. A change of that item triggers a rule which does further processing.

I understand that every transformation service needs to return a single string and no list. Thats's fine. The previous implementation of JSONPATH, which results a list of ids as a string works perfectly.

But, most important, the single deciding factor for the number of elements in a JSONPATH resultset, should be the JSONPATH expression used. If a JSONPATH expression returns more than a single value, why does the transformation service override the correct resultset?

User: "JSONPATH, please give me all temperatures in my JSON object." JSONPATH: "No, I will give you nothing because the resultset contains more than one temperature and i guess you want it displayed as a label and this wouldn't look nice. Have a nice day."

The best solution is already described by @Josar and @openhab-5iver.

To keep the results somehow consistant, the brackets should be removed in the converted list.

Josar commented 6 years ago

I think returning an single element striped of from [""] is perfect if it is a single element.

And returning an JSON or a string without [] if multiple results are requested is also fine.

I think it only bytes with the intent to raise awerness with an error when the jsonpath transformation is used with multiple results for labels etc.

But this solution sound like my first suggestion, just remove the warning, return multiple elements if the request yields it, return a stripped off string if single element. https://community.openhab.org/t/jsonpath-transformation-service-vs-jsonpath/40439/2?u=josar

jboeddeker commented 6 years ago

oops, sorry @Josar . In all the discussion there, i lost the idea from you, just saw it a s @openhab-5iver wrote it.

I think it only bytes with the intent to raise awerness with an error when the jsonpath transformation is used with multiple results for labels etc.

I seriously hope this is not the case. No direction could be more wrong. ;-)

Josar commented 6 years ago

@jboeddeker no problem, it also means more then one thought of the same solution. ;-)

My intention here is just to get a good solution.

jboeddeker commented 6 years ago

I just thought about a really ugly solution for my problem. I could read the resultset from the openhab-log. :D

Just need to throw this at the exec-binding: cat /var/log/openhab2/openhab.log | grep JsonPath\ expressions\ with\ more\ than\ one\ result\ are\ not\ allowed,\ please\ adapt\ your\ selector.\ Result: | tail -n 1 | awk '{print $22;}'

Too bad i use JSONPATH on two similar occasions. :(

sjsf commented 6 years ago

JSONPATH tranformation is also used for collecting data in items

To me this sound pretty much like a misuse of items. Instead of using the http binding to fetch the data and store the fill dataset in an item, why don't you use the sendHttpGetRequest in your rules and process the data directly? Or even better: if the use-cases are so complicated and specific, write a dedicated bindings for this service? Others might benefit from it too...

Items are meant to be a functional abstraction of your house's state, not datastores or intermediate variables! You are free to do so anyway, of course, but that shouldn't influence design decisions of the framework in order to make such hacks more comfortable.

Too bad i use JSONPATH on two similar occasions. :(

In any case, this is the exact purpose of my suggestion: To make sure both occasions have a viable option to use jsonpath tranformations. And no @Josar, returning strings that you will need to split and trim in rules is a hack, but not a proper solution. Neither is using reflection and thread dumps to guess where the transformation was called from, apart from the fact that Java doesn't allow two different return types on the same method signature. This simply is not an option!

Josar commented 6 years ago

I still don't understand. The JSON is a string as it is called JSON String, no matter what type you cast it to.

We receive it as string and we return it as string.

{
    "key" : "value",
    "array" : [
        { "key" : 1 },
        { "key" : 2, "dictionary": {
                "a": "Apple",
                "b": "Butterfly",
                "c": "Cat",
                "d": "Dog"
            } },
        { "key" : 3 }
    ]
}
$.array[?(@.key=2)].dictionary.b

Would return the string ["Butterfly"] but returns Butterfly instead all good. Everyone wants that. It perfectly fits the use case.

But this $.array[?(@.key=2)].dictionary retuns an error. But could return a string which contains the JSON. And someone could use this in the rule as he wich. Store it, display it, parse it again with a json path transformation.

[{"a": "Apple","b": "Butterfly","c": "Cat","d": "Dog"}]

Also the implementation could be quite easy, just checkt the result for a colon. If there is one don't change the result just return it. If not Strip []{}"" if neccessary.

I can not understand where the conflict arises here. Please could you elaborate!

jboeddeker commented 6 years ago

To me this sound pretty much like a misuse of items.

@sjka, no, thats's a wrong assumption. Data collection was the wrong word for that. This item exactly represents the functional abstraction of the state of the part of the service i am querying. The service just doesn't provide it in a condensed view.

Items are meant to be a functional abstraction of your house's state, not datastores or intermediate variables!

Which it is, sorry i used a wrong wording.

Too bad i use JSONPATH on two similar occasions. :(

In any case, this is the exact purpose of my suggestion: To make sure both occasions have a viable option to use jsonpath tranformations.

This was just directed at the joke about reading the resultset from the log. The log entry doesn't state the item, so i can't decide which item the query result was withold from. This is still a joke and no valid idea!

What is exact benefit of returning null instead of the requested resultset? Put the warning in the log AND provide the resultset! Where is the problem with that?

We don't have another option than to return a string, so the number of elements must be condensed to a string somehow. If a single element is the resultset, do what you do now. If more than one element is returned, condense the elements somehow to a string. This is perfectly consistant and if someone is unhappy with the display in any UI he is allowed to optimize his JSONPATH query. The issue of @ThomDietrich is solved and it works for other usecases as well. All parties happy.

Josar commented 6 years ago

@jboeddeker is this not the same as i said? Still waiting for an elaborated explanation from @sjka.

ThomDietrich commented 6 years ago

I can fully agree with everything @sjka said so far.

@jboeddeker:

User: "JSONPATH, please give me all temperatures in my JSON object." JSONPATH: "No, I will give you nothing because the resultset contains more than one temperature and i guess you want it displayed as a label and this wouldn't look nice. Have a nice day."

I know that the example was there to prove your point but it actually describes exactly how it should work. The provided jsonpath selector was not able to retrieve a simple datatype element and hence yields an error. You've wrongly configured your jsonpath transformation service. @sjka said it quite well: "Items are meant to be a functional abstraction of your house's state, not datastores or intermediate variables!". @Josar the same goes for your butterfly example. Instead of one Item to store the json string, your should define four and apply a specific jsonpath transformation service setting.


Now why did I write "jsonpath transformation service" specifically? Well because all of those arguments are valid for transformation services: http://docs.openhab.org/addons/transformations.html I can see how both your use cases are relevant in real world. You want to retrieve and process complex json strings in a rule, to eventually store processed data. You should be able to do so but the jsonpath transformation service is not for you, you are misusing it at best.

@sjka stated:

My suggestion is to offer a completely different way of using jsonpath, ideally directly and not encapsulated into ESH's transformation API.

If I got this correctly, I agree. You should be able to call the jayway jsonpath methods in rules. @sjka is that what you were imagining and if so, how much work is it to make that possible?


The last issue remains: What should the jsonpath transformation service do if the transformation result is a non-trivial json string? I have no clear opinion here. However let me compare:

  1. Return null: The transformation was not "successful" and a json string is not a valid value for an Item. Hence the Item can not be updated and you should improve your settings.
  2. Return the json string: A complex json string is not a trivial information and not in line with the idea of Items as elementary representations of your home. You probably don't want to present the Item in your sitemap. The only real use case for the item is to be the container for data processed in a rule. If this is your overall workflow, the Item concept is misused and the container Item should be avoided by a direct data retrieval in a rule or even better by using a dedicated binding.

To prove this point, please check out the tutorial on how to retrieve data from the iCloud: https://community.openhab.org/t/icloud-device-data-integration-in-openhab/32329 There a complex json response is directly retrieved and processed inside a rule. Later the original poster realized that the whole idea should be implemented in a binidng and did so, the result was the very popular iCloud binding available since openHAB 2.2

jboeddeker commented 6 years ago

With all due respect, @ThomDietrich & @sjka, you are wrong with your assumption. The resultset in my case is definitely a state, my rule triggers only on state change! The value is not used for further processing. So it's no container or datastore , it's a state. It just consists of more than a single element.

@ThomDietrich, i know how to put that stuff in a rule, the difference is the triggering element. With the 2.2 jsonpath implementation, the workload stays on http binding level until a change is detected. The rule just runs IF a change was detected. Moving the jsonpath query into the rule, the rule needs to be called by a cron expression and the workload is in the rules engine. (BTW, i tried that and the system was much more unresponsive.)

Still, why is it neccessary to REMOVE existing functionality, because "I have no clear opinion here.". Remove it IF you have a clear opinion how to do it ideologically correct. Not before.

BTW, i have a clear opinion here. The jsonpath expression defines the resultset, and it's not the business of a transformation service to override the request of the user. We can talk about primary use cases, which should be supported easily, so stripping the brackets is fine. But why do you want to patronize the users?

Binding, yes would be nice. But due to time constraints, i am unable to do that currently.

Josar commented 6 years ago

What you say the JsonPath transformation should be is a jsonpath element selector.

not datastores or intermediate variables!".

Then this, from your link, is not vaid

{ "temperature": 23.2 }
Number  Kitchen_Temperature_C    "Temperature [JSONPATH($.temperature):%.1f °C]" {/*Some Binding*/}

As there is a JSON string saved in the Item and transformed to the label. So the items contains a intermediate variable JSON string vs string with value. IMHO.

If we are not so nitpicking and and say this JSON is a valid a functional abstraction Then in a real world scenario how did it get there?

It will be delivered in a JSON as we all know, then has to be processed, maybe in a rule, to only contain the key:value pair with a JSONPATH querry. And when this happens it should/could directly be converted to the value. IMHO.

Basically what you said would make the JSONPATH transformation invalid for lables. And this would also count for the XPath and XSLT.

It would only be valid in rules where it would better be a full fledged JSON validator. Or in the special case the binding has it implemented as preproccessor for the Things channel return value. Which is not common. And here it would be again good to be a json selector.

sjsf commented 6 years ago

I sincerely respect your opinions. Just let me know once your are interested in a constructive discussion again.

Josar commented 6 years ago

@sjka i did not mean to offend any one here. And i don't see why this is not constructive, but ok then i will keep my humble opinion to myself.

jboeddeker commented 6 years ago

@sjka i also did not mean to offend, and i do not see why you could be offended ... if it was something i wrote, we could talk about it in private.

sjsf commented 6 years ago

I'm not offended at all. I just don't see this discussion is leading anywhere. I thought I outlined well enough what the primary purpose of the transformation service API is and why it is not possible to twist it in a way which suits the need of the rule engine scripts. And I would really love to focus on finding a suitable solution instead of nitpicking on wording and examples.

If I got this correctly, I agree. You should be able to call the jayway jsonpath methods in rules. @sjka is that what you were imagining [...]?

Yes, indeed. Maybe it helps to give a concrete example. Let's imagine the jsonpath bundle would bring its own ActionService implementation, with an imaginary command called jsonpath(...). It would allow calling the jayway jsonpath library directly and simply return whatever it originally returns. A rule then could look like this:

    var String json = '
{
    "array" : [
        { "key" : 1 },
        { "key" : 2 },
        { "key" : 3 }
    ]
}    
'
    var output = jsonpath("$.array", json)
    logInfo("rule debugging", output + "")
    logInfo("rule debugging", output.getClass().getName())

The output would be:

16:51:36.003 INFO  o.e.s.m.script.rule debugging[:53] - [{"key":1},{"key":2},{"key":3}]
16:51:36.003 INFO  o.e.s.m.script.rule debugging[:53] - net.minidev.json.JSONArray

As you notice: The beauty of it is the return type. No need to parse, cast, of guess anything. And the toString() method gives you perfectly valid json string again, in case you want that.

how much work is it to make that possible

A couple of minutes only. And the fact that with the ActionService interface we would be using a concept which probably won't survive the switch to the new automation engine. So it might be required to break this again. On the other hand it is used for Transformation, Audio, Voice, Persistence and Thing related extensions to the language currently already. We will have to find a solution for those anyway, and jsonpath will be the least problem in that context, I think.

jboeddeker commented 6 years ago

@sjka, this doesn't help my case. I would still need a cron expression to trigger the rule instead of a state change of the item.

I want to understand, what is so problematic to simply return the string, which is already logged?

[WARN ] [ternal.JSonPathTransformationService] - JsonPath expressions with more than one result are not allowed, please adapt your selector. Result: [1519879170,1519877970]

The result i am interested in is this [1519879170,1519877970]. I don't care if the brackets are removed, to be in line with the single element string. 1519879170,1519877970 would be fine as well. And a change of this element triggers my rule. No cron expression to trigger the rule necessary.

What harm could be done if this result was returned instead of null? (Like it was in openHAB 2.2 and before.)

The result is also in line with the states of ColorItems: [vent.ItemStateChangedEvent] - SZDL_COLOR changed from 88,17,99 to 88,16,98

rkoshak commented 6 years ago

Seems to me a middle path could be too keep the JSONPATH transform as it is and added a JSON library to the class path used by the Rules. There have been a number of occasions where having the ability to properly process JSON in a rule would have been handy.

I don't know how hard that would be too do but it provides a way to achieve @sjka's approach to have a separate transform for rules.