[HttpUtil] Suggestion for using a builder like pattern for Http requests

[martinvw]

Hello,

I was using the HttpUtil and found it a little bit inconvenient to use with all those null parameters in between, so I prepared a small suggestion which makes it more readable in my opinion. Wdyt?

Is:

String httpMethod = "POST";
String url = "http://" + host + "/cgi-bin/MUH44TP_getsetparams.cgi";
String content = "{tag:ptn}";
InputStream stream = new ByteArrayInputStream(content.getBytes(StandardCharsets.UTF_8));
int timeout = 5000;
String response = HttpUtil.executeUrl(httpMethod, url, null, stream, null, timeout);

Suggestion:

HttpUtil.postTo(url)
    .withContent("{tag:ptn}")
    .withTimeout(5, TimeUnit.SECONDS)
    .getContentAsString();

Current:

String METHOD = "GET";
int TIMEOUT = 30 * 1000; // 30 seconds
Properties HEADERS = new Properties();
HEADERS.put("User-Agent", "openHAB / senseBox binding " + version.toString());
String query = SENSEMAP_API_URL_BASE + "/boxes/" + senseBoxId;
String body = HttpUtil.executeUrl(METHOD, query, HEADERS, null, null, TIMEOUT);

Suggestion:

HttpUtil.getFrom(SENSEMAP_API_URL_BASE + "/boxes/" + senseBoxId)
    .header("User-Agent", "openHAB / senseBox binding " + version)
    .withTimeout(30, TimeUnit.SECONDS)
    .getContentAsString();

Any thoughts on this?

[igilham]

Looks way better and should make it easier to avoid putting params in the wrong positions, NPEs etc.

[martinvw]

@sjka wdyt? If enough people like it I might be able to build a first sketch of it in the train to home tonight :-)

[kaikreuzer]

Would be ok for me. But please note that the HttpUtil was meant to be a helper class to make simple HTTP interaction possible for rules, i.e. the main "client" of that class should be org.eclipse.smarthome.model.script.actions.HTTP for which the syntax is pretty irrelevant.

HttpUtil is NOT meant to be the HTTP client of choice for bindings or other Java code - we actually suggest to use the Jetty client directly. So in this context, I am not sure how much effort we should spend on enhancing the HttpUtil class.

[martinvw]

In some openHAB PR's I though you or maybe others suggested the use of it because some corner case where handled incorrectly in their code with the risks of resource leaks etc. Given that we only have to do it once correct I also started suggesting it, but I did not think I made it up mysql :-)

Some overview of the current usage:

• WeatherUnderground uses it to download images
• YahooWeather uses it for any json request
• WEMO uses it for SOAP calls
• Hue uses it to detect bridges
• ZonePlayer uses it for some meta information and retrieving images
• FritzahaWebInterface uses it
• Freebox uses it
• The cometvisu clientinstaller uses it
• Sensebox uses it
• hdanywhere uses it
• TankerkoenigService uses it
• SynopAnalyzer uses it
• Windcentrale uses it

[lolodomo]

Why bindings should not use it ?

[martinvw]

And I found your suggestion :-)

https://github.com/openhab/openhab2-addons/pull/1535#discussion_r120370329

URLConnection is not really advised as it is sometimes a bit difficult to manage regarding error handling and proxy support. I'd suggest to go for HttpUtil.executeUrl() instead, which ESH provides and encapsulates all code using the Jetty HTTP client.

@kaikreuzer please share your view and what should be the best practice from our current point of view and how best to proceed: advice on using the HttpUtil and improve it or start advising not to use it and use pure Jetty.

[martinvw]

@kaikreuzer ping :-)

[kaikreuzer]

I'd say that if no advanced features are required, the suggestion for bindings would be to use HttpUtil as it will avoid implementation errors that people might otherwise do. For advanced use, people should directly make use of the Jetty APIs.

So yes, I think it is perfectly fine if you wish to provide a more convenient way to do HTTP requests. My concern above merely want to warn you that HttpUtil is used within the rules and thus all static methods are implicitly made available there, so we must avoid adding anything that leads to confusion in rules.

You might therefore rather want to introduce a new class like HttpRequestBuilder, which assembles an HttpRequest instance, which could then be passed to a new method HttpUtil.execute(HttpRequest) or something along these lines.

[martinvw]

My concern above merely want to warn you that HttpUtil is used within the rules and thus all static methods are implicitly made available there, so we must avoid adding anything that leads to confusion in rules.

Is this limited to the HttpUtil class or any static method in this package?

[kaikreuzer]

Limited to HttpUtil.

[triller-telekom]

@martinvw Since you have started an implementation on your branch, are you planing to create an official PR for it?

[martinvw]

Let me move it a little bit more to the top of my action list, it would be great to have it wrapped up.

[triller-telekom]

Great, thank you!